plugin: Add ste modem initd integration

PATCH Superseded Sjur Brændeland 3 participants 33 messages Dec 15, 2010 – Jan 11, 2011 Kbuild

lore

Share & tools

lore

Sjur Brændeland

author

2010-12-15 21:49 UTC #0 lore raw

PATCH v2 1/1 plugin: Add ste modem initd integration

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M5XX, M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Multiple modem instances, and reset handling is supported.
---
Changes:
- Support for multiple STE modems.
- Better naming of STE modem instances, using received dbus path.
- CAIF Link layer to use for AT channels specified pr modem instance.
- Added support for reset.

 Makefile.am      |    5 +
 configure.ac     |    6 +
 plugins/stemid.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemid.c

Makefile.am +5 -0

index 12b3c33..98096ae 100644
@@ -82,6 +82,11 @@ gatchat_sources = gatchat/gatchat.h gatchat/gatchat.c \
 
 udev_files = plugins/ofono.rules
 
+if STE_MODEM_INITD
+builtin_modules += stemid
+builtin_sources += plugins/stemid.c
+endif
+
 if UDEV
 builtin_modules += udev
 builtin_sources += plugins/udev.c

configure.ac +6 -0

index 5c18f68..f733fc4 100644
@@ -177,6 +177,12 @@ AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 
 AM_CONDITIONAL(DATAFILES, test "${enable_datafiles}" != "no")
 
+AC_ARG_ENABLE(ste_modem_initd, AC_HELP_STRING([--disable-ste-modem-initd],
+			[disable auto discovery of STE modem using modem init daemon]),
+					[enable_ste_modem_initd=${enableval}])
+
+AM_CONDITIONAL(STE_MODEM_INITD, test "${enable_ste_modem_initd}" != "no")
+
 if (test "${prefix}" = "NONE"); then
 	dnl no prefix and no localstatedir, so default to /var
 	if (test "$localstatedir" = '${prefix}/var'); then

plugins/stemid.c new file +310 -0

310 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Sjur Brændeland

author

2010-12-15 21:49 UTC #1 lore raw

PATCH v2 1/2 stemodem: Create network interfaces statically

From: Sjur Brændeland <[email protected]>

Changes:
o Restructure code so interfaces are created statically when probe is called, instead of creating interfaces dynamically at activation.
o Changed debug messages.
o Removed some of the unnecessary initializations at declaration.
o No longer reporting "default gateway" for PtP IP Interface.
o Bugfix: Handle network initiated deactivation.

---
 drivers/stemodem/gprs-context.c |  149 +++++++++++++++++++++-----------------
 1 files changed, 82 insertions(+), 67 deletions(-)

drivers/stemodem/gprs-context.c +82 ~~-67~~

index faa3124..7bdcdb8 100644
@@ -28,6 +28,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <errno.h>
 
 #include <glib.h>

@@ -65,10 +66,18 @@ struct gprs_context_data {
 };
 
 struct conn_info {
+	/*
+	 * cid is allocated in oFono Core and is identifying
+	 * the Account. cid = 0 indicates that it is currently unused.
+	 */
 	unsigned int cid;
-	unsigned int device;
+	/* Id used by CAIF and EPPSD to identify the CAIF channel*/
 	unsigned int channel_id;
-	char interface[10];
+	/* Linux Interface Id */
+	unsigned int ifindex;
+	/* Linux Interface name */
+	char interface[IF_NAMESIZE];
+	gboolean created;
 };
 
 struct eppsd_response {

@@ -76,7 +85,6 @@ struct eppsd_response {
 	char ip_address[IP_ADDR_LEN];
 	char subnet_mask[IP_ADDR_LEN];
 	char mtu[IP_ADDR_LEN];
-	char default_gateway[IP_ADDR_LEN];
 	char dns_server1[IP_ADDR_LEN];
 	char dns_server2[IP_ADDR_LEN];
 	char p_cscf_server[IP_ADDR_LEN];

@@ -96,8 +104,6 @@ static void start_element_handler(GMarkupParseContext *context,
 		rsp->current = rsp->subnet_mask;
 	else if (!strcmp(element_name, "mtu"))
 		rsp->current = rsp->mtu;
-	else if (!strcmp(element_name, "default_gateway"))
-		rsp->current = rsp->default_gateway;
 	else if (!strcmp(element_name, "dns_server") &&
 					rsp->dns_server1[0] == '\0')
 		rsp->current = rsp->dns_server1;

@@ -123,7 +129,7 @@ static void text_handler(GMarkupParseContext *context,
 
 	if (rsp->current) {
 		strncpy(rsp->current, text, IP_ADDR_LEN);
-		rsp->current[IP_ADDR_LEN] = 0;
+		rsp->current[IP_ADDR_LEN] = '\0';
 	}
 }

@@ -153,8 +159,7 @@ static gint conn_compare_by_cid(gconstpointer a, gconstpointer b)
 	return 0;
 }
 
-static struct conn_info *conn_info_create(unsigned int device,
-						unsigned int channel_id)
+static struct conn_info *conn_info_create(unsigned int channel_id)
 {
 	struct conn_info *connection = g_try_new0(struct conn_info, 1);

@@ -162,7 +167,6 @@ static struct conn_info *conn_info_create(unsigned int device,
 		return NULL;
 
 	connection->cid = 0;
-	connection->device = device;
 	connection->channel_id = channel_id;
 
 	return connection;

@@ -171,7 +175,7 @@ static struct conn_info *conn_info_create(unsigned int device,
 /*
  * Creates a new IP interface for CAIF.
  */
-static gboolean caif_if_create(const char *interface, unsigned int connid)
+static gboolean caif_if_create(struct conn_info *conn)
 {
 	return FALSE;
 }

@@ -179,9 +183,8 @@ static gboolean caif_if_create(const char *interface, unsigned int connid)
 /*
  * Removes IP interface for CAIF.
  */
-static gboolean caif_if_remove(const char *interface, unsigned int connid)
+static void caif_if_remove(struct conn_info *conn)
 {
-	return FALSE;
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,

@@ -210,21 +213,13 @@ static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,
 		DBG("Did not find data (used caif device) for"
 					"connection with cid; %d",
 					gcd->active_context);
-		goto error;
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
 	}
 
 	conn = l->data;
-
-	if (!caif_if_remove(conn->interface, conn->channel_id)) {
-		DBG("Failed to remove caif interface %s.",
-				conn->interface);
-	}
-
 	conn->cid = 0;
-	return;
-
-error:
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
 }
 
 static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)

@@ -233,7 +228,7 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_gprs_context_up_cb_t cb = cbd->cb;
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct conn_info *conn = NULL;
+	struct conn_info *conn;
 	GAtResultIter iter;
 	GSList *l;
 	int i;

@@ -241,17 +236,15 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	const char *res_string;
 	const char *dns[MAX_DNS + 1];
 	struct eppsd_response rsp;
-	GMarkupParseContext *context = NULL;
+	GMarkupParseContext *context;
 
 	l = g_slist_find_custom(g_caif_devices,
 				GUINT_TO_POINTER(gcd->active_context),
 				conn_compare_by_cid);
 
 	if (l == NULL) {
-		DBG("Did not find data (device and channel id)"
-					"for connection with cid; %d",
-					gcd->active_context);
-		goto error;
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+		goto error_no_device;
 	}
 
 	conn = l->data;

@@ -291,31 +284,21 @@ static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	dns[1] = rsp.dns_server2;
 	dns[2] = NULL;
 
-	sprintf(conn->interface, "caif%u", conn->device);
-
-	if (!caif_if_create(conn->interface, conn->channel_id)) {
-		ofono_error("Failed to create caif interface %s.",
-				conn->interface);
-		CALLBACK_WITH_SUCCESS(cb, NULL, FALSE, rsp.ip_address,
-				rsp.subnet_mask, rsp.default_gateway,
+	CALLBACK_WITH_SUCCESS(cb, conn->interface, TRUE, rsp.ip_address,
+				rsp.subnet_mask, NULL,
 				dns, cbd->data);
-	} else {
-		CALLBACK_WITH_SUCCESS(cb, conn->interface,
-				FALSE, rsp.ip_address, rsp.subnet_mask,
-				rsp.default_gateway, dns, cbd->data);
-	}
-
 	return;
 
 error:
-	DBG("ste_eppsd_up_cb error");
-
 	if (context)
 		g_markup_parse_context_free(context);
 
 	if (conn)
 		conn->cid = 0;
 
+error_no_device:
+	DBG("ste_eppsd_up_cb error");
+	gcd->active_context = 0;
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 }

@@ -325,44 +308,45 @@ static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	ofono_gprs_context_up_cb_t cb = cbd->cb;
 	struct ofono_gprs_context *gc = cbd->user;
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
-	struct cb_data *ncbd = NULL;
+	struct cb_data *ncbd;
 	char buf[128];
 	struct conn_info *conn;
 	GSList *l;
 
+	l = g_slist_find_custom(g_caif_devices,
+				GUINT_TO_POINTER(gcd->active_context),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("CAIF Device gone missing (cid:%d)", gcd->active_context);
+		goto error_no_device;
+	}
+
+	conn = l->data;
+
 	if (!ok) {
 		struct ofono_error error;
 
+		conn->cid = 0;
 		gcd->active_context = 0;
 		decode_at_error(&error, g_at_result_final_response(result));
 		cb(&error, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
 		return;
 	}
 
-	ncbd = g_memdup(cbd, sizeof(struct cb_data));
-
-	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
-				conn_compare_by_cid);
-
-	if (l == NULL) {
-		DBG("at_cgdcont_cb, no more available devices");
-		goto error;
-	}
-
-	conn = l->data;
-	conn->cid = gcd->active_context;
 	snprintf(buf, sizeof(buf), "AT*EPPSD=1,%u,%u",
 			conn->channel_id, conn->cid);
+	ncbd = g_memdup(cbd, sizeof(struct cb_data));
 
 	if (g_at_chat_send(gcd->chat, buf, NULL,
 				ste_eppsd_up_cb, ncbd, g_free) > 0)
 		return;
 
-error:
 	g_free(ncbd);
+	conn->cid = 0;
 
+error_no_device:
 	gcd->active_context = 0;
-
 	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL,
 				NULL, NULL, cbd->data);
 }

@@ -375,13 +359,32 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	struct cb_data *cbd = cb_data_new(cb, data);
 	char buf[AUTH_BUF_LENGTH];
 	int len;
+	GSList *l;
+	struct conn_info *conn;
 
 	if (cbd == NULL)
-		goto error;
+		goto error_no_device;
 
 	gcd->active_context = ctx->cid;
 	cbd->user = gc;
 
+	/* Find free connection with cid zero */
+	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
+				conn_compare_by_cid);
+
+	if (!l) {
+		DBG("No more available CAIF devices");
+		goto error_no_device;
+	}
+
+	conn = l->data;
+	conn->cid = ctx->cid;
+
+	if (!conn->created) {
+		DBG("CAIF interface not created (rtnl error?)");
+		goto error;
+	}
+
 	len = snprintf(buf, sizeof(buf), "AT+CGDCONT=%u,\"IP\"", ctx->cid);
 
 	if (ctx->apn)

