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
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
