[RFC 3/3] STE-plugin: Adding STE plugin

[RFC 3/3] STE-plugin: Adding STE plugin

[sjur.brandeland]

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

From: Sjur Brandeland <sjur.brandeland@stericsson.com>

Added implementation for STE modem; STE modem driver, and STE specific
drivers for gprs, network registration and voice call. 

This patch uses CAIF sockets. CAIF patch for net-next-2.6 will be
contributed on netdev(a)vger.kernel.org soon.

---
 Makefile.am                             |   11 +
 drivers/stemodem/gprs-context.c         |  612 ++++++++++++++++++++++++++++++
 drivers/stemodem/network-registration.c |  225 +++++++++++
 drivers/stemodem/stemodem.c             |  109 ++++++
 drivers/stemodem/stemodem.h             |   33 ++
 drivers/stemodem/voicecall.c            |  615 +++++++++++++++++++++++++++++++
 plugins/ste.c                           |  246 ++++++++++++
 7 files changed, 1851 insertions(+), 0 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 276b478..bae8dcf 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -129,6 +129,14 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/calypsomodem/calypsomodem.c \
 			drivers/calypsomodem/voicecall.c
 
+builtin_modules += stemodem
+builtin_sources += drivers/atmodem/atutil.h \
+			drivers/stemodem/stemodem.h \
+			drivers/stemodem/stemodem.c \
+			drivers/stemodem/voicecall.c \
+			drivers/stemodem/network-registration.c \
+			drivers/stemodem/gprs-context.c
+
 builtin_modules += hfpmodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/hfpmodem/hfpmodem.h \
@@ -168,6 +176,9 @@ builtin_sources += plugins/g1.c
 builtin_modules += calypso
 builtin_sources += plugins/calypso.c
 
+builtin_modules += ste
+builtin_sources += plugins/ste.c
+
 builtin_modules += mbm
 builtin_sources += plugins/mbm.c
 