@@ -405,6 +408,9 @@ static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
 	return;
 
 error:
+	conn->cid = 0;
+
+error_no_device:
 	gcd->active_context = 0;
 	g_free(cbd);

@@ -431,8 +437,8 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 				conn_compare_by_cid);
 
 	if (l == NULL) {
-		DBG("at_gprs_deactivate_primary, did not find"
-			"data (channel id) for connection with cid; %d", id);
+		DBG("did not find data (channel id) "
+				"for connection with cid; %d", id);
 		goto error;
 	}

@@ -446,7 +452,6 @@ static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
 
 error:
 	g_free(cbd);
-
 	CALLBACK_WITH_FAILURE(cb, data);
 }

@@ -457,6 +462,7 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 	gint cid, state;
 	GAtResultIter iter;
+	GSList *l;
 
 	if (!ok)
 		return;

@@ -480,7 +486,13 @@ static void ste_cgact_read_cb(gboolean ok, GAtResult *result,
 		ofono_gprs_context_deactivated(gc, gcd->active_context);
 		gcd->active_context = 0;
 
-		break;
+		/* Mark interface as unused */
+		l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(cid),
+				conn_compare_by_cid);
+		if (l != NULL) {
+			struct conn_info *conn = l->data;
+			conn->cid = 0;
+		}
 	}
 }

@@ -524,9 +536,11 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 	ofono_gprs_context_set_data(gc, gcd);
 
 	for (i = 0; i < MAX_CAIF_DEVICES; i++) {
-		ci = conn_info_create(i, i+1);
-		if (ci)
-			g_caif_devices = g_slist_append(g_caif_devices, ci);
+		ci = conn_info_create(i+1);
+		if (!ci)
+			return -ENOMEM;
+		caif_if_create(ci);
+		g_caif_devices = g_slist_append(g_caif_devices, ci);
 	}
 
 	return 0;

@@ -536,6 +550,7 @@ static void ste_gprs_context_remove(struct ofono_gprs_context *gc)
 {
 	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
 
+	g_slist_foreach(g_caif_devices, (GFunc) caif_if_remove, NULL);
 	g_slist_foreach(g_caif_devices, (GFunc) g_free, NULL);
 	g_slist_free(g_caif_devices);
 	g_caif_devices = NULL;

Signature · 1 line

1.7.0.4

Marcel Holtmann

2010-12-21 14:38 UTC #2 lore raw

Re: [PATCHv2 1/2] stemodem: Create network interfaces statically

Hi Sjur,

Quoted 10 lines

Changes:
o Restructure code so interfaces are created statically when probe is called, instead of creating interfaces dynamically at activation.
o Changed debug messages.
o Removed some of the unnecessary initializations at declaration.
o No longer reporting "default gateway" for PtP IP Interface.
o Bugfix: Handle network initiated deactivation.
---
 drivers/stemodem/gprs-context.c |  149 +++++++++++++++++++++-----------------
 1 files changed, 82 insertions(+), 67 deletions(-)

looks good to me now. Patch has been applied.

Regards

Marcel

Sjur Brændeland

author

2010-12-15 21:49 UTC #3 lore raw

PATCH v2 2/2 stemodem: Use RTNL to create network interfaces.

In reply to Sjur Brændeland

From: Sjur Brændeland <[email protected]>

---
 drivers/stemodem/gprs-context.c |   43 ++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 7 deletions(-)

drivers/stemodem/gprs-context.c +36 -7

index 7bdcdb8..1762fb3 100644
@@ -47,6 +47,7 @@
 #include "stemodem.h"
 #include "caif_socket.h"
 #include "if_caif.h"
+#include "caif_rtnl.h"
 
 #define MAX_CAIF_DEVICES 4
 #define MAX_DNS 2

@@ -172,12 +173,20 @@ static struct conn_info *conn_info_create(unsigned int channel_id)
 	return connection;
 }
 
-/*
- * Creates a new IP interface for CAIF.
- */
-static gboolean caif_if_create(struct conn_info *conn)
+static void rtnl_callback(int ifindex, const char *ifname, void *user_data)
 {
-	return FALSE;
+	struct conn_info *conn = user_data;
+
+	if (ifindex < 0) {
+		conn->created = FALSE;
+		ofono_error("Failed to create caif interface %s",
+			conn->interface);
+		return;
+	}
+
+	strncpy(conn->interface, ifname, sizeof(conn->interface));
+	conn->ifindex = ifindex;
+	conn->created = TRUE;
 }
 
 /*

@@ -185,6 +194,17 @@ static gboolean caif_if_create(struct conn_info *conn)
  */
 static void caif_if_remove(struct conn_info *conn)
 {
+	if (!conn->created)
+		return;
+
+	if (caif_rtnl_delete_interface(conn->ifindex) < 0) {
+		ofono_error("Failed to delete caif interface %s",
+			conn->interface);
+		return;
+	}
+
+	DBG("removed CAIF interface ch:%d ifname:%s ifindex:%d\n",
+		conn->channel_id, conn->interface, conn->ifindex);
 }
 
 static void ste_eppsd_down_cb(gboolean ok, GAtResult *result,

@@ -526,7 +546,7 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 	GAtChat *chat = data;
 	struct gprs_context_data *gcd;
 	struct conn_info *ci;
-	int i;
+	int i, err;
 
 	gcd = g_new0(struct gprs_context_data, 1);
 	gcd->chat = g_at_chat_clone(chat);

@@ -539,7 +559,14 @@ static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
 		ci = conn_info_create(i+1);
 		if (!ci)
 			return -ENOMEM;
-		caif_if_create(ci);
+		err = caif_rtnl_create_interface(IFLA_CAIF_IPV4_CONNID,
+						ci->channel_id, FALSE,
+						rtnl_callback, ci);
+		if (err < 0) {
+			DBG("Failed to create IP interface for CAIF");
+			return err;
+		}
+
 		g_caif_devices = g_slist_append(g_caif_devices, ci);
 	}

