[PATCH 1/3] plugins: Implementation of Network Time plugin

[PATCH 1/3] plugins: Implementation of Network Time plugin

[Antti Paila]

[-- Attachment #1: Type: text/plain, Size: 8465 bytes --]

---
 include/dbus.h    |    1 +
 plugins/nettime.c |  293 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 294 insertions(+), 0 deletions(-)
 create mode 100644 plugins/nettime.c

diff --git a/include/dbus.h b/include/dbus.h
index 9e29afb..0c48f83 100644
--- a/include/dbus.h
+++ b/include/dbus.h
@@ -45,6 +45,7 @@ extern "C" {
 #define OFONO_MESSAGE_WAITING_INTERFACE "org.ofono.MessageWaiting"
 #define OFONO_NETWORK_REGISTRATION_INTERFACE "org.ofono.NetworkRegistration"
 #define OFONO_NETWORK_OPERATOR_INTERFACE "org.ofono.NetworkOperator"
+#define OFONO_NETWORK_TIME_INTERFACE "org.ofono.NetworkTime"
 #define OFONO_PHONEBOOK_INTERFACE "org.ofono.Phonebook"
 #define OFONO_RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
 #define OFONO_AUDIO_SETTINGS_INTERFACE "org.ofono.AudioSettings"
diff --git a/plugins/nettime.c b/plugins/nettime.c
new file mode 100644
index 0000000..0f99bb1
--- /dev/null
+++ b/plugins/nettime.c
@@ -0,0 +1,293 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/nettime.h>
+#include <ofono/types.h>
+#include <gdbus.h>
+#include "ofono.h"
+
+#include "common.h"
+
+#define MAX_TIME_STR_LEN 128
+#define TIME_FORMAT "%FT%TZ"
+
+
+struct nettime_data {
+	struct ofono_network_time nw_time;
+	time_t received;
+};
+
+static void nettime_register(struct ofono_nettime_context *);
+
+static gboolean encode_time_format(struct ofono_network_time *time,
+				 struct tm *tm)
+{
+	if (time->year < 0)
+		return FALSE;
+
+	tm->tm_year = time->year - 1900;
+	tm->tm_mon = time->mon - 1;
+	tm->tm_mday = time->mday;
+	tm->tm_hour = time->hour;
+	tm->tm_min = time->min;
+	tm->tm_sec = time->sec;
+	tm->tm_gmtoff = time->utcoff;
+	tm->tm_isdst = time->dst;
+
+	return TRUE;
+}
+
+static time_t get_monotonic_time()
+{
+	struct timespec ts;
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	return ts.tv_sec;
+}
+
+static struct tm *refresh_nw_time(struct tm *nw_time,
+				time_t received)
+{
+	time_t now, new_nw_time;
+
+	now = get_monotonic_time();
+	new_nw_time = timegm(nw_time) + now - received;
+	return gmtime(&new_nw_time);
+}
+
+static int fill_time_changed_signal(DBusMessage *signal,
+		struct nettime_data *ntd)
+{
+	DBusMessageIter iter, iter_array;
+	int time_available;
+	char buf[MAX_TIME_STR_LEN];
+	struct tm time_tm, *new_time_tm;
+	const char *str = buf;
+
+	dbus_message_iter_init_append(signal, &iter);
+	dbus_message_iter_open_container(&iter,
+					DBUS_TYPE_ARRAY,
+					"{sv}",
+					&iter_array);
+
+	time_available = encode_time_format(&ntd->nw_time, &time_tm);
+	if (time_available != 0) {
+		new_time_tm = refresh_nw_time(&time_tm, ntd->received);
+		strftime(buf, MAX_TIME_STR_LEN, TIME_FORMAT, new_time_tm);
+
+		ofono_dbus_dict_append(&iter_array,
+					"DateAndTime",
+					DBUS_TYPE_STRING,
+					&str);
+	}
+
+	ofono_dbus_dict_append(&iter_array,
+				"Timezone",
+				DBUS_TYPE_INT32,
+				&ntd->nw_time.utcoff);
+	ofono_dbus_dict_append(&iter_array,
+				"DST",
+				DBUS_TYPE_UINT32,
+				&ntd->nw_time.dst);
+	dbus_message_iter_close_container(&iter, &iter_array);
+	return 0;
+}
+
+static DBusMessage *create_time_changed_signal(
+			struct ofono_nettime_context *context)
+{
+	DBusMessage *signal;
+	struct nettime_data *ntd = context->data;
+	const char *path = ofono_modem_get_path(context->modem);
+
+	if (path == NULL) {
+		ofono_error("Fetching path for modem failed");
+		return NULL;
+	}
+
+	signal = dbus_message_new_signal(path, OFONO_NETWORK_TIME_INTERFACE,
+					"NetworkTimeChanged");
+	fill_time_changed_signal(signal, ntd);
+
+	return signal;
+}
+
+static void init_time(struct ofono_nettime_context *context)
+{
+	struct nettime_data *nettime_data;
+	context->data = g_try_new0(struct nettime_data, 1);
+
+	nettime_data = context->data;
+	nettime_data->nw_time.year = -1;
+
+}
+
+static int nettime_probe(struct ofono_nettime_context *context)
+{
+	ofono_debug("Network Time Probe for modem: %p", context->modem);
+
+	init_time(context);
+
+	nettime_register(context);
+	return 0;
+}
+
+static void nettime_remove(struct ofono_nettime_context *context)
+{
+	DBusConnection *conn;
+	const char *path;
+
+	ofono_debug("Network Time Remove for modem: %p", context->modem);
+
+	g_free(context->data);
+
+	conn = ofono_dbus_get_connection();
+	path = ofono_modem_get_path(context->modem);
+
+	if (!g_dbus_unregister_interface(conn,
+					path,
+					OFONO_NETWORK_TIME_INTERFACE))
+		ofono_error("Could not unregister %s interface",
+				OFONO_NETWORK_TIME_INTERFACE);
+
+	ofono_modem_remove_interface(context->modem,
+			OFONO_NETWORK_TIME_INTERFACE);
+}
+
+static void nettime_info_received(struct ofono_nettime_context *context,
+				struct ofono_network_time *info)
+{
+	DBusMessage *nt_signal;
+	struct nettime_data *ntd = context->data;
+
+	if (info == NULL)
+		return;
+
+	ofono_debug("Received a network time notification on modem: %p",
+			context->modem);
+
+	ntd->nw_time = *info;
+	ntd->received = get_monotonic_time();
+
+	nt_signal = create_time_changed_signal(context);
+	if (nt_signal == NULL) {
+		ofono_error("Failed to create NetworkTimeChanged signal");
+		return;
+	}
+
+	g_dbus_send_message(ofono_dbus_get_connection(), nt_signal);
+}
+
+static struct ofono_nettime_driver nettime_driver = {
+	.name		= "Network Time",
+	.probe		= nettime_probe,
+	.remove		= nettime_remove,
+	.info_received	= nettime_info_received,
+};
+
+static int nettime_init(void)
+{
+	return ofono_nettime_driver_register(&nettime_driver);
+}
+
+static void nettime_exit(void)
+{
+	ofono_nettime_driver_unregister(&nettime_driver);
+}
+
+static DBusMessage *nettime_get_time(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	DBusMessage *reply;
+	int status;
+	struct ofono_nettime_context *context = data;
+
+
+	ofono_debug("Time requested from modem %p", context->modem);
+
+	reply = dbus_message_new_method_return(msg);
+	if (reply == NULL) {
+		ofono_error("Message allocation failed");
+		return NULL;
+	}
+
+	status = fill_time_changed_signal(reply, context->data);
+	if (status != 0) {
+		ofono_error("NetworkTimeChaged signal not filled, status: %d",
+			status);
+		return NULL;
+	}
+
+	return reply;
+}
+
+static GDBusMethodTable nettime_methods[] = {
+	{ "GetNetworkTime", "", "a{sv}", nettime_get_time },
+	{ }
+};
+
+static GDBusSignalTable nettime_signals[] = {
+	{ "NetworkTimeChanged", "a{sv}" },
+	{ }
+};
+
+static void nettime_register(struct ofono_nettime_context *context)
+{
+	DBusConnection *conn;
+	const char *path;
+
+	ofono_debug("Registering Network time for modem %s" ,
+		ofono_modem_get_path(context->modem));
+
+	conn = ofono_dbus_get_connection();
+
+	path = ofono_modem_get_path(context->modem);
+	if (path == NULL) {
+		ofono_error("No path for modem found");
+		return;
+	}
+
+	if (!g_dbus_register_interface(conn, path,
+				OFONO_NETWORK_TIME_INTERFACE,
+				nettime_methods,
+				nettime_signals,
+				NULL, context, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_NETWORK_TIME_INTERFACE);
+		return;
+	}
+
+	ofono_modem_add_interface(context->modem, OFONO_NETWORK_TIME_INTERFACE);
+}
+
+OFONO_PLUGIN_DEFINE(nettime, "Network Time Plugin",
+		VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+		nettime_init, nettime_exit)
+
-- 
1.7.1

[PATCH 2/3] plugins: Enabling nettime plugin in Makefile.am

[Antti Paila]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

---
 Makefile.am |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index a4c47e8..94dbaa1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -296,6 +296,9 @@ builtin_modules += example_nettime
 builtin_sources += examples/nettime.c
 endif
 
+builtin_modules += nettime
+builtin_sources += plugins/nettime.c
+
 builtin_modules += smart_messaging
 builtin_sources += plugins/smart-messaging.c
 
-- 
1.7.1

[PATCH 3/3] plugins: Test scripts for nettime plugin

[Antti Paila]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

---
 test/get-nettime  |   25 +++++++++++++++++++++++++
 test/test-nettime |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 0 deletions(-)
 create mode 100755 test/get-nettime
 create mode 100755 test/test-nettime

diff --git a/test/get-nettime b/test/get-nettime
new file mode 100755
index 0000000..a0fb1bc
--- /dev/null
+++ b/test/get-nettime
@@ -0,0 +1,25 @@
+#!/usr/bin/python
+
+import gobject
+import dbus
+import sys
+import time
+import dbus.mainloop.glib
+
+def time_changed( arg ) :
+	print "--time_changed callback called"
+	if "DateAndTime" in arg :
+		print "Time from mobile: %s" % arg["DateAndTime"]
+	print "DST: %d" % arg["DST"]
+	print "Timezone: %d" % arg["Timezone"]
+	print "Local time is: %s"  % time.asctime()
+
+bus = dbus.SystemBus()
+manager = dbus.Interface(bus.get_object('org.ofono','/'), 'org.ofono.Manager')
+path = manager.GetModems()[0][0]
+
+modem_proxy = bus.get_object('org.ofono', path)
+nettime_iface = dbus.Interface(modem_proxy, 'org.ofono.NetworkTime')
+answer = nettime_iface.GetNetworkTime()
+time_changed(answer)
+
diff --git a/test/test-nettime b/test/test-nettime
new file mode 100755
index 0000000..f78c9c6
--- /dev/null
+++ b/test/test-nettime
@@ -0,0 +1,46 @@
+#!/usr/bin/python
+
+import gobject
+import dbus
+import sys
+import time
+import dbus.mainloop.glib
+
+
+def time_changed( arg ) :
+	print "Time from mobile: %s" % arg["DateAndTime"]
+	print "DST: %d" % arg["DST"]
+	print "Timezone: %d" % arg["Timezone"]
+	print "Local time is: %s"  % time.asctime()
+
+
+dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+bus = dbus.SystemBus()
+manager = dbus.Interface(bus.get_object('org.ofono', '/'), 'org.ofono.Manager')
+modems = manager.GetModems()
+path = modems[0][0]
+
+print "Connecting modem %s..." % path
+modem = dbus.Interface(bus.get_object('org.ofono', path),
+						'org.ofono.Modem')
+
+print "Powering up modem %s..." % (path)
+modem.SetProperty("Powered", dbus.Boolean(1))
+
+print "Go Online modem %s..." % (path)
+modem.SetProperty("Online", dbus.Boolean(1))
+print "Go Offline modem %s..." % (path)
+modem.SetProperty("Online", dbus.Boolean(0))
+print "Go Online modem %s..." % (path)
+modem.SetProperty("Online", dbus.Boolean(1))
+
+nettime = dbus.Interface(bus.get_object('org.ofono', path),
+				'org.ofono.NetworkTime')
+nettime.connect_to_signal("NetworkTimeChanged", time_changed)
+
+mainloop = gobject.MainLoop()
+mainloop.run()
+
+
+
-- 
1.7.1

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

Hi Antti,

2010/12/3 Antti Paila <antti.paila@nokia.com>:
> +static GDBusMethodTable nettime_methods[] = {
> +       { "GetNetworkTime", "", "a{sv}", nettime_get_time },
> +       { }
> +};
> +
> +static GDBusSignalTable nettime_signals[] = {
> +       { "NetworkTimeChanged", "a{sv}" },
> +       { }
> +};

The only thing I would change here are the names. It's customary to
avoid repeating the interface name in the signal or method, but of
course the problem here is then what to call these. Maybe just Get()
and Changed()? ;-)

Oh, and you probably should have included a short description of what
this plugin is for (I forgot to mention this before).

The thinking behind this plugin is that timed in MeeGo wants to get
the information, and a D-Bus API is a fine way to accomplish this.

Cheers,
Aki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

Hi Aki,

> The only thing I would change here are the names. It's customary to
> avoid repeating the interface name in the signal or method, but of
> course the problem here is then what to call these. Maybe just Get()
> and Changed()? ;-)
> 
> Oh, and you probably should have included a short description of what
> this plugin is for (I forgot to mention this before).
> 
> The thinking behind this plugin is that timed in MeeGo wants to get
> the information, and a D-Bus API is a fine way to accomplish this.

