[PATCH 0/4 v4] Network Time Plugin

[PATCH 0/4 v4] Network Time Plugin

[Antti Paila]

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

 This series of patches introduces the network time part of
 the NITZ feature as outlined in 3GPP spec 22.042.
 The plugin is for delivering network indicated time information
 to timed process which is responsible for maintaining the system
 time. The delivery is achieved by timed implementing an interface
 with a method that is called by the nettime plugin with time related
 info as a parameter of the method.

Antti Paila (4):
  nettime: Network time plugin implementation
  nettime: Makefile.am modification
  nettime: Documentation
  nettime: Mock Timed for testing

 Makefile.am              |    9 +-
 doc/network-time-api.txt |   36 +++++
 plugins/nettime.c        |  326 ++++++++++++++++++++++++++++++++++++++++++++++
 test/test-nettime        |   35 +++++
 4 files changed, 404 insertions(+), 2 deletions(-)
 create mode 100644 doc/network-time-api.txt
 create mode 100644 plugins/nettime.c
 create mode 100755 test/test-nettime

[PATCH 1/4] nettime: Network time plugin implementation

[Antti Paila]

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

---
 plugins/nettime.c |  326 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 326 insertions(+), 0 deletions(-)
 create mode 100644 plugins/nettime.c

diff --git a/plugins/nettime.c b/plugins/nettime.c
new file mode 100644
index 0000000..894dce7
--- /dev/null
+++ b/plugins/nettime.c
@@ -0,0 +1,326 @@
+/*
+ *
+ *  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 <stdlib.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 TIMED_PATH "/com/meego/time"
+#define TIMED_SERVICE "com.meego.time"
+
+struct nt_data {
+	gboolean time_available;
+	gboolean time_pending;
+	time_t nw_time_utc;
+	time_t received;
+	int dst;
+	int time_zone;
+	const char *mcc;
+	const char *mnc;
+	unsigned int timed_watch;
+	gboolean timed_present;
+	struct ofono_netreg *netreg;
+	unsigned int netreg_st_watch;
+
+};
+
+static void nettime_register(struct ofono_nettime_context *);
+
+static gboolean encode_time_format(struct ofono_network_time *time,
+				 struct tm *tm)
+{
+
+	tm->tm_gmtoff = time->utcoff;
+	tm->tm_isdst = time->dst;
+
+	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;
+
+	return TRUE;
+}
+
+static time_t get_monotonic_time()
+{
+	struct timespec ts;
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	return ts.tv_sec;
+}
+
+static int fill_time_notification(DBusMessage *msg,
+		struct nt_data *ntd)
+{
+	DBusMessageIter iter, iter_array;
+	int64_t utc;
+
+	dbus_message_iter_init_append(msg, &iter);
+	dbus_message_iter_open_container(&iter,
+					DBUS_TYPE_ARRAY,
+					"{sv}",
+					&iter_array);
+
+	if (ntd->time_pending) {
+		if (ntd->time_available) {
+			utc = ntd->nw_time_utc - ntd->received;
+			ofono_dbus_dict_append(&iter_array,
+						"UTC",
+						DBUS_TYPE_INT64,
+						&utc);
+		}
+
+		ofono_dbus_dict_append(&iter_array,
+					"DST",
+					DBUS_TYPE_INT32,
+					&ntd->dst);
+		ofono_dbus_dict_append(&iter_array,
+					"Timezone",
+					DBUS_TYPE_INT32,
+					&ntd->time_zone);
+	}
+
+	ofono_dbus_dict_append(&iter_array,
+			"MobileCountryCode",
+			DBUS_TYPE_STRING,
+			&ntd->mcc);
+	ofono_dbus_dict_append(&iter_array,
+			"MobileNetworkCode",
+			DBUS_TYPE_STRING,
+			&ntd->mnc);
+
+	dbus_message_iter_close_container(&iter, &iter_array);
+	return 0;
+}
+
+static DBusMessage *create_time_notification(
+			struct ofono_nettime_context *context)
+{
+	DBusMessage *message;
+	struct nt_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;
+	}
+
+	message = dbus_message_new_method_call(TIMED_SERVICE, TIMED_PATH,
+					"com.meego.NetworkTime", "Notify");
+	if (message == NULL)
+		return NULL;
+
+	dbus_message_set_no_reply(message, TRUE);
+	fill_time_notification(message, ntd);
+
+	return message;
+}
+
+static void init_time(struct ofono_nettime_context *context)
+{
+	struct nt_data *nt_data = g_new0(struct nt_data, 1);
+
+	nt_data->time_available = FALSE;
+	nt_data->time_pending = FALSE;
+	nt_data->dst = 0;
+	nt_data->time_zone = 0;
+
+	context->data = nt_data;
+}
+
+static int nettime_probe(struct ofono_nettime_context *context)
+{
+	DBG("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)
+{
+	struct nt_data *ntd = context->data;
+
+	DBG("Network Time Remove for modem: %p", context->modem);
+	g_dbus_remove_watch(ofono_dbus_get_connection(),
+				ntd->timed_watch);
+	g_free(ntd);
+}
+
+static void notify(int status, int lac, int ci, int tech, const char *mcc,
+			const char *mnc, void *data)
+{
+	struct ofono_nettime_context *context = data;
+	struct nt_data *ntd = context->data;
+	DBusMessage *message;
+
+	if (mcc == NULL || mnc == NULL)
+		return;
+
+	ntd->mcc = mcc;
+	ntd->mnc = mnc;
+
+	if (ntd->timed_present == FALSE) {
+		DBG("Timed not present. Caching time info");
+		return;
+	}
+
+	message = create_time_notification(context);
+	if (message == NULL) {
+		ofono_error("Failed to create Notification message");
+		return;
+	}
+
+	g_dbus_send_message(ofono_dbus_get_connection(), message);
+	ntd->time_pending = FALSE;
+}
+
+static void nr_st_watch_destroy(void *data)
+{
+	struct ofono_nettime_context *context = data;
+	struct nt_data *ntd = context->data;
+	DBG("");
+
+	ntd->netreg_st_watch = 0;
+}
+
+static void nettime_info_received(struct ofono_nettime_context *context,
+				struct ofono_network_time *info)
+{
+	struct tm t;
+	struct nt_data *ntd = context->data;
+	const char *mcc, *mnc;
+
+	DBG("Network time notification received, modem: %p",
+			context->modem);
+
+	if (info == NULL)
+		return;
+
+	ntd->received = get_monotonic_time();
+	ntd->time_pending = TRUE;
+
+	ntd->time_available = encode_time_format(info, &t);
+	if (ntd->time_available == TRUE)
+		ntd->nw_time_utc = timegm(&t);
+
+	ntd->dst = info->dst;
+	ntd->time_zone = info->utcoff;
+
+	ntd->netreg = __ofono_atom_get_data(__ofono_modem_find_atom(
+				context->modem, OFONO_ATOM_TYPE_NETREG));
+
+	mcc = ofono_netreg_get_mcc(ntd->netreg);
+	mnc = ofono_netreg_get_mnc(ntd->netreg);
+	if ((mcc == NULL) || (mnc == NULL)) {
+		DBG("Mobile country/network code missing");
+
+		if (ntd->netreg_st_watch != 0)
+			return;
+
+		ntd->netreg_st_watch = __ofono_netreg_add_status_watch(
+					ntd->netreg, notify,
+					context, nr_st_watch_destroy);
+	} else {
+		notify(0, 0, 0, 0, mcc, mnc, context);
+	}
+
+}
+
+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 void timed_connect(DBusConnection *connection, void *user_data)
+{
+	struct ofono_nettime_context *context = user_data;
+	struct nt_data *ntd = context->data;
+	const char *mcc, *mnc;
+
+	DBG("");
+
+	ntd->timed_present = TRUE;
+	mcc = ofono_netreg_get_mcc(ntd->netreg);
+	mnc = ofono_netreg_get_mnc(ntd->netreg);
+
+	notify(0, 0, 0, 0, mcc, mnc, context);
+}
+
+static void timed_disconnect(DBusConnection *connection, void *user_data)
+{
+	struct ofono_nettime_context *context = user_data;
+	struct nt_data *ntd = context->data;
+
+	DBG("");
+
+	ntd->timed_present = FALSE;
+}
+
+static void nettime_register(struct ofono_nettime_context *context)
+{
+	DBusConnection *conn;
+	struct nt_data *ntd = context->data;
+
+	DBG("Registering Network time for modem %s" ,
+		ofono_modem_get_path(context->modem));
+
+	conn = ofono_dbus_get_connection();
+
+	ntd->timed_watch = g_dbus_add_service_watch(conn, TIMED_SERVICE,
+					timed_connect, timed_disconnect,
+					context, NULL);
+}
+
+OFONO_PLUGIN_DEFINE(nettime, "Network Time Plugin",
+		VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+		nettime_init, nettime_exit)
+
-- 
1.7.1

[PATCH 2/4] nettime: Makefile.am modification

[Antti Paila]

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

---
 Makefile.am |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index a38fcb9..2d54025 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -337,6 +337,9 @@ builtin_modules += example_provision
 builtin_sources += examples/provision.c
 endif
 
+builtin_modules += nettime
+builtin_sources += plugins/nettime.c
+
 builtin_modules += smart_messaging
 builtin_sources += plugins/smart-messaging.c
 
@@ -404,7 +407,8 @@ doc_files = doc/overview.txt doc/ofono-paper.txt doc/release-faq.txt \
 			doc/phonebook-api.txt doc/radio-settings-api.txt \
 			doc/sim-api.txt doc/stk-api.txt \
 			doc/audio-settings-api.txt doc/text-telephony-api.txt \
-			doc/calypso-modem.txt doc/message-api.txt
+			doc/calypso-modem.txt doc/message-api.txt \
+			doc/network-time-api.txt
 
 
 test_scripts = test/backtrace \
@@ -478,7 +482,8 @@ test_scripts = test/backtrace \
 		test/cdma-dial-number \
 		test/cdma-hangup \
 		test/disable-call-forwarding \
-		test/list-messages
+		test/list-messages \
+		test/test-nettime
 
 if TEST
 testdir = $(pkglibdir)/test
-- 
1.7.1

[PATCH 3/4] nettime: Documentation

[Antti Paila]

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

---
 doc/network-time-api.txt |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100644 doc/network-time-api.txt

diff --git a/doc/network-time-api.txt b/doc/network-time-api.txt
new file mode 100644
index 0000000..9133a73
--- /dev/null
+++ b/doc/network-time-api.txt
@@ -0,0 +1,36 @@
+Network time hierarchy
+======================
+
+Interface	com.meego.NetworkTime
+Object path	[variable]
+
+Methods		void Notify(dict info)
+
+			Notifies the service of current time and date
+			as notified by the cellular network.  The info
+			argument contains a dictionary with the
+			following possible keys:
+
+			int64 UTC [optional]
+				Network time in seconds from epoch
+				normalized to device boot time.
+				Reveicing entity obtains current real
+				time by adding the value from monotonic
+				clock e.g.
+				clock_gettime(CLOCK_MONOTONIC,...).
+
+			int32 Timezone [optional]
+				Current timezone offset in seconds from
+				UTC.
+
+			int32 DST [optional]
+				Current daylight saving setting in
+				hours.
+
+			string MobileCountryCode
+				The Mobile country code of the
+				current network operator.
+
+			string MobileNetworkCode
+				The Mobile network code of the
+				current network operator.
-- 
1.7.1

[PATCH 4/4] nettime: Mock Timed for testing

[Antti Paila]

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

---
 test/test-nettime |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100755 test/test-nettime

diff --git a/test/test-nettime b/test/test-nettime
new file mode 100755
index 0000000..908a5db
--- /dev/null
+++ b/test/test-nettime
@@ -0,0 +1,35 @@
+#!/usr/bin/python
+
+import gobject
+import dbus
+import sys
+import time
+import dbus.service
+import dbus.mainloop.glib
+
+
+class NetworkTime(dbus.service.Object):
+	def __init__(self):
+		busName = dbus.service.BusName('com.meego.time',
+					bus = dbus.SystemBus())
+		dbus.service.Object.__init__(self, busName, '/com/meego/time')
+	@dbus.service.method(dbus_interface="com.meego.NetworkTime",
+					in_signature="a{sv}", out_signature="")
+	def Notify(self, arg):
+		print arg
+		print
+		print "Time from mobile: %d" % arg["UTC"]
+		print "DST: %d" % arg["DST"]
+		print "Timezone: %d" % arg["Timezone"]
+		print "MNC: %s" % arg["MobileNetworkCode"]
+		print "MCC: %s" % arg["MobileCountryCode"]
+
+if __name__ == '__main__':
+	dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+	agent = NetworkTime()
+
+	mainloop = gobject.MainLoop()
+	mainloop.run()
+
+
+
-- 
1.7.1

RE: [PATCH 3/4] nettime: Documentation

[Jeevaka.Badrappan]

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

Hi Antti,

ofono-bounces(a)ofono.org wrote:
> ---
>  doc/network-time-api.txt |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)  create
> mode 100644 doc/network-time-api.txt
> 
> diff --git a/doc/network-time-api.txt
> b/doc/network-time-api.txt new file mode 100644 index
> 0000000..9133a73 --- /dev/null +++ b/doc/network-time-api.txt
> @@ -0,0 +1,36 @@
> +Network time hierarchy
> +======================
> +
> +Interface	com.meego.NetworkTime
> +Object path	[variable]
> +
> +Methods		void Notify(dict info)
> +
> +			Notifies the service of current time and date
> +			as notified by the cellular network.  The info
> +			argument contains a dictionary with the
> +			following possible keys:
> +
> +			int64 UTC [optional]
> +				Network time in seconds from epoch
> +				normalized to device boot time.
> +				Reveicing entity obtains current real
> +				time by adding the value from monotonic
> +				clock e.g.
> +				clock_gettime(CLOCK_MONOTONIC,...).
> +
> +			int32 Timezone [optional]
> +				Current timezone offset in seconds from
> +				UTC.
> +
> +			int32 DST [optional]
> +				Current daylight saving setting in
> +				hours.

few hours back I delivered a patch for converting the DST from hours to
seconds in ofono driver code.
are we going to take that into consideration?

Aki??

Regards,
Jeevaka

RE: [PATCH 3/4] nettime: Documentation

[Aki Niemi]

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

Hi Jeevaka,

On Tue, 2011-02-01 at 16:54 +0200, ext Jeevaka.Badrappan(a)elektrobit.com
wrote:
> > +			int32 DST [optional]
> > +				Current daylight saving setting in
> > +				hours.
> 
> few hours back I delivered a patch for converting the DST from hours to
> seconds in ofono driver code.
> are we going to take that into consideration?

Ah, well. Might as well then change the dst to be hours. Would you mind
resubmitting those patches, plus one that fixes struct
ofono_network_time as well?

Cheers,
Aki

Re: [PATCH 1/4] nettime: Network time plugin implementation

[Aki Niemi]

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

Hi Antti,

2011/2/1 Antti Paila <antti.paila@nokia.com>:
> ---
>  plugins/nettime.c |  326 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 326 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/nettime.c
>
> diff --git a/plugins/nettime.c b/plugins/nettime.c
> new file mode 100644
> index 0000000..894dce7
> --- /dev/null
> +++ b/plugins/nettime.c
> @@ -0,0 +1,326 @@
> +/*
> + *
> + *  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 <stdlib.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 TIMED_PATH "/com/meego/time"
> +#define TIMED_SERVICE "com.meego.time"

s/time/timed

> +struct nt_data {
> +       gboolean time_available;
> +       gboolean time_pending;
> +       time_t nw_time_utc;
> +       time_t received;
> +       int dst;
> +       int time_zone;
> +       const char *mcc;
> +       const char *mnc;
> +       unsigned int timed_watch;
> +       gboolean timed_present;
> +       struct ofono_netreg *netreg;
> +       unsigned int netreg_st_watch;

Let's call this just netereg_watch, as there is only one watch that is used.

> +};
> +
> +static void nettime_register(struct ofono_nettime_context *);
> +
> +static gboolean encode_time_format(struct ofono_network_time *time,
> +                                struct tm *tm)
> +{
> +
> +       tm->tm_gmtoff = time->utcoff;
> +       tm->tm_isdst = time->dst;

Why are you setting these? The values you are using in struct tm are
UTC. Also, tm_isdst is a boolean, and time->dst can be any of -1, 0,
1, 2.

> +       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;
> +
> +       return TRUE;
> +}
> +
> +static time_t get_monotonic_time()
> +{
> +       struct timespec ts;
> +       clock_gettime(CLOCK_MONOTONIC, &ts);
> +       return ts.tv_sec;
> +}
> +
> +static int fill_time_notification(DBusMessage *msg,
> +               struct nt_data *ntd)
> +{
> +       DBusMessageIter iter, iter_array;
> +       int64_t utc;
> +
> +       dbus_message_iter_init_append(msg, &iter);
> +       dbus_message_iter_open_container(&iter,
> +                                       DBUS_TYPE_ARRAY,
> +                                       "{sv}",
> +                                       &iter_array);
> +
> +       if (ntd->time_pending) {
> +               if (ntd->time_available) {
> +                       utc = ntd->nw_time_utc - ntd->received;

I would just store the ofono_network_time struct, and check if year < 0.

> +                       ofono_dbus_dict_append(&iter_array,
> +                                               "UTC",
> +                                               DBUS_TYPE_INT64,
> +                                               &utc);
> +               }
> +
> +               ofono_dbus_dict_append(&iter_array,
> +                                       "DST",
> +                                       DBUS_TYPE_INT32,
> +                                       &ntd->dst);
> +               ofono_dbus_dict_append(&iter_array,
> +                                       "Timezone",
> +                                       DBUS_TYPE_INT32,
> +                                       &ntd->time_zone);
> +       }
> +
> +       ofono_dbus_dict_append(&iter_array,
> +                       "MobileCountryCode",
> +                       DBUS_TYPE_STRING,
> +                       &ntd->mcc);
> +       ofono_dbus_dict_append(&iter_array,
> +                       "MobileNetworkCode",
> +                       DBUS_TYPE_STRING,
> +                       &ntd->mnc);
> +
> +       dbus_message_iter_close_container(&iter, &iter_array);
> +       return 0;
> +}
> +
> +static DBusMessage *create_time_notification(
> +                       struct ofono_nettime_context *context)
> +{
> +       DBusMessage *message;
> +       struct nt_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;
> +       }
> +
> +       message = dbus_message_new_method_call(TIMED_SERVICE, TIMED_PATH,
> +                                       "com.meego.NetworkTime", "Notify");
> +       if (message == NULL)
> +               return NULL;
> +
> +       dbus_message_set_no_reply(message, TRUE);
> +       fill_time_notification(message, ntd);
> +
> +       return message;
> +}
> +
> +static void init_time(struct ofono_nettime_context *context)
> +{
> +       struct nt_data *nt_data = g_new0(struct nt_data, 1);
> +
> +       nt_data->time_available = FALSE;
> +       nt_data->time_pending = FALSE;
> +       nt_data->dst = 0;
> +       nt_data->time_zone = 0;

That '0' after new actually zeroes out the memory, so these are not necessary.

> +       context->data = nt_data;
> +}
> +
> +static int nettime_probe(struct ofono_nettime_context *context)
> +{
> +       DBG("Network Time Probe for modem: %p", context->modem);
> +
> +       init_time(context);

As the function above reduces to just a g_new0() call and setting it
to context->data, just move all that here.

> +       nettime_register(context);
> +
> +       return 0;
> +}
> +
> +static void nettime_remove(struct ofono_nettime_context *context)
> +{
> +       struct nt_data *ntd = context->data;
> +
> +       DBG("Network Time Remove for modem: %p", context->modem);
> +       g_dbus_remove_watch(ofono_dbus_get_connection(),
> +                               ntd->timed_watch);

Just to be safe, you should check that ntd->timed_watch > 0 here. I
think you should also do the same with the netreg_watch.

> +       g_free(ntd);
> +}
> +
> +static void notify(int status, int lac, int ci, int tech, const char *mcc,
> +                       const char *mnc, void *data)
> +{
> +       struct ofono_nettime_context *context = data;
> +       struct nt_data *ntd = context->data;
> +       DBusMessage *message;
> +
> +       if (mcc == NULL || mnc == NULL)
> +               return;
> +
> +       ntd->mcc = mcc;
> +       ntd->mnc = mnc;

You're not checking whether MCC has changed since last time.

> +       if (ntd->timed_present == FALSE) {
> +               DBG("Timed not present. Caching time info");
> +               return;
> +       }

What are you caching here?

> +       message = create_time_notification(context);
> +       if (message == NULL) {
> +               ofono_error("Failed to create Notification message");
> +               return;
> +       }
> +
> +       g_dbus_send_message(ofono_dbus_get_connection(), message);
> +       ntd->time_pending = FALSE;
> +}

Please split the actual D-Bus message sending out to a separate
function. That way, you don't need to call this function with (0, 0,
0, 0, ... arguments all over the place, which looks weird.

> +static void nr_st_watch_destroy(void *data)
> +{
> +       struct ofono_nettime_context *context = data;
> +       struct nt_data *ntd = context->data;
> +       DBG("");
> +
> +       ntd->netreg_st_watch = 0;
> +}

Don't give a destroy callback unless you have memory to clean up.

Cheers,
Aki

Re: [PATCH 1/4] nettime: Network time plugin implementation

[Marcel Holtmann]

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

Hi Antti,

>  plugins/nettime.c |  326 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 326 insertions(+), 0 deletions(-)
>  create mode 100644 plugins/nettime.c

I would prefer if we call this nokia-timed.c or in case this actually
becomes default part of MeeGo, them maybe meego-timed.c. I would be also
fine with {nokia,meego}-nettime.c.

Just calling it nettime.c is too generic. It needs to be clear what this
is for.

> +#define TIMED_PATH "/com/meego/time"
> +#define TIMED_SERVICE "com.meego.time"

So the intention is to convert the Nokia timed into a MeeGo specific
daemon? I would have expected to see com.nokia.timed here.

> +struct nt_data {
> +	gboolean time_available;
> +	gboolean time_pending;
> +	time_t nw_time_utc;
> +	time_t received;
> +	int dst;
> +	int time_zone;
> +	const char *mcc;
> +	const char *mnc;

Why do you bother with these here. You might better just reference the
netreg atom. The memory is only valid if netreg atom is present.

> +	unsigned int timed_watch;
> +	gboolean timed_present;
> +	struct ofono_netreg *netreg;
> +	unsigned int netreg_st_watch;
> +
> +};
> +
> +static void nettime_register(struct ofono_nettime_context *);
> +
> +static gboolean encode_time_format(struct ofono_network_time *time,
> +				 struct tm *tm)
> +{
> +
> +	tm->tm_gmtoff = time->utcoff;
> +	tm->tm_isdst = time->dst;
> +
> +	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;
> +
> +	return TRUE;
> +}
> +
> +static time_t get_monotonic_time()
> +{
> +	struct timespec ts;
> +	clock_gettime(CLOCK_MONOTONIC, &ts);
> +	return ts.tv_sec;
> +}
> +
> +static int fill_time_notification(DBusMessage *msg,
> +		struct nt_data *ntd)
> +{
> +	DBusMessageIter iter, iter_array;
> +	int64_t utc;
> +
> +	dbus_message_iter_init_append(msg, &iter);
> +	dbus_message_iter_open_container(&iter,
> +					DBUS_TYPE_ARRAY,
> +					"{sv}",
> +					&iter_array);
> +
> +	if (ntd->time_pending) {
> +		if (ntd->time_available) {
> +			utc = ntd->nw_time_utc - ntd->received;
> +			ofono_dbus_dict_append(&iter_array,
> +						"UTC",
> +						DBUS_TYPE_INT64,
> +						&utc);
> +		}
> +
> +		ofono_dbus_dict_append(&iter_array,
> +					"DST",
> +					DBUS_TYPE_INT32,
> +					&ntd->dst);
> +		ofono_dbus_dict_append(&iter_array,
> +					"Timezone",
> +					DBUS_TYPE_INT32,
> +					&ntd->time_zone);
> +	}
> +
> +	ofono_dbus_dict_append(&iter_array,
> +			"MobileCountryCode",
> +			DBUS_TYPE_STRING,
> +			&ntd->mcc);
> +	ofono_dbus_dict_append(&iter_array,
> +			"MobileNetworkCode",
> +			DBUS_TYPE_STRING,
> +			&ntd->mnc);
> +
> +	dbus_message_iter_close_container(&iter, &iter_array);
> +	return 0;
> +}
> +
> +static DBusMessage *create_time_notification(
> +			struct ofono_nettime_context *context)
> +{
> +	DBusMessage *message;
> +	struct nt_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;
> +	}
> +
> +	message = dbus_message_new_method_call(TIMED_SERVICE, TIMED_PATH,
> +					"com.meego.NetworkTime", "Notify");
> +	if (message == NULL)
> +		return NULL;
> +
> +	dbus_message_set_no_reply(message, TRUE);
> +	fill_time_notification(message, ntd);
> +
> +	return message;
> +}
> +
> +static void init_time(struct ofono_nettime_context *context)
> +{
> +	struct nt_data *nt_data = g_new0(struct nt_data, 1);
> +
> +	nt_data->time_available = FALSE;
> +	nt_data->time_pending = FALSE;
> +	nt_data->dst = 0;
> +	nt_data->time_zone = 0;
> +
> +	context->data = nt_data;
> +}
> +
> +static int nettime_probe(struct ofono_nettime_context *context)
> +{
> +	DBG("Network Time Probe for modem: %p", context->modem);
> +
> +	init_time(context);

Please just don't bother with putting this in separate function. It only
has one caller and I prefer the context->data allocation being in the
probe() callback directly.

> +	nettime_register(context);

Same for this one. Also I don't see the point of the forward
declaration.

> +static void nettime_remove(struct ofono_nettime_context *context)
> +{
> +	struct nt_data *ntd = context->data;
> +
> +	DBG("Network Time Remove for modem: %p", context->modem);
> +	g_dbus_remove_watch(ofono_dbus_get_connection(),
> +				ntd->timed_watch);
> +	g_free(ntd);
> +}

You can avoid certain checks here, but then you need to have clear error
handling in probe() callback.

> +static void notify(int status, int lac, int ci, int tech, const char *mcc,
> +			const char *mnc, void *data)
> +{
> +	struct ofono_nettime_context *context = data;
> +	struct nt_data *ntd = context->data;
> +	DBusMessage *message;
> +
> +	if (mcc == NULL || mnc == NULL)
> +		return;
> +
> +	ntd->mcc = mcc;
> +	ntd->mnc = mnc;
> +
> +	if (ntd->timed_present == FALSE) {
> +		DBG("Timed not present. Caching time info");
> +		return;
> +	}
> +
> +	message = create_time_notification(context);
> +	if (message == NULL) {
> +		ofono_error("Failed to create Notification message");
> +		return;
> +	}
> +
> +	g_dbus_send_message(ofono_dbus_get_connection(), message);
> +	ntd->time_pending = FALSE;
> +}
> +
> +static void nr_st_watch_destroy(void *data)
> +{
> +	struct ofono_nettime_context *context = data;
> +	struct nt_data *ntd = context->data;
> +	DBG("");
> +
> +	ntd->netreg_st_watch = 0;
> +}
> +
> +static void nettime_info_received(struct ofono_nettime_context *context,
> +				struct ofono_network_time *info)
> +{
> +	struct tm t;
> +	struct nt_data *ntd = context->data;
> +	const char *mcc, *mnc;
> +
> +	DBG("Network time notification received, modem: %p",
> +			context->modem);
> +
> +	if (info == NULL)
> +		return;
> +
> +	ntd->received = get_monotonic_time();
> +	ntd->time_pending = TRUE;
> +
> +	ntd->time_available = encode_time_format(info, &t);
> +	if (ntd->time_available == TRUE)
> +		ntd->nw_time_utc = timegm(&t);
> +
> +	ntd->dst = info->dst;
> +	ntd->time_zone = info->utcoff;
> +
> +	ntd->netreg = __ofono_atom_get_data(__ofono_modem_find_atom(
> +				context->modem, OFONO_ATOM_TYPE_NETREG));
> +
> +	mcc = ofono_netreg_get_mcc(ntd->netreg);
> +	mnc = ofono_netreg_get_mnc(ntd->netreg);
> +	if ((mcc == NULL) || (mnc == NULL)) {
> +		DBG("Mobile country/network code missing");
> +
> +		if (ntd->netreg_st_watch != 0)
> +			return;
> +
> +		ntd->netreg_st_watch = __ofono_netreg_add_status_watch(
> +					ntd->netreg, notify,
> +					context, nr_st_watch_destroy);
> +	} else {
> +		notify(0, 0, 0, 0, mcc, mnc, context);
> +	}
> +
> +}
> +
> +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);
> +}

So in general the plugin init/exit functions should be last in the
source code. Just above the OFONO_PLUGIN_DEFINE. That way it is
consistent and a lot easier to read.

Also the generic nettime_* namespacing might be better done as
nokia_timed_* or just short timed_*.

> +static void timed_connect(DBusConnection *connection, void *user_data)
> +{
> +	struct ofono_nettime_context *context = user_data;
> +	struct nt_data *ntd = context->data;
> +	const char *mcc, *mnc;
> +
> +	DBG("");
> +
> +	ntd->timed_present = TRUE;
> +	mcc = ofono_netreg_get_mcc(ntd->netreg);
> +	mnc = ofono_netreg_get_mnc(ntd->netreg);
> +
> +	notify(0, 0, 0, 0, mcc, mnc, context);
> +}
> +
> +static void timed_disconnect(DBusConnection *connection, void *user_data)
> +{
> +	struct ofono_nettime_context *context = user_data;
> +	struct nt_data *ntd = context->data;
> +
> +	DBG("");
> +
> +	ntd->timed_present = FALSE;
> +}
> +
> +static void nettime_register(struct ofono_nettime_context *context)
> +{
> +	DBusConnection *conn;
> +	struct nt_data *ntd = context->data;
> +
> +	DBG("Registering Network time for modem %s" ,
> +		ofono_modem_get_path(context->modem));
> +
> +	conn = ofono_dbus_get_connection();
> +
> +	ntd->timed_watch = g_dbus_add_service_watch(conn, TIMED_SERVICE,
> +					timed_connect, timed_disconnect,
> +					context, NULL);
> +}

Please reorder the functions a bit better. That makes the whole code
more readable and gives us a lot more benefit in the long term.

I only wanna see forward declaration if they are 100% unavoidable.

> +
> +OFONO_PLUGIN_DEFINE(nettime, "Network Time Plugin",
> +		VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
> +		nettime_init, nettime_exit)
> +

Regards

Marcel

Re: [PATCH 3/4] nettime: Documentation

[Marcel Holtmann]

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

Hi Antti,

>  doc/network-time-api.txt |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
>  create mode 100644 doc/network-time-api.txt
> 
> diff --git a/doc/network-time-api.txt b/doc/network-time-api.txt
> new file mode 100644
> index 0000000..9133a73
> --- /dev/null
> +++ b/doc/network-time-api.txt
> @@ -0,0 +1,36 @@
> +Network time hierarchy
> +======================
> +
> +Interface	com.meego.NetworkTime
> +Object path	[variable]

the object path is not really variable. However this part of the
documentation should be really inside timed. Since timed is exposing
this interface in the first place.

And we are moving from com.nokia.timed towards com.meego.timed?

Regards

Marcel

Re: [PATCH 1/4] nettime: Network time plugin implementation

[Antti Paila]

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

Hi Marcel,

On Mon, 2011-02-07 at 10:58 -0800, ext Marcel Holtmann wrote:
> Hi Antti,
> 
> >  plugins/nettime.c |  326 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 326 insertions(+), 0 deletions(-)
> >  create mode 100644 plugins/nettime.c
> 
> I would prefer if we call this nokia-timed.c or in case this actually
> becomes default part of MeeGo, them maybe meego-timed.c. I would be also
> fine with {nokia,meego}-nettime.c.
> 
> Just calling it nettime.c is too generic. It needs to be clear what this
> is for.
> 
> > +#define TIMED_PATH "/com/meego/time"
> > +#define TIMED_SERVICE "com.meego.time"
> 
> So the intention is to convert the Nokia timed into a MeeGo specific
> daemon? I would have expected to see com.nokia.timed here.

My understanding is that this will be Meego specific plugin since timed
is part of Meego. Hence, the service 'com.meego.timed'.  

> > +struct nt_data {
> > +	gboolean time_available;
> > +	gboolean time_pending;
> > +	time_t nw_time_utc;
> > +	time_t received;
> > +	int dst;
> > +	int time_zone;
> > +	const char *mcc;
> > +	const char *mnc;
> 
> Why do you bother with these here. You might better just reference the
> netreg atom. The memory is only valid if netreg atom is present.

Timed expects to receive the mcc and mnc in the time notification.
However, if the mcc and mnc don't change we don't resend the
information. These fields contain the mnc and mcc that oFono sent
in the previous time notification.

Best Regards,
  Antti

Re: [PATCH 1/4] nettime: Network time plugin implementation

[Marcel Holtmann]

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

Hi Antti,

> > >  plugins/nettime.c |  326 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 326 insertions(+), 0 deletions(-)
> > >  create mode 100644 plugins/nettime.c
> > 
> > I would prefer if we call this nokia-timed.c or in case this actually
> > becomes default part of MeeGo, them maybe meego-timed.c. I would be also
> > fine with {nokia,meego}-nettime.c.
> > 
> > Just calling it nettime.c is too generic. It needs to be clear what this
> > is for.
> > 
> > > +#define TIMED_PATH "/com/meego/time"
> > > +#define TIMED_SERVICE "com.meego.time"
> > 
> > So the intention is to convert the Nokia timed into a MeeGo specific
> > daemon? I would have expected to see com.nokia.timed here.
> 
> My understanding is that this will be Meego specific plugin since timed
> is part of Meego. Hence, the service 'com.meego.timed'.  

this is fine with me, but so far the timed.git tree has nothing alike
this mentioned. It currently still uses com.nokia.timed.

> > > +struct nt_data {
> > > +	gboolean time_available;
> > > +	gboolean time_pending;
> > > +	time_t nw_time_utc;
> > > +	time_t received;
> > > +	int dst;
> > > +	int time_zone;
> > > +	const char *mcc;
> > > +	const char *mnc;
> > 
> > Why do you bother with these here. You might better just reference the
> > netreg atom. The memory is only valid if netreg atom is present.
> 
> Timed expects to receive the mcc and mnc in the time notification.
> However, if the mcc and mnc don't change we don't resend the
> information. These fields contain the mnc and mcc that oFono sent
> in the previous time notification.

But this const pointer points to memory inside netreg. It will never
change. It always points to the same address.

Regards

Marcel