@@ -571,10 +598,12 @@ static struct ofono_gprs_context_driver driver = {
 
 void ste_gprs_context_init()
 {
+	caif_rtnl_init();
 	ofono_gprs_context_driver_register(&driver);
 }
 
 void ste_gprs_context_exit()
 {
 	ofono_gprs_context_driver_unregister(&driver);
+	caif_rtnl_exit();
 }

Signature · 1 line

1.7.0.4

Marcel Holtmann

2010-12-21 14:38 UTC #4 lore raw

Re: [PATCHv2 2/2] stemodem: Use RTNL to create network interfaces.

Hi Sjur,

Quoted 2 lines

 drivers/stemodem/gprs-context.c |   43 ++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 7 deletions(-)

patch has been applied.

Regards

Marcel

Marcel Holtmann

2010-12-21 14:36 UTC #5 lore raw

In reply to Sjur Brændeland

Hi Sjur,

Quoted 7 lines

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M5XX, M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Multiple modem instances, and reset handling is supported.

I don't really like on how STE, ST and TI do abbreviations these days.

It is MID for modem init daemon, it is UIM for user init manager, KIM for kernel init manager and TI calls a shared transport ST. This will end up in a massive confusion at some point ;)

Quoted 7 lines

---
Changes:
- Support for multiple STE modems.
- Better naming of STE modem instances, using received dbus path.
- CAIF Link layer to use for AT channels specified pr modem instance.
- Added support for reset.

Quoted 5 lines

 Makefile.am      |    5 +
 configure.ac     |    6 +
 plugins/stemid.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemid.c

Can we just call this something like stemgr for STE Manager or STE Init Manager. The MID part is just confusing.

Quoted 21 lines

Makefile.am +5 -0

index 12b3c33..98096ae 100644
@@ -82,6 +82,11 @@ gatchat_sources = gatchat/gatchat.h gatchat/gatchat.c \
 
 udev_files = plugins/ofono.rules
 
+if STE_MODEM_INITD
+builtin_modules += stemid
+builtin_sources += plugins/stemid.c
+endif
+
 if UDEV
 builtin_modules += udev
 builtin_sources += plugins/udev.c

configure.ac +0 -0

index 5c18f68..f733fc4 100644

configure.ac AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles], · line 185

Quoted 9 lines

configure.ac +6 -0