where is this timed running? I prefer the plugin just send a D-Bus
message to the timed directly instead of a Get/Changed API kind of
thing.

Doing it this way looks highly complicated to me and doesn't serve any
purpose if timed and ofonod are both using the D-Bus system bus.

Regards

Marcel

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

Hi Marcel,

2010/12/4 Marcel Holtmann <marcel@holtmann.org>:
> where is this timed running? I prefer the plugin just send a D-Bus
> message to the timed directly instead of a Get/Changed API kind of
> thing.

AFAIK, timed is in system bus. However, I fail to see how you can make
this any simpler. In the end, one of the two entities has to implement
a service that the other one uses. There also needs to be some
synchronization for when the entities start up. A getter/signal API is
also what timed uses in Harmattan, but of course if you have a better
idea, I'm all ears.

Cheers,
Aki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

Hi Aki,

> > where is this timed running? I prefer the plugin just send a D-Bus
> > message to the timed directly instead of a Get/Changed API kind of
> > thing.
> 
> AFAIK, timed is in system bus. However, I fail to see how you can make
> this any simpler. In the end, one of the two entities has to implement
> a service that the other one uses. There also needs to be some
> synchronization for when the entities start up. A getter/signal API is
> also what timed uses in Harmattan, but of course if you have a better
> idea, I'm all ears.