diff --git a/drivers/stemodem/gprs-context.c b/drivers/stemodem/gprs-context.c
new file mode 100644
index 0000000..1d5d8db
--- /dev/null
+++ b/drivers/stemodem/gprs-context.c
@@ -0,0 +1,612 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
+ *
+ *  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
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/gprs-context.h>
+#include <ofono/gprs.h>
+
+#include <linux/types.h>
+#include <net/if.h>
+#include <sys/ioctl.h>
+#include <arpa/inet.h>
+#include <linux/caif/caif_socket.h>
+#include <linux/caif/if_caif.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+#include "stemodem.h"
+
+#define MAX_CAIF_DEVICES 7
+#define MAX_DNS 5
+#define AUTH_BUF_LENGTH (OFONO_GPRS_MAX_USERNAME_LENGTH + \
+			OFONO_GPRS_MAX_PASSWORD_LENGTH + 128)
+
+static const char *cgact_prefix[] = { "+CGACT:", NULL };
+static const char *none_prefix[] = { NULL };
+
+static GSList *g_caif_devices;
+
+struct gprs_context_data {
+	GAtChat *chat;
+	unsigned int active_context;
+	char *username;
+	char *password;
+};
+
+struct conn_info {
+	unsigned int cid;
+	unsigned int device;
+	unsigned int channel_id;
+	char *interface;
+};
+
+static gint conn_compare_by_cid(gconstpointer a, gconstpointer b)
+{
+	const struct conn_info *conn = a;
+	unsigned int used = GPOINTER_TO_UINT(b);
+
+	if (used != conn->cid)
+		return 1;
+
+	return 0;
+}
+
+/* TODO: should parse_xml function to be moved to e.g. atutil? */
+static char *parse_xml(char * xml, char* tag)
+{
+    char *begin;
+    char *end;
+    int len;
+    char *res = NULL;
+    char *start = (char *)g_malloc(strlen(tag)+3);
+    char *stop = (char *)g_malloc(strlen(tag)+4);
+
+    sprintf(start, "<%s>", tag);
+    sprintf(stop, "</%s>", tag);
+
+    begin = strstr(xml, start);
+	if (begin == NULL)
+		goto error;
+
+	end = strstr(begin, stop);
+	if (end == NULL)
+		goto error;
+
+	begin += strlen(start);
+	len = end - begin;
+	res = (char *)g_malloc(len+1);
+	strncpy(res, begin, len);
+	res[len] = 0;
+
+error:
+	free(start);
+	free(stop);
+	return res;
+}
+
+static struct conn_info *conn_info_create(
+							unsigned int device,
+							unsigned int channel_id)
+{
+	struct conn_info *connection = g_try_new0(struct conn_info, 1);
+
+	if (!connection)
+		return NULL;
+
+	connection->cid = 0;
+	connection->device = device;
+	connection->channel_id = channel_id;
+
+	return connection;
+}
+
+/* Creates a new or removes an existing IP interface for CAIF.
+ * Sets the ip address, subnet mask and MTU size at creation.
+ *
+*/
+static gboolean caif_if_create_remove(const char *interface,
+					const char *ip, const char *mtu,
+					const char *mask, unsigned int connid,
+					gboolean create)
+{
+	int s;
+	struct sockaddr_in sin;
+	static struct ifcaif_param param;
+	static struct ifreq ifr;
+
+	param.ipv4_connid = connid;
+	ifr.ifr_data = (void *) &param;
+	strcpy(ifr.ifr_name, interface);
+
+	s = socket(AF_CAIF, SOCK_SEQPACKET, CAIFPROTO_AT);
+	if (s < 0) {
+		ofono_debug("Failed to create socket for CAIF interface");
+		goto error;
+	}
+
+	if (create) {
+		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
+			ofono_debug("Failed to create IP interface for CAIF");
+			goto error;
+		}
+
+		s = socket(PF_INET, SOCK_DGRAM, 0);
+		if (s < 0) {
+			ofono_debug("Failed to create socket.");
+			goto error;
+		}
+
+		/* Set IP address */
+		memset(&sin, 0, sizeof(struct sockaddr));
+		sin.sin_family = AF_INET;
+
+		if (inet_pton(AF_INET, ip, &sin.sin_addr) <= 0) {
+			ofono_debug("inet_pton failed, will not be"
+				"able to set the IP address");
+			goto error;
+		}
+		memcpy(&ifr.ifr_addr, &sin, sizeof(struct sockaddr));
+
+		if (ioctl(s, SIOCSIFADDR, &ifr) < 0) {
+			ofono_debug("Failed to set IP address for"
+			" interface: %s", ifr.ifr_name);
+			goto error;
+		}
+
+		/* Set subnet mask */
+		memset(&sin, 0, sizeof(struct sockaddr));
+		sin.sin_family = AF_INET;
+
+		if (inet_pton(AF_INET, mask, &sin.sin_addr) <= 0) {
+			ofono_debug("inet_pton failed, will not be"
+				"able to set the subnet mask");
+		} else {
+			memcpy(&ifr.ifr_addr, &sin, sizeof(struct sockaddr));
+
+			if (ioctl(s, SIOCSIFNETMASK, &ifr) < 0)
+				ofono_debug("Failed to set subnet mask for"
+					"interface: %s", ifr.ifr_name);
+		}
+
+		/* Set MTU */
+		ifr.ifr_mtu = atoi(mtu);
+
+		if (ioctl(s, SIOCSIFMTU, &ifr) < 0)
+			ofono_debug("Failed to set MTU for interface: %s",
+				ifr.ifr_name);
+	} else {
+		if (ioctl(s, SIOCGIFINDEX, &ifr) == 0) {
+			if (ioctl(s, SIOCCAIFNETREMOVE, &ifr) < 0) {
+				ofono_debug("Failed to remove IP interface"
+					"for CAIF");
+				goto error;
+			}
+		} else {
+			ofono_debug("Did not find interface (%s) to remove",
+					interface);
+			goto error;
+		}
+	}
+
+	return TRUE;
+
+error:
+	return FALSE;
+}
+
+static void ste_eppsd_down_cb(gboolean ok,
+					GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_gprs_context_cb_t cb = cbd->cb;
+	struct ofono_gprs_context *gc = cbd->user;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct ofono_error error;
+	struct conn_info *conn;
+	GSList *l;
+
+	dump_response("ste_eppsd_down_cb", ok, result);
+
+	if (!ok)
+		goto error;
+
+	l = g_slist_find_custom(g_caif_devices,
+				GUINT_TO_POINTER(gcd->active_context),
+				conn_compare_by_cid);
+
+	if (!l) {
+		ofono_debug("Did not find data (used caif device) for"
+					"connection with cid; %d",
+					gcd->active_context);
+		goto error;
+	}
+	conn = l->data;
+	if (!caif_if_create_remove(conn->interface, NULL,
+		NULL, NULL, conn->channel_id, FALSE)) {
+		ofono_debug("Failed to remove caif interface %s.",
+			conn->interface);
+	}
+	conn->cid = 0;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, cbd->data);
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+}
+
+static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = 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;
+	GAtResultIter iter;
+	GSList *l;
+	int i;
+	int numdns = 0;
+	char *res_string;
+	const char *ip = NULL;
+	const char *netmask = NULL;
+	const char *gateway = NULL;
+	const char *mtu = NULL;
+	const char *dns[MAX_DNS + 1];
+	const char *str;
+
+	dump_response("ste_eppsd_up_cb", ok, result);
+
+	l = g_slist_find_custom(g_caif_devices,
+				GUINT_TO_POINTER(gcd->active_context),
+				conn_compare_by_cid);
+
+	if (!l) {
+		ofono_debug("Did not find data (device and channel id)"
+					"for connection with cid; %d",
+					gcd->active_context);
+		goto error;
+	}
+	conn = l->data;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+	for (i = 0; i < g_at_result_num_response_lines(result); i++) {
+		g_at_result_iter_next(&iter, NULL);
+		res_string = strdup(g_at_result_iter_raw_line(&iter));
+
+		if (strstr(res_string, "ip_address")) {
+			ip = g_strdup(parse_xml(res_string,
+						"ip_address"));
+		} else if ((strstr(res_string, "subnet_mask"))) {
+			netmask = g_strdup(parse_xml(res_string,
+						"subnet_mask"));
+		} else if ((strstr(res_string, "mtu"))) {
+			mtu = g_strdup(parse_xml(res_string,
+						"mtu"));
+		} else if ((strstr(res_string, "default_gateway"))) {
+			gateway = g_strdup(parse_xml(res_string,
+						"default_gateway"));
+		} else if ((strstr(res_string, "dns_server"))) {
+			str = g_strdup(parse_xml(res_string,
+						"dns_server"));
+
+			if (numdns < MAX_DNS)
+				dns[numdns++] = str;
+		}
+	}
+	dns[numdns] = NULL;
+
+	conn->interface = g_malloc(10);
+	if (!conn->interface)
+		goto error;
+
+	sprintf(conn->interface, "caif%u", conn->device);
+
+	if (!caif_if_create_remove(conn->interface, ip,
+			mtu, netmask, conn->channel_id, TRUE)) {
+		ofono_error("Failed to create caif interface %s.",
+			conn->interface);
+		conn->interface = NULL;
+	}
+
+	/* Set back to strict syntax check */
+	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsmv1());
+
+	CALLBACK_WITH_SUCCESS(cb, conn->interface,
+				FALSE, ip, netmask, gateway, dns, cbd->data);
+	return;
+
+error:
+	ofono_debug("ste_eppsd_up_cb error");
+
+	if (conn)
+		conn->cid = 0;
+
+	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, cbd->data);
+}
+
+static void ste_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = 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 conn_info *conn;
+	char buf[AUTH_BUF_LENGTH];
+	GSList *l;
+
+	dump_response("cgdcont_cb", ok, result);
+
+	if (!ok) {
+		struct ofono_error error;
+
+		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;
+	}
+
+	/* Set username and password */
+	sprintf(buf, "AT*EIAAUW=%d,1,\"%s\",\"%s\"", gcd->active_context,
+		gcd->username, gcd->password);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				NULL, NULL, NULL) == 0)
+		goto error;
+
+	ncbd = g_memdup(cbd, sizeof(struct cb_data));
+
+	/* Need to change to gsm_permissive syntax in order to
+	 * parse the response from EPPSD (xml) */
+	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsm_permissive());
+
+	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(0),
+				conn_compare_by_cid);
+
+	if (!l) {
+		ofono_debug("at_cgdcont_cb, no more available devices");
+		goto error;
+	}
+	conn = l->data;
+	conn->cid = gcd->active_context;
+	sprintf(buf, "AT*EPPSD=1,%u,%u", conn->channel_id, conn->cid);
+
+	if (g_at_chat_send(gcd->chat, buf, NULL,
+				ste_eppsd_up_cb, ncbd, g_free) > 0)
+		return;
+error:
+	if (ncbd)
+		g_free(ncbd);
+
+	gcd->active_context = 0;
+
+	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL,
+						NULL, NULL, cbd->data);
+}
+
+static void ste_gprs_activate_primary(struct ofono_gprs_context *gc,
+				const struct ofono_gprs_primary_context *ctx,
+				 ofono_gprs_context_up_cb_t cb, void *data)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[OFONO_GPRS_MAX_APN_LENGTH + 128];
+	int len;
+
+	if (!cbd)
+		goto error;
+
+	gcd->active_context = ctx->cid;
+	gcd->username = g_strdup(ctx->username);
+	gcd->password = g_strdup(ctx->password);
+	cbd->user = gc;
+
+	len = sprintf(buf, "AT+CGDCONT=%u,\"IP\"", ctx->cid);
+
+	if (ctx->apn)
+		snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"",
+				ctx->apn);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				ste_cgdcont_cb, cbd, g_free) > 0)
+		return;
+error:
+	if (cbd)
+		g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, NULL, 0, NULL, NULL, NULL, NULL, data);
+}
+
+static void ste_gprs_deactivate_primary(struct ofono_gprs_context *gc,
+					unsigned int id,
+					ofono_gprs_context_cb_t cb, void *data)
+{
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	struct conn_info *conn;
+	char buf[64];
+	GSList *l;
+
+	if (!cbd)
+		goto error;
+
+	gcd->active_context = id;
+	cbd->user = gc;
+
+	l = g_slist_find_custom(g_caif_devices, GUINT_TO_POINTER(id),
+				conn_compare_by_cid);
+
+	if (!l) {
+		ofono_debug("at_gprs_deactivate_primary, did not find"
+			"data (channel id) for connection with cid; %d", id);
+		goto error;
+	}
+	conn = l->data;
+
+	sprintf(buf, "AT*EPPSD=0,%u,%u", conn->channel_id, id);
+
+	if (g_at_chat_send(gcd->chat, buf, none_prefix,
+				ste_eppsd_down_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	if (cbd)
+		g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void ste_cgact_read_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	gint cid, state;
+	GAtResultIter iter;
+
+	dump_response("cgact_read_cb", ok, result);
+
+	if (!ok)
+		return;
+	g_at_result_iter_init(&iter, result);
+
+	while (g_at_result_iter_next(&iter, "+CGACT:")) {
+
+		if (!g_at_result_iter_next_number(&iter, &cid))
+			continue;
+
+		if ((unsigned int) cid != gcd->active_context)
+			continue;
+
+		if (!g_at_result_iter_next_number(&iter, &state))
+			continue;
+
+		if (state == 1)
+			continue;
+
+		ofono_gprs_context_deactivated(gc, gcd->active_context);
+		gcd->active_context = 0;
+
+		break;
+	}
+}
+
+static void cgev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs_context *gc = user_data;
+	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+	GAtResultIter iter;
+	const char *event;
+
+	dump_response("cgev_notify", TRUE, result);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CGEV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
+		return;
+
+	if (g_str_has_prefix(event, "NW REACT ") ||
+			g_str_has_prefix(event, "NW DEACT ") ||
+			g_str_has_prefix(event, "ME DEACT ")) {
+		/* Ask what primary contexts are active now */
+
+		g_at_chat_send(gcd->chat, "AT+CGACT?", cgact_prefix,
+				ste_cgact_read_cb, gc, NULL);
+
+		return;
+	}
+}
+
+static int ste_gprs_context_probe(struct ofono_gprs_context *gc,
+					unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct gprs_context_data *gcd;
+	struct conn_info *ci;
+	int i;
+
+	gcd = g_new0(struct gprs_context_data, 1);
+	gcd->chat = chat;
+
+	g_at_chat_register(gcd->chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
+
+	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);
+	}
+
+	return 0;
+}
+
+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) g_free, NULL);
+	g_slist_free(g_caif_devices);
+	g_caif_devices = NULL;
+
+	ofono_gprs_context_set_data(gc, NULL);
+	g_free(gcd);
+}
+
+static struct ofono_gprs_context_driver driver = {
+	.name			= "stemodem",
+	.probe			= ste_gprs_context_probe,
+	.remove			= ste_gprs_context_remove,
+	.activate_primary	= ste_gprs_activate_primary,
+	.deactivate_primary	= ste_gprs_deactivate_primary,
+};
+
+void ste_gprs_context_init()
+{
+	ofono_gprs_context_driver_register(&driver);
+}
+
+void ste_gprs_context_exit()
+{
+	ofono_gprs_context_driver_unregister(&driver);
+}
diff --git a/drivers/stemodem/network-registration.c b/drivers/stemodem/network-registration.c
new file mode 100644
index 0000000..4eeb239
--- /dev/null
+++ b/drivers/stemodem/network-registration.c
@@ -0,0 +1,225 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen@stericsson.com.
+ *
+ *  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
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/netreg.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+#include "stemodem.h"
+
+static const char *cind_prefix[] = { "+CIND:", NULL };
+
+struct netreg_data {
+	GAtChat *chat;
+	char mcc[OFONO_MAX_MCC_LENGTH + 1];
+	char mnc[OFONO_MAX_MNC_LENGTH + 1];
+};
+
+static void ciev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	int strength, ind;
+	GAtResultIter iter;
+
+	dump_response("ciev_notify", TRUE, result);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIEV:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &ind))
+		return;
+
+	if (ind == 2) { /* signal strength indication */
+		if (!g_at_result_iter_next_number(&iter, &strength))
+			return;
+
+		strength = (strength * 100) / 5;
+		ofono_netreg_strength_notify(netreg, strength);
+	}
+}
+
+static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_netreg_strength_cb_t cb = cbd->cb;
+	int strength;
+	GAtResultIter iter;
+	struct ofono_error error;
+
+	dump_response("cind_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		cb(&error, -1, cbd->data);
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIND:")) {
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+		return;
+	}
+
+	/* Skip battery charge level, which is the first reported  */
+	g_at_result_iter_skip_next(&iter);
+
+	g_at_result_iter_next_number(&iter, &strength);
+
+	strength = (strength * 100) / 5;
+
+	cb(&error, strength, cbd->data);
+}
+
+static void ste_signal_strength(struct ofono_netreg *netreg,
+				ofono_netreg_strength_cb_t cb, void *data)
+{
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	if (!cbd)
+		goto error;
+
+	if (g_at_chat_send(nd->chat, "AT+CIND?", cind_prefix,
+				cind_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	if (cbd)
+		g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, -1, data);
+}
+
+static void creg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	GAtResultIter iter;
+	int status;
+	int lac = -1, ci = -1, tech = -1;
+	const char *str;
+
+	dump_response("creg_notify", TRUE, result);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CREG:"))
+		return;
+
+	g_at_result_iter_next_number(&iter, &status);
+
+	if (g_at_result_iter_next_string(&iter, &str) == TRUE)
+		lac = strtol(str, NULL, 16);
+	else
+		goto out;
+
+	if (g_at_result_iter_next_string(&iter, &str) == TRUE)
+		ci = strtol(str, NULL, 16);
+	else
+		goto out;
+
+	g_at_result_iter_next_number(&iter, &tech);
+
+out:
+	ofono_debug("creg_notify: %d, %d, %d, %d", status, lac, ci, tech);
+
+	ofono_netreg_status_notify(netreg, status, lac, ci, tech);
+}
+
+static void ste_network_registration_initialized(gboolean ok, GAtResult *result,
+							gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+
+	if (!ok) {
+		ofono_error("Unable to initialize Network Registration");
+		ofono_netreg_remove(netreg);
+		return;
+	}
+
+	g_at_chat_register(nd->chat, "+CREG:",
+				creg_notify, FALSE, netreg, NULL);
+
+	g_at_chat_register(nd->chat, "+CIEV:",
+				ciev_notify, FALSE, netreg, NULL);
+
+	ofono_netreg_register(netreg);
+}
+
+static int ste_netreg_probe(struct ofono_netreg *netreg, unsigned int vendor,
+				void *data)
+{
+	GAtChat *chat = data;
+	struct netreg_data *nd;
+
+	nd = g_new0(struct netreg_data, 1);
+
+	nd->chat = chat;
+	ofono_netreg_set_data(netreg, nd);
+
+	g_at_chat_send(chat, "AT+CMER=3,0,0,1", NULL, NULL, NULL, NULL);
+
+	g_at_chat_send(chat, "AT+CREG=2", NULL,
+				ste_network_registration_initialized,
+				netreg, NULL);
+	return 0;
+}
+
+static struct ofono_netreg_driver driver = {
+	.name				= "stemodem",
+	.probe				= ste_netreg_probe,
+	.remove				= NULL,
+	.registration_status 	= NULL,
+	.current_operator 		= NULL,
+	.list_operators			= NULL,
+	.register_auto			= NULL,
+	.register_manual		= NULL,
+	.deregister			= NULL,
+	.strength			= ste_signal_strength,
+};
+
+void ste_netreg_init()
+{
+	ofono_netreg_driver_register(&driver);
+}
+
+void ste_netreg_exit()
+{
+	ofono_netreg_driver_unregister(&driver);
+}
diff --git a/drivers/stemodem/stemodem.c b/drivers/stemodem/stemodem.c
new file mode 100644
index 0000000..0d840ea
--- /dev/null
+++ b/drivers/stemodem/stemodem.c
@@ -0,0 +1,109 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
+ * 
+ *  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 <glib.h>
+#include <gatchat.h>
+#include <ofono/log.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/types.h>
+#include <ofono/voicecall.h>
+#include <ofono/netreg.h>
+
+#include "stemodem.h"
+
+static int stemodem_init(void)
+{
+	/* Initialize voicecall driver */
+	struct ofono_voicecall_driver *at_vcdrv;
+	struct ofono_voicecall_driver *ste_vcdrv;
+	struct ofono_netreg_driver *at_netdrv;
+	struct ofono_netreg_driver *ste_netdrv;
+
+	ste_voicecall_init();
+
+	at_vcdrv = ofono_voicecall_driver_get("atmodem");
+	ste_vcdrv = ofono_voicecall_driver_get("stemodem");
+
+	if (at_vcdrv && ste_vcdrv) {
+		ste_vcdrv->remove = at_vcdrv->remove;
+		ste_vcdrv->swap_without_accept = at_vcdrv->swap_without_accept;
+		ste_vcdrv->send_tones = at_vcdrv->send_tones;
+	} else {
+		if (!ste_vcdrv)
+			ofono_debug("Could not get ofono_voicecall_driver"
+					"from stemodem");
+		if (!at_vcdrv)
+			ofono_debug("Could not get ofono_voicecall_driver"
+					"from atmodem");
+	}
+
+	/* Initialize netreg driver */
+	ste_netreg_init();
+
+	at_netdrv = ofono_netreg_driver_get("atmodem");
+	ste_netdrv = ofono_netreg_driver_get("stemodem");
+
+	if (at_netdrv && ste_netdrv) {
+		ste_netdrv->remove = at_netdrv->remove;
+		ste_netdrv->registration_status =
+			at_netdrv->registration_status;
+		ste_netdrv->current_operator = at_netdrv->current_operator;
+		ste_netdrv->list_operators = at_netdrv->list_operators;
+		ste_netdrv->register_auto = at_netdrv->register_auto;
+		ste_netdrv->register_manual = at_netdrv->register_manual;
+	} else {
+		if (!ste_netdrv)
+			ofono_debug("Could not get ofono_netreg_driver driver"
+					"from stemodem");
+		if (!at_netdrv)
+			ofono_debug("Could not get ofono_netreg_driver driver"
+					"from atemodem");
+	}
+
+	/* Initialize gprs driver */
+	ste_gprs_context_init();
+
+	return 0;
+}
+
+static void stemodem_exit(void)
+{
+	ste_voicecall_exit();
+	ste_netreg_exit();
+	ste_gprs_context_exit();
+}
+
+/* atmodem must be registered before stemodem (for the ofono_netreg_driver_get
+ * to return correct information). This is achieved by setting priority low
+ * for stemodem; OFONO_PLUGIN_PRIORITY_LOW */
+
+OFONO_PLUGIN_DEFINE(stemodem, "STE modem driver", VERSION,
+			OFONO_PLUGIN_PRIORITY_LOW,
+			stemodem_init, stemodem_exit)
diff --git a/drivers/stemodem/stemodem.h b/drivers/stemodem/stemodem.h
new file mode 100644
index 0000000..83bdcaa
--- /dev/null
+++ b/drivers/stemodem/stemodem.h
@@ -0,0 +1,33 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
+ *
+ *  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
+ *
+ */
+
+#include <drivers/atmodem/atutil.h>
+
+extern void ste_voicecall_init();
+extern void ste_voicecall_exit();
+
+extern void	ste_netreg_init();
+extern void	ste_netreg_exit();
+
+extern void	ste_gprs_context_init();
+extern void	ste_gprs_context_exit();
diff --git a/drivers/stemodem/voicecall.c b/drivers/stemodem/voicecall.c
new file mode 100644
index 0000000..c00fc61
--- /dev/null
+++ b/drivers/stemodem/voicecall.c
@@ -0,0 +1,615 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
+ *
+ *  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
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/voicecall.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+#include "common.h"
+
+#include "stemodem.h"
+
+enum call_status_ste {
+	STE_CALL_STATUS_IDLE = 0,
+	STE_CALL_STATUS_CALLING = 1,
+	STE_CALL_STATUS_CONNECTING = 2,
+	STE_CALL_STATUS_ACTIVE = 3,
+	STE_CALL_STATUS_HOLD = 4,
+	STE_CALL_STATUS_WAITING = 5,
+	STE_CALL_STATUS_ALERTING = 6,
+	STE_CALL_STATUS_BUSY = 7
+};
+
+static const char *none_prefix[] = { NULL };
+
+struct voicecall_data {
+	GSList *calls;
+	unsigned int local_release;
+	GAtChat *chat;
+};
+
+struct release_id_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int id;
+};
+
+struct change_state_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int affected_types;
+};
+
+/* Translate from the ECAV-based STE-status to CLCC based status */
+static int call_status_ste_to_ofono(int status)
+{
+	switch (status) {
+	case STE_CALL_STATUS_IDLE:
+		return CALL_STATUS_DISCONNECTED;
+	case STE_CALL_STATUS_CALLING:
+		return CALL_STATUS_DIALING;
+	case STE_CALL_STATUS_CONNECTING:
+		return CALL_STATUS_ALERTING;
+	case STE_CALL_STATUS_ACTIVE:
+		return CALL_STATUS_ACTIVE;
+	case STE_CALL_STATUS_HOLD:
+		return CALL_STATUS_HELD;
+	case STE_CALL_STATUS_WAITING:
+		return CALL_STATUS_WAITING;
+	case STE_CALL_STATUS_ALERTING:
+		return CALL_STATUS_INCOMING;
+	case STE_CALL_STATUS_BUSY:
+		return CALL_STATUS_DISCONNECTED;
+	default:
+		return CALL_STATUS_DISCONNECTED;
+	}
+}
+
+static gint call_compare(gconstpointer a, gconstpointer b)
+{
+	const struct ofono_call *ca = a;
+	const struct ofono_call *cb = b;
+
+	if (ca->id < cb->id)
+		return -1;
+
+	if (ca->id > cb->id)
+		return 1;
+
+	return 0;
+}
+
+static gint call_compare_by_id(gconstpointer a, gconstpointer b)
+{
+	const struct ofono_call *call = a;
+	unsigned int id = GPOINTER_TO_UINT(b);
+
+	if (id < call->id)
+		return -1;
+
+	if (id > call->id)
+		return 1;
+
+	return 0;
+}
+
+static struct ofono_call *create_call(struct ofono_voicecall *vc, int type,
+					int direction, int status,
+					const char *num, int num_type, int clip)
+{
+	struct voicecall_data *d = ofono_voicecall_get_data(vc);
+	struct ofono_call *call;
+
+	/* Generate a call structure for the waiting call */
+	call = g_try_new0(struct ofono_call, 1);
+	if (!call)
+		return NULL;
+
+	call->type = type;
+	call->direction = direction;
+	call->status = status;
+
+	if (clip != 2) {
+		strncpy(call->phone_number.number, num,
+			OFONO_MAX_PHONE_NUMBER_LENGTH);
+		call->phone_number.type = num_type;
+	}
+
+	call->clip_validity = clip;
+
+	d->calls = g_slist_insert_sorted(d->calls, call, call_compare);
+
+	return call;
+}
+
+static void ste_generic_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct change_state_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	dump_response("ste_generic_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok && req->affected_types) {
+		GSList *l;
+		struct ofono_call *call;
+
+		for (l = vd->calls; l; l = l->next) {
+			call = l->data;
+
+			if (req->affected_types & (0x1 << call->status))
+				vd->local_release |= (0x1 << call->id);
+		}
+	}
+
+	/* We have to callback after we schedule a poll if required */
+	req->cb(&error, req->data);
+}
+
+static void release_id_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct release_id_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	dump_response("release_id_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok)
+		vd->local_release = 0x1 << req->id;
+
+	/* We have to callback after we schedule a poll if required */
+	req->cb(&error, req->data);
+}
+
+static void atd_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct ofono_error error;
+	ofono_voicecall_cb_t cb = cbd->cb;
+
+	dump_response("atd_cb", ok, result);
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok)
+		goto out;
+
+out:
+	cb(&error, cbd->data);
+}
+
+static void ste_dial(struct ofono_voicecall *vc,
+			const struct ofono_phone_number *ph,
+			enum ofono_clir_option clir, enum ofono_cug_option cug,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[256];
+
+	if (!cbd)
+		goto error;
+
+	cbd->user = vc;
+
+	if (ph->type == 145)
+		sprintf(buf, "ATD+%s", ph->number);
+	else
+		sprintf(buf, "ATD%s", ph->number);
+
+	switch (clir) {
+	case OFONO_CLIR_OPTION_INVOCATION:
+		strcat(buf, "I");
+		break;
+	case OFONO_CLIR_OPTION_SUPPRESSION:
+		strcat(buf, "i");
+		break;
+	default:
+		break;
+	}
+
+	switch (cug) {
+	case OFONO_CUG_OPTION_INVOCATION:
+		strcat(buf, "G");
+		break;
+	default:
+		break;
+	}
+
+	strcat(buf, ";");
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				atd_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	if (cbd)
+		g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void ste_template(const char *cmd, struct ofono_voicecall *vc,
+			GAtResultFunc result_cb, unsigned int affected_types,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
+
+	if (!req)
+		goto error;
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->affected_types = affected_types;
+
+	if (g_at_chat_send(vd->chat, cmd, none_prefix,
+				result_cb, req, g_free) > 0)
+		return;
+
+error:
+	if (req)
+		g_free(req);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void ste_answer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	ste_template("ATA", vc, ste_generic_cb, 0, cb, data);
+}
+
+static void ste_hangup(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	/* Hangup all calls */
+	ste_template("AT+CHUP", vc, ste_generic_cb, 0x3f, cb, data);
+}
+
+static void ste_hold_all_active(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	ste_template("AT+CHLD=2", vc, ste_generic_cb, 0, cb, data);
+}
+
+static void ste_release_all_held(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int held_status = 0x1 << 1;
+	ste_template("AT+CHLD=0", vc, ste_generic_cb, held_status, cb, data);
+}
+
+static void ste_set_udub(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int incoming_or_waiting = (0x1 << 4) | (0x1 << 5);
+	ste_template("AT+CHLD=0", vc, ste_generic_cb, incoming_or_waiting,
+			cb, data);
+}
+
+static void ste_release_all_active(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	ste_template("AT+CHLD=1", vc, ste_generic_cb, 0x1, cb, data);
+}
+
+static void ste_release_specific(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct release_id_req *req = g_try_new0(struct release_id_req, 1);
+	char buf[32];
+	struct ofono_call *call;
+	GSList *l;
+	int ac = 0;
+
+	if (!req)
+		goto error;
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->id = id;
+
+	/* Count active calls. If there is more than one active call
+	 * we cannot use ATH, as it will terminate all calls.
+	 * The reason for using ATH and not CHLD is that
+	 * emergency calls can not be terminated with AT+CHLD.
+	 */
+	for (l = vd->calls; l; l = l->next) {
+		call = l->data;
+
+		if (call->status == CALL_STATUS_ACTIVE)
+			ac++;
+	}
+
+	l = g_slist_find_custom(vd->calls, GUINT_TO_POINTER(id),
+				call_compare_by_id);
+	if (l == NULL) {
+		ofono_error("Hangup request for unknow call");
+		goto error;
+	}
+	call = l->data;
+	/* Check the state of the call, as AT+CHLD does not terminate
+	 * calls in state Dialing, Alerting and Incoming */
+	if (call->status == CALL_STATUS_DIALING ||
+	    call->status == CALL_STATUS_ALERTING ||
+	    call->status == CALL_STATUS_INCOMING)
+		sprintf(buf, "ATH");
+
+	/* Waiting call can not be terminated by at+chld=1x,
+	 * have to use at+chld = 0, but that will also terminate
+	 * other held calls. Bug in STE's AT module.
+	 */
+	else if (call->status == CALL_STATUS_WAITING)
+		sprintf(buf, "AT+CHLD=0");
+
+	else {
+		/* A held call can not be released by ATH, need to use CHLD */
+		if (ac > 1 || call->status == CALL_STATUS_HELD)
+			sprintf(buf, "AT+CHLD=1%d", id);
+		else /* This is the last call, ok to use ATH. */
+			sprintf(buf, "ATH");
+	}
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				release_id_cb, req, g_free) > 0)
+		return;
+
+error:
+	if (req)
+		g_free(req);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void ste_private_chat(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	char buf[32];
+
+	sprintf(buf, "AT+CHLD=2%d", id);
+	ste_template(buf, vc, ste_generic_cb, 0, cb, data);
+}
+
+static void ste_create_multiparty(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	ste_template("AT+CHLD=3", vc, ste_generic_cb, 0, cb, data);
+}
+
+static void ste_transfer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	/* Held & Active */
+	unsigned int transfer = 0x1 | 0x2;
+
+	/* Transfer can puts held & active calls together and disconnects
+	 * from both.  However, some networks support transfering of
+	 * dialing/ringing calls as well.
+	 */
+	transfer |= 0x4 | 0x8;
+
+	ste_template("AT+CHLD=4", vc, ste_generic_cb, transfer, cb, data);
+}
+
+static void ste_deflect(struct ofono_voicecall *vc,
+			const struct ofono_phone_number *ph,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	char buf[128];
+	unsigned int incoming_or_waiting = (0x1 << 4) | (0x1 << 5);
+
+	sprintf(buf, "AT+CTFR=\"%s\",%d", ph->number, ph->type);
+	ste_template(buf, vc, ste_generic_cb, incoming_or_waiting, cb, data);
+}
+
+static void ecav_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GAtResultIter iter;
+	const char *num;
+	int id;
+	int status;
+	int call_type;
+	int num_type;
+	struct ofono_call *new_call;
+	struct ofono_call *existing_call = NULL;
+	GSList *l;
+
+	/* Parse ECAV */
+	dump_response("ecav_notify", TRUE, result);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "*ECAV:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &id))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &call_type))
+		return;
+
+	/* Skip process id and exit cause */
+	g_at_result_iter_skip_next(&iter);
+	g_at_result_iter_skip_next(&iter);
+
+	status = call_status_ste_to_ofono(status);
+	if (status == CALL_STATUS_DIALING ||
+	    status == CALL_STATUS_WAITING ||
+	    status == CALL_STATUS_INCOMING) {
+		if (!g_at_result_iter_next_string(&iter, &num))
+			return;
+
+		if (!g_at_result_iter_next_number(&iter, &num_type))
+			return;
+	}
+
+	/* Handle the call according to the status.
+	 * If it doesn't exists we make a new one
+	 */
+	l = g_slist_find_custom(vd->calls, GUINT_TO_POINTER(id),
+				call_compare_by_id);
+	if (l)
+		existing_call = l->data;
+
+	if (l == NULL && status != CALL_STATUS_DIALING &&
+					 status != CALL_STATUS_WAITING &&
+					 status != CALL_STATUS_INCOMING) {
+		ofono_error("ECAV notification for unknow call. "
+					"id: %d, status: %d", id, status);
+		return;
+	}
+
+	switch (status) {
+	case CALL_STATUS_DISCONNECTED: {
+		enum ofono_disconnect_reason reason;
+		existing_call->status = status;
+
+		if (vd->local_release & (0x1 << existing_call->id))
+			reason = OFONO_DISCONNECT_REASON_LOCAL_HANGUP;
+		else
+			reason = OFONO_DISCONNECT_REASON_REMOTE_HANGUP;
+
+		if (call_type == BEARER_CLASS_VOICE)
+			ofono_voicecall_disconnected(vc,
+					existing_call->id, reason, NULL);
+		vd->calls = g_slist_remove(vd->calls, l->data);
+		break;
+	}
+	case CALL_STATUS_DIALING:
+	case CALL_STATUS_WAITING:
+	case CALL_STATUS_INCOMING: {
+		int clip_validity;
+		int direction;
+
+		if (status == CALL_STATUS_DIALING)
+			direction = CALL_DIRECTION_MOBILE_ORIGINATED;
+		else
+			direction = CALL_DIRECTION_MOBILE_TERMINATED;
+
+		if ((strlen(num)) > 0)
+			clip_validity = CLIP_VALIDITY_VALID;
+		else
+			clip_validity = CLIP_VALIDITY_NOT_AVAILABLE;
+
+		new_call = create_call(vc, call_type, direction,
+							status, num, num_type,
+							clip_validity);
+
+		if (!new_call) {
+			ofono_error("Unable to malloc."
+				"Call management is fubar");
+			return;
+		}
+		new_call->id = id;
+
+		if (call_type == BEARER_CLASS_VOICE)
+			ofono_voicecall_notify(vc, new_call);
+		break;
+	}
+	case CALL_STATUS_ALERTING:
+	case CALL_STATUS_ACTIVE:
+	case CALL_STATUS_HELD:
+		existing_call->status = status;
+
+		if (call_type == BEARER_CLASS_VOICE)
+			ofono_voicecall_notify(vc, existing_call);
+		break;
+	default:
+		break;
+	}
+}
+
+static int ste_voicecall_probe(struct ofono_voicecall *vc, unsigned int vendor,
+				void *data)
+{
+	GAtChat *chat = data;
+	struct voicecall_data *vd;
+
+	vd = g_new0(struct voicecall_data, 1);
+	vd->chat = chat;
+
+	ofono_voicecall_set_data(vc, vd);
+
+	g_at_chat_send(chat, "AT*ECAM=1", NULL, NULL, NULL, NULL);
+	g_at_chat_register(chat, "*ECAV:", ecav_notify, FALSE, vc, NULL);
+	ofono_voicecall_register(vc);
+
+	return 0;
+}
+
+static struct ofono_voicecall_driver driver = {
+	.name			= "stemodem",
+	.probe			= ste_voicecall_probe,
+	.remove			= NULL,
+	.dial			= ste_dial,
+	.answer			= ste_answer,
+	.hangup			= ste_hangup,
+	.hold_all_active	= ste_hold_all_active,
+	.release_all_held	= ste_release_all_held,
+	.set_udub		    = ste_set_udub,
+	.release_all_active	= ste_release_all_active,
+	.release_specific	= ste_release_specific,
+	.private_chat		= ste_private_chat,
+	.create_multiparty	= ste_create_multiparty,
+	.transfer		    = ste_transfer,
+	.deflect		    = ste_deflect,
+	.swap_without_accept = NULL,
+	.send_tones		    = NULL
+};
+
+void ste_voicecall_init()
+{
+	ofono_voicecall_driver_register(&driver);
+}
+
+void ste_voicecall_exit()
+{
+	ofono_voicecall_driver_unregister(&driver);
+}
diff --git a/plugins/ste.c b/plugins/ste.c
new file mode 100644
index 0000000..72477a5
--- /dev/null
+++ b/plugins/ste.c
@@ -0,0 +1,246 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010 ST-Ericsson AB.
+ *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
+ *
+ *  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 <stdlib.h>
+
+#include <glib.h>
+#include <gatchat.h>
+#include <gattty.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/call-barring.h>
+#include <ofono/call-forwarding.h>
+#include <ofono/call-meter.h>
+#include <ofono/call-settings.h>
+#include <ofono/devinfo.h>
+#include <ofono/message-waiting.h>
+#include <ofono/netreg.h>
+#include <ofono/phonebook.h>
+#include <ofono/sim.h>
+#include <ofono/sms.h>
+#include <ofono/ssn.h>
+#include <ofono/ussd.h>
+#include <ofono/call-volume.h>
+#include <ofono/voicecall.h>
+#include <drivers/atmodem/vendor.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
+#include <linux/caif/caif_socket.h>
+#include <unistd.h>
+
+struct ste_data {
+	GAtChat *chat;
+};
+
+static int ste_probe(struct ofono_modem *modem)
+{
+	struct ste_data *data;
+
+	DBG("%p", modem);
+
+	data = g_try_new0(struct ste_data, 1);
+
+	if (!data)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+	return 0;
+}
+
+static void ste_remove(struct ofono_modem *modem)
+{
+	struct ste_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_modem_set_data(modem, NULL);
+
+	g_at_chat_unref(data->chat);
+	g_free(data);
+}
+
+static void ste_debug(const char *str, void *user_data)
+{
+	ofono_info("%s", str);
+}
+
+static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+
+	DBG("");
+
+	if (!ok)
+		ofono_modem_set_powered(modem, FALSE);
+
+	ofono_modem_set_powered(modem, TRUE);
+}
+
+static int ste_enable(struct ofono_modem *modem)
+{
+	struct ste_data *data = ofono_modem_get_data(modem);
+	GIOChannel *channel;
+	GAtSyntax *syntax;
+	int fd;
+	struct sockaddr_caif addr = {
+		.family = AF_CAIF,
+		.u.at.type = CAIF_ATTYPE_PLAIN
+	};
+
+	DBG("%p", modem);
+
+	/* Create a CAIF socket for AT Service */
+	fd = socket(AF_CAIF, SOCK_SEQPACKET, CAIFPROTO_AT);
+
+	/* Connect to the AT Service at the modem */
+	connect(fd, (struct sockaddr *) &addr, sizeof(addr));
+	channel = g_io_channel_unix_new(fd);
+
+	if (!channel)  {
+		close(fd);
+		return -EIO;
+	}
+	g_io_channel_set_close_on_unref(channel, TRUE);
+
+	syntax = g_at_syntax_new_gsmv1();
+
+	data->chat = g_at_chat_new(channel, syntax);
+	g_at_syntax_unref(syntax);
+	g_io_channel_unref(channel);
+
+	if (!data->chat)
+		return -ENOMEM;
+
+	if (getenv("OFONO_AT_DEBUG"))
+		g_at_chat_set_debug(data->chat, ste_debug, NULL);
+
+	g_at_chat_send(data->chat, "AT+CMEE=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(data->chat, "AT+CFUN=1", NULL, cfun_enable, modem, NULL);
+
+	return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ste_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_shutdown(data->chat);
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+}
+
+static int ste_disable(struct ofono_modem *modem)
+{
+	struct ste_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	if (!data->chat)
+		return 0;
+
+	g_at_chat_cancel_all(data->chat);
+	g_at_chat_unregister_all(data->chat);
+	g_at_chat_send(data->chat, "AT+CFUN=4", NULL,
+					cfun_disable, modem, NULL);
+
+	return -EINPROGRESS;
+}
+
+static void ste_pre_sim(struct ofono_modem *modem)
+{
+	struct ste_data *data = ofono_modem_get_data(modem);
+	DBG("%p", modem);
+
+	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
+	ofono_sim_create(modem, 0, "atmodem", data->chat);
+	ofono_voicecall_create(modem, 0, "stemodem", data->chat);
+}
+
+static void ste_post_sim(struct ofono_modem *modem)
+{
+	struct ste_data *data = ofono_modem_get_data(modem);
+	struct ofono_message_waiting *mw;
+	struct ofono_gprs *gprs;
+	struct ofono_gprs_context *gc;
+
+	DBG("%p", modem);
+	ofono_ussd_create(modem, 0, "atmodem", data->chat);
+	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
+	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
+	ofono_netreg_create(modem, 0, "stemodem", data->chat);
+	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
+	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
+	ofono_ssn_create(modem, 0, "atmodem", data->chat);
+	ofono_sms_create(modem, 0, "atmodem", data->chat);
+	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
+	ofono_call_volume_create(modem, 0, "atmodem", data->chat);
+
+	gprs = ofono_gprs_create(modem,
+			OFONO_VENDOR_STE, "atmodem", data->chat);
+	gc = ofono_gprs_context_create(modem, 0, "stemodem", data->chat);
+
+	if (gprs && gc)
+		ofono_gprs_add_context(gprs, gc);
+
+	mw = ofono_message_waiting_create(modem);
+
+	if (mw)
+		ofono_message_waiting_register(mw);
+}
+
+static struct ofono_modem_driver ste_driver = {
+	.name		= "ste",
+	.probe		= ste_probe,
+	.remove		= ste_remove,
+	.enable		= ste_enable,
+	.disable	= ste_disable,
+	.pre_sim	= ste_pre_sim,
+	.post_sim	= ste_post_sim,
+};
+
+static int ste_init(void)
+{
+	return ofono_modem_driver_register(&ste_driver);
+}
+
+static void ste_exit(void)
+{
+	ofono_modem_driver_unregister(&ste_driver);
+}
+
+OFONO_PLUGIN_DEFINE(ste, "ST-Ericsson modem driver", VERSION,
+		OFONO_PLUGIN_PRIORITY_DEFAULT, ste_init, ste_exit)
-- 
1.6.0.4

Re: [RFC 3/3] STE-plugin: Adding STE plugin

[Marcel Holtmann]

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

Hi Sjur,

> Added implementation for STE modem; STE modem driver, and STE specific
> drivers for gprs, network registration and voice call. 
> 
> This patch uses CAIF sockets. CAIF patch for net-next-2.6 will be
> contributed on netdev(a)vger.kernel.org soon.

can you please split these into smaller patches so they are easier to
review. I am thinking of something the like this; one per network
registration, one per voice call, one per GPRS.

You are making some core changes and we need to have a close look at
them and what the potential impact would be.

This is not a complete review, but I making some comments on obvious
things.

> index 0000000..1d5d8db
> --- /dev/null
> +++ b/drivers/stemodem/gprs-context.c
> @@ -0,0 +1,612 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2009  Intel Corporation. All rights reserved.
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.

As mentioned before, we track authorship via GIT. So ensure that the GIT
commits are properly done.

> +#include <linux/types.h>
> +#include <net/if.h>
> +#include <sys/ioctl.h>
> +#include <arpa/inet.h>
> +#include <linux/caif/caif_socket.h>
> +#include <linux/caif/if_caif.h>

This is dangerous until the CAIF subsystem is actually merged and
present everywhere. Please consider adding an option to enable stemodem
driver (like we do for atmodem and isimodem).

It might make sense to have a local copy of the required structure and
constants to allow an easier complication. Of course this depends on
having CAIF at least in net-next-2.6 tree.

> +/* TODO: should parse_xml function to be moved to e.g. atutil? */

First question. Why not use the XML parse that comes with GLib.

> +static char *parse_xml(char * xml, char* tag)
> +{

Please use consistent coding style. It is "char *xml". And it is always
like this. No extra space after * or char*.

> +    char *begin;
> +    char *end;
> +    int len;
> +    char *res = NULL;
> +    char *start = (char *)g_malloc(strlen(tag)+3);
> +    char *stop = (char *)g_malloc(strlen(tag)+4);

No casting of malloc function. It is not needed. And extra spaces
between operation. Meaning malloc(strlen(tag) + 3).

This applies to all code.

> +	if (create) {
> +		if (ioctl(s, SIOCCAIFNETNEW, &ifr) < 0) {
> +			ofono_debug("Failed to create IP interface for CAIF");
> +			goto error;
> +		}
> +
> +		s = socket(PF_INET, SOCK_DGRAM, 0);
> +		if (s < 0) {
> +			ofono_debug("Failed to create socket.");
> +			goto error;
> +		}
> +
> +		/* Set IP address */
> +		memset(&sin, 0, sizeof(struct sockaddr));
> +		sin.sin_family = AF_INET;
> +
> +		if (inet_pton(AF_INET, ip, &sin.sin_addr) <= 0) {
> +			ofono_debug("inet_pton failed, will not be"
> +				"able to set the IP address");
> +			goto error;
> +		}
> +		memcpy(&ifr.ifr_addr, &sin, sizeof(struct sockaddr));
> +
> +		if (ioctl(s, SIOCSIFADDR, &ifr) < 0) {
> +			ofono_debug("Failed to set IP address for"
> +			" interface: %s", ifr.ifr_name);
> +			goto error;
> +		}

oFono is never setting the IP address, netmask or any other
configuration option. The only thing that oFono does is bringing the
interface up. Systems like ConnMan do the IP configuration.

Please see the comments in the documentation on how we expose IP
interfaces. Check with ConnMan and how we configure them.

> +		if (ioctl(s, SIOCSIFMTU, &ifr) < 0)
> +			ofono_debug("Failed to set MTU for interface: %s",
> +				ifr.ifr_name);
> +	} else {
> +		if (ioctl(s, SIOCGIFINDEX, &ifr) == 0) {
> +			if (ioctl(s, SIOCCAIFNETREMOVE, &ifr) < 0) {
> +				ofono_debug("Failed to remove IP interface"
> +					"for CAIF");
> +				goto error;
> +			}
> +		} else {
> +			ofono_debug("Did not find interface (%s) to remove",
> +					interface);
> +			goto error;
> +		}
> +	}

Having create and remove in the same function seems hard to read. Why
not have one for creating the interface and one for removing it. From
what I see so far, it is not much more code. And makes it a lot easier
to read and understand for other people.

> +	g_at_result_iter_init(&iter, result);
> +	for (i = 0; i < g_at_result_num_response_lines(result); i++) {
> +		g_at_result_iter_next(&iter, NULL);
> +		res_string = strdup(g_at_result_iter_raw_line(&iter));
> +
> +		if (strstr(res_string, "ip_address")) {
> +			ip = g_strdup(parse_xml(res_string,
> +						"ip_address"));
> +		} else if ((strstr(res_string, "subnet_mask"))) {
> +			netmask = g_strdup(parse_xml(res_string,
> +						"subnet_mask"));
> +		} else if ((strstr(res_string, "mtu"))) {
> +			mtu = g_strdup(parse_xml(res_string,
> +						"mtu"));
> +		} else if ((strstr(res_string, "default_gateway"))) {
> +			gateway = g_strdup(parse_xml(res_string,
> +						"default_gateway"));
> +		} else if ((strstr(res_string, "dns_server"))) {
> +			str = g_strdup(parse_xml(res_string,
> +						"dns_server"));
> +
> +			if (numdns < MAX_DNS)
> +				dns[numdns++] = str;
> +		}

I would prefer if you do the parsing in one function that goes through
the XML ones. Just give it pointers to the fields you wanna have it
extract or a special combined structure. This looks highly inefficient
to me.

> +	conn->interface = g_malloc(10);
> +	if (!conn->interface)
> +		goto error;

Please double check with the GLib documentation on memory allocation.
The g_malloc failure would result in halt of the program. What you want
here is g_try_malloc.

Also for 10 bytes, please don't bother with an allocation. Just include
a char array in the conn structure.

> +
> +	sprintf(conn->interface, "caif%u", conn->device);
> +
> +	if (!caif_if_create_remove(conn->interface, ip,
> +			mtu, netmask, conn->channel_id, TRUE)) {
> +		ofono_error("Failed to create caif interface %s.",
> +			conn->interface);
> +		conn->interface = NULL;
> +	}

Yeah, I really prefer caif_if_create() and caif_if_remove(). This is not
really intuitive at all.

> +	/* Need to change to gsm_permissive syntax in order to
> +	 * parse the response from EPPSD (xml) */
> +	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsm_permissive());

Is this an issue with your modem firmware or an issue in the v1 parser.
If it is the modem firmware, then just use the permissive parser all the
time and switch to E0.

> +static void cgev_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs_context *gc = user_data;
> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> +	GAtResultIter iter;
> +	const char *event;
> +
> +	dump_response("cgev_notify", TRUE, result);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CGEV:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
> +		return;
> +
> +	if (g_str_has_prefix(event, "NW REACT ") ||
> +			g_str_has_prefix(event, "NW DEACT ") ||
> +			g_str_has_prefix(event, "ME DEACT ")) {
> +		/* Ask what primary contexts are active now */
> +
> +		g_at_chat_send(gcd->chat, "AT+CGACT?", cgact_prefix,
> +				ste_cgact_read_cb, gc, NULL);
> +
> +		return;
> +	}
> +}

The return statement in the if clause is kinda pointless. Seems like
either your code tried to be more complex or you are missing something.

> +static int stemodem_init(void)
> +{
> +	/* Initialize voicecall driver */
> +	struct ofono_voicecall_driver *at_vcdrv;
> +	struct ofono_voicecall_driver *ste_vcdrv;
> +	struct ofono_netreg_driver *at_netdrv;
> +	struct ofono_netreg_driver *ste_netdrv;
> +
> +	ste_voicecall_init();
> +
> +	at_vcdrv = ofono_voicecall_driver_get("atmodem");
> +	ste_vcdrv = ofono_voicecall_driver_get("stemodem");
> +
> +	if (at_vcdrv && ste_vcdrv) {
> +		ste_vcdrv->remove = at_vcdrv->remove;
> +		ste_vcdrv->swap_without_accept = at_vcdrv->swap_without_accept;
> +		ste_vcdrv->send_tones = at_vcdrv->send_tones;
> +	} else {
> +		if (!ste_vcdrv)
> +			ofono_debug("Could not get ofono_voicecall_driver"
> +					"from stemodem");
> +		if (!at_vcdrv)
> +			ofono_debug("Could not get ofono_voicecall_driver"
> +					"from atmodem");
> +	}
> +
> +	/* Initialize netreg driver */
> +	ste_netreg_init();
> +
> +	at_netdrv = ofono_netreg_driver_get("atmodem");
> +	ste_netdrv = ofono_netreg_driver_get("stemodem");
> +
> +	if (at_netdrv && ste_netdrv) {
> +		ste_netdrv->remove = at_netdrv->remove;
> +		ste_netdrv->registration_status =
> +			at_netdrv->registration_status;
> +		ste_netdrv->current_operator = at_netdrv->current_operator;
> +		ste_netdrv->list_operators = at_netdrv->list_operators;
> +		ste_netdrv->register_auto = at_netdrv->register_auto;
> +		ste_netdrv->register_manual = at_netdrv->register_manual;

So far this, I really like to see a description on what's the difference
in the network registration and voice call atoms.

We do need to understand what is actually needed.

> +	/* Create a CAIF socket for AT Service */
> +	fd = socket(AF_CAIF, SOCK_SEQPACKET, CAIFPROTO_AT);
> +
> +	/* Connect to the AT Service at the modem */
> +	connect(fd, (struct sockaddr *) &addr, sizeof(addr));
> +	channel = g_io_channel_unix_new(fd);

You need to check the results of socket() and connect() calls.

Regards

Marcel

Re: [RFC 3/3] STE-plugin: Adding STE plugin

[Denis Kenzior]

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

Hi Sjur,

> diff --git a/drivers/stemodem/network-registration.c
>  b/drivers/stemodem/network-registration.c new file mode 100644
> index 0000000..4eeb239
> --- /dev/null
> +++ b/drivers/stemodem/network-registration.c
> +static void ciev_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_netreg *netreg = user_data;
> +	int strength, ind;
> +	GAtResultIter iter;
> +
> +	dump_response("ciev_notify", TRUE, result);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CIEV:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &ind))
> +		return;
> +
> +	if (ind == 2) { /* signal strength indication */
> +		if (!g_at_result_iter_next_number(&iter, &strength))
> +			return;
> +
> +		strength = (strength * 100) / 5;
> +		ofono_netreg_strength_notify(netreg, strength);
> +	}
> +}
> +
> +static void cind_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_netreg_strength_cb_t cb = cbd->cb;
> +	int strength;
> +	GAtResultIter iter;
> +	struct ofono_error error;
> +
> +	dump_response("cind_cb", ok, result);
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		cb(&error, -1, cbd->data);
> +		return;
> +	}
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CIND:")) {
> +		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> +		return;
> +	}
> +
> +	/* Skip battery charge level, which is the first reported  */
> +	g_at_result_iter_skip_next(&iter);
> +
> +	g_at_result_iter_next_number(&iter, &strength);
> +
> +	strength = (strength * 100) / 5;
> +
> +	cb(&error, strength, cbd->data);
> +}
> +
> +static void ste_signal_strength(struct ofono_netreg *netreg,
> +				ofono_netreg_strength_cb_t cb, void *data)
> +{
> +	struct netreg_data *nd = ofono_netreg_get_data(netreg);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +
> +	if (!cbd)
> +		goto error;
> +
> +	if (g_at_chat_send(nd->chat, "AT+CIND?", cind_prefix,
> +				cind_cb, cbd, g_free) > 0)
> +		return;
> +
> +error:
> +	if (cbd)
> +		g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, -1, data);
> +}