@@ -177,6 +177,12 @@ AC_ARG_ENABLE(datafiles, AC_HELP_STRING([--disable-datafiles],
 
 AM_CONDITIONAL(DATAFILES, test "${enable_datafiles}" != "no")
 
+AC_ARG_ENABLE(ste_modem_initd, AC_HELP_STRING([--disable-ste-modem-initd],
+			[disable auto discovery of STE modem using modem init daemon]),
+					[enable_ste_modem_initd=${enableval}])
+
+AM_CONDITIONAL(STE_MODEM_INITD, test "${enable_ste_modem_initd}" != "no")
+

Don't bother with this and just always compile this in. For now that is fine and in case we start with more fine grained control here, I will do that on modem technology based and not on plugins.

Right now we do atmodem and isimodem. If we feel like it is needed, we would do stemodem, but right now, I don't see it.

Quoted 52 lines

configure.ac +0 -0

 if (test "${prefix}" = "NONE"); then
 	dnl no prefix and no localstatedir, so default to /var
 	if (test "$localstatedir" = '${prefix}/var'); then

plugins/stemid.c new file +43 -0

new file mode 100644
index 0000000..9f5da22
@@ -0,0 +1,310 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MID_SERVICE		"com.stericsson.modeminit"
+#define MID_INTERFACE		MID_SERVICE ".Modem"

plugins/stemid.c line 53

Quoted 10 lines

plugins/stemid.c new file +10 -0

+
+/* The signal StateChange sends the modem state.*/
+#define STATUS_CHANGED		"StateChange"
+
+#define MID_ACTION_ON		"on"		/* Modem is on */
+#define MID_ACTION_RESET	"dumping"	/* Modem is resetting */
+#define MID_ACTION_UNKNOWN	"unknown"	/* Don't care */
+
+/* ResetState requests resending of StateChange. */
+#define MID_RESEND_STATE	"ResendState"

This is a bad D-Bus API btw. Using a D-Bus method to trigger signal sending. You will wake up any listener for that signal. What is wrong with just having a GetState?

plugins/stemid.c line 59

Quoted 5 lines

plugins/stemid.c new file +5 -0

+/* GetCaifIfName requests the CAIF Interface Name. */
+#define MID_GET_CAIFIF		"GetCaifIfName"
+
+#define TIMEOUT		5000
+#define STE_DRIVER_NAME	"ste"

I don't think you should be doing the naming like this. Don't you have a proper serial number? If not, then use at least

plugins/stemid.c line 60

Quoted 1 line

plugins/stemid.c new file +1 -0
+#define STE_INTERFACE_PROPERTY	"Interface"

One time it is a property, and then you have a method call? What is it now?

Quoted 9 lines

plugins/stemid.c new file +9 -0

+enum ste_state {
+	STE_PENDING,    /* Waiting for caifif and "on" notification */
+	STE_PENDING_IF, /* Waiting for caifif, already got "on" */
+	STE_REGISTERED, /* Modem is registered */
+	STE_RESETTING   /* Modem is resetting */
+};
+
+struct ste_modem {
+	char *path;

plugins/stemid.c line 80

Quoted 10 lines

plugins/stemid.c new file +10 -0

+	struct ofono_modem *modem;
+	enum ste_state state;
+	gboolean got_caifif;
+	gboolean pending_call;
+};
+
+static GHashTable *modem_list;
+static guint mid_api_watch;
+static guint mid_state_watch;
+static DBusConnection *connection;

<snip>

I have not looked at anything in between just yet.

plugins/stemid.c line 292

Quoted 5 lines

plugins/stemid.c new file +5 -0

+static int stemid_init()
+{
+	connection = ofono_dbus_get_connection();
+	if (connection == NULL)
+		return -EIO;

Don't bother with the check here. It will not fail. It if fails, you have a seriously other problem.

plugins/stemid.c line 303

Quoted 10 lines

plugins/stemid.c new file +10 -0

+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	mid_api_watch = g_dbus_add_service_watch(connection, MID_SERVICE,
+				mid_connect, mid_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemid_exit()
+{
+	DBusConnection *connection = ofono_dbus_get_connection();

Why not use the global one you stored. Why call the function again?

plugins/stemid.c line 310

Quoted 7 lines

plugins/stemid.c new file +7 -0

+
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, mid_api_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemid, "STE Modem Init Daemon dbus api", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemid_init, stemid_exit)

The plugin description is wrong. You are not providing a D-Bus API here.

Regards

Marcel

Sjur Brændeland

author

2010-12-21 15:06 UTC #6 lore raw

Hi Marcel.

Quoted 7 lines

plugins/stemid.c line 53
Quoted 3 lines
plugins/stemid.c new file +3 -0
+
+/* ResetState requests resending of StateChange. */
+#define MID_RESEND_STATE     "ResendState"
This is a bad D-Bus API btw. Using a D-Bus method to trigger signal sending. You will wake up any listener for that signal. What is wrong with just having a GetState?

My problem is that I need to get the path, or modem serial-number if you like, to the
modem(s). As I don't know the start order I have to query the state at startup. So this will happens only once. The simples way for me to get the path is to send "ResendState" to path "/". This will then send the state info for all modem
paths.

I don't think I have the path them available at connect...
If you have any other ideas of how to query the modem instances I'm all ears?

The other is feedback makes sens to me ;-)
Thanks for review and feedback.

Regards,
Sjur

Marcel Holtmann

2010-12-21 15:18 UTC #7 lore raw

Hi Sjur,

Quoted 16 lines

Quoted 7 lines
plugins/stemid.c line 53
Quoted 3 lines
plugins/stemid.c new file +3 -0
+
+/* ResetState requests resending of StateChange. */
+#define MID_RESEND_STATE     "ResendState"
This is a bad D-Bus API btw. Using a D-Bus method to trigger signal sending. You will wake up any listener for that signal. What is wrong with just having a GetState?
My problem is that I need to get the path, or modem serial-number if you like, to the
modem(s). As I don't know the start order I have to query the state at startup. So this will happens only once. The simples way for me to get the path is to send "ResendState" to path "/". This will then send the state info for all modem
paths.

Quoted 2 lines

I don't think I have the path them available at connect...
If you have any other ideas of how to query the modem instances I'm all ears?

I think you need to resend the D-Bus API of your init daemon. I need to have a second look.

Regards

Marcel

Sjur Brændeland

author

2010-12-21 15:37 UTC #8 lore raw

Hi Marcel.

Quoted 12 lines

Quoted 12 lines
Quoted 3 lines
This is a bad D-Bus API btw. Using a D-Bus method to trigger signal sending. You will wake up any listener for that signal. What is wrong with just having a GetState?
My problem is that I need to get the path, or modem serial-number if you like, to the
modem(s). As I don't know the start order I have to query the state at startup. So this will happens only once. The simples way for me to get the path is to send "ResendState" to path "/". This will then send the state info for all modem
paths.

Quoted 5 lines

Quoted 2 lines
I don't think I have the path them available at connect...
If you have any other ideas of how to query the modem instances I'm all ears?
I think you need to resend the D-Bus API of your init daemon. I need to have a second look.

This is an extract of the Modem Init Daemon D-Bus API. I have removed the GetState API as we don't know the object path of the modem when oFOno has just booted. Instead we have added the ResendState API. This will be called initially when oFono starts to query the modem instances and their state. After startup state changes are handed as signals with StateChange. (There are some exceptions to this but they should be only corner cases).

Modem Init Deamon
================

Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path /{modem-serial} or /

Methods
void ResendState()

Resends state information for all modems. This method must use with '/' as object path.

string GetCaifIfName()

Returns the CAIF Link Layer interface used for AT channels for a specific modem. This method must use the same object path as used in StateChange.

Signals StateChange(string status)

The modems state sent from Modem Init Daemon when a modem state change occurs. The object path must be the identifier of the modem (typically the serial-id or IMEI of the HW).

"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"on" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.
"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

Regards,
Sjur

Marcel Holtmann

2010-12-23 02:48 UTC #9 lore raw

Hi Sjur,

Quoted 55 lines

Quoted 12 lines
Quoted 12 lines
Quoted 3 lines
This is a bad D-Bus API btw. Using a D-Bus method to trigger signal sending. You will wake up any listener for that signal. What is wrong with just having a GetState?
My problem is that I need to get the path, or modem serial-number if you like, to the
modem(s). As I don't know the start order I have to query the state at startup. So this will happens only once. The simples way for me to get the path is to send "ResendState" to path "/". This will then send the state info for all modem
paths.
Quoted 5 lines
Quoted 2 lines
I don't think I have the path them available at connect...
If you have any other ideas of how to query the modem instances I'm all ears?
I think you need to resend the D-Bus API of your init daemon. I need to have a second look.
This is an extract of the Modem Init Daemon D-Bus API. I have removed the GetState API as we don't know the object path of the modem when oFOno has just booted. Instead we have added the ResendState API. This will be called initially when oFono starts to query the modem instances and their state. After startup state changes are handed as signals with StateChange. (There are some exceptions to this but they should be only corner cases).
Modem Init Deamon
================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path /{modem-serial} or /
Methods
void ResendState()
Resends state information for all modems. This method must use with '/' as object path.
string GetCaifIfName()
Returns the CAIF Link Layer interface used for AT channels for a specific modem. This method must use the same object path as used in StateChange.
Signals StateChange(string status)
The modems state sent from Modem Init Daemon when a modem state change occurs. The object path must be the identifier of the modem (typically the serial-id or IMEI of the HW).

Quoted 12 lines

"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"on" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.
"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

is this the full API or only part of it. If it is just a part of it, please send the full API for the daemon. Just looking at this piece, I am not really thinking that this is a good API. It is actually pretty much broken :(

Regards

Marcel

Sjur Brændeland

author

2011-01-03 21:42 UTC #10 lore raw

Hi Marcel.

Quoted 4 lines

is this the full API or only part of it. If it is just a part of it, please send the full API for the daemon. Just looking at this piece, I am not really thinking that this is a good API. It is actually pretty much broken :(

Ok, here's an updated version of the Modem Init Daemon DBus API. I hope this is aligned with what we discussed on the IRC.

STE Modem Init Deamon Manager
================

Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Manager
Object path /

Methods array{object,dict} GetModems()

Get array of STE Modem objects and their state and properties (out signature 'a(oa{sv})').

The method should only be call once per application. Further changes shall be monitored via StateChange signals.

STE Modem
================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path variable

Signals StateChange(string State)

The modems state sent from when
a modem state change occurs. State is the only dynamic property in this Interface.

Properties string State [readonly]

The modems state is dynamic can can have the following values:
"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"on" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.
"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

string CaifAtInterface [readonly]

CAIF Link Layer interface to be used for AT channels for a modem.

Regards,
Sjur

Marcel Holtmann

2011-01-03 21:55 UTC #11 lore raw

Hi Sjur,

Quoted 15 lines

Quoted 4 lines
is this the full API or only part of it. If it is just a part of it, please send the full API for the daemon. Just looking at this piece, I am not really thinking that this is a good API. It is actually pretty much broken :(
Ok, here's an updated version of the Modem Init Daemon DBus API. I hope this is aligned with what we discussed on the IRC.
STE Modem Init Deamon Manager
================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Manager
Object path /

Quoted 8 lines

Methods array{object,dict} GetModems()
Get array of STE Modem objects and their state and properties (out signature 'a(oa{sv})').
The method should only be call once per application. Further changes shall be monitored via StateChange signals.

You also need the following signals:

ModemAdded(object, dict)

ModemRemoved(object)

Quoted 11 lines

STE Modem
================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path variable
Signals StateChange(string State)
The modems state sent from when
a modem state change occurs. State is the only dynamic property in this Interface.

I would personally just go straight for PropertyChanged signal here and not bother with StateChanged. It is actually "...Changed" since at that time the state has already changed ;)

Quoted 16 lines

Properties string State [readonly]
The modems state is dynamic can can have the following values:
"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"on" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.
"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

I personally would call it "ready" instead of "on" since the modem is actually on, it is just not ready yet.

Quoted 4 lines

string CaifAtInterface [readonly]
CAIF Link Layer interface to be used for AT channels for a modem.

I would really just call this "Interface" to make it simpler. Don't think that you are expecting more than just CAIF interface here anyway.

And in addition if we can have the modem serial number here as "Serial" as well would be good. Even it is is not right away available, you can signal a change via PropertyChanged signal.

That way we can construct a proper modem object path inside oFono. I really rather use the serial number and only fallback to the interface name.

Regards

Marcel

Sjur Brændeland

author

2011-01-03 22:30 UTC #12 lore raw

Hi Marcel,

Quoted 9 lines

Quoted 5 lines
Signals StateChange(string State)
The modems state sent from when
a modem state change occurs. State is the only dynamic property in this Interface.
I would personally just go straight for PropertyChanged signal here and not bother with StateChanged. It is actually "...Changed" since at that time the state has already changed ;)

OK, I'll look into this. I thought StateChange was the only dynamic parameter, but I might have been wrong here (see below).

Quoted 5 lines

You also need the following signals:
ModemAdded(object, dict)
ModemRemoved(object)

I think I'd rather add this when I see a use case for it. The Modem Init Deamon would need to
know what GPIOs are associated with what modems, and what CAIF interfaces to use etc.
This information is not dynamic, at least not at the moment. So ModemAdded and ModemRemoved
will not happen in the current implementation, all the modems will be known when the Manager
interface becomes available.

...

Quoted 7 lines

Quoted 4 lines
string CaifAtInterface [readonly]
CAIF Link Layer interface to be used for AT channels for a modem.
I would really just call this "Interface" to make it simpler. Don't think that you are expecting more than just CAIF interface here anyway.

OK, Fair enough.

Quoted 8 lines

And in addition if we can have the modem serial number here as "Serial" as well would be good. Even it is is not right away available, you can signal a change via PropertyChanged signal.
That way we can construct a proper modem object path inside oFono. I really rather use the serial number and only fallback to the interface name.

OK, a Serial property is doable, but I think this is only available after state "on" (ready)
has been reached. The drawback is that my assumption of State being the only dynamic property wrong.
Crap you were right - I might need to add a PropertyChanged signal.

Regards,
Sjur

Marcel Holtmann

2011-01-03 22:54 UTC #13 lore raw

Hi Sjur,

Quoted 19 lines

Quoted 9 lines
Quoted 5 lines
Signals StateChange(string State)
The modems state sent from when
a modem state change occurs. State is the only dynamic property in this Interface.
I would personally just go straight for PropertyChanged signal here and not bother with StateChanged. It is actually "...Changed" since at that time the state has already changed ;)
OK, I'll look into this. I thought StateChange was the only dynamic parameter, but I might have been wrong here (see below).
Quoted 5 lines
You also need the following signals:
ModemAdded(object, dict)
ModemRemoved(object)

Quoted 9 lines

I think I'd rather add this when I see a use case for it. The Modem Init Deamon would need to
know what GPIOs are associated with what modems, and what CAIF interfaces to use etc.
This information is not dynamic, at least not at the moment. So ModemAdded and ModemRemoved
will not happen in the current implementation, all the modems will be known when the Manager
interface becomes available.

what about potential USB based CAIF devices?

Quoted 15 lines

...
Quoted 7 lines
Quoted 4 lines
string CaifAtInterface [readonly]
CAIF Link Layer interface to be used for AT channels for a modem.
I would really just call this "Interface" to make it simpler. Don't think that you are expecting more than just CAIF interface here anyway.
OK, Fair enough.
Quoted 4 lines
And in addition if we can have the modem serial number here as "Serial" as well would be good. Even it is is not right away available, you can signal a change via PropertyChanged signal.

Quoted 7 lines

Quoted 4 lines
That way we can construct a proper modem object path inside oFono. I really rather use the serial number and only fallback to the interface name.
OK, a Serial property is doable, but I think this is only available after state "on" (ready)

That is fine. We have the same case in oFono that the SubscriberIdentity only becomes available a bit later. That is in the end easy to handle.

I would just ask to send the property changed signal for the serial number before sending the signal for on/ready.

Quoted 3 lines

has been reached. The drawback is that my assumption of State being the only dynamic property wrong.
Crap you were right - I might need to add a PropertyChanged signal.

This way you are a lot more flexible in the future.

Regards

Marcel

Sjur Brændeland

author

2011-01-03 23:12 UTC #14 lore raw

Hi Marcel,

Quoted 1 line

what about potential USB based CAIF devices?

Good question, but in a Bridge Setup for a phone this will not really be devices that come and go.
I belive it will still be a fixed HW setup where the Modem Init Daemon will have static knowledge of the
USB modems. But I'll discuss this with my coleagues tomorrow. In case of true USB dongles the modem should look pretty much identical to MBM USB dongles.

Quoted 2 lines

I would just ask to send the property changed signal for the serial number before sending the signal for on/ready.

It will require a re-design of the state machine in the Modem Init Daemon, but I'll check into this tomorrow.

Here is the latest version of the Dbus API:

STE Modem Init Deamon Manager
=============================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.ModemManager
Object path /

Methods array{object,dict} GetModems()

Get array of STE Modem objects and their state and properties (out signature 'a(oa{sv})').

The method should only be call once per application. Further changes shall be monitored via StateChange signals.

STE Modem
=========
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path variable

Signals PropertyChanged(string property, variant value)

This signal indicates a changed value of the given property.

Properties string State [readonly]

"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

string AtInterface[readonly]

CAIF Link Layer interface to be used for AT channels for a modem.

string Serial[optional,readonly]

Serial Number or IMEI for the Modem. The Serial will not be available until the modem can open an AT channel.

Regards,
Sjur

Marcel Holtmann

2011-01-04 09:49 UTC #15 lore raw

Hi Sjur,

Quoted 8 lines

Quoted 1 line
what about potential USB based CAIF devices?
Good question, but in a Bridge Setup for a phone this will not really be devices that come and go.
I belive it will still be a fixed HW setup where the Modem Init Daemon will have static knowledge of the
USB modems. But I'll discuss this with my coleagues tomorrow. In case of true USB dongles the modem should look pretty much identical to MBM USB dongles.

I personally would prefer that USB devices just expose CAIF directly. The whole handling of multiple hardcoded TTY and network interface is only sub-optimal.

I know it is kinda nice to use standard USB class drivers, but the flexibility that CAIF actually gives you goes away.

From my point of view, it should be similar to Phonet/ISI over USB. At least on Linux we have a CAIF subsystem ;)

Quoted 11 lines

Quoted 2 lines
I would just ask to send the property changed signal for the serial number before sending the signal for on/ready.
It will require a re-design of the state machine in the Modem Init Daemon, but I'll check into this tomorrow.
Here is the latest version of the Dbus API:
STE Modem Init Deamon Manager
=============================
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.ModemManager

Just call it com.stericsson.modeminit.Manager. That gives you the chance to also include other methods/signals later on if needed.

Quoted 38 lines

Object path /
Methods array{object,dict} GetModems()
Get array of STE Modem objects and their state and properties (out signature 'a(oa{sv})').
The method should only be call once per application. Further changes shall be monitored via StateChange signals.
STE Modem
=========
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path variable
Signals PropertyChanged(string property, variant value)
This signal indicates a changed value of the given property.
Properties string State [readonly]
The modems state is dynamic can can have the following values:
"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"on" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.
NOTE: Consider change name to "ready"

Quoted 7 lines

"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.
string AtInterface[readonly]
CAIF Link Layer interface to be used for AT channels for a modem.

Why is there an "At" in here? I know that mainly only AT command channels will be used, but the CAIF interface can be also used to request debug channels and other things. So I would just call it Interface only to say what's the interface name is to bind to.

Quoted 4 lines

string Serial[optional,readonly]
Serial Number or IMEI for the Modem. The Serial will not be available until the modem can open an AT channel.

Otherwise, this looks a lot simpler and cleaner.

Regards

Marcel

Sjur Brændeland

author

2011-01-04 19:07 UTC #16 lore raw

v4 cover STE Modem Init Daemon integration

From: Sjur Brændeland <[email protected]>

This is the latest version of the Modem Init Daemon API.

STE Modem Init Deamon Manager
=============================

Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Manager
Object path /

Methods array{object,dict} GetModems()

Get array of STE Modem objects and their state and properties (out signature 'a(oa{sv})').

The method should only be call once per application. Further changes shall be monitored via StateChange signals.

STE Modem
=========
Service com.stericsson.modeminit
Interface com.stericsson.modeminit.Modem
Object path variable

Signals PropertyChanged(string property, variant value)

This signal indicates a changed value of the given property.

Properties string State [readonly]

The modems state is dynamic can can have the following values:
"booting" Modem is powered up (flashed version)
or Modem is powered up and firmware upload is completed. (flashless version)
"upgrading" Firmware upgrade on going
or Flashing manager under execution - modem in service mode.
"ready" Modem has booted and is ready for use.
This implies that all AT channels are available, the modem might be in e.g. flight mode.

"dumping" Modem has crashed and dump is ongoing
"off" Modem is powered off.

string Interface[readonly]

CAIF Link Layer interface to be used for CAIF channels for a modem.

string Serial[optional,readonly]

Serial Number or IMEI for the Modem. The Serial will not be available until the modem can open an AT channel.

Sjur Brændeland (1):
plugin: Add ste modem initd integration

 Makefile.am      |    4 +
 plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Sjur Brændeland

author

2011-01-04 19:07 UTC #17 lore raw

PATCH v4 1/1 plugin: Add ste modem initd integration

In reply to Marcel Holtmann

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.
---
Changes:
- Adapted to new Dbus API, using GetModems method and PropertyChange signal
- Using Serial as modem id
- Review comments from last v3

 Makefile.am      |    4 +
 plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Makefile.am +4 -0

index 8a8555d..6fc4c62 100644
@@ -223,6 +223,10 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/gprs-context.c \
 			drivers/ifxmodem/stk.c
 
+
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.h \

plugins/stemgr.c new file +345 -0

345 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Marcel Holtmann

2011-01-05 22:06 UTC #18 lore raw

Hi Sjur,

Quoted 23 lines

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.
---
Changes:
- Adapted to new Dbus API, using GetModems method and PropertyChange signal
- Using Serial as modem id
- Review comments from last v3
 Makefile.am      |    4 +
 plugins/stemgr.c |  345 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 349 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c
Makefile.am +0 -0
index 8a8555d..6fc4c62 100644

Makefile.am builtin_sources += drivers/atmodem/atutil.h \ · line 229

Quoted 7 lines

Makefile.am +4 -0

@@ -223,6 +223,10 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/gprs-context.c \
 			drivers/ifxmodem/stk.c
 
+
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+

This is a minor nitpick, but I think this is better placed between ste and caif plugin details. And not on top of stemodem.

Quoted 43 lines

Makefile.am +0 -0

 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.h \

plugins/stemgr.c new file +34 -0

new file mode 100644
index 0000000..467d0f8
@@ -0,0 +1,345 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011 ST-Ericsson AB.
+ *
+ *  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 <errno.h>
+#include <string.h>
+#include <net/if.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>

plugins/stemgr.c line 44

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/dbus.h>
+
+/*
+ * ST-Ericsson's Modem Init Daemon is used for controling the modem power
+ * cycles and provides a dbus API for modem state and properties.
+ */
+#define MGR_SERVICE		"com.stericsson.modeminit"
+#define MGR_MGR_INTERFACE	MGR_SERVICE ".Manager"

Really? MGR_MGR and not MGR_MANAGER ;)

Quoted 5 lines

plugins/stemgr.c new file +5 -0

+#define MGR_GET_MODEMS		"GetModems"
+#define GET_MODEMS_TIMEOUT	5000
+
+#define MGR_MODEM_INTERFACE	MGR_SERVICE ".Modem"
+#define PROPERTY_CHANGED	"PropertyChanged"

plugins/stemgr.c line 59

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+
+enum ste_state {
+	STE_PENDING,
+	STE_REGISTERED,
+	STE_RESETTING
+};
+
+struct ste_modem {
+	struct ofono_modem *modem;
+	char *path;

You don't have to necessarily do it this way, but since you are using the path memory also as index for your hash table it might be good to put it first in your struct.

plugins/stemgr.c line 66

Quoted 7 lines

plugins/stemgr.c new file +7 -0

+	enum ste_state state;
+	char *serial;
+	char *interface;
+};
+
+static GHashTable *modem_list;
+static guint mgr_api_watch;

It is more like a service watch or daemon watch.

plugins/stemgr.c line 67

Quoted 1 line

plugins/stemgr.c new file +1 -0
+static guint mgr_state_watch;

And this is property changed watch, right?

plugins/stemgr.c line 69

Quoted 2 lines

plugins/stemgr.c new file +2 -0

+static DBusConnection *connection;
+static gboolean get_modems_call_pending;

What is this exactly for? How many do you expect to have pending?

plugins/stemgr.c line 79

Quoted 9 lines

plugins/stemgr.c new file +9 -0

+static inline gboolean is_ready(const char *action)
+{
+	return g_strcmp0(action, "ready") == 0;
+}
+
+static inline gboolean is_reset(const char *action)
+{
+	return g_strcmp0(action, "dumping") == 0;
+}

This is nasty. Why not convert the state into an enum once and be done with it.

plugins/stemgr.c line 83

Quoted 3 lines

plugins/stemgr.c new file +3 -0

+static void state_change(struct ste_modem *stemodem, const char *action)
+{
+

Scrap this empty line ;)

Quoted 43 lines

plugins/stemgr.c new file +43 -0

+	switch (stemodem->state) {
+	case STE_PENDING:
+
+		if (!is_ready(action))
+			break;
+
+		stemodem->modem = ofono_modem_create(stemodem->serial,
+							"ste");
+		if (stemodem->modem == NULL) {
+			ofono_error("Could not create modem %s, %s",
+					stemodem->path, stemodem->serial);
+			return;
+		}
+
+		DBG("register modem %s, %s", stemodem->path, stemodem->serial);
+		ofono_modem_set_string(stemodem->modem, "Interface",
+							stemodem->interface);
+		ofono_modem_register(stemodem->modem);
+		stemodem->state = STE_REGISTERED;
+		break;
+	case STE_REGISTERED:
+
+		if (is_reset(action)) {
+			DBG("reset ongoing %s", stemodem->path);
+			/* Note: Consider to power off modem here? */
+			stemodem->state = STE_RESETTING;
+		} else if (!is_ready(action)) {
+			DBG("STE modem unregistering %s %s", stemodem->path,
+				action);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_PENDING;
+		}
+
+		break;
+	case STE_RESETTING:
+
+		if (is_ready(action)) {
+			DBG("STE modem reset complete %s", stemodem->path);
+			if (ofono_modem_get_powered(stemodem->modem))
+				ofono_modem_reset(stemodem->modem);
+			stemodem->state = STE_REGISTERED;
+		} else if (!is_reset(action)) {

plugins/stemgr.c line 136

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+			DBG("STE modem unregistering %s %s", stemodem->path,
+				action);
+			ofono_modem_remove(stemodem->modem);
+			stemodem->modem = NULL;
+			stemodem->state = STE_PENDING;
+		}
+
+		break;
+	}
+}