is timed actually open source and I can have a look at it?

What is the advantage of oFono storing the time value from NITZ and
provide it over a getter/signal API. In that case we also need to store
a timestamp when we received the NITZ result. Otherwise the value has no
real meaning since we can not correlate it to anything.

That said, ofonod is not responsible for time tracking and so is not the
modem. This means that our time source is inaccurate by definition. Ans
so is our timestamp. And that makes the NITZ information useless that
retreive via a getter method. What do you wanna do with an inaccurate
time information?

Instead of trying to keep track of time inside oFono, I would propose
that we just send a method call with the currently received NITZ and
only when we receive them to a timed (if started).

Regards

Marcel

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

Hi Aki,

> > > where is this timed running? I prefer the plugin just send a D-Bus
> > > message to the timed directly instead of a Get/Changed API kind of
> > > thing.
> > 
> > AFAIK, timed is in system bus. However, I fail to see how you can make
> > this any simpler. In the end, one of the two entities has to implement
> > a service that the other one uses. There also needs to be some
> > synchronization for when the entities start up. A getter/signal API is
> > also what timed uses in Harmattan, but of course if you have a better
> > idea, I'm all ears.
> 
> is timed actually open source and I can have a look at it?

so what about this? Where is the timed respository? I really wanna have
a look at it and see what it is doing.

Regards

Marcel

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

Hi Marcel,

2010/12/8 Marcel Holtmann <marcel@holtmann.org>:
>> is timed actually open source and I can have a look at it?
>
> so what about this? Where is the timed respository? I really wanna have
> a look at it and see what it is doing.

Knock yourself out:

http://meego.gitorious.org/meego-middleware/timed

Cheers,
Aki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Saturday 04 December 2010 02:02:56 ext Marcel Holtmann, you wrote:
> Hi Aki,
> 
> > The only thing I would change here are the names. It's customary to
> > avoid repeating the interface name in the signal or method, but of
> > course the problem here is then what to call these. Maybe just Get()
> > and Changed()? ;-)
> > 
> > Oh, and you probably should have included a short description of what
> > this plugin is for (I forgot to mention this before).
> > 
> > The thinking behind this plugin is that timed in MeeGo wants to get
> > the information, and a D-Bus API is a fine way to accomplish this.
> 
> where is this timed running? I prefer the plugin just send a D-Bus
> message to the timed directly instead of a Get/Changed API kind of
> thing.

AFAIK, Get/Changed is the simplest functional way to avoid races, where say 
oFono would receive the time before timed can process it. The getter also 
enables implementing trivial a tool à la ntpdate.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

Hi Remi,

> > > The only thing I would change here are the names. It's customary to
> > > avoid repeating the interface name in the signal or method, but of
> > > course the problem here is then what to call these. Maybe just Get()
> > > and Changed()? ;-)
> > > 
> > > Oh, and you probably should have included a short description of what
> > > this plugin is for (I forgot to mention this before).
> > > 
> > > The thinking behind this plugin is that timed in MeeGo wants to get
> > > the information, and a D-Bus API is a fine way to accomplish this.
> > 
> > where is this timed running? I prefer the plugin just send a D-Bus
> > message to the timed directly instead of a Get/Changed API kind of
> > thing.
> 
> AFAIK, Get/Changed is the simplest functional way to avoid races, where say 
> oFono would receive the time before timed can process it. The getter also 
> enables implementing trivial a tool à la ntpdate.