I really prefer the above to be integrated into drivers/atmodem/network-
registration.c and quirked.  There are many modems that will need this (MBM 
modems in particular.)

> +
> +static void creg_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_netreg *netreg = user_data;
> +	GAtResultIter iter;
> +	int status;
> +	int lac = -1, ci = -1, tech = -1;
> +	const char *str;
> +
> +	dump_response("creg_notify", TRUE, result);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CREG:"))
> +		return;
> +
> +	g_at_result_iter_next_number(&iter, &status);
> +
> +	if (g_at_result_iter_next_string(&iter, &str) == TRUE)
> +		lac = strtol(str, NULL, 16);
> +	else
> +		goto out;
> +
> +	if (g_at_result_iter_next_string(&iter, &str) == TRUE)
> +		ci = strtol(str, NULL, 16);
> +	else
> +		goto out;
> +
> +	g_at_result_iter_next_number(&iter, &tech);
> +
> +out:
> +	ofono_debug("creg_notify: %d, %d, %d, %d", status, lac, ci, tech);
> +
> +	ofono_netreg_status_notify(netreg, status, lac, ci, tech);
> +}

What is different about this function from the regular CREG unsolicited parser 
in drivers/atmodem/atutil.c at_util_parse_reg_unsolicited?

> +
> +	g_at_chat_register(nd->chat, "+CIEV:",
> +				ciev_notify, FALSE, netreg, NULL);