I find this all a bit complicated. Can you quickly describe the logic here in plain words.

Quoted 88 lines

plugins/stemgr.c new file +88 -0

+static void update_property(struct ste_modem *stemodem, const char *prop,
+				DBusMessageIter *iter, const char **state)
+{
+	const char *value;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_STRING)
+		return;
+
+	dbus_message_iter_get_basic(iter, &value);
+
+	if (g_strcmp0(prop, "State") == 0)
+		*state = value;
+	else if (g_strcmp0(prop, "Interface") == 0) {
+		g_free(stemodem->interface);
+		stemodem->interface = g_strdup(value);
+	} else if (g_strcmp0(prop, "Serial") == 0) {
+		g_free(stemodem->serial);
+		stemodem->serial = g_strdup(value);
+	}
+}
+
+static void update_modem_properties(const char *path, DBusMessageIter *iter)
+{
+	const char *state = NULL;
+	struct ste_modem *stemodem = g_hash_table_lookup(modem_list, path);
+
+	if (stemodem == NULL) {
+		stemodem = g_try_new0(struct ste_modem, 1);
+		if (stemodem == NULL)
+			return;
+
+		stemodem->path = g_strdup(path);
+		stemodem->state = STE_PENDING;
+		g_hash_table_insert(modem_list, stemodem->path, stemodem);
+	}
+
+	while (dbus_message_iter_get_arg_type(iter) == DBUS_TYPE_DICT_ENTRY) {
+		DBusMessageIter entry, value;
+		const char *key, *str;
+
+		dbus_message_iter_recurse(iter, &entry);
+		dbus_message_iter_get_basic(&entry, &key);
+
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &value);
+
+		update_property(stemodem, key, &value, &state);
+
+		dbus_message_iter_next(iter);
+	}
+
+	if (state != NULL)
+		state_change(stemodem, state);
+}
+
+static void get_modems_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusMessageIter iter, list;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	get_modems_call_pending = FALSE;
+	dbus_error_init(&err);
+
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("%s: %s\n", err.name, err.message);
+		dbus_error_free(&err);
+		goto done;
+	}
+
+	if (!dbus_message_has_signature(reply, "a(oa{sv})"))
+		goto done;
+
+	if (!dbus_message_iter_init(reply, &iter))
+		goto done;
+
+	dbus_message_iter_recurse(&iter, &list);
+
+	while (dbus_message_iter_get_arg_type(&list) == DBUS_TYPE_STRUCT) {
+		DBusMessageIter entry, dict;
+		const char *path;
+
+		dbus_message_iter_recurse(&list, &entry);
+		dbus_message_iter_get_basic(&entry, &path);
+		dbus_message_iter_next(&entry);
+		dbus_message_iter_recurse(&entry, &dict);
+
+		update_modem_properties(path, &dict);

plugins/stemgr.c line 235

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+
+		dbus_message_iter_next(&list);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+static void get_modems(const char *path)
+{

This path parameter is rather pointless. It is always "/".

plugins/stemgr.c line 240

Quoted 5 lines

plugins/stemgr.c new file +5 -0

+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	if (get_modems_call_pending)
+		return;

I still haven't figured out what you are trying to protect here.

Quoted 33 lines

plugins/stemgr.c new file +33 -0

+	message = dbus_message_new_method_call(MGR_SERVICE, path,
+					MGR_MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	get_modems_call_pending = TRUE;
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter, value;
+	struct ste_modem *stemodem;

plugins/stemgr.c line 284

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+	const char *key, *str;
+	const char *state = NULL;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL) {
+		get_modems("/");
+		return TRUE;
+	}

So here we go. So you are expecting a property changed signal before GetModems finished and therefor you just call it again. This is rather pointless since you get the properties with the return of the GetModems call and these should be the actual ones.

So if the modem path is not in your hash-table, then just ignore the property changed signal.

Quoted 25 lines

plugins/stemgr.c new file +25 -0

+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &state);
+
+	if (state != NULL)
+		state_change(stemodem, state);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	mgr_state_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems("/");
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)