then please answer my questions from my other email. If we receive the
time before timed (or any other time daemon) is running, who does ensure
the current timestamp of this received time and what it correlates to.

If no time daemon is running it is safer to just set this time as system
time right away and then let the time daemon adjust it later. But just
handing out a random time from a random time before is not helping. And
oFono is not suppose to track time and it corelation to it.

Regards

Marcel

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

On Tuesday 07 December 2010 01:29:34 ext Marcel Holtmann, you wrote:
> > > where is this timed running? I prefer the plugin just send a D-Bus
> > > message to the timed directly instead of a Get/Changed API kind of
> > > thing.
> > 
> > AFAIK, Get/Changed is the simplest functional way to avoid races, where
> > say oFono would receive the time before timed can process it. The getter
> > also enables implementing trivial a tool à la ntpdate.
> 
> then please answer my questions from my other email. If we receive the
> time before timed (or any other time daemon) is running, who does ensure
> the current timestamp of this received time and what it correlates to.

The answer is in the patch. It makes me wonder if you even read it? When we 
receive NITZ from the modem, we take a timestamp from the monotonic clock.

From there, we have plenty of options, mostly boiling down to:

(1) Give an estimate of the current wall time:
   nitz + (now(CLOCK_MONOTONIC) - timestamp)
This induces avoidable imprecision, but it is easiest to understand.

(2) Give both the NITZ time and the monotonic timestamp, and let the time 
daemon do all the maths. IIRC, that's what timed would like, although I find it 
needlessly complicated.

(3) Give an estimate of the time of day of the monotonic clock origin, which 
is to say the time of day of the system boot:
  nitz - timestamp.
The time daemon can then add the current monotonic time. This is a bit more 
precise than (1), and a bit simpler than (2), which is why I was suggesting 
that.

> If no time daemon is running it is safer to just set this time as system
> time right away and then let the time daemon adjust it later.

Either oFono maintains the time (may be a configuration option), or it does 
not. I would really hate to have oFono set the system time depending on the 
effect of the phase of moon on the ordering of the boot sequence.

> But just
> handing out a random time from a random time before is not helping. And
> oFono is not suppose to track time and it corelation to it.

Antti's patch returns an extrapolation of the _current_ wall time by adding 
the time since the NITZ was received to the NITZ. This is not at all random. 
Obviously, we need a working monotonic clock - but I think a mobile device 
without a working timer is about as useful as a brick.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

Hi Remi,

> > > > where is this timed running? I prefer the plugin just send a D-Bus
> > > > message to the timed directly instead of a Get/Changed API kind of
> > > > thing.
> > > 
> > > AFAIK, Get/Changed is the simplest functional way to avoid races, where
> > > say oFono would receive the time before timed can process it. The getter
> > > also enables implementing trivial a tool à la ntpdate.
> > 
> > then please answer my questions from my other email. If we receive the
> > time before timed (or any other time daemon) is running, who does ensure
> > the current timestamp of this received time and what it correlates to.
> 
> The answer is in the patch. It makes me wonder if you even read it? When we 
> receive NITZ from the modem, we take a timestamp from the monotonic clock.
> 
> From there, we have plenty of options, mostly boiling down to:
> 
> (1) Give an estimate of the current wall time:
>    nitz + (now(CLOCK_MONOTONIC) - timestamp)
> This induces avoidable imprecision, but it is easiest to understand.
> 
> (2) Give both the NITZ time and the monotonic timestamp, and let the time 
> daemon do all the maths. IIRC, that's what timed would like, although I find it 
> needlessly complicated.
> 
> (3) Give an estimate of the time of day of the monotonic clock origin, which 
> is to say the time of day of the system boot:
>   nitz - timestamp.
> The time daemon can then add the current monotonic time. This is a bit more 
> precise than (1), and a bit simpler than (2), which is why I was suggesting 
> that.
> 
> > If no time daemon is running it is safer to just set this time as system
> > time right away and then let the time daemon adjust it later.
> 
> Either oFono maintains the time (may be a configuration option), or it does 
> not. I would really hate to have oFono set the system time depending on the 
> effect of the phase of moon on the ordering of the boot sequence.
> 
> > But just
> > handing out a random time from a random time before is not helping. And
> > oFono is not suppose to track time and it corelation to it.
> 
> Antti's patch returns an extrapolation of the _current_ wall time by adding 
> the time since the NITZ was received to the NITZ. This is not at all random. 
> Obviously, we need a working monotonic clock - but I think a mobile device 
> without a working timer is about as useful as a brick.

I have seen a bunch of devices where we have not a proper clock value to
begin with. And in that case there is nothing really useful we can do
with it, except just set the system clock right away.

So the other problem with the timestamp and NITZ and the time daemon not
running is that we have no idea what happens in between. While this
might be short time and could potentially work, I still don't see this
as a good solution at all.

Right now, I still think that either we set the system clock right away
if no time daemon is running and if time daemon is running, then we send
a D-Bus method to that time daemon. And we can easily track this D-Bus
name owner notifications.

Regards

Marcel

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Antti Paila]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

Hi Marcel,

> I have seen a bunch of devices where we have not a proper clock value to
> begin with. And in that case there is nothing really useful we can do
> with it, except just set the system clock right away.

We are not using any absolute time value from any clock, but instead we
use relative elapsed time from monotonic clock. The reference point is
the monotonic clock time, when the network time indication arrived to
modem. I believe every device that takes ofono into use have a monotonic
time source.

> So the other problem with the timestamp and NITZ and the time daemon not
> running is that we have no idea what happens in between. While this
> might be short time and could potentially work, I still don't see this
> as a good solution at all.

That is why the nettime plugin implements both notification signal and a
polling method call. If timed is running it will receive the DBUS
notification about the network time. Otherwise timed can poll the time
from ofono as it becomes active. One should also notice that the NITZ
feature is optional both from terminal and network point of view, so
anything too fatal should not happen even if timed is not up when
nettime indication is sent. 

> Right now, I still think that either we set the system clock right away
> if no time daemon is running and if time daemon is running, then we send
> a D-Bus method to that time daemon. And we can easily track this D-Bus
> name owner notifications.