Move this to drivers/atmodem/network-registration.c and quirk it.

> +static int ste_netreg_probe(struct ofono_netreg *netreg, unsigned int
>  vendor, +				void *data)
> +{
> +	GAtChat *chat = data;
> +	struct netreg_data *nd;
> +
> +	nd = g_new0(struct netreg_data, 1);
> +
> +	nd->chat = chat;
> +	ofono_netreg_set_data(netreg, nd);
> +
> +	g_at_chat_send(chat, "AT+CMER=3,0,0,1", NULL, NULL, NULL, NULL);

Same as above.

> +
> +	g_at_chat_send(chat, "AT+CREG=2", NULL,
> +				ste_network_registration_initialized,
> +				netreg, NULL);

Assuming STE modems support AT+CREG=?, the atmodem netreg driver should take 
care of this properly as well.  With these changes you no longer need a 
dedicated stemodem driver for netreg or the ofono_netreg_driver_get part.

> +++ b/drivers/stemodem/voicecall.c
> +static gint call_compare(gconstpointer a, gconstpointer b)
> +{
> +	const struct ofono_call *ca = a;
> +	const struct ofono_call *cb = b;
> +
> +	if (ca->id < cb->id)
> +		return -1;
> +
> +	if (ca->id > cb->id)
> +		return 1;
> +
> +	return 0;
> +}

Please use at_util_call_compare

> +
> +static gint call_compare_by_id(gconstpointer a, gconstpointer b)
> +{
> +	const struct ofono_call *call = a;
> +	unsigned int id = GPOINTER_TO_UINT(b);
> +
> +	if (id < call->id)
> +		return -1;
> +
> +	if (id > call->id)
> +		return 1;
> +
> +	return 0;
> +}

You might find that you simply don't need this function, but if you do, please 
add it to atutil.c

> +static void ste_release_specific(struct ofono_voicecall *vc, int id,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct release_id_req *req = g_try_new0(struct release_id_req, 1);
> +	char buf[32];
> +	struct ofono_call *call;
> +	GSList *l;
> +	int ac = 0;
> +
> +	if (!req)
> +		goto error;
> +
> +	req->vc = vc;
> +	req->cb = cb;
> +	req->data = data;
> +	req->id = id;
> +
> +	/* Count active calls. If there is more than one active call
> +	 * we cannot use ATH, as it will terminate all calls.
> +	 * The reason for using ATH and not CHLD is that
> +	 * emergency calls can not be terminated with AT+CHLD.
> +	 */
> +	for (l = vd->calls; l; l = l->next) {
> +		call = l->data;
> +
> +		if (call->status == CALL_STATUS_ACTIVE)
> +			ac++;
> +	}
> +
> +	l = g_slist_find_custom(vd->calls, GUINT_TO_POINTER(id),
> +				call_compare_by_id);
> +	if (l == NULL) {
> +		ofono_error("Hangup request for unknow call");
> +		goto error;
> +	}
> +	call = l->data;
> +	/* Check the state of the call, as AT+CHLD does not terminate
> +	 * calls in state Dialing, Alerting and Incoming */
> +	if (call->status == CALL_STATUS_DIALING ||
> +	    call->status == CALL_STATUS_ALERTING ||
> +	    call->status == CALL_STATUS_INCOMING)
> +		sprintf(buf, "ATH");
> +
> +	/* Waiting call can not be terminated by at+chld=1x,
> +	 * have to use at+chld = 0, but that will also terminate
> +	 * other held calls. Bug in STE's AT module.
> +	 */
> +	else if (call->status == CALL_STATUS_WAITING)
> +		sprintf(buf, "AT+CHLD=0");
> +
> +	else {
> +		/* A held call can not be released by ATH, need to use CHLD */
> +		if (ac > 1 || call->status == CALL_STATUS_HELD)
> +			sprintf(buf, "AT+CHLD=1%d", id);
> +		else /* This is the last call, ok to use ATH. */
> +			sprintf(buf, "ATH");
> +	}
> +
> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
> +				release_id_cb, req, g_free) > 0)
> +		return;
> +
> +error:
> +	if (req)
> +		g_free(req);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}