plugins/stemgr.c line 320

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, mgr_state_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);

As a small minor style nitpick, I would add an empty line here.

Quoted 9 lines

plugins/stemgr.c new file +9 -0

+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init()
+{
+	connection = ofono_dbus_get_connection();

plugins/stemgr.c line 339

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);
+	mgr_api_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit()
+{

The only thing you missed here is to remove the property changed watch in case it was registered (aka the init daemon started).

plugins/stemgr.c line 345

Quoted 6 lines

plugins/stemgr.c new file +6 -0

+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, mgr_api_watch);
+}
+
+OFONO_PLUGIN_DEFINE(stemgr, "ST-Ericsson Modem Init Daemon detection", VERSION,
+			OFONO_PLUGIN_PRIORITY_DEFAULT, stemgr_init, stemgr_exit)

Regards

Marcel

Sjur Brændeland

author

2011-01-06 09:38 UTC #19 lore raw

PATCH v5 1/1 plugin: Add ste modem initd integration

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.
---
Hi Marcel.

Quoted 7 lines

Review
Quoted 4 lines
(quoted diff) +4 -0
+
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
This is a minor nitpick, but I think this is better placed between ste and caif plugin details. And not on top of stemodem.

Sure, I've moved this entry.

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+#define MGR_SERVICE          "com.stericsson.modeminit"
+#define MGR_MGR_INTERFACE    MGR_SERVICE ".Manager"
Really? MGR_MGR and not MGR_MANAGER ;)

