[RFC 0/3] STE-plugin: Plugin for ST-Ericsson modems
From: Sjur Brandeland sjur.brandel...@stericsson.com We are happy to announce ST-Ericsson's participation in the oFono community and contribution of an oFono plug-in for ST-Ericsson modems. This patch set adds a plug-in for use with ST-Ericsson modems. It is implemented using AT-commands over a CAIF link to communicate with the modem. CAIF is the ST-Ericsson IPC mechanism used between host and modem. Feedback on this patch set is welcome! This patch-set is based on commit: e433ddc100bda3a437fb81805ea44348f22e9fcb Sjur Brandeland (3): STE-plugin: Add vendor STE STE-plugin: Mechanism for inheritance STE-plugin: Add STE plugin Makefile.am | 11 + drivers/atmodem/gprs.c | 15 +- drivers/atmodem/vendor.h|5 + 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 +++ include/netreg.h|5 + include/voicecall.h |7 +- plugins/modemconf.c |5 + plugins/ste.c | 246 src/network.c | 22 ++ src/voicecall.c | 22 ++ 14 files changed, 1929 insertions(+), 3 deletions(-) ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[RFC 2/3] STE-plugin: Mechanism for inheritance
From: Sjur Brandeland sjur.brandel...@stericsson.com
This patch adds a mechanism to inherit/specialize from the standard atmodem.
This is used in the initialization of voice call driver and the network
registration driver in order to re-use functions from atmodems whenever
possible. We realize this is not the standard way of doing it, so I would
appreciate your comments on this approach.
---
include/netreg.h|5 +
include/voicecall.h |7 ++-
src/network.c | 22 ++
src/voicecall.c | 22 ++
4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/include/netreg.h b/include/netreg.h
old mode 100644
new mode 100755
index 0079477..ee0b201
--- a/include/netreg.h
+++ b/include/netreg.h
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
#ifndef __OFONO_NETREG_H
@@ -96,6 +100,7 @@ void ofono_netreg_status_notify(struct ofono_netreg *netreg,
int status,
int ofono_netreg_driver_register(const struct ofono_netreg_driver *d);
void ofono_netreg_driver_unregister(const struct ofono_netreg_driver *d);
+struct ofono_netreg_driver *ofono_netreg_driver_get(const char *driver);
struct ofono_netreg *ofono_netreg_create(struct ofono_modem *modem,
unsigned int vendor,
diff --git a/include/voicecall.h b/include/voicecall.h
index 6ceb3d8..ca43c91 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
#ifndef __OFONO_VOICECALL_H
@@ -29,7 +33,7 @@ extern C {
#include ofono/types.h
struct ofono_voicecall;
-
+struct ofono_modem;
typedef void (*ofono_voicecall_cb_t)(const struct ofono_error *error,
void *data);
@@ -102,6 +106,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall
*vc, int id,
int ofono_voicecall_driver_register(const struct ofono_voicecall_driver *d);
void ofono_voicecall_driver_unregister(const struct ofono_voicecall_driver *d);
+struct ofono_voicecall_driver *ofono_voicecall_driver_get(const char *driver);
struct ofono_voicecall *ofono_voicecall_create(struct ofono_modem *modem,
unsigned int vendor,
diff --git a/src/network.c b/src/network.c
index 8b4eb09..75ee98b 100644
--- a/src/network.c
+++ b/src/network.c
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
#ifdef HAVE_CONFIG_H
@@ -1596,6 +1600,24 @@ void ofono_netreg_driver_unregister(const struct
ofono_netreg_driver *d)
g_drivers = g_slist_remove(g_drivers, (void *)d);
}
+struct ofono_netreg_driver *ofono_netreg_driver_get(const char *driver)
+{
+ GSList *l;
+
+ if (driver == NULL)
+ return NULL;
+
+ for (l = g_drivers; l; l = l-next) {
+ struct ofono_netreg_driver *drv = l-data;
+
+ if (g_strcmp0(drv-name, driver))
+ continue;
+ else
+ return drv;
+ }
+ return NULL;
+}
+
static void netreg_unregister(struct ofono_atom *atom)
{
struct ofono_netreg *netreg = __ofono_atom_get_data(atom);
diff --git a/src/voicecall.c b/src/voicecall.c
index 73de35f..1cd5bd7 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -17,6 +17,10 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*
+ * Copyright (C) 2010 ST-Ericsson AB.
+ * Author: Marit Henriksen, marit.xx.henrik...@stericsson.com.
+ * STE specific implementation.
+ *
*/
#ifdef HAVE_CONFIG_H
@@ -1751,6 +1755,24 @@ void ofono_voicecall_driver_unregister(const struct
ofono_voicecall_driver *d)
g_drivers = g_slist_remove(g_drivers, (void *)d);
}
+struct ofono_voicecall_driver *ofono_voicecall_driver_get(const char *driver)
+{
+ GSList *l;
+
+ if (driver == NULL)
+ return NULL;
+
+ for (l = g_drivers; l; l = l-next) {
+ struct ofono_voicecall_driver *drv = l-data;
+
+ if (g_strcmp0(drv-name, driver))
+ continue;
+ else
+ return drv;
+ }
+ return
[RFC 3/3] STE-plugin: Adding STE plugin
From: Sjur Brandeland sjur.brandel...@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 net...@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 000..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.henrik...@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
oFono Features
Hello I am interested to know which kind of feature range the ofono platform is intended to have once. Specially are there plans to provide a framework of ready-to-use gadgets, i.e. for user interfaces, graphics, application components ? Regards, Robert ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [RFC] HFP support into oFono and BlueZ
Hi, On Thu, Jan 14, 2010 at 1:39 AM, Gustavo F. Padovan pado...@profusion.mobi wrote: Hi, On Mon, Jan 11, 2010 at 5:05 PM, Gustavo F. Padovan pado...@profusion.mobi wrote: On Mon, Jan 11, 2010 at 3:08 PM, Gustavo F. Padovan pado...@profusion.mobi wrote: Hi, These patches implement the new API for the Audio Gateway in BlueZ. It follows the last version of the HandsfreeGateway and HandsfreeAgent Intefaces API. The first two patches is for BlueZ and the other for oFono. You can test it with using enable-modem and test-voicecall scripts into the test dir of oFono. Feel free to test it and send me your comments. We have some bugs yet. New version of the patches: now we do not handle tty stuff on the BlueZ side. New version of the patches. Known issues: 'org.bluez.HandsfreeAgent.Release' part not implemented and it's not working with more than one bluetooth devices in some cases. Comments are welcome. Since nobody really respond me in the thread about merging endpoints with this spec I will comment in this one, although Johan disagree on having it as a MediaEndpoint I guess it is probably a good idea to have a similar design as we will have in endpoints, so the registration actually happen in adapter path rather than device path, which probably should avoid the having to resolve the device path while probing if we just send the device path as a parameter of NewConnection so we can do while in DEFER_SETUP stage so we don't risk missing or getting timeouts while transferring the the fd to ofono. The audio part is not working yet. We are going to work on pulseaudio this week to get this done soon. Well I suppose we will be using the endpoint to handle this. Regards, -- Gustavo F. Padovan ProFUSION embedded systems - http://profusion.mobi -- Gustavo F. Padovan ProFUSION embedded systems - http://profusion.mobi -- Gustavo F. Padovan ProFUSION embedded systems - http://profusion.mobi -- Luiz Augusto von Dentz Engenheiro de Computação ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC 3/3] STE-plugin: Adding STE plugin
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
Re: [RFC 3/3] STE-plugin: Adding STE plugin
Hi Sjur,
diff --git a/drivers/stemodem/network-registration.c
b/drivers/stemodem/network-registration.c new file mode 100644
index 000..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
Re: [RFC] HFP support into oFono and BlueZ
Hi Luiz, having it as a MediaEndpoint I guess it is probably a good idea to have a similar design as we will have in endpoints, so the registration actually happen in adapter path rather than device path, which probably should avoid the having to resolve the device path while probing if we just send the device path as a parameter of NewConnection so we can do while in DEFER_SETUP stage so we don't risk missing or getting timeouts while transferring the the fd to ofono. Personally I like this to be a per-device agent rather than per-adapter as it will make agent code inside oFono a bit easier to write. Marcel / Johan, care to weigh in? Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC 3/3] STE-plugin: Adding STE plugin
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
