Hi Denis, First off, thanks for your quick review.
On Sun, Apr 29, 2012 at 08:14:36PM -0500, Denis Kenzior wrote: > Hi Pablo, > > On 04/30/2012 09:45 AM, [email protected] wrote: > > From: Pablo Neira Ayuso <[email protected]> > > > > This patch adds support for voice calls and SMS for Wavecom > > Q2403/Q2686. > > > > The existing Wavecom driver in tree slightly differs from these > > modems. Thus, it doesn't work work with them. We (the osmocom > > team) are use these Wavecom Q2403/Q2686 modems in our testbed. > > --- > > Makefile.am | 3 + > > drivers/atmodem/sim.c | 8 ++ > > drivers/atmodem/sms.c | 31 ++++++-- > > drivers/atmodem/vendor.h | 1 + > > gatchat/gatchat.c | 3 +- > > plugins/udev.c | 2 + > > plugins/wavecom_q.c | 192 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > Can you please break this patch up, we prefer one patch for every > directory. See HACKING document for patch submission guidelines. OK, I'll do it like that if you prefer it. I think it breaks a bit of the logic of "one patch one new feature" and it leaves the repository in intermediate state between patches, but this is your project, you rule here :-). > > 7 files changed, 234 insertions(+), 6 deletions(-) > > create mode 100644 plugins/wavecom_q.c > > > > diff --git a/Makefile.am b/Makefile.am > > index 9cb490d..0bcbb8a 100644 > > --- a/Makefile.am > > +++ b/Makefile.am > > @@ -371,6 +371,9 @@ builtin_sources += plugins/samsung.c > > builtin_modules += sim900 > > builtin_sources += plugins/sim900.c > > > > +builtin_modules += wavecom_q > > +builtin_sources += plugins/wavecom_q.c > > + > > if BLUETOOTH > > builtin_modules += bluetooth > > builtin_sources += plugins/bluetooth.c plugins/bluetooth.h > > diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c > > index dd86980..64f38a8 100644 > > --- a/drivers/atmodem/sim.c > > +++ b/drivers/atmodem/sim.c > > @@ -146,6 +146,14 @@ static void at_sim_read_info(struct ofono_sim *sim, > > int fileid, > > return; > > } > > } > > + /* AT+CRSM not supported by Q2403. */ > > + if (sd->vendor == OFONO_VENDOR_WAVECOM_Q) { > > + unsigned char access[3] = { 0x00, 0x00, 0x00 }; > > + > > + CALLBACK_WITH_SUCCESS(cb, 4, 0, 0, access, > > + EF_STATUS_VALID, data); > > Why don't you simply return an error here? Without it, the modem initialization does not complete. > > + return; > > + } > > > > cbd = cb_data_new(cb, data); > > > > diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c > > index 27dc2c0..af53880 100644 > > --- a/drivers/atmodem/sms.c > > +++ b/drivers/atmodem/sms.c > > @@ -984,8 +984,12 @@ static gboolean set_cpms(gpointer user_data) > > const char *incoming = storages[data->incoming]; > > char buf[128]; > > > > - snprintf(buf, sizeof(buf), "AT+CPMS=\"%s\",\"%s\",\"%s\"", > > - store, store, incoming); > > + if (data->vendor != OFONO_VENDOR_WAVECOM_Q) { > > + snprintf(buf, sizeof(buf), "AT+CPMS=\"%s\",\"%s\",\"%s\"", > > + store, store, incoming); > > + } else { > > + snprintf(buf, sizeof(buf), "AT+CPMS=\"%s\"", store); > > + } > > There is no need for any curly braces. You mean for both snprintf, right? > > g_at_chat_send(data->chat, buf, cpms_prefix, > > at_cpms_set_cb, sms, NULL); > > @@ -1037,7 +1041,7 @@ static void at_cpms_query_cb(gboolean ok, GAtResult > > *result, > > gboolean supported = FALSE; > > > > if (ok) { > > - int mem = 0; > > + int mem = 0, mem_max; > > GAtResultIter iter; > > const char *store; > > gboolean me_supported[3]; > > @@ -1053,7 +1057,23 @@ static void at_cpms_query_cb(gboolean ok, GAtResult > > *result, > > if (!g_at_result_iter_next(&iter, "+CPMS:")) > > goto out; > > > > - for (mem = 0; mem < 3; mem++) { > > + /* Wavecom Q replies something like this: > > + * > > + * +CPMS: (("SM","BM","SR"),("SM")) > > + * > > + * It does not provide the way income messages are stored. > > + * 3GPP TS 07.05 allows mem2 and mem3 to be optional. > > + */ > > Just how old is this modem and what version of 07.05 are you using? > > For reference, 07.05 version 7.0.1 from July 1999: > " > +CPMS=? > +CPMS: (list of supported <mem1>s),(list of supported <mem2>s), > (list of supported <mem3>s) > " > > So the comment should probably be changed to indicate that this modem is > just broken. > From: "3.2.2 Preferred Message Storage +CPMS" (TS 07.05 July 1999): +CPMS=<mem1>[, <mem2>[,<mem3>]] The brackets around <mem3> sound like it is optional thing. Unless I'm missing anything I think the comment is fine. > > + if (data->vendor == OFONO_VENDOR_WAVECOM_Q) { > > + /* skip initial `(' */ > > + if (!g_at_result_iter_open_list(&iter)) > > + goto out; > > + > > + mem_max = 2; > > + } else > > + mem_max = 3; > > + > > + for (mem = 0; mem < mem_max; mem++) { > > if (!g_at_result_iter_open_list(&iter)) > > goto out; > > > > @@ -1070,7 +1090,8 @@ static void at_cpms_query_cb(gboolean ok, GAtResult > > *result, > > goto out; > > } > > > > - if (!sm_supported[2] && !me_supported[2] && !mt_supported[2]) > > + if (data->vendor != OFONO_VENDOR_WAVECOM_Q && > > + !sm_supported[2] && !me_supported[2] && !mt_supported[2]) > > Please don't use spaces for indentation, always tabs OK, so you prefer this, right? if (data->vendor != OFONO_VENDOR_WAVECOM_Q && !sm_supported[2] && !me_supported[2] && !mt_supported[2]) > > goto out; > > > > if (sm_supported[0] && sm_supported[1]) { > > diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h > > index 44b037f..1b296a0 100644 > > --- a/drivers/atmodem/vendor.h > > +++ b/drivers/atmodem/vendor.h > > @@ -39,4 +39,5 @@ enum ofono_vendor { > > OFONO_VENDOR_SPEEDUP, > > OFONO_VENDOR_SAMSUNG, > > OFONO_VENDOR_SIMCOM, > > + OFONO_VENDOR_WAVECOM_Q, > > }; > > diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c > > index 7a0ef35..eb5d83a 100644 > > --- a/gatchat/gatchat.c > > +++ b/gatchat/gatchat.c > > @@ -478,7 +478,8 @@ static struct terminator_info terminator_table[] = { > > { "NO ANSWER", -1, FALSE }, > > { "+CMS ERROR:", 11, FALSE }, > > { "+CME ERROR:", 11, FALSE }, > > - { "+EXT ERROR:", 11, FALSE } > > + { "+EXT ERROR:", 11, FALSE }, > > + { "+CPIN: READY", -1, TRUE }, > > Are you sure you can't reuse the same behavior as OFONO_VENDOR_WAVECOM > quirk inside drivers/atmodem/sim.c? Yes, I remember to have tried that. That quirk didn't work for me though. > e.g. in at_sim_probe(): > switch (sd->vendor) { > case OFONO_VENDOR_WAVECOM: > g_at_chat_add_terminator(sd->chat, "+CPIN:", 6, TRUE); > break; > > > }; > > > > static void at_chat_add_terminator(struct at_chat *chat, char *terminator, > > diff --git a/plugins/udev.c b/plugins/udev.c > > index 8cb87a5..e599296 100644 > > --- a/plugins/udev.c > > +++ b/plugins/udev.c > > @@ -286,6 +286,8 @@ done: > > add_nokiacdma(modem, udev_device); > > else if (g_strcmp0(driver, "sim900") == 0) > > add_sim900(modem, udev_device); > > + else if (g_strcmp0(driver, "wavecom_q") == 0) > > + add_calypso(modem, udev_device); > > This is probably wrong, I suspect we need to add a generic function to > register (real) serial tty based modems. I should have added some add_wavecom function instead. I used that because I noticed that: add_sim900 add_nokiacdma add_tc65 ... and so on. look the same. So I guess that it is indeed a good idea to remove redundant code and provide some generic one. > > > } > > > > static gboolean devpath_remove(gpointer key, gpointer value, gpointer > > user_data) > > diff --git a/plugins/wavecom_q.c b/plugins/wavecom_q.c > > new file mode 100644 > > index 0000000..73e478a > > --- /dev/null > > +++ b/plugins/wavecom_q.c > > @@ -0,0 +1,192 @@ > > +/* > > + * > > + * oFono - Open Source Telephony > > + * > > + * Copyright (C) 2008-2011 Intel Corporation. All rights reserved. > > + * > > + * 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/ussd.h> > > +#include <ofono/voicecall.h> > > + > > +#include <drivers/atmodem/vendor.h> > > + > > + > > +static int wavecom_q_probe(struct ofono_modem *modem) > > +{ > > + return 0; > > +} > > + > > +static void wavecom_q_remove(struct ofono_modem *modem) > > +{ > > +} > > + > > +static void wavecom_q_debug(const char *str, void *user_data) > > +{ > > + const char *prefix = user_data; > > + > > + ofono_info("%s%s", prefix, str); > > +} > > + > > +static int wavecom_q_enable(struct ofono_modem *modem) > > +{ > > + GAtChat *chat; > > + GIOChannel *channel; > > + GAtSyntax *syntax; > > + const char *device; > > + GHashTable *options; > > + > > + DBG("%p", modem); > > + > > + device = ofono_modem_get_string(modem, "Device"); > > + if (device == NULL) > > + return -EINVAL; > > + > > + options = g_hash_table_new(g_str_hash, g_str_equal); > > + > > + if (options == NULL) > > + return -ENOMEM; > > + > > + g_hash_table_insert(options, "Baud", "115200"); > > + g_hash_table_insert(options, "Parity", "none"); > > + g_hash_table_insert(options, "StopBits", "1"); > > + g_hash_table_insert(options, "DataBits", "8"); > > + > > + channel = g_at_tty_open(device, options); > > + > > + g_hash_table_destroy(options); > > + > > + if (channel == NULL) > > + return -EIO; > > + > > + /* > > + * Could not figure out whether it is fully compliant or not, use > > + * permissive for now > > + * */ > > + syntax = g_at_syntax_new_gsm_permissive(); > > + > > + chat = g_at_chat_new(channel, syntax); > > + g_at_syntax_unref(syntax); > > + g_io_channel_unref(channel); > > + > > + if (chat == NULL) > > + return -ENOMEM; > > + > > + if (getenv("OFONO_AT_DEBUG")) > > + g_at_chat_set_debug(chat, wavecom_q_debug, ""); > > + > > + ofono_modem_set_data(modem, chat); > > + > > + return 0; > > +} > > + > > +static int wavecom_q_disable(struct ofono_modem *modem) > > +{ > > + GAtChat *chat = ofono_modem_get_data(modem); > > + > > + DBG("%p", modem); > > + > > + ofono_modem_set_data(modem, NULL); > > + > > + g_at_chat_unref(chat); > > + > > + return 0; > > +} > > + > > +static void wavecom_q_pre_sim(struct ofono_modem *modem) > > +{ > > + GAtChat *chat = ofono_modem_get_data(modem); > > + struct ofono_sim *sim; > > + > > + DBG("%p", modem); > > + > > + ofono_devinfo_create(modem, 0, "atmodem", chat); > > + sim = ofono_sim_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat); > > + ofono_voicecall_create(modem, 0, "atmodem", chat); > > + > > + if (sim) > > + ofono_sim_inserted_notify(sim, TRUE); > > +} > > + > > +static void wavecom_q_post_sim(struct ofono_modem *modem) > > +{ > > + GAtChat *chat = ofono_modem_get_data(modem); > > + struct ofono_message_waiting *mw; > > + > > + DBG("%p", modem); > > + > > + ofono_ussd_create(modem, 0, "atmodem", chat); > > + ofono_call_forwarding_create(modem, 0, "atmodem", chat); > > + ofono_call_settings_create(modem, 0, "atmodem", chat); > > + ofono_netreg_create(modem, 0, "atmodem", chat); > > + ofono_call_meter_create(modem, 0, "atmodem", chat); > > + ofono_call_barring_create(modem, 0, "atmodem", chat); > > + /* We need to specify the vendor to support AT+CPMS=? reply. */ > > + ofono_sms_create(modem, OFONO_VENDOR_WAVECOM_Q, "atmodem", chat); > > + ofono_phonebook_create(modem, 0, "atmodem", chat); > > + > > + mw = ofono_message_waiting_create(modem); > > + if (mw) > > + ofono_message_waiting_register(mw); > > +} > > + > > +static struct ofono_modem_driver wavecom_q_driver = { > > + .name = "wavecom_q", > > + .probe = wavecom_q_probe, > > + .remove = wavecom_q_remove, > > + .enable = wavecom_q_enable, > > + .disable = wavecom_q_disable, > > + .pre_sim = wavecom_q_pre_sim, > > + .post_sim = wavecom_q_post_sim, > > +}; > > + > > +static int wavecom_q_init(void) > > +{ > > + return ofono_modem_driver_register(&wavecom_q_driver); > > +} > > + > > +static void wavecom_q_exit(void) > > +{ > > + ofono_modem_driver_unregister(&wavecom_q_driver); > > +} > > + > > +OFONO_PLUGIN_DEFINE(wavecom_q, "Wavecom-Q driver", VERSION, > > + OFONO_PLUGIN_PRIORITY_DEFAULT, wavecom_q_init, wavecom_q_exit) > > Is there a way to just re-use the wavecom driver instead of creating a > brand new one? I didn't find any cleaner solution I could like, but if you propose any other solution, I'll make it. _______________________________________________ ofono mailing list [email protected] http://lists.ofono.org/listinfo/ofono