Architecturally I prefer that the system time is altered by one entity
alone, being timed. As we can have many modems active simultaneously, I
would not like if they all are allowed to directly alter the system
time. Timed should have a way to decide what is the proper time.

-Antti 

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Tuesday 07 December 2010 10:58:37 ext Marcel Holtmann, you wrote:
> I have seen a bunch of devices where we have not a proper clock value to
> begin with. And in that case there is nothing really useful we can do
> with it, except just set the system clock right away.

Are you kidding me? You claim devices have no working hardware timer? And you 
use them? Come on.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 7153 bytes --]

Nitpicking...

On Friday 03 December 2010 11:47:26 ext Antti Paila, you wrote:
> diff --git a/plugins/nettime.c b/plugins/nettime.c
> new file mode 100644
> index 0000000..0f99bb1
> --- /dev/null
> +++ b/plugins/nettime.c
> @@ -0,0 +1,293 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2010  Nokia Corporation and/or its
> subsidiary(-ies). + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
> USA + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +#include <glib.h>
> +
> +#define OFONO_API_SUBJECT_TO_CHANGE
> +#include <ofono/plugin.h>
> +#include <ofono/log.h>
> +#include <ofono/nettime.h>
> +#include <ofono/types.h>
> +#include <gdbus.h>
> +#include "ofono.h"
> +
> +#include "common.h"
> +
> +#define MAX_TIME_STR_LEN 128
> +#define TIME_FORMAT "%FT%TZ"
> +
> +
> +struct nettime_data {
> +	struct ofono_network_time nw_time;
> +	time_t received;
> +};
> +
> +static void nettime_register(struct ofono_nettime_context *);
> +
> +static gboolean encode_time_format(struct ofono_network_time *time,
> +				 struct tm *tm)
> +{
> +	if (time->year < 0)
> +		return FALSE;
> +
> +	tm->tm_year = time->year - 1900;
> +	tm->tm_mon = time->mon - 1;
> +	tm->tm_mday = time->mday;
> +	tm->tm_hour = time->hour;
> +	tm->tm_min = time->min;
> +	tm->tm_sec = time->sec;
> +	tm->tm_gmtoff = time->utcoff;
> +	tm->tm_isdst = time->dst;
> +
> +	return TRUE;
> +}
> +
> +static time_t get_monotonic_time()
> +{
> +	struct timespec ts;
> +	clock_gettime(CLOCK_MONOTONIC, &ts);
> +	return ts.tv_sec;
> +}
> +
> +static struct tm *refresh_nw_time(struct tm *nw_time,
> +				time_t received)
> +{
> +	time_t now, new_nw_time;
> +
> +	now = get_monotonic_time();
> +	new_nw_time = timegm(nw_time) + now - received;
> +	return gmtime(&new_nw_time);
> +}

Brrr, the result is somewhere in static data. This can cause quite confusing 
behaviour if/when the code is changed. I would much rather use gmtime_r().

That being said, wouldn't it be far simpler to return a second timestamp 
instead of a broken-down time structure over D-Bus? Eventually, timed/whatever 
will want a timestamp anyway to set the system clock or whatever else it wants 
to do with the NITZ.

> +static int fill_time_changed_signal(DBusMessage *signal,
> +		struct nettime_data *ntd)
> +{
> +	DBusMessageIter iter, iter_array;
> +	int time_available;
> +	char buf[MAX_TIME_STR_LEN];
> +	struct tm time_tm, *new_time_tm;
> +	const char *str = buf;
> +
> +	dbus_message_iter_init_append(signal, &iter);
> +	dbus_message_iter_open_container(&iter,
> +					DBUS_TYPE_ARRAY,
> +					"{sv}",
> +					&iter_array);
> +
> +	time_available = encode_time_format(&ntd->nw_time, &time_tm);
> +	if (time_available != 0) {
> +		new_time_tm = refresh_nw_time(&time_tm, ntd->received);
> +		strftime(buf, MAX_TIME_STR_LEN, TIME_FORMAT, new_time_tm);
> +
> +		ofono_dbus_dict_append(&iter_array,
> +					"DateAndTime",
> +					DBUS_TYPE_STRING,
> +					&str);
> +	}
> +
> +	ofono_dbus_dict_append(&iter_array,
> +				"Timezone",
> +				DBUS_TYPE_INT32,
> +				&ntd->nw_time.utcoff);
> +	ofono_dbus_dict_append(&iter_array,
> +				"DST",
> +				DBUS_TYPE_UINT32,
> +				&ntd->nw_time.dst);
> +	dbus_message_iter_close_container(&iter, &iter_array);
> +	return 0;
> +}

Are the time zone offset and the DST always available, but the current time 
not? This seems odd.

Passing the time over D-Bus may cause extra drift/imprecision than we already 
have. I wonder if it would make sense to provide not just the current time but 
both:
- the current time for trivial programs, and
- the system boot time (i.e. nw_time - received) for smarter programs;
  this value cannot drift.