All of this logic needs to go away.  The core already handles all of this, 
including selection of ATH/CHLD, waiting/held.  Please review src/voicecall.c.  
If the core logic is not sufficient or does not properly handle a certain case, 
then lets see if we can fix the core first.  Drivers should not concern 
themselves with this stuff.

With this in mind, you might not need to track any state in this driver at 
all.  See drivers/calypsomodem/voicecall.c for details. TI's CPI notifications 
are almost exactly the same as the STE ECAV.

RE: [RFC 3/3] STE-plugin: Adding STE plugin

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.
Thank you for your feedback. We hope to get new patch-set out tomorrow
with most of your comments fixed.

Marcel Holtmann wrote:
> Hi Sjur,
> 
> review. I am thinking of something the like this; one per network
> registration, one per voice call, one per GPRS.  
> 
> You are making some core changes and we need to have a close look at
> them and what the potential impact would be. 

Sure, will split up better next time. 

>> +#include <arpa/inet.h>
>> +#include <linux/caif/caif_socket.h>
>> +#include <linux/caif/if_caif.h>
> 
> This is dangerous until the CAIF subsystem is actually merged and
> present everywhere. Please consider adding an option to enable
> stemodem driver (like we do for atmodem and isimodem).  

OK, we'll put this on our todo-list.