Oops, s/mid/mgr didn't do it right this time ;-)
Let's call it MGR_INTERFACE instead.

Quoted 7 lines

Review
Quoted 3 lines
(quoted diff) +3 -0
+struct ste_modem {
+     struct ofono_modem *modem;
+     char *path;
You don't have to necessarily do it this way, but since you are using the path memory also as index for your hash table it might be good to put it first in your struct.

Ok, I've put path first in struct.

Quoted 3 lines

Quoted 1 line
+static guint mgr_api_watch;
It is more like a service watch or daemon watch.

Yes, I renamed this to modem_daemon_watch.

Quoted 4 lines

Quoted 2 lines
+static guint mgr_state_watch;
And this is property changed watch, right?

Yes, renamed this to property_changed_watch

Quoted 1 line

What is this exactly for? How many do you expect to have pending?

I have removed this one. get_modems is *only* called when the Modem Init Daemon is first connected.

Quoted 4 lines

(quoted diff) +4 -0

+static inline gboolean is_ready(const char *action)
+{
+     return g_strcmp0(action, "ready") == 0;
+}

...

Quoted 2 lines

This is nasty. Why not convert the state into an enum once and be done with it.

Nasty? Anyway I've changed this to a ste_operation enum,
and I'll give you that - it does make the state machine a more readable.

Quoted 3 lines

I find this all a bit complicated. Can you quickly describe the logic here in plain words.

Sure, I've written some prose describing the state machine. I've also done this function like a standard FSM implementation:

switch(state) {
case STATE_A:

     switch (operation) {
     case OPERATION_1:

...

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+static void get_modems(const char *path)
+{
This path parameter is rather pointless. It is always "/".

Agree.

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+     if (get_modems_call_pending)
+             return;
I still haven't figured out what you are trying to protect here.

....

Quoted 3 lines

(quoted diff) +3 -0

+static gboolean property_changed(DBusConnection *connection,
+                                     DBusMessage *message, void *user_data)
+{

...

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+     stemodem = g_hash_table_lookup(modem_list,
+                                     dbus_message_get_path(message));
+
+     if (stemodem == NULL) {
+             get_modems("/");
+             return TRUE;
+     }

Quoted 7 lines

So here we go. So you are expecting a property changed signal before GetModems finished and therefor you just call it again. This is rather pointless since you get the properties with the return of the GetModems call and these should be the actual ones.
So if the modem path is not in your hash-table, then just ignore the property changed signal.

Yes, agree. This was a left over from previous API. I have removed this completely. get_modems() is only called when Modem Init Daemon API becomes available. If modems ever pop up dynamically I will add a new signal for this in the DBus API as suggested.

Quoted 5 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+static void stemgr_exit()
+{
The only thing you missed here is to remove the property changed watch in case it was registered (aka the init daemon started).

Good catch thanks, I'v removed this watch.

Thank you for reviewing this code, hopefully we're close to completion now ;-)

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  385 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 388 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Makefile.am +3 -0

index 8a8555d..1e4f9df 100644
@@ -292,6 +292,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c
 endif

plugins/stemgr.c new file +385 -0

385 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Marcel Holtmann

2011-01-11 00:58 UTC #20 lore raw

Hi Sjur,

Review

Quoted 2 lines

(quoted diff) +2 -0
+static void get_modems()
+{

so these are declared as get_modems(void) btw. The compiler should have warned you about this.

Quoted 72 lines

plugins/stemgr.c new file +72 -0

+	DBusMessage *message;
+	DBusPendingCall *call;
+
+	message = dbus_message_new_method_call(MGR_SERVICE, "/",
+					MGR_INTERFACE, MGR_GET_MODEMS);
+	if (message == NULL) {
+		ofono_error("Unable to allocate new D-Bus message");
+		goto error;
+	}
+
+	dbus_message_set_auto_start(message, FALSE);
+
+	if (!dbus_connection_send_with_reply(connection, message, &call,
+						GET_MODEMS_TIMEOUT)) {
+		ofono_error("Sending D-Bus message failed");
+		goto error;
+	}
+
+	if (call == NULL) {
+		DBG("D-Bus connection not available");
+		goto error;
+	}
+
+	dbus_pending_call_set_notify(call, get_modems_reply, NULL, NULL);
+	dbus_pending_call_unref(call);
+
+error:
+	dbus_message_unref(message);
+}
+
+static gboolean property_changed(DBusConnection *connection,
+					DBusMessage *message, void *user_data)
+{
+	DBusMessageIter iter;
+	struct ste_modem *stemodem;
+	const char *key;
+	enum ste_operation operation;
+	gboolean operation_valid;
+
+	stemodem = g_hash_table_lookup(modem_list,
+					dbus_message_get_path(message));
+
+	if (stemodem == NULL)
+		return TRUE;
+
+
+	if (!dbus_message_iter_init(message, &iter))
+		return TRUE;
+
+	dbus_message_iter_get_basic(&iter, &key);
+	dbus_message_iter_next(&iter);
+
+	update_property(stemodem, key, &iter, &operation, &operation_valid);
+
+	if (operation_valid)
+		state_change(stemodem, operation);
+
+	return TRUE;
+}
+
+static void mgr_connect(DBusConnection *connection, void *user_data)
+{
+	property_changed_watch = g_dbus_add_signal_watch(connection, NULL,
+						NULL,
+						MGR_MODEM_INTERFACE,
+						PROPERTY_CHANGED,
+						property_changed,
+						NULL, NULL);
+	get_modems();
+}
+
+static void mgr_disconnect(DBusConnection *connection, void *user_data)

plugins/stemgr.c line 359

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+{
+	g_hash_table_remove_all(modem_list);
+	g_dbus_remove_watch(connection, property_changed_watch);
+}
+
+static void destroy_stemodem(gpointer data)
+{
+	struct ste_modem *stemodem = data;
+
+	ofono_modem_remove(stemodem->modem);

I would add an extra empty line for pure visual sake here.

Quoted 12 lines

plugins/stemgr.c new file +12 -0

+	g_free(stemodem->interface);
+	g_free(stemodem->path);
+	g_free(stemodem->serial);
+	g_free(stemodem);
+}
+
+static int stemgr_init()
+{
+	connection = ofono_dbus_get_connection();
+
+	modem_list = g_hash_table_new_full(g_str_hash, g_str_equal,
+						NULL, destroy_stemodem);

plugins/stemgr.c line 381

Quoted 10 lines

plugins/stemgr.c new file +10 -0

+	modem_daemon_watch = g_dbus_add_service_watch(connection, MGR_SERVICE,
+				mgr_connect, mgr_disconnect, NULL, NULL);
+	return 0;
+}
+
+static void stemgr_exit()
+{
+	g_hash_table_destroy(modem_list);
+	g_dbus_remove_watch(connection, modem_daemon_watch);
+	g_dbus_remove_watch(connection, property_changed_watch);

This is not right. You need to remove the property_changed_watch in the disconnect callback. Otherwise if your daemon restarts twice you actually have two watches.

Regards

Marcel

Sjur Brændeland

author

2011-01-11 17:06 UTC #21 lore raw

Hi Marcel.

Quoted 5 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+static void get_modems()
+{
so these are declared as get_modems(void) btw. The compiler should have warned you about this.

I don't get a compiler warning for this one, not even with -Wall -Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:

M15: Use void if function has no parameters
====================================
A function with no parameters must use void in the parameter list. Example:
1)
void foo(void)
{
}

2)
void foo() // Wrong
{
}

...

Quoted 6 lines

plugins/stemgr.c new file +6 -0

+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+     g_hash_table_remove_all(modem_list);
+     g_dbus_remove_watch(connection, property_changed_watch);
+}
+

....

Quoted 9 lines

plugins/stemgr.c line 381
Quoted 5 lines
plugins/stemgr.c new file +5 -0
+static void stemgr_exit()
+{
+     g_hash_table_destroy(modem_list);
+     g_dbus_remove_watch(connection, modem_daemon_watch);
+     g_dbus_remove_watch(connection, property_changed_watch);
This is not right. You need to remove the property_changed_watch in the disconnect callback. Otherwise if your daemon restarts twice you actually have two watches.

It's already present in mgr_disconnect, did you miss that?

Quoted 4 lines · In the previous review 2011/1/5 you said:

plugins/stemgr.c new file +2 -0
+static void stemgr_exit()
+{
The only thing you missed here is to remove the property changed watch in case it was registered (aka the init daemon started).

Anyway, I think the watch handing in the latest patch is correct, we need to do remove_watch for property_changed_watch in both stemgr_exit and in stemgr_disconnect.

Regards,
Sjur

Marcel Holtmann

2011-01-11 21:52 UTC #22 lore raw

Hi Sjur,

Quoted 16 lines

Quoted 2 lines
so these are declared as get_modems(void) btw. The compiler should have warned you about this.
I don't get a compiler warning for this one, not even with -Wall -Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
====================================
A function with no parameters must use void in the parameter list. Example:
1)
void foo(void)
{
}

Quoted 4 lines

2)
void foo() // Wrong
{
}

please go ahead and send a patch for this. I am happily adding it.

Quoted 17 lines

...
Quoted 6 lines
Quoted 6 lines
plugins/stemgr.c new file +6 -0
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+     g_hash_table_remove_all(modem_list);
+     g_dbus_remove_watch(connection, property_changed_watch);
+}
+
....
Quoted 9 lines
plugins/stemgr.c line 381
Quoted 5 lines
plugins/stemgr.c new file +5 -0
+static void stemgr_exit()
+{
+     g_hash_table_destroy(modem_list);
+     g_dbus_remove_watch(connection, modem_daemon_watch);
+     g_dbus_remove_watch(connection, property_changed_watch);
This is not right. You need to remove the property_changed_watch in the disconnect callback. Otherwise if your daemon restarts twice you actually have two watches.

Quoted 1 line

It's already present in mgr_disconnect, did you miss that?

I did miss that. So you are correct, but you should add

property_changed_watch = 0

to mgr_disconnect.

And then do this in stemgr_exit

	if (property_changed_watch > 0)
		g_dbus_remove_watch(...

Quoted 9 lines

Quoted 4 lines · In the previous review 2011/1/5 you said:
plugins/stemgr.c new file +2 -0
+static void stemgr_exit()
+{
The only thing you missed here is to remove the property changed watch in case it was registered (aka the init daemon started).
Anyway, I think the watch handing in the latest patch is correct, we need to do remove_watch for property_changed_watch in both stemgr_exit and in stemgr_disconnect.

I got lost a little bit, but you still need to make sure to not remove a watch on plugin exit in case modem init manager not running as I mentioned above.

Regards

Marcel

Denis Kenzior

2011-01-11 21:56 UTC #23 lore raw

Hi Marcel,

Quoted 18 lines · Marcel Holtmann

Hi Sjur,
Quoted 16 lines
Quoted 2 lines
so these are declared as get_modems(void) btw. The compiler should have warned you about this.
I don't get a compiler warning for this one, not even with -Wall -Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
====================================
A function with no parameters must use void in the parameter list. Example:
1)
void foo(void)
{
}

Quoted 7 lines

Quoted 4 lines
2)
void foo() // Wrong
{
}
please go ahead and send a patch for this. I am happily adding it.

Sorry I'm not following why this is a good idea?

Regards,
-Denis

Marcel Holtmann

2011-01-11 22:05 UTC #24 lore raw

Hi Denis,

Quoted 21 lines

Quoted 21 lines
Quoted 20 lines
Quoted 2 lines
so these are declared as get_modems(void) btw. The compiler should have warned you about this.
I don't get a compiler warning for this one, not even with -Wall -Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
====================================
A function with no parameters must use void in the parameter list. Example:
1)
void foo(void)
{
}
2)
void foo() // Wrong
{
}

Quoted 4 lines

Quoted 2 lines
please go ahead and send a patch for this. I am happily adding it.
Sorry I'm not following why this is a good idea?

we have always declared functions with no parameters as (void) and not as (). At least I have.

Regards

Marcel

Sjur Brændeland

author

2011-01-11 22:39 UTC #25 lore raw

PATCH 1/1 coding-style: Use void if function has no parameters

In reply to Denis Kenzior

From: Sjur Brændeland <[email protected]>

---
Marcel, Denis,

Here is the patch, please apply it if you agree. I'm easy :-)

Regards,
Sjur

 doc/coding-style.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

doc/coding-style.txt +15 -0

index 3bee240..40bb36b 100644
@@ -279,6 +279,21 @@ memset(stuff, 0, sizeof(*stuff));
 memset(stuff, 0, sizeof *stuff); // Wrong
 
 
+M15: Use void if function has no parameters
+===========================================================
+A function with no parameters must use void in the parameter list.
+
+Example:
+1)
+void foo(void)
+{
+}
+
+2)
+void foo()	// Wrong
+{
+}
+
 O1: Shorten the name
 ====================
 Better to use abbreviation, rather than full name, to name a variable,

Signature · 1 line

1.7.0.4

Marcel Holtmann

2011-01-11 22:48 UTC #26 lore raw

Re: [PATCH] coding-style: Use void if function has no parameters

Hi Sjur,

Quoted 2 lines

 doc/coding-style.txt |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

patch has been applied. Thanks.

Regards

Marcel

Sjur Brændeland

author

2011-01-11 22:24 UTC #27 lore raw

PATCH v6 1/1 plugin: Add ste modem initd integration

In reply to Marcel Holtmann

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.

---
 Makefile.am      |    3 +
 plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 393 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Makefile.am +3 -0

index 09dc9ad..6ccc13b 100644
@@ -293,6 +293,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c
 endif

plugins/stemgr.c new file +390 -0

390 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Marcel Holtmann

2011-01-11 22:35 UTC #28 lore raw

Hi Sjur,

Quoted 7 lines

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.

Quoted 5 lines

---
 Makefile.am      |    3 +
 plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 393 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

you are out of luck today ;)