> +static DBusMessage *create_time_changed_signal(
> +			struct ofono_nettime_context *context)
> +{
> +	DBusMessage *signal;
> +	struct nettime_data *ntd = context->data;
> +	const char *path = ofono_modem_get_path(context->modem);
> +
> +	if (path == NULL) {
> +		ofono_error("Fetching path for modem failed");
> +		return NULL;
> +	}
> +
> +	signal = dbus_message_new_signal(path, OFONO_NETWORK_TIME_INTERFACE,
> +					"NetworkTimeChanged");
> +	fill_time_changed_signal(signal, ntd);
> +
> +	return signal;
> +}
> +
> +static void init_time(struct ofono_nettime_context *context)
> +{
> +	struct nettime_data *nettime_data;
> +	context->data = g_try_new0(struct nettime_data, 1);

Hmm, if you don't check the result, I suspect you should g_new0() instead.

> +	nettime_data = context->data;
> +	nettime_data->nw_time.year = -1;
> +}
> +
> +static int nettime_probe(struct ofono_nettime_context *context)
> +{
> +	ofono_debug("Network Time Probe for modem: %p", context->modem);
> +
> +	init_time(context);
> +
> +	nettime_register(context);
> +	return 0;
> +}
> +
> +static void nettime_remove(struct ofono_nettime_context *context)
> +{
> +	DBusConnection *conn;
> +	const char *path;
> +
> +	ofono_debug("Network Time Remove for modem: %p", context->modem);
> +
> +	g_free(context->data);
> +
> +	conn = ofono_dbus_get_connection();
> +	path = ofono_modem_get_path(context->modem);
> +
> +	if (!g_dbus_unregister_interface(conn,
> +					path,
> +					OFONO_NETWORK_TIME_INTERFACE))
> +		ofono_error("Could not unregister %s interface",
> +				OFONO_NETWORK_TIME_INTERFACE);
> +
> +	ofono_modem_remove_interface(context->modem,
> +			OFONO_NETWORK_TIME_INTERFACE);
> +}
> +
> +static void nettime_info_received(struct ofono_nettime_context *context,
> +				struct ofono_network_time *info)
> +{
> +	DBusMessage *nt_signal;
> +	struct nettime_data *ntd = context->data;
> +
> +	if (info == NULL)
> +		return;
> +
> +	ofono_debug("Received a network time notification on modem: %p",
> +			context->modem);
> +
> +	ntd->nw_time = *info;
> +	ntd->received = get_monotonic_time();

That works. But I would do the gmtime() conversion here, and store the result 
once and for all, instead of copying the raw data and recompute the conversion 
every time.

> +	nt_signal = create_time_changed_signal(context);
> +	if (nt_signal == NULL) {
> +		ofono_error("Failed to create NetworkTimeChanged signal");
> +		return;
> +	}
> +
> +	g_dbus_send_message(ofono_dbus_get_connection(), nt_signal);
> +}

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 6969 bytes --]

Hi Rémi,

2010/12/6 Rémi Denis-Courmont <remi.denis-courmont@nokia.com>:
>> +static struct tm *refresh_nw_time(struct tm *nw_time,
>> +                             time_t received)
>> +{
>> +     time_t now, new_nw_time;
>> +
>> +     now = get_monotonic_time();
>> +     new_nw_time = timegm(nw_time) + now - received;
>> +     return gmtime(&new_nw_time);
>> +}
>
> Brrr, the result is somewhere in static data. This can cause quite confusing
> behaviour if/when the code is changed. I would much rather use gmtime_r().

I suppose that's a good point.

> That being said, wouldn't it be far simpler to return a second timestamp
> instead of a broken-down time structure over D-Bus? Eventually, timed/whatever
> will want a timestamp anyway to set the system clock or whatever else it wants
> to do with the NITZ.
>
>> +static int fill_time_changed_signal(DBusMessage *signal,
>> +             struct nettime_data *ntd)
>> +{
>> +     DBusMessageIter iter, iter_array;
>> +     int time_available;
>> +     char buf[MAX_TIME_STR_LEN];
>> +     struct tm time_tm, *new_time_tm;
>> +     const char *str = buf;
>> +
>> +     dbus_message_iter_init_append(signal, &iter);
>> +     dbus_message_iter_open_container(&iter,
>> +                                     DBUS_TYPE_ARRAY,
>> +                                     "{sv}",
>> +                                     &iter_array);
>> +
>> +     time_available = encode_time_format(&ntd->nw_time, &time_tm);
>> +     if (time_available != 0) {
>> +             new_time_tm = refresh_nw_time(&time_tm, ntd->received);
>> +             strftime(buf, MAX_TIME_STR_LEN, TIME_FORMAT, new_time_tm);
>> +
>> +             ofono_dbus_dict_append(&iter_array,
>> +                                     "DateAndTime",
>> +                                     DBUS_TYPE_STRING,
>> +                                     &str);
>> +     }
>> +
>> +     ofono_dbus_dict_append(&iter_array,
>> +                             "Timezone",
>> +                             DBUS_TYPE_INT32,
>> +                             &ntd->nw_time.utcoff);
>> +     ofono_dbus_dict_append(&iter_array,
>> +                             "DST",
>> +                             DBUS_TYPE_UINT32,
>> +                             &ntd->nw_time.dst);
>> +     dbus_message_iter_close_container(&iter, &iter_array);
>> +     return 0;
>> +}
>
> Are the time zone offset and the DST always available, but the current time
> not? This seems odd.

Tell me about it, but this is how the spec works. Operators have the
flexibility to send just the timezone, the timezone and DST, or the
works.

> Passing the time over D-Bus may cause extra drift/imprecision than we already
> have. I wonder if it would make sense to provide not just the current time but
> both:
> - the current time for trivial programs, and
> - the system boot time (i.e. nw_time - received) for smarter programs;
>  this value cannot drift.

I remember we had similar argument in the past with the timed folks,
which actually resulted in a much more convoluted API... I'm still not
convinced time drift is a problem, since NITZ itself is accurate to
the second.

However, thinking about this, I actually don't see any value in
providing a human readable time over D-Bus. No application should
directly ask oFono for time, this is why timed is there to begin with.

So sending a plain time_t over D-Bus is actually sufficient. This
leaves us with having to send timestamps over D-Bus, which is ugly.
Can you explain how you see clock drift a problem here exactly?