> It might make sense to have a local copy of the required structure
> and constants to allow an easier complication. Of course this depends
> on having CAIF at least in net-next-2.6 tree.  

OK, agree. I'll supply the CAIF specific patches next time.
Should we put the CAIF header files into ofono/include/caif?

> 
>> +/* TODO: should parse_xml function to be moved to e.g. atutil? */
> 
> First question. Why not use the XML parse that comes with GLib.

OK. Is it "Simple XML Subset Parser" you refer to? If you have any pointer 
to sample code using this XML parser we would appreciate it.


> oFono is never setting the IP address, netmask or any other
> configuration option. The only thing that oFono does is bringing the
> interface up. Systems like ConnMan do the IP configuration.  
> 
> Please see the comments in the documentation on how we expose IP
> interfaces. Check with ConnMan and how we configure them.

OK, we're returning the relevant parameters from activate_primary so that
all PDP Context parameters including interface name,
ip-address, netmask, dns,  etc are available in PrimaryDataContext. 
And leave the interface created but not configured.
Does this sound ok?
 
> 
> Having create and remove in the same function seems hard to read. Why
> not have one for creating the interface and one for removing it. From
> what I see so far, it is not much more code. And makes it a lot
> easier to read and understand for other people.   
> 
OK, we'll split this up.

> I would prefer if you do the parsing in one function that goes
> through the XML ones. Just give it pointers to the fields you wanna
> have it extract or a special combined structure. This looks highly
> inefficient to me.   

OK, see comments on XML above.

> 
>> +	conn->interface = g_malloc(10);
>> +	if (!conn->interface)
>> +		goto error;
> 
> Please double check with the GLib documentation on memory allocation.
> The g_malloc failure would result in halt of the program. What you
> want here is g_try_malloc. 
> 
> Also for 10 bytes, please don't bother with an allocation. Just
> include a char array in the conn structure. 

Yes I agree, we will fix this.

> 
> Yeah, I really prefer caif_if_create() and caif_if_remove(). This is
> not really intuitive at all. 

OK, sure.

> 
>> +	/* Need to change to gsm_permissive syntax in order to
>> +	 * parse the response from EPPSD (xml) */
>> +	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsm_permissive());
> 
> Is this an issue with your modem firmware or an issue in the v1
> parser. 
> If it is the modem firmware, then just use the permissive parser all
> the time and switch to E0. 

Setting permissive mode was done in order to be able to parse 
the XML response from  EPPSD (Propriatery Activate PDP context). 
But we can try if we can run with this mode default if you think 
this is better.

> 
>> +static void cgev_notify(GAtResult *result, gpointer user_data) {
>> +	struct ofono_gprs_context *gc = user_data;
>> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
>> +	GAtResultIter iter; +	const char *event;
>> +
>> +	dump_response("cgev_notify", TRUE, result);
>> +
>> +	g_at_result_iter_init(&iter, result);
>> +
>> +	if (!g_at_result_iter_next(&iter, "+CGEV:"))
>> +		return;
>> +
>> +	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
>> +		return; +
>> +	if (g_str_has_prefix(event, "NW REACT ") ||
>> +			g_str_has_prefix(event, "NW DEACT ") ||
>> +			g_str_has_prefix(event, "ME DEACT ")) {
>> +		/* Ask what primary contexts are active now */
>> +
>> +		g_at_chat_send(gcd->chat, "AT+CGACT?", cgact_prefix,
>> +				ste_cgact_read_cb, gc, NULL);
>> +
>> +		return;
>> +	}
>> +}
> 
> The return statement in the if clause is kinda pointless. Seems like
> either your code tried to be more complex or you are missing
> something.  

Sure we can fix this, but this is actually just copied from gprs_context in "atmodem".
BTW, the iter_init seems to be missing in atmodem's implementation, this is probably
a bug in "atmodem".

