[PATCH] location-reporting.h: driver properties handling

[PATCH] location-reporting.h: driver properties handling

[Giacinto Cifelli]

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

The GNSS receiver configuration is highly proprietary.
Some are configured through the GNSS port itself, some through other
ports. The properties themselves vary among the manufacturers, with no
standard available. Besides, ofono doesn't handle any state for a GNSS
receiver.

Therefore, this extensions allows to get and set directly D-Bus
properties by the driver.
---
 include/location-reporting.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/location-reporting.h b/include/location-reporting.h
index 0717f710..d3f013d2 100644
--- a/include/location-reporting.h
+++ b/include/location-reporting.h
@@ -52,6 +52,9 @@ struct ofono_location_reporting_driver {
 			ofono_location_reporting_enable_cb_t cb, void *data);
 	void (*disable)(struct ofono_location_reporting *lr,
 			ofono_location_reporting_disable_cb_t cb, void *data);
+	void (*get_properties)(struct ofono_location_reporting *lr,
+								void *_dict);
+	void* (*set_property)(struct ofono_location_reporting *lr, void *_msg);
 };
 
 int ofono_location_reporting_driver_register(
-- 
2.17.1

[PATCH] src/location-reporting: driver properties handling

[Giacinto Cifelli]

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

---
 src/location-reporting.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/location-reporting.c b/src/location-reporting.c
index 6a3e03c4..d787f0c0 100644
--- a/src/location-reporting.c
+++ b/src/location-reporting.c
@@ -64,7 +64,6 @@ static const char *location_reporting_type_to_string(
 
 static DBusMessage *location_reporting_get_properties(DBusConnection *conn,
 						DBusMessage *msg, void *data)
-
 {
 	struct ofono_location_reporting *lr = data;
 	DBusMessage *reply;
@@ -74,7 +73,7 @@ static DBusMessage *location_reporting_get_properties(DBusConnection *conn,
 	dbus_bool_t value;
 
 	reply = dbus_message_new_method_return(msg);
-	if (reply == NULL)
+	if (lr == NULL || reply == NULL)
 		return NULL;
 
 	dbus_message_iter_init_append(reply, &iter);
@@ -88,11 +87,25 @@ static DBusMessage *location_reporting_get_properties(DBusConnection *conn,
 	type = location_reporting_type_to_string(lr->driver->type);
 	ofono_dbus_dict_append(&dict, "Type", DBUS_TYPE_STRING, &type);
 
+	if (lr->driver && lr->driver->get_properties)
+		lr->driver->get_properties(lr, &dict);
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
 }
 
+static DBusMessage *location_reporting_set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_location_reporting *lr = data;
+
+	if (!lr || !lr->driver || !lr->driver->set_property)
+		return __ofono_error_not_available(msg);
+
+	return lr->driver->set_property(lr, msg);
+}
+
 static void client_remove(struct ofono_location_reporting *lr)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -243,6 +256,9 @@ static const GDBusMethodTable location_reporting_methods[] = {
 	{ GDBUS_METHOD("GetProperties",
 			NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
 			location_reporting_get_properties) },
+	{ GDBUS_ASYNC_METHOD("SetProperty",
+			GDBUS_ARGS({ "property", "s" }, { "value", "v" }),
+			NULL, location_reporting_set_property) },
 	{ GDBUS_ASYNC_METHOD("Request",
 			NULL, GDBUS_ARGS({ "fd", "h" }),
 			location_reporting_request) },
-- 
2.17.1

[PATCH] gemalto/location-reporting: add driver properties

[Giacinto Cifelli]

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

---
 drivers/gemaltomodem/location-reporting.c | 317 ++++++++++++++++++++--
 1 file changed, 291 insertions(+), 26 deletions(-)

diff --git a/drivers/gemaltomodem/location-reporting.c b/drivers/gemaltomodem/location-reporting.c
index bcfe00e5..375b7bfa 100644
--- a/drivers/gemaltomodem/location-reporting.c
+++ b/drivers/gemaltomodem/location-reporting.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
  *
  *  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
@@ -31,38 +32,62 @@
 #include <unistd.h>
 
 #include <glib.h>
+#include <gdbus.h>
 
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/location-reporting.h>
 
+#include <src/storage.h>
+
 #include "gatchat.h"
 #include "gatresult.h"
 #include "gattty.h"
 
+#include "ofono.h"
+#include "common.h"
+
 #include "gemaltomodem.h"
 
+struct ofono_location_reporting {
+	DBusMessage *pending;
+	const struct ofono_location_reporting_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+	ofono_bool_t enabled;
+	char *client_owner;
+	guint disconnect_watch;
+};
+
 static const char *sgpsc_prefix[] = { "^SGPSC:", NULL };
 
+#define MAX_GNSS_PROPERTIES		(64)
+#define MAX_GNSS_STRLEN			(32)
+
+typedef struct _gnss_property {
+	uint type; // dynamic or stored, exclude, value_s or value_i
+	char name[MAX_GNSS_STRLEN];
+	union {
+		char value_s[32];
+		int value_i;
+	};
+} gnss_property;
+
 struct gps_data {
 	GAtChat *chat;
+	gnss_property properties[MAX_GNSS_PROPERTIES];
 };
 
 static void gemalto_gps_disable_cb(gboolean ok, GAtResult *result,
 							gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
-	struct ofono_location_reporting *lr = cbd->user;
 	ofono_location_reporting_disable_cb_t cb = cbd->cb;
 
-	DBG("lr=%p, ok=%d", lr, ok);
-
 	if (!ok) {
 		struct ofono_error error;
-
 		decode_at_error(&error, g_at_result_final_response(result));
 		cb(&error, cbd->data);
-
 		return;
 	}
 
@@ -77,8 +102,6 @@ static void gemalto_location_reporting_disable(
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 	struct cb_data *cbd = cb_data_new(cb, data);
 
-	DBG("lr=%p", lr);
-
 	cbd->user = lr;
 
 	if (g_at_chat_send(gd->chat, "AT^SGPSC=\"Engine\",0", sgpsc_prefix,
@@ -86,36 +109,32 @@ static void gemalto_location_reporting_disable(
 		return;
 
 	CALLBACK_WITH_FAILURE(cb, data);
-
 	g_free(cbd);
 }
 
 static int enable_data_stream(struct ofono_location_reporting *lr)
 {
 	struct ofono_modem *modem;
-	const char *gps_dev;
+	const char *gnss_dev;
 	GHashTable *options;
 	GIOChannel *channel;
 	int fd;
 
 	modem = ofono_location_reporting_get_modem(lr);
-	gps_dev = ofono_modem_get_string(modem, "GPS");
-
+	gnss_dev = ofono_modem_get_string(modem, "GNSS");
 	options = g_hash_table_new(g_str_hash, g_str_equal);
-	if (options == NULL)
+
+	if (!gnss_dev || !*gnss_dev || !options)
 		return -1;
 
 	g_hash_table_insert(options, "Baud", "115200");
-
-	channel = g_at_tty_open(gps_dev, options);
-
+	channel = g_at_tty_open(gnss_dev, options);
 	g_hash_table_destroy(options);
 
-	if (channel == NULL)
+	if (!channel)
 		return -1;
 
 	fd = g_io_channel_unix_get_fd(channel);
-
 	g_io_channel_set_close_on_unref(channel, FALSE);
 	g_io_channel_unref(channel);
 
@@ -131,8 +150,6 @@ static void gemalto_sgpsc_cb(gboolean ok, GAtResult *result,
 	struct ofono_error error;
 	int fd;
 
-	DBG("lr=%p ok=%d", lr, ok);
-
 	decode_at_error(&error, g_at_result_final_response(result));
 
 	if (!ok) {
@@ -153,15 +170,14 @@ static void gemalto_sgpsc_cb(gboolean ok, GAtResult *result,
 	close(fd);
 }
 
-static void gemalto_location_reporting_enable(struct ofono_location_reporting *lr,
+static void gemalto_location_reporting_enable(
+					struct ofono_location_reporting *lr,
 					ofono_location_reporting_enable_cb_t cb,
 					void *data)
 {
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 	struct cb_data *cbd = cb_data_new(cb, data);
 
-	DBG("lr=%p", lr);
-
 	cbd->user = lr;
 
 	if (g_at_chat_send(gd->chat, "AT^SGPSC=\"Engine\",2", sgpsc_prefix,
@@ -172,6 +188,205 @@ static void gemalto_location_reporting_enable(struct ofono_location_reporting *l
 	g_free(cbd);
 }
 
+static void gemalto_location_reporting_get_properties(
+			struct ofono_location_reporting *lr, void *_dict)
+{
+	DBusMessageIter *dict = _dict;
+	struct gps_data *gd = ofono_location_reporting_get_data(lr);
+	int property_pos = 0;
+	struct ofono_modem *modem = ofono_location_reporting_get_modem(lr);
+	const char *port = ofono_modem_get_string(modem, "GNSS");
+	const char *prop_port = "Port";
+
+	while(property_pos < MAX_GNSS_PROPERTIES &&
+					*gd->properties[property_pos].name) {
+		char *propval = gd->properties[property_pos].value_s;
+		ofono_dbus_dict_append(dict, gd->properties[property_pos].name,
+						DBUS_TYPE_STRING, &propval);
+		property_pos++;
+	}
+
+	/* add port outside of the modem property list */
+	DBG("%s=%s", prop_port, port);
+	if (port)
+		ofono_dbus_dict_append(dict, prop_port, DBUS_TYPE_STRING, &port);
+}
+
+static void change_property_list(GAtResult *result, gpointer user_data) {
+	struct ofono_location_reporting *lr = user_data;
+	struct gps_data *gd = ofono_location_reporting_get_data(lr);
+	GAtResultIter iter;
+	int property_pos;
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(lr->atom);
+
+	g_at_result_iter_init(&iter, result);
+
+	/* supported format: ^SGPSC: "Nmea/Output","off" */
+	while (g_at_result_iter_next(&iter, "^SGPSC:")) {
+		const char *name = "";
+		const char *val = "";
+
+		if (!g_at_result_iter_next_string(&iter, &name))
+			continue;
+
+		/*
+		 * skip the "Info" property:
+		 * different line format and different usage
+		 */
+		if (g_str_equal(name,"Info"))
+			continue;
+
+		if (!g_at_result_iter_next_string(&iter, &val))
+			continue;
+
+		for(property_pos = 0; property_pos < MAX_GNSS_PROPERTIES &&
+			   *gd->properties[property_pos].name; property_pos++) {
+
+			if (!g_str_equal(gd->properties[property_pos].name,
+									name))
+				continue;
+
+			strncpy(gd->properties[property_pos].value_s,val,
+					MAX_GNSS_STRLEN);
+			ofono_dbus_signal_property_changed(conn, path,
+					OFONO_LOCATION_REPORTING_INTERFACE,
+					name, DBUS_TYPE_STRING, &val);
+		}
+	}
+}
+
+static void gemalto_location_reporting_set_property_cb(gboolean ok,
+					GAtResult *result, gpointer user_data)
+{
+	struct ofono_location_reporting *lr = user_data;
+
+	if (!ok || !lr)
+		return;
+
+	change_property_list(result, lr);
+}
+
+static void *gemalto_location_reporting_set_property(
+				struct ofono_location_reporting *lr, void *_msg)
+{
+	DBusMessage *msg = _msg;
+	struct gps_data *gd;
+	DBusMessageIter iter, var;
+	const char *name;
+	int property_pos;
+
+	DBG("");
+	if (!lr)
+		return __ofono_error_not_available(msg);
+
+	gd = ofono_location_reporting_get_data(lr);
+
+	if (!gd)
+		return __ofono_error_not_available(msg);
+
+	if (dbus_message_iter_init(msg, &iter) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &name);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+	property_pos = 0;
+
+	while(property_pos < MAX_GNSS_PROPERTIES &&
+					*gd->properties[property_pos].name) {
+
+		if (g_str_equal(name, gd->properties[property_pos].name))
+		{
+			char s[128];
+			char *value;
+
+			if (dbus_message_iter_get_arg_type(&var) !=
+							DBUS_TYPE_STRING)
+				return __ofono_error_invalid_args(msg);
+
+			dbus_message_iter_get_basic(&var, &value);
+
+			/* do not set the same value and do not notify change */
+			if (g_str_equal(value,
+					gd->properties[property_pos].value_s))
+				return __ofono_error_not_available(msg);
+
+			sprintf(s, "AT^SGPSC=\"%s\",\"%s\"",name, value);
+			DBG("setting %s", s);
+
+			/* TODO: report value changed */
+
+			if (g_at_chat_send(gd->chat, s, sgpsc_prefix,
+				    gemalto_location_reporting_set_property_cb,
+								lr, NULL) > 0)
+
+				return dbus_message_new_method_return(msg);
+
+			return __ofono_error_not_available(msg);
+		}
+		property_pos++;
+
+	}
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static void build_property_list(GAtResult *result, gpointer user_data) {
+	struct ofono_location_reporting *lr = user_data;
+	struct gps_data *gd = ofono_location_reporting_get_data(lr);
+	GAtResultIter iter;
+	int property_pos = 0;
+
+	/* skip pre-set */
+	while (property_pos < MAX_GNSS_PROPERTIES &&
+					*gd->properties[property_pos].name)
+		property_pos++;
+
+	g_at_result_iter_init(&iter, result);
+
+	/* supported format: ^SGPSC: "Nmea/Output","off" */
+	while ((property_pos < MAX_GNSS_PROPERTIES) &&
+				g_at_result_iter_next(&iter, "^SGPSC:")) {
+		const char *name = "";
+		const char *val = "";
+
+		if (!g_at_result_iter_next_string(&iter, &name))
+			continue;
+
+		/*
+		 * skip the "Info" property:
+		 * different line format and different usage
+		 */
+		if (g_str_equal(name,"Info"))
+			continue;
+
+		if (!g_at_result_iter_next_string(&iter, &val))
+			continue;
+
+		strncpy(gd->properties[property_pos].name, name,
+							MAX_GNSS_STRLEN);
+		strncpy(gd->properties[property_pos].value_s, val,
+							MAX_GNSS_STRLEN);
+		property_pos++;
+	}
+
+	property_pos = 0;
+
+	while ((property_pos < MAX_GNSS_PROPERTIES) &&
+					*gd->properties[property_pos].name) {
+		property_pos++;
+	}
+}
+
 static void gemalto_location_reporting_support_cb(gboolean ok, GAtResult *result,
 							gpointer user_data)
 {
@@ -179,13 +394,50 @@ static void gemalto_location_reporting_support_cb(gboolean ok, GAtResult *result
 
 	if (!ok) {
 		ofono_location_reporting_remove(lr);
-
 		return;
 	}
 
+	build_property_list(result, user_data);
 	ofono_location_reporting_register(lr);
 }
 
+static void gemalto_locrep_stored(struct ofono_location_reporting *lr) {
+	struct ofono_modem *modem = ofono_location_reporting_get_modem(lr);
+	struct gps_data *gd = ofono_location_reporting_get_data(lr);
+	const char *vid = ofono_modem_get_string(modem, "Vendor");
+	const char *pid = ofono_modem_get_string(modem, "Model");
+	char store[32];
+	int index;
+	char *property, *value;
+	char key[32];
+	GKeyFile *f;
+	char *command;
+
+	sprintf(store,"%s-%s/location-reporting", vid, pid);
+	f = storage_open(NULL, store);
+
+	if (!f)
+		return;
+
+	for (index = 0; ; index++) {
+		sprintf(key, "property_%d", index);
+		property = g_key_file_get_string(f, "Properties", key, NULL);
+
+		sprintf(key, "value_%d", index);
+		value = g_key_file_get_string(f, "Properties", key, NULL);
+
+		if (!property || !value)
+			break;
+
+		command = g_strdup_printf("AT^SGPSC=%s,%s", property, value);
+		DBG("setting GNSS property: %s", command);
+		g_at_chat_send(gd->chat, command, NULL, NULL, NULL, NULL);
+		free(command);
+	}
+
+	storage_close(NULL, store, f, FALSE);
+}
+
 static int gemalto_location_reporting_probe(struct ofono_location_reporting *lr,
 						unsigned int vendor, void *data)
 {
@@ -193,24 +445,35 @@ static int gemalto_location_reporting_probe(struct ofono_location_reporting *lr,
 	struct gps_data *gd;
 
 	gd = g_try_new0(struct gps_data, 1);
+
 	if (gd == NULL)
 		return -ENOMEM;
 
 	gd->chat = g_at_chat_clone(chat);
-
 	ofono_location_reporting_set_data(lr, gd);
 
-	g_at_chat_send(gd->chat, "AT^SGPSC=?", sgpsc_prefix,
+	/*
+	 * Storage of parameters is read-only.
+	 * It is intended for preconfiguring the devide by tbe manufacturer.
+	 * The parameters are stored by USB VID-PID, because
+	 * they are not standardized, and each model can have its own set.
+	 */
+	gemalto_locrep_stored(lr);
+
+	g_at_chat_send(gd->chat, "AT^SGPSC?", sgpsc_prefix,
 					gemalto_location_reporting_support_cb,
 					lr, NULL);
 
 	return 0;
 }
 
-static void gemalto_location_reporting_remove(struct ofono_location_reporting *lr)
+static void gemalto_location_reporting_remove(struct ofono_location_reporting
+									*lr)
 {
 	struct gps_data *gd = ofono_location_reporting_get_data(lr);
 
+	/* TODO: store modified params if [Settings] sync==yes */
+
 	ofono_location_reporting_set_data(lr, NULL);
 
 	g_at_chat_unref(gd->chat);
@@ -224,6 +487,8 @@ static struct ofono_location_reporting_driver driver = {
 	.remove			= gemalto_location_reporting_remove,
 	.enable			= gemalto_location_reporting_enable,
 	.disable		= gemalto_location_reporting_disable,
+	.get_properties		= gemalto_location_reporting_get_properties,
+	.set_property		= gemalto_location_reporting_set_property
 };
 
 void gemalto_location_reporting_init()
-- 
2.17.1

Re: [PATCH] location-reporting.h: driver properties handling

[Denis Kenzior]

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

Hi Giacinto,

On 09/23/2018 01:19 AM, Giacinto Cifelli wrote:
> The GNSS receiver configuration is highly proprietary.
> Some are configured through the GNSS port itself, some through other
> ports. The properties themselves vary among the manufacturers, with no
> standard available. Besides, ofono doesn't handle any state for a GNSS
> receiver.
> 
> Therefore, this extensions allows to get and set directly D-Bus
> properties by the driver.
> ---
>   include/location-reporting.h | 3 +++
>   1 file changed, 3 insertions(+)
>

No.  We're never going to do stuff like this.  Drivers are never going 
to get D-Bus messages.  They don't even know anything about D-Bus.

Either properly support the format you have in 
doc/location-reporting-api.txt or feel free to create a vendor specific 
D-Bus API for Gemalto inside plugins/gemalto.c

Regards,
-Denis

Re: [PATCH] location-reporting.h: driver properties handling

[Giacinto Cifelli]

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

Hi Denis,

On Mon, Sep 24, 2018 at 11:07 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> On 09/23/2018 01:19 AM, Giacinto Cifelli wrote:
> > The GNSS receiver configuration is highly proprietary.
> > Some are configured through the GNSS port itself, some through other
> > ports. The properties themselves vary among the manufacturers, with no
> > standard available. Besides, ofono doesn't handle any state for a GNSS
> > receiver.
> >
> > Therefore, this extensions allows to get and set directly D-Bus
> > properties by the driver.
> > ---
> >   include/location-reporting.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
>
> No.  We're never going to do stuff like this.  Drivers are never going
> to get D-Bus messages.  They don't even know anything about D-Bus.
>
> Either properly support the format you have in
> doc/location-reporting-api.txt or feel free to create a vendor specific
> D-Bus API for Gemalto inside plugins/gemalto.c
>

ok, another vendor-specific extension.
Note that the location-reporting as it is is pretty much useless, because
it cannot configure anything,
and doesn't work well with gpsd (that requires a port, not a fd).


>
> Regards,
> -Denis
>

Regards,
Giacinto

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1796 bytes --]