>> +static DBusMessage *create_time_changed_signal(
>> +                     struct ofono_nettime_context *context)
>> +{
>> +     DBusMessage *signal;
>> +     struct nettime_data *ntd = context->data;
>> +     const char *path = ofono_modem_get_path(context->modem);
>> +
>> +     if (path == NULL) {
>> +             ofono_error("Fetching path for modem failed");
>> +             return NULL;
>> +     }
>> +
>> +     signal = dbus_message_new_signal(path, OFONO_NETWORK_TIME_INTERFACE,
>> +                                     "NetworkTimeChanged");
>> +     fill_time_changed_signal(signal, ntd);
>> +
>> +     return signal;
>> +}
>> +
>> +static void init_time(struct ofono_nettime_context *context)
>> +{
>> +     struct nettime_data *nettime_data;
>> +     context->data = g_try_new0(struct nettime_data, 1);
>
> Hmm, if you don't check the result, I suspect you should g_new0() instead.

Good point.

>> +     nettime_data = context->data;
>> +     nettime_data->nw_time.year = -1;
>> +}
>> +
>> +static int nettime_probe(struct ofono_nettime_context *context)
>> +{
>> +     ofono_debug("Network Time Probe for modem: %p", context->modem);
>> +
>> +     init_time(context);
>> +
>> +     nettime_register(context);
>> +     return 0;
>> +}
>> +
>> +static void nettime_remove(struct ofono_nettime_context *context)
>> +{
>> +     DBusConnection *conn;
>> +     const char *path;
>> +
>> +     ofono_debug("Network Time Remove for modem: %p", context->modem);
>> +
>> +     g_free(context->data);
>> +
>> +     conn = ofono_dbus_get_connection();
>> +     path = ofono_modem_get_path(context->modem);
>> +
>> +     if (!g_dbus_unregister_interface(conn,
>> +                                     path,
>> +                                     OFONO_NETWORK_TIME_INTERFACE))
>> +             ofono_error("Could not unregister %s interface",
>> +                             OFONO_NETWORK_TIME_INTERFACE);
>> +
>> +     ofono_modem_remove_interface(context->modem,
>> +                     OFONO_NETWORK_TIME_INTERFACE);
>> +}
>> +
>> +static void nettime_info_received(struct ofono_nettime_context *context,
>> +                             struct ofono_network_time *info)
>> +{
>> +     DBusMessage *nt_signal;
>> +     struct nettime_data *ntd = context->data;
>> +
>> +     if (info == NULL)
>> +             return;
>> +
>> +     ofono_debug("Received a network time notification on modem: %p",
>> +                     context->modem);
>> +
>> +     ntd->nw_time = *info;
>> +     ntd->received = get_monotonic_time();
>
> That works. But I would do the gmtime() conversion here, and store the result
> once and for all, instead of copying the raw data and recompute the conversion
> every time.

Sounds good.

Also, should use memcpy() for the info above.

Cheers,
Aki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Antti Paila]