> 
>> +static int stemodem_init(void)
>> +{
>> +	/* Initialize voicecall driver */
>> +	struct ofono_voicecall_driver *at_vcdrv;
>> +	struct ofono_voicecall_driver *ste_vcdrv;
>> +	struct ofono_netreg_driver *at_netdrv;
>> +	struct ofono_netreg_driver *ste_netdrv;
>> +
>> +	ste_voicecall_init();
>> +
>> +	at_vcdrv = ofono_voicecall_driver_get("atmodem");
>> +	ste_vcdrv = ofono_voicecall_driver_get("stemodem"); +
>> +	if (at_vcdrv && ste_vcdrv) {
>> +		ste_vcdrv->remove = at_vcdrv->remove;
>> +		ste_vcdrv->swap_without_accept = at_vcdrv->swap_without_accept;
>> +		ste_vcdrv->send_tones = at_vcdrv->send_tones;
>> +	} else {
>> +		if (!ste_vcdrv)
>> +			ofono_debug("Could not get ofono_voicecall_driver" +					"from
>> stemodem"); +		if (!at_vcdrv)
>> +			ofono_debug("Could not get ofono_voicecall_driver" +					"from
>> atmodem"); +	}
>> +
>> +	/* Initialize netreg driver */
>> +	ste_netreg_init();
>> +
>> +	at_netdrv = ofono_netreg_driver_get("atmodem");
>> +	ste_netdrv = ofono_netreg_driver_get("stemodem"); +
>> +	if (at_netdrv && ste_netdrv) {
>> +		ste_netdrv->remove = at_netdrv->remove;
>> +		ste_netdrv->registration_status =
>> +			at_netdrv->registration_status;
>> +		ste_netdrv->current_operator = at_netdrv->current_operator;
>> +		ste_netdrv->list_operators = at_netdrv->list_operators;
>> +		ste_netdrv->register_auto = at_netdrv->register_auto;
>> +		ste_netdrv->register_manual = at_netdrv->register_manual;
> 
> So far this, I really like to see a description on what's the
> difference in the network registration and voice call atoms. 
> 
> We do need to understand what is actually needed.
>
For Voice call the main difference is that we're not doing polling with 
CLCC, but are using our proprietary call events. This impacts most functions.
The functions reusable from "atmodem" are "remove", "swap_without_accept", and 
"send_tones". I guess we could copy the three common functions from "atmodem" into 
"stemodem" voicecall.c if you prefer this.

For Network Registration we have almost identical implementation, the difference
is that for signal strength reporting we are using "AT+CIND" instead of "CSQ" and "CIEV".
"CSQ" is not working for WCDMA in our case. 
I see that in the initialization function there is a switch on vendor.
I guess we could go for this approach if you think this is better.
> 
>> +	/* Create a CAIF socket for AT Service */
>> +	fd = socket(AF_CAIF, SOCK_SEQPACKET, CAIFPROTO_AT); +
>> +	/* Connect to the AT Service at the modem */
>> +	connect(fd, (struct sockaddr *) &addr, sizeof(addr));
>> +	channel = g_io_channel_unix_new(fd);
> 
> You need to check the results of socket() and connect() calls.
Sure, will fix this.

Regards
Sjur Brændeland

RE: [RFC 3/3] STE-plugin: Adding STE plugin

[Marcel Holtmann]

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

Hi Sjur,

> Thank you for your feedback. We hope to get new patch-set out tomorrow
> with most of your comments fixed.

sounds great.

> > It might make sense to have a local copy of the required structure
> > and constants to allow an easier complication. Of course this depends
> > on having CAIF at least in net-next-2.6 tree.  
> 
> OK, agree. I'll supply the CAIF specific patches next time.
> Should we put the CAIF header files into ofono/include/caif?

Have a look at how we do it for the Phonet/ISI modem. Also we only need
a few constants (AF_CAIF, proto etc.) and mainly only the socket
structure for CAIF.

This should be easy to fix actually. More important is that we can
disable stemodem support via configure so people with an old kernel can
compile it. We are in a chicken and egg problem with the AF_CAIF
constant anyway. Until the CAIF subsystem is in net-next-2.6, the
actually number for it is not assigned.

> >> +/* TODO: should parse_xml function to be moved to e.g. atutil? */
> > 
> > First question. Why not use the XML parse that comes with GLib.
> 
> OK. Is it "Simple XML Subset Parser" you refer to? If you have any pointer 
> to sample code using this XML parser we would appreciate it.

Yes, I am talking about that one. It is similar to expact and the only
example, I have is in the BlueZ source code. I think Google code search
will reveal more useful examples.

> > oFono is never setting the IP address, netmask or any other
> > configuration option. The only thing that oFono does is bringing the
> > interface up. Systems like ConnMan do the IP configuration.  
> > 
> > Please see the comments in the documentation on how we expose IP
> > interfaces. Check with ConnMan and how we configure them.
> 
> OK, we're returning the relevant parameters from activate_primary so that
> all PDP Context parameters including interface name,
> ip-address, netmask, dns,  etc are available in PrimaryDataContext. 
> And leave the interface created but not configured.
> Does this sound ok?

Yes, sounds good. Please have a look at how we do it for Ericsson MBM
and Option HSO modems. We just send the D-Bus signal with all the
details and that is it.

I forgot to mention the reasoning behind. oFono is incapable of making
any kind of good decision when it comes to handling IP collision since
it only knows about its scope. Systems like ConnMan have the full
insight in whole networking to make proper decisions.

> > 
> >> +	/* Need to change to gsm_permissive syntax in order to
> >> +	 * parse the response from EPPSD (xml) */
> >> +	g_at_chat_set_syntax(gcd->chat, g_at_syntax_new_gsm_permissive());
> > 
> > Is this an issue with your modem firmware or an issue in the v1
> > parser. 
> > If it is the modem firmware, then just use the permissive parser all
> > the time and switch to E0. 
> 
> Setting permissive mode was done in order to be able to parse 
> the XML response from  EPPSD (Propriatery Activate PDP context). 
> But we can try if we can run with this mode default if you think 
> this is better.

Can you post an example (manually via cu or minicom and with ATE1) on
how this looks like. My question here is if you are actually following
the strict v1 syntax. If you don't then that is basically a bug on the
modem side. I don't even know if v1 has any kind of capabilities to
handle XML properly in the first place.

The point here is if you can not use v1 parser, then just use permissive
from the beginning and avoid switching at runtime. The capability of
switching at runtime mainly only exists for testing purposes. With the
permissive parser you just have to ensure ATE0 inside the modem plugin.
See the other users for an example.

> >> +static void cgev_notify(GAtResult *result, gpointer user_data) {
> >> +	struct ofono_gprs_context *gc = user_data;
> >> +	struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> >> +	GAtResultIter iter; +	const char *event;
> >> +
> >> +	dump_response("cgev_notify", TRUE, result);
> >> +
> >> +	g_at_result_iter_init(&iter, result);
> >> +
> >> +	if (!g_at_result_iter_next(&iter, "+CGEV:"))
> >> +		return;
> >> +
> >> +	if (!g_at_result_iter_next_unquoted_string(&iter, &event))
> >> +		return; +
> >> +	if (g_str_has_prefix(event, "NW REACT ") ||
> >> +			g_str_has_prefix(event, "NW DEACT ") ||
> >> +			g_str_has_prefix(event, "ME DEACT ")) {
> >> +		/* Ask what primary contexts are active now */
> >> +
> >> +		g_at_chat_send(gcd->chat, "AT+CGACT?", cgact_prefix,
> >> +				ste_cgact_read_cb, gc, NULL);
> >> +
> >> +		return;
> >> +	}
> >> +}
> > 
> > The return statement in the if clause is kinda pointless. Seems like
> > either your code tried to be more complex or you are missing
> > something.  
> 
> Sure we can fix this, but this is actually just copied from gprs_context in "atmodem".
> BTW, the iter_init seems to be missing in atmodem's implementation, this is probably
> a bug in "atmodem".

Sounds like a bug in atmodem then. Can you point me to the lines where
you are seeing this. I will fix it if needed.

And btw. the { for functions belongs on the line after. I will do a
thorough review of your next patchset and look out for these :)

> >> +static int stemodem_init(void)
> >> +{
> >> +	/* Initialize voicecall driver */
> >> +	struct ofono_voicecall_driver *at_vcdrv;
> >> +	struct ofono_voicecall_driver *ste_vcdrv;
> >> +	struct ofono_netreg_driver *at_netdrv;
> >> +	struct ofono_netreg_driver *ste_netdrv;
> >> +
> >> +	ste_voicecall_init();
> >> +
> >> +	at_vcdrv = ofono_voicecall_driver_get("atmodem");
> >> +	ste_vcdrv = ofono_voicecall_driver_get("stemodem"); +
> >> +	if (at_vcdrv && ste_vcdrv) {
> >> +		ste_vcdrv->remove = at_vcdrv->remove;
> >> +		ste_vcdrv->swap_without_accept = at_vcdrv->swap_without_accept;
> >> +		ste_vcdrv->send_tones = at_vcdrv->send_tones;
> >> +	} else {
> >> +		if (!ste_vcdrv)
> >> +			ofono_debug("Could not get ofono_voicecall_driver" +					"from
> >> stemodem"); +		if (!at_vcdrv)
> >> +			ofono_debug("Could not get ofono_voicecall_driver" +					"from
> >> atmodem"); +	}
> >> +
> >> +	/* Initialize netreg driver */
> >> +	ste_netreg_init();
> >> +
> >> +	at_netdrv = ofono_netreg_driver_get("atmodem");
> >> +	ste_netdrv = ofono_netreg_driver_get("stemodem"); +
> >> +	if (at_netdrv && ste_netdrv) {
> >> +		ste_netdrv->remove = at_netdrv->remove;
> >> +		ste_netdrv->registration_status =
> >> +			at_netdrv->registration_status;
> >> +		ste_netdrv->current_operator = at_netdrv->current_operator;
> >> +		ste_netdrv->list_operators = at_netdrv->list_operators;
> >> +		ste_netdrv->register_auto = at_netdrv->register_auto;
> >> +		ste_netdrv->register_manual = at_netdrv->register_manual;
> > 
> > So far this, I really like to see a description on what's the
> > difference in the network registration and voice call atoms. 
> > 
> > We do need to understand what is actually needed.
> >
> For Voice call the main difference is that we're not doing polling with 
> CLCC, but are using our proprietary call events. This impacts most functions.
> The functions reusable from "atmodem" are "remove", "swap_without_accept", and 
> "send_tones". I guess we could copy the three common functions from "atmodem" into 
> "stemodem" voicecall.c if you prefer this.

I actually prefer if you copy them. If useful, we can create some common
helpers in drivers/atmodem/atutil.c. I leave this up to Denis to comment
on that.

> For Network Registration we have almost identical implementation, the difference
> is that for signal strength reporting we are using "AT+CIND" instead of "CSQ" and "CIEV".
> "CSQ" is not working for WCDMA in our case. 
> I see that in the initialization function there is a switch on vendor.
> I guess we could go for this approach if you think this is better.

We do exactly that for OFONO_VENDOR_CALYPSO and OFONO_VENDOR_OPTION_HSO
and so I think it is best that you guys follow that approach for now.

If this gets massively convoluted at some point, we will be thinking of
something. Right now I prefer to just follow exactly what we are doing
for other modems with the same problem.

Regards

Marcel

RE: [RFC 3/3] STE-plugin: Adding STE plugin

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Marcel.
Thank you for lots of feedback on patches. Marit who has done most of the 
implementation is gone for a few days so we'll have to wait until end of
next week with the next oFono patch-set.


Marcel Holtmann wrote:
>>>> +	/* Need to change to gsm_permissive syntax in order to
>>>> +	 * parse the response from EPPSD (xml) */
>>>> +	g_at_chat_set_syntax(gcd->chat,
>>>> +g_at_syntax_new_gsm_permissive());
>>> 
>>> Is this an issue with your modem firmware or an issue in the v1
>>> parser. If it is the modem firmware, then just use the permissive
>>> parser all the time and switch to E0.
>> 
>> Setting permissive mode was done in order to be able to parse the XML
>> response from  EPPSD (Propriatery Activate PDP context).
>> But we can try if we can run with this mode default if you think this
>> is better.
> 
> Can you post an example (manually via cu or minicom and with ATE1) on
> how this looks like. My question here is if you are actually
> following the strict v1 syntax. If you don't then that is basically a
> bug on the modem side. I don't even know if v1 has any kind of
> capabilities to handle XML properly in the first place.    
> 
> The point here is if you can not use v1 parser, then just use
> permissive from the beginning and avoid switching at runtime. The
> capability of switching at runtime mainly only exists for testing
> purposes. With the permissive parser you just have to ensure ATE0
> inside the modem plugin.    
> See the other users for an example.

Here is an example of the output from AT*EPPSD. 

ofonod[26268]: < \r\nOK\r\n
ofonod[26268]: > AT*EPPSD=1,1,1\r
ofonod[26268]: < AT*EPPSD=1,1,1\r
ofonod[26268]: < \r\n<?xml version="1.0"?>\r\n
ofonod[26268]: < <connection_parameters>\r\n   <ip_address>10.64.151.14</ip_address>\r\n   <subnet_mask>255.255.255.255</subnet_mask>\r\n   <mtu>1500</mtu>\r\n   <dns_server>10.64.148.4</dns_server>\r\n   <dns_server>10.64.148.5</dns_server>\r\n</connection_parameters>\r\n
ofonod[26268]: < \r\nOK\r\n
ofonod[26268]: ste_eppsd_up_cb got result: 1
ofonod[26268]: Final response: OK
ofonod[26268]: Response line: <?xml version="1.0"?>
ofonod[26268]: Response line: <connection_parameters>
ofonod[26268]: Response line:    <ip_address>10.64.151.14</ip_address>
ofonod[26268]: Response line:    <subnet_mask>255.255.255.255</subnet_mask>
ofonod[26268]: Response line:    <mtu>1500</mtu>
ofonod[26268]: Response line:    <dns_server>10.64.148.4</dns_server>
ofonod[26268]: Response line:    <dns_server>10.64.148.5</dns_server>
ofonod[26268]: Response line: </connection_parameters>
ofonod[26268]: src/gprs.c:pri_activate_callback() 0x9bf22f8 caif0

 
>> Sure we can fix this, but this is actually just copied from
>> gprs_context in "atmodem". 
>> BTW, the iter_init seems to be missing in atmodem's implementation,
>> this is probably a bug in "atmodem".
> 
> Sounds like a bug in atmodem then. Can you point me to the lines
> where you are seeing this. I will fix it if needed. 

cgev_notify() is on line 208 in drivers/atmodem/gprs-context.c line
missing call to g_at_result_iter_init(&iter, result);


BR/Sjur

Re: [RFC 3/3] STE-plugin: Adding STE plugin

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Denis.

Sorry if you get this mail twice, something went wrong when posting this last time so
I'm resending it.

We have done some testing with the STE modem for call termination, and this is the result:

TC |Call #1 | Call #2 | Call #3  | Command   | Result
---|--------------------------------------------------------------------
1  |ACTIVE  | ACTIVE  | ..       | ATH         | call 1, 2 terminated	
2  |ACTIVE  | ACTIVE  | ..       | AT+CHUP     | call 1, 2 terminated	
3  |ACTIVE  | ACTIVE  | HELD     | ATH         | call 1, 2 terminated	
4  |ACTIVE  | ACTIVE  | HELD     | AT+CHUP     | call 1, 2 terminated
5  |ACTIVE  | ACTIVE  | HELD     | AT+CHLD=0;H | call 1, 2 and 3 terminated
6  |ACTIVE  | ACTIVE  | WAITING  | ATH         | call 1, 2 terminated
7  |ACTIVE  | ACTIVE  | WAITING  | AT+CHUP     | call 1, 2 terminated
8  |ACTIVE  | HELD    | WAITING  | CHLD=0      | call 3 terminated, 
9  |ACTIVE  | HELD    | WAITING  | ATH         | call 1 terminated
10 |ACTIVE  | HELD    | WAITING  | AT+CHUP     | call 1 terminated
11 |HELD    | HELD    | ACTIVE   | AT+CHLD=0   | call 1, 2 terminated
12 |HELD    | HELD    | ACTIVE   | AT+CHLD=0;H | call 1, 2 and 3 terminated
13 |HELD    | DIALING | ..       | ATH         | call 2 (MO) terminated
14 |HELD    | DIALING | ..       | CHUP        | call 2 (MO) terminated
15 |HELD    | DIALING | ..       | AT+CHLD=12  | call 2 (MO) NOT terminated
16 |HELD    | WAITING | ..       | AT+CHLD=0   | call 2 terminated
17 |HELD    | ..      | ..       | ATH         | call 1 NOT terminated	


Denis Kenzior wrote:
>>> oFono already takes care of this for single calls (see 
>>> src/voicecall.c voicecall_hangup.)  So this is only an issue in the 
>>> case of three way calls, is this what you're referring to here?
>> 
>> Kind of. This is very good, it takes care of the situation with 
>> emergency  call which cannot be terminated with CHLD commands.
>> 
>> But I think there are more issues. If I am not mistaken STE-modems 
>> have the following behavior:
>> CHLD=1X can only terminate call in state ACTIVE or HELD. (I think 
>> this is as STE interprets the standards).
> 
> The standards specify that CHLD=1X can only terminate an ACTIVE call.
> Most modems implement it this way.  There are vendor extensions that 
> provide this functionality (e.g. CHLD=7X on TI.)  By default oFono 
> assumes that release_specific will simply fail if a user attempts to
> use it on an e.g. HELD call with no modem support.    

For the STE modem, AT+CHLD=1x terminates calls in state ACTIVE and HELD.

>> 
>> a) If you are in a active call and receives a new incoming call
>> (ALERTING) and want to reject the new ALERTING call, then STE modem 
>> cannot terminate this call with CHLD=1X. It has to be terminated with 
>> CHLD=0 (cause=BUSY) or ATH (possible CHUP).
> 
> Ok, lets get the terminology clear here.  In this case the incoming 
> call is not ALERTING, it is WAITING.  WAITING calls are always 
> rejected by using CHLD=0.  ALERTING calls are always outgoing calls
> that transitioned from DIALING to alerting the user.   
> 
>> 
>> b) Or you may have the following situation. One call on HOLD, another 
>> ACTIVE call, and then you receive a new incoming call ALERTING. If 
>> you try to terminate the new incoming (ALERTING) call with CHLD=0, I 
>> think you as a side effect will terminate the call on hold as well.
>> If I am not mistaken ATH (possible CHUP) would be the correct in this 
>> situation for STE modems
> 
> The standards are quite clear here, the WAITING call always takes 
> precedence and thus only the WAITING call is affected. Can you check 
> that STE modems do indeed get this wrong?  If the modem is standards 
> compliant, oFono does the right thing here.

STE is standard compliant, only the WAITING call is terminated with AT+CHLD=0. (TC 8)
 
>> 
>> c) If you have an call on hold and initiate a new call, but want to 
>> terminate the newly initiated call (DIALING), then this call cannot 
>> be terminated with CHLD=1X, but you would have to use ATH (or 
>> possible CHUP).
> 
> Yes, so this is the case that we do need to take care of in the core.
> Most
> modems let us get away with sending release_specific up to this point.
> 

For the STE modem, calls in state DIALING and ALERTING will have to be 
terminated with ATH or AT+CHUP, AT+CHLD=1x does not work.
This means that the current implementation, using release_specific 
(and thus AT+CHLD=1x) will not work. 

>>> What I have been considering to take care of this case is to add
>>> end_all and end_all_active callbacks.  According to 27.007/22.030
>>> ATH should end all calls (active + held) except waiting calls, while
>>> +CHUP should only end the currently active call.  At least on one TI
>>> modem I tried this works as expected.  Do your modems implement the
>>> same behavior?
>> 
>> No, I don't think so. I think ATH will only terminate one call.
>> In order to terminate all calls you would probably need to do
>> something like: AT+CHLD=0;H But I'm not sure this works in all
>> possible scenarios either...
> 
> Can you check the behavior of ATH vs CHUP on STE modems?  We need to
> send the 
> right one here or both HELD and ACTIVE/DIALING/ALERTING will be
> terminated. 
> If using CHUP and ATH doesn't work out we'll have to come up with
> another 
> solution.

For the STE modem, ATH will only terminate the active call, 
not the held call. (TC 9). For more information about ATH and AT+CHUP,
please see the table above.

BR/Sjur

RE: [RFC 3/3] STE-plugin: Adding STE plugin

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Denis.

We have done some testing with the STE modem for call termination, and this is the result:

TC |Call #1 | Call #2 | Call #3  | Command   | Result
---|--------------------------------------------------------------------
1  |ACTIVE  | ACTIVE  | ..       | ATH         | call 1, 2 terminated	
2  |ACTIVE  | ACTIVE  | ..       | AT+CHUP     | call 1, 2 terminated	
3  |ACTIVE  | ACTIVE  | HELD     | ATH         | call 1, 2 terminated	
4  |ACTIVE  | ACTIVE  | HELD     | AT+CHUP     | call 1, 2 terminated
5  |ACTIVE  | ACTIVE  | HELD     | AT+CHLD=0;H | call 1, 2 and 3 terminated
6  |ACTIVE  | ACTIVE  | WAITING  | ATH         | call 1, 2 terminated
7  |ACTIVE  | ACTIVE  | WAITING  | AT+CHUP     | call 1, 2 terminated
8  |ACTIVE  | HELD    | WAITING  | CHLD=0      | call 3 terminated, 
9  |ACTIVE  | HELD    | WAITING  | ATH         | call 1 terminated
10 |ACTIVE  | HELD    | WAITING  | AT+CHUP     | call 1 terminated
11 |HELD    | HELD    | ACTIVE   | AT+CHLD=0   | call 1, 2 terminated
12 |HELD    | HELD    | ACTIVE   | AT+CHLD=0;H | call 1, 2 and 3 terminated
13 |HELD    | DIALING | ..       | ATH         | call 2 (MO) terminated
14 |HELD    | DIALING | ..       | CHUP        | call 2 (MO) terminated
15 |HELD    | DIALING | ..       | AT+CHLD=12  | call 2 (MO) NOT terminated
16 |HELD    | WAITING | ..       | AT+CHLD=0   | call 2 terminated
17 |HELD    | ..      | ..       | ATH         | call 1 NOT terminated	


Denis Kenzior wrote:
>>> oFono already takes care of this for single calls (see 
>>> src/voicecall.c voicecall_hangup.)  So this is only an issue in the 
>>> case of three way calls, is this what you're referring to here?
>> 
>> Kind of. This is very good, it takes care of the situation with 
>> emergency  call which cannot be terminated with CHLD commands.
>> 
>> But I think there are more issues. If I am not mistaken STE-modems 
>> have the following behavior:
>> CHLD=1X can only terminate call in state ACTIVE or HELD. (I think 
>> this is as STE interprets the standards).
> 
> The standards specify that CHLD=1X can only terminate an ACTIVE call.
> Most modems implement it this way.  There are vendor extensions that 
> provide this functionality (e.g. CHLD=7X on TI.)  By default oFono 
> assumes that release_specific will simply fail if a user attempts to
> use it on an e.g. HELD call with no modem support.    

For the STE modem, AT+CHLD=1x terminates calls in state ACTIVE and HELD.

>> 
>> a) If you are in a active call and receives a new incoming call
>> (ALERTING) and want to reject the new ALERTING call, then STE modem 
>> cannot terminate this call with CHLD=1X. It has to be terminated with 
>> CHLD=0 (cause=BUSY) or ATH (possible CHUP).
> 
> Ok, lets get the terminology clear here.  In this case the incoming 
> call is not ALERTING, it is WAITING.  WAITING calls are always 
> rejected by using CHLD=0.  ALERTING calls are always outgoing calls
> that transitioned from DIALING to alerting the user.   
> 
>> 
>> b) Or you may have the following situation. One call on HOLD, another 
>> ACTIVE call, and then you receive a new incoming call ALERTING. If 
>> you try to terminate the new incoming (ALERTING) call with CHLD=0, I 
>> think you as a side effect will terminate the call on hold as well.
>> If I am not mistaken ATH (possible CHUP) would be the correct in this 
>> situation for STE modems
> 
> The standards are quite clear here, the WAITING call always takes 
> precedence and thus only the WAITING call is affected. Can you check 
> that STE modems do indeed get this wrong?  If the modem is standards 
> compliant, oFono does the right thing here.

STE is standard compliant, only the WAITING call is terminated with AT+CHLD=0. (TC 8)
 
>> 
>> c) If you have an call on hold and initiate a new call, but want to 
>> terminate the newly initiated call (DIALING), then this call cannot 
>> be terminated with CHLD=1X, but you would have to use ATH (or 
>> possible CHUP).
> 
> Yes, so this is the case that we do need to take care of in the core.
> Most
> modems let us get away with sending release_specific up to this point.
> 

For the STE modem, calls in state DIALING and ALERTING will have to be terminated with ATH or AT+CHUP, AT+CHLD=1x does not work.
This means that the current implementation, using release_specific (and thus AT+CHLD=1x) will not work. 

>>> What I have been considering to take care of this case is to add 
>>> end_all and end_all_active callbacks.  According to 27.007/22.030 
>>> ATH should end all calls (active + held) except waiting calls, while
>>> +CHUP should only end the currently active call.  At least on one TI
>>> modem I tried this works as expected.  Do your modems implement the 
>>> same behavior?
>> 
>> No, I don't think so. I think ATH will only terminate one call.
>> In order to terminate all calls you would probably need to do 
>> something like: AT+CHLD=0;H But I'm not sure this works in all 
>> possible scenarios either...
> 
> Can you check the behavior of ATH vs CHUP on STE modems?  We need to 
> send the right one here or both HELD and ACTIVE/DIALING/ALERTING will 
> be terminated.
> If using CHUP and ATH doesn't work out we'll have to come up with 
> another solution.

For the STE modem, ATH will only terminate the active call, not the held call. (TC 9). For more information about ATH and AT+CHUP, please see the table above.

BTW: Sorry if you get this mail twice, something went wrong when posting this last time so I'm had to re-send this.
BR/Sjur

Re: [RFC 3/3] STE-plugin: Adding STE plugin

[Denis Kenzior]

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

Hi Sjur,

> Hi Denis.
> 
> We have done some testing with the STE modem for call termination, and this
>  is the result:
> 
> TC |Call #1 | Call #2 | Call #3  | Command   | Result
> ---|--------------------------------------------------------------------
> 1  |ACTIVE  | ACTIVE  | ..       | ATH         | call 1, 2 terminated
> 2  |ACTIVE  | ACTIVE  | ..       | AT+CHUP     | call 1, 2 terminated
> 3  |ACTIVE  | ACTIVE  | HELD     | ATH         | call 1, 2 terminated
> 4  |ACTIVE  | ACTIVE  | HELD     | AT+CHUP     | call 1, 2 terminated
> 5  |ACTIVE  | ACTIVE  | HELD     | AT+CHLD=0;H | call 1, 2 and 3 terminated
> 6  |ACTIVE  | ACTIVE  | WAITING  | ATH         | call 1, 2 terminated
> 7  |ACTIVE  | ACTIVE  | WAITING  | AT+CHUP     | call 1, 2 terminated
> 8  |ACTIVE  | HELD    | WAITING  | CHLD=0      | call 3 terminated,
> 9  |ACTIVE  | HELD    | WAITING  | ATH         | call 1 terminated
> 10 |ACTIVE  | HELD    | WAITING  | AT+CHUP     | call 1 terminated
> 11 |HELD    | HELD    | ACTIVE   | AT+CHLD=0   | call 1, 2 terminated
> 12 |HELD    | HELD    | ACTIVE   | AT+CHLD=0;H | call 1, 2 and 3 terminated
> 13 |HELD    | DIALING | ..       | ATH         | call 2 (MO) terminated
> 14 |HELD    | DIALING | ..       | CHUP        | call 2 (MO) terminated
> 15 |HELD    | DIALING | ..       | AT+CHLD=12  | call 2 (MO) NOT terminated
> 16 |HELD    | WAITING | ..       | AT+CHLD=0   | call 2 terminated
> 17 |HELD    | ..      | ..       | ATH         | call 1 NOT terminated

Great table, makes it very easy to see what is going on.  It looks like STE 
modems treat ATH and CHUP as only affecting dialing/alerting/active calls and 
incoming calls, not held calls or waiting calls.

> > The standards are quite clear here, the WAITING call always takes
> > precedence and thus only the WAITING call is affected. Can you check
> > that STE modems do indeed get this wrong?  If the modem is standards
> > compliant, oFono does the right thing here.
> 
> STE is standard compliant, only the WAITING call is terminated with
>  AT+CHLD=0. (TC 8)

Excellent, I would have been surprised if STE behaved otherwise :)

> 
> >> c) If you have an call on hold and initiate a new call, but want to
> >> terminate the newly initiated call (DIALING), then this call cannot
> >> be terminated with CHLD=1X, but you would have to use ATH (or
> >> possible CHUP).
> >
> > Yes, so this is the case that we do need to take care of in the core.
> > Most
> > modems let us get away with sending release_specific up to this point.
> 
> For the STE modem, calls in state DIALING and ALERTING will have to be
>  terminated with ATH or AT+CHUP, AT+CHLD=1x does not work. This means that
>  the current implementation, using release_specific (and thus AT+CHLD=1x)
>  will not work.

Yep, so please critique my earlier suggestion of splitting up hangup into two 
methods: hangup_all and hangup_active.  Modem drivers will need to provide one 
or the other or both.

The core can then use the hangup_active (if available) in those cases where 
release_specific might not work.  The condition for hangup_active will be that 
it does not affect held or waiting calls.  For ST-E the hangup_active 
implementation will simply be +CHUP.  For modems that do not have this 
available we will fall back to release_specific and assume that on those 
CHLD=1X or equivalent can affect dialing/alerting calls.

hangup_all can also be used for cases where we loop release_specific over all 
active/dialing/alerting/held/incoming calls.  For ST-E the hangup_all 
implementation might be +CHUP;CHLD=1n;...;+CHLD=1m where n, m, etc are ids of 
the HELD calls.  We should not hangup waiting calls to be compliant with 
section 6.5.5.1 of 3GPP 22.030: "Entering END	-	Releases the subscriber from 
all calls (except a possible waiting call)."

If this sounds OK then I will work on implementing the logic in the next few 
days.

Regards,
-Denis