Applying: plugin: Add ste modem initd integration
error: Makefile.am: does not match index
Patch failed at 0001 plugin: Add ste modem initd integration

Regards

Marcel

Sjur Brændeland

author

2011-01-11 22:41 UTC #29 lore raw

Hi Marcel.

Quoted 1 line

you are out of luck today ;)

Darn, I forgot to pull. Hang on I'll be back shortly....

Regards,
Sjur

Sjur Brændeland

author

2011-01-11 22:56 UTC #30 lore raw

PATCH v7 1/1 plugin: Add ste modem initd integration

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.
---
Hi Marcel,

Am I still out of luck?

$ git pull
Already up-to-date.
$ git am 0001-plugin-Add-ste-modem-initd-integration.patch
Applying: plugin: Add ste modem initd integration

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  390 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 393 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Makefile.am +3 -0

index cc30624..4dec90a 100644
@@ -298,6 +298,9 @@ builtin_sources += plugins/ifx.c
 builtin_modules += ste
 builtin_sources += plugins/ste.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += caif
 builtin_sources += plugins/caif.c

plugins/stemgr.c new file +390 -0

390 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Marcel Holtmann

2011-01-11 22:58 UTC #31 lore raw

Hi Sjur,

Quoted 7 lines

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.

<snip>

Quoted 6 lines

Am I still out of luck?

$ git pull
Already up-to-date.
$ git am 0001-plugin-Add-ste-modem-initd-integration.patch
Applying: plugin: Add ste modem initd integration

patch has been applied. Thanks.

Regards

Marcel

Sjur Brændeland

author

2010-12-21 22:54 UTC #32 lore raw

PATCH v3 1/1 plugin: Add ste modem initd integration

In reply to Marcel Holtmann

From: Sjur Brændeland <[email protected]>

This patch introduces auto discovery of ST-Ericsson modems. ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a Modem Init Daemon responsible for start, power cycles, flashing etc. This STE plugin monitors the modem state exposed from the Modem Init Damon's Dbus API. When the modem is in state "on" the STE modem is created and registered. Muliple modem instances, and reset handling is supported.
---
Hi Marcel.

I think I have resolved most of your comments on my last patch set, including the massive confusion caused by calling this MID ;-)

The only remark left is "ResendState". I have done this as simple as possible where you just re-send all state information for all modem instances and implicit get the object-path (serial-id) for each modem when the StateChange signal is received.

I really don't think resending the state information should be of much concern as this only happens once - when oFono starts. The extra cost of waking the other two STE daemons should be negligible, as they most likely are initializing and preparing for communicating with the STE-modem anyway.

Regards,
Sjur

 Makefile.am      |    3 +
 plugins/stemgr.c |  310 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 313 insertions(+), 0 deletions(-)
 create mode 100644 plugins/stemgr.c

Makefile.am +3 -0

index 12b3c33..d9c0505 100644
@@ -221,6 +221,9 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ifxmodem/gprs-context.c \
 			drivers/ifxmodem/stk.c
 
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
 builtin_modules += stemodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/stemodem/stemodem.h \

plugins/stemgr.c new file +309 -0

309 lines not on this page · raw message

Signature · 1 line

1.7.0.4