[-- Attachment #1: Type: text/plain, Size: 7599 bytes --]


> Hi Rémi,
> 
> 2010/12/6 Rémi Denis-Courmont <remi.denis-courmont@nokia.com>:
> >> +static struct tm *refresh_nw_time(struct tm *nw_time,
> >> +                             time_t received)
> >> +{
> >> +     time_t now, new_nw_time;
> >> +
> >> +     now = get_monotonic_time();
> >> +     new_nw_time = timegm(nw_time) + now - received;
> >> +     return gmtime(&new_nw_time);
> >> +}
> >
> > Brrr, the result is somewhere in static data. This can cause quite confusing
> > behaviour if/when the code is changed. I would much rather use gmtime_r().
> 
> I suppose that's a good point.
> 
> > That being said, wouldn't it be far simpler to return a second timestamp
> > instead of a broken-down time structure over D-Bus? Eventually, timed/whatever
> > will want a timestamp anyway to set the system clock or whatever else it wants
> > to do with the NITZ.
> >
> >> +static int fill_time_changed_signal(DBusMessage *signal,
> >> +             struct nettime_data *ntd)
> >> +{
> >> +     DBusMessageIter iter, iter_array;
> >> +     int time_available;
> >> +     char buf[MAX_TIME_STR_LEN];
> >> +     struct tm time_tm, *new_time_tm;
> >> +     const char *str = buf;
> >> +
> >> +     dbus_message_iter_init_append(signal, &iter);
> >> +     dbus_message_iter_open_container(&iter,
> >> +                                     DBUS_TYPE_ARRAY,
> >> +                                     "{sv}",
> >> +                                     &iter_array);
> >> +
> >> +     time_available = encode_time_format(&ntd->nw_time, &time_tm);
> >> +     if (time_available != 0) {
> >> +             new_time_tm = refresh_nw_time(&time_tm, ntd->received);
> >> +             strftime(buf, MAX_TIME_STR_LEN, TIME_FORMAT, new_time_tm);
> >> +
> >> +             ofono_dbus_dict_append(&iter_array,
> >> +                                     "DateAndTime",
> >> +                                     DBUS_TYPE_STRING,
> >> +                                     &str);
> >> +     }
> >> +
> >> +     ofono_dbus_dict_append(&iter_array,
> >> +                             "Timezone",
> >> +                             DBUS_TYPE_INT32,
> >> +                             &ntd->nw_time.utcoff);
> >> +     ofono_dbus_dict_append(&iter_array,
> >> +                             "DST",
> >> +                             DBUS_TYPE_UINT32,
> >> +                             &ntd->nw_time.dst);
> >> +     dbus_message_iter_close_container(&iter, &iter_array);
> >> +     return 0;
> >> +}
> >
> > Are the time zone offset and the DST always available, but the current time
> > not? This seems odd.
> 
> Tell me about it, but this is how the spec works. Operators have the
> flexibility to send just the timezone, the timezone and DST, or the
> works.

I suppose Remi means that if the DST and time zone are optional we should at least
verify that those fields are part of the MM/GMM message. Specs 24008
states that network may include the following fields to MM information
message: Local time zone, Universal time and local time zone, Network
Daylight Saving Time.

> > Passing the time over D-Bus may cause extra drift/imprecision than we already
> > have. I wonder if it would make sense to provide not just the current time but
> > both:
> > - the current time for trivial programs, and
> > - the system boot time (i.e. nw_time - received) for smarter programs;
> >  this value cannot drift.
> 
> I remember we had similar argument in the past with the timed folks,
> which actually resulted in a much more convoluted API... I'm still not
> convinced time drift is a problem, since NITZ itself is accurate to
> the second.
> 
> However, thinking about this, I actually don't see any value in
> providing a human readable time over D-Bus. No application should
> directly ask oFono for time, this is why timed is there to begin with.
> 
> So sending a plain time_t over D-Bus is actually sufficient. This
> leaves us with having to send timestamps over D-Bus, which is ugly.
> Can you explain how you see clock drift a problem here exactly?

I wonder what is drifting since the reception time is updated only when
network indicates new time. While polling time, the correction is
calculated with respect to this reception time. The exactness of the
network time is questionable anyway since the network delivers the time
which may have been when the information was sent. Accuracy is of the
order of minutes rather that seconds.

> 
> >> +static DBusMessage *create_time_changed_signal(
> >> +                     struct ofono_nettime_context *context)
> >> +{
> >> +     DBusMessage *signal;
> >> +     struct nettime_data *ntd = context->data;
> >> +     const char *path = ofono_modem_get_path(context->modem);
> >> +
> >> +     if (path == NULL) {
> >> +             ofono_error("Fetching path for modem failed");
> >> +             return NULL;
> >> +     }
> >> +
> >> +     signal = dbus_message_new_signal(path, OFONO_NETWORK_TIME_INTERFACE,
> >> +                                     "NetworkTimeChanged");
> >> +     fill_time_changed_signal(signal, ntd);
> >> +
> >> +     return signal;
> >> +}
> >> +
> >> +static void init_time(struct ofono_nettime_context *context)
> >> +{
> >> +     struct nettime_data *nettime_data;
> >> +     context->data = g_try_new0(struct nettime_data, 1);
> >
> > Hmm, if you don't check the result, I suspect you should g_new0() instead.
> 
> Good point.
> 
> >> +     nettime_data = context->data;
> >> +     nettime_data->nw_time.year = -1;
> >> +}
> >> +
> >> +static int nettime_probe(struct ofono_nettime_context *context)
> >> +{
> >> +     ofono_debug("Network Time Probe for modem: %p", context->modem);
> >> +
> >> +     init_time(context);
> >> +
> >> +     nettime_register(context);
> >> +     return 0;
> >> +}
> >> +
> >> +static void nettime_remove(struct ofono_nettime_context *context)
> >> +{
> >> +     DBusConnection *conn;
> >> +     const char *path;
> >> +
> >> +     ofono_debug("Network Time Remove for modem: %p", context->modem);
> >> +
> >> +     g_free(context->data);
> >> +
> >> +     conn = ofono_dbus_get_connection();
> >> +     path = ofono_modem_get_path(context->modem);
> >> +
> >> +     if (!g_dbus_unregister_interface(conn,
> >> +                                     path,
> >> +                                     OFONO_NETWORK_TIME_INTERFACE))
> >> +             ofono_error("Could not unregister %s interface",
> >> +                             OFONO_NETWORK_TIME_INTERFACE);
> >> +
> >> +     ofono_modem_remove_interface(context->modem,
> >> +                     OFONO_NETWORK_TIME_INTERFACE);
> >> +}
> >> +
> >> +static void nettime_info_received(struct ofono_nettime_context *context,
> >> +                             struct ofono_network_time *info)
> >> +{
> >> +     DBusMessage *nt_signal;
> >> +     struct nettime_data *ntd = context->data;
> >> +
> >> +     if (info == NULL)
> >> +             return;
> >> +
> >> +     ofono_debug("Received a network time notification on modem: %p",
> >> +                     context->modem);
> >> +
> >> +     ntd->nw_time = *info;
> >> +     ntd->received = get_monotonic_time();
> >
> > That works. But I would do the gmtime() conversion here, and store the result
> > once and for all, instead of copying the raw data and recompute the conversion
> > every time.
> 
> Sounds good.
> 
> Also, should use memcpy() for the info above.

Thanks for the comments.

-Antti

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On Tuesday 07 December 2010 09:51:57 ext Aki Niemi, you wrote:
> > Passing the time over D-Bus may cause extra drift/imprecision than we
> > already have. I wonder if it would make sense to provide not just the
> > current time but both:
> > - the current time for trivial programs, and
> > - the system boot time (i.e. nw_time - received) for smarter programs;
> >  this value cannot drift.
> 
> I remember we had similar argument in the past with the timed folks,
> which actually resulted in a much more convoluted API... I'm still not
> convinced time drift is a problem, since NITZ itself is accurate to
> the second.

> However, thinking about this, I actually don't see any value in
> providing a human readable time over D-Bus. No application should
> directly ask oFono for time, this is why timed is there to begin with.
> 
> So sending a plain time_t over D-Bus is actually sufficient. This
> leaves us with having to send timestamps over D-Bus, which is ugly.
> Can you explain how you see clock drift a problem here exactly?

More than one second delay over D-Bus is not unheard of, especially on mobile 
devices. I don't see the point of sending _both_ the reception timestamp and 
the original NITZ value, but I do see the point in sending a value who 
transmission is not time-sensitive.

And even if drift were not a problem, it's best to have an API that does 
fundamentally not suffer from the problem. Otherwise, you will need to justify 
every second month why drift is a negligible issue.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[Aki Niemi]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

Hi Rémi,

On Tue, 2010-12-07 at 18:12 +0200, ext Rémi Denis-Courmont wrote:
> More than one second delay over D-Bus is not unheard of, especially on mobile 
> devices. I don't see the point of sending _both_ the reception timestamp and 
> the original NITZ value, but I do see the point in sending a value who 
> transmission is not time-sensitive.

Over a second round trip delay? We're running the D-Bus daemon behind a
GPRS link these days are we? ;)

> And even if drift were not a problem, it's best to have an API that does 
> fundamentally not suffer from the problem. Otherwise, you will need to justify 
> every second month why drift is a negligible issue.

It's still a constant imprecision caused by D-Bus latency (round-trip in
the case timed used the getter), so I wouldn't call it "drift".

That said, I get your point, and providing a value estimating the device
boot-time, rooted in the system monotonic clock is indeed a good
approach.

+1

Cheers,
Aki

Re: [PATCH 1/3] plugins: Implementation of Network Time plugin

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Wednesday 08 December 2010 10:13:45 ext Aki Niemi, you wrote:
> Hi Rémi,
> 
> On Tue, 2010-12-07 at 18:12 +0200, ext Rémi Denis-Courmont wrote:
> > More than one second delay over D-Bus is not unheard of, especially on
> > mobile devices. I don't see the point of sending _both_ the reception
> > timestamp and the original NITZ value, but I do see the point in sending
> > a value who transmission is not time-sensitive.
> 
> Over a second round trip delay? We're running the D-Bus daemon behind a
> GPRS link these days are we? ;)

No, we're running D-Bus on a busy device and a kernel with a suboptimal I/O 
scheduler (for our use cases). Not getting scheduled for several seconds is 
entirely possible.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki