Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[Denis Kenzior]

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

Hi Pablo,

On 04/30/2012 09:45 AM, pablo(a)gnumonks.org wrote:
> From: Pablo Neira Ayuso <pablo@gnumonks.org>
> 
> 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.

>  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?

> +		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.

>  
>  	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.

> +		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

>  			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?

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.

>  }
>  
>  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?

Regards,
-Denis

Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[Denis Kenzior]

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

Hi Pablo,

>>> +	/* 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.

Can you fallback to CSIM?

<snip>

>>> -	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?
> 

Yep

>>> -		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>]]

That is a set operation, we're doing a support query.

<snip>

>>> @@ -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])
> 

See doc/coding-style.txt

if (... &&
<tab><tab>...)
<tab>Statement

<snip>

>> 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.

I think we need to dig a bit deeper as to why, the VENDOR_WAVECOM
behavior seems to be addressing exactly this case.

>> 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.
> 

If they are all the same, then adding a generic function is fine.

<snip>

>>> +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.

Right now to me it seems like the differences between the -Q version is
a slightly different set of quirks.  Can't we query the model / firmware
versions and set the quirks accordingly?

Regards,
-Denis

[PATCH] Wavecom Q2403/Q2686 support

[pablo]

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

From: Pablo Neira Ayuso <pablo@gnumonks.org>

Hi!

The following patch adds support for Wavecom Q2403 and Q2686.

We (the Osmocom team) are using these modems with ofono in our
testbed setup.

Any chance this can be applied to mainstream?

Thank you!

Pablo Neira Ayuso (1):
  wavecom: add support for Wavecom Q2403/Q2686 modems

 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 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 234 insertions(+), 6 deletions(-)
 create mode 100644 plugins/wavecom_q.c

-- 
1.7.9.5

[PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[pablo]

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

From: Pablo Neira Ayuso <pablo@gnumonks.org>

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 ++++++++++++++++++++++++++++++++++++++++++++++
 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);
+		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);
+	}
 
 	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.
+		 */
+		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])
 			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 },
 };
 
 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);
 }
 
 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)
-- 
1.7.9.5

Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[Pablo Neira Ayuso]

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

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, pablo(a)gnumonks.org wrote:
> > From: Pablo Neira Ayuso <pablo@gnumonks.org>
> > 
> > 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.

Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[Pablo Neira Ayuso]

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

Hi Denis,

On Sun, Apr 29, 2012 at 10:51:17PM -0500, Denis Kenzior wrote:
> Hi Pablo,
> 
> >>> +	/* 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.
> 
> Can you fallback to CSIM?

I don't know what you mean, could you provide more information?

<snip>
> >>> -		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>]]
> 
> That is a set operation, we're doing a support query.

You're right, then it's a broken modem indeed.

> >> 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.
> 
> I think we need to dig a bit deeper as to why, the VENDOR_WAVECOM
> behavior seems to be addressing exactly this case.

I think the other wavecom vendor provides a different reply. I can add
a different quirk to at_cpin_cb instead and see how it goes.

> >> 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.
> > 
> 
> If they are all the same, then adding a generic function is fine.

I'll send you a patch for this.

> <snip>
> 
> >>> +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.
> 
> Right now to me it seems like the differences between the -Q version is
> a slightly different set of quirks.  Can't we query the model / firmware
> versions and set the quirks accordingly?

Is there any facility that ofono provides to do this? If so, please
point to some existing code.

Thank you.

Re: [PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[Denis Kenzior]

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

Hi Pablo,

On 05/28/2012 09:14 PM, pablo(a)gnumonks.org wrote:
> From: Pablo Neira Ayuso<pablo@gnumonks.org>
>
> This patch extends the existing wavecom plugin to include the new
> wavecom 2xxx plugin.
> ---
>   plugins/udev.c    |   15 ++++++++++++
>   plugins/wavecom.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 86 insertions(+), 6 deletions(-)
>

Please break this patch into two, one for adding the udev logic and one 
for wavecom stuff...

> diff --git a/plugins/udev.c b/plugins/udev.c
> index 8cb87a5..6e3205e 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -167,6 +167,19 @@ static void add_calypso(struct ofono_modem *modem,
>   	ofono_modem_register(modem);
>   }
>
> +static void add_wavecom_q2xxx(struct ofono_modem *modem,
> +					struct udev_device *udev_device)

please rename this into add_wavecom.

> +{
> +	const char *devnode;
> +
> +	DBG("modem %p", modem);
> +
> +	devnode = udev_device_get_devnode(udev_device);
> +	ofono_modem_set_string(modem, "Device", devnode);

Since this is a serial device, you can easily do something like this in 
your rules file:

ENV{OFONO_WAVECOM_MODEL}="Q2XXX"

This setting can then be forwarded to the modem driver by using 
ofono_modem_set_string(modem, "Model" (or Quirk, or whatever), ...);

There is no need to create a separate driver for the wavecom.

See add_ifx() for an example of this.

> +
> +	ofono_modem_register(modem);
> +}
> +
>   static void add_tc65(struct ofono_modem *modem,
>   			struct udev_device *udev_device)
>   {
> @@ -286,6 +299,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_q2xxx") == 0)
> +		add_wavecom_q2xxx(modem, udev_device);
>   }
>
>   static gboolean devpath_remove(gpointer key, gpointer value, gpointer user_data)
> diff --git a/plugins/wavecom.c b/plugins/wavecom.c
> index 5d30f39..c8bd55b 100644
> --- a/plugins/wavecom.c
> +++ b/plugins/wavecom.c
> @@ -66,7 +66,7 @@ static void wavecom_debug(const char *str, void *user_data)
>   	ofono_info("%s%s", prefix, str);
>   }
>
> -static int wavecom_enable(struct ofono_modem *modem)
> +static int wavecom_generic_enable(struct ofono_modem *modem, int vendor)
>   {
>   	GAtChat *chat;
>   	GIOChannel *channel;
> @@ -104,6 +104,8 @@ static int wavecom_enable(struct ofono_modem *modem)
>   	syntax = g_at_syntax_new_gsm_permissive();
>
>   	chat = g_at_chat_new(channel, syntax);
> +	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
> +		g_at_chat_add_terminator(chat, "+CPIN: READY", -1, TRUE);

Please do this inside drivers/atmodem/sim.c?  In at_sim_probe() 
specifically.

>   	g_at_syntax_unref(syntax);
>   	g_io_channel_unref(channel);
>
> @@ -118,6 +120,16 @@ static int wavecom_enable(struct ofono_modem *modem)
>   	return 0;
>   }
>
> +static int wavecom_enable(struct ofono_modem *modem)
> +{
> +	return wavecom_generic_enable(modem, OFONO_VENDOR_WAVECOM);
> +}
> +
> +static int wavecom_q2xxx_enable(struct ofono_modem *modem)
> +{
> +	return wavecom_generic_enable(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
> +}
> +
>   static int wavecom_disable(struct ofono_modem *modem)
>   {
>   	GAtChat *chat = ofono_modem_get_data(modem);
> @@ -131,18 +143,32 @@ static int wavecom_disable(struct ofono_modem *modem)
>   	return 0;
>   }
>
> -static void wavecom_pre_sim(struct ofono_modem *modem)
> +static void wavecom_generic_pre_sim(struct ofono_modem *modem, int vendor)
>   {
>   	GAtChat *chat = ofono_modem_get_data(modem);
> +	struct ofono_sim *sim;
>
>   	DBG("%p", modem);
>
>   	ofono_devinfo_create(modem, 0, "atmodem", chat);
> -	ofono_sim_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat);
> +	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
> +	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
> +		ofono_sim_inserted_notify(sim, TRUE);
> +

Do something like:
vendor = 0;

if ofono_modem_get_string(modem, "Model") equals "Q2XXX":
	vendor = OFONO_VENDOR_WAVECOM_Q2XXX;

ofono_sim_create(...);

>   	ofono_voicecall_create(modem, 0, "atmodem", chat);
>   }
>
> -static void wavecom_post_sim(struct ofono_modem *modem)
> +static void wavecom_pre_sim(struct ofono_modem *modem)
> +{
> +	wavecom_generic_pre_sim(modem, OFONO_VENDOR_WAVECOM);
> +}
> +
> +static void wavecom_q2xxx_pre_sim(struct ofono_modem *modem)
> +{
> +	wavecom_generic_pre_sim(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
> +}
> +
> +static void wavecom_generic_post_sim(struct ofono_modem *modem, int vendor)
>   {
>   	GAtChat *chat = ofono_modem_get_data(modem);
>   	struct ofono_message_waiting *mw;
> @@ -155,7 +181,8 @@ static void wavecom_post_sim(struct ofono_modem *modem)
>   	ofono_netreg_create(modem, 0, "atmodem", chat);
>   	ofono_call_meter_create(modem, 0, "atmodem", chat);
>   	ofono_call_barring_create(modem, 0, "atmodem", chat);
> -	ofono_sms_create(modem, 0, "atmodem", chat);
> +	/* We have to specify the vendor to support AT+CPMS=? reply in Q2xxx */
> +	ofono_sms_create(modem, vendor, "atmodem", chat);
>   	ofono_phonebook_create(modem, 0, "atmodem", chat);
>
>   	mw = ofono_message_waiting_create(modem);
> @@ -163,6 +190,16 @@ static void wavecom_post_sim(struct ofono_modem *modem)
>   		ofono_message_waiting_register(mw);
>   }
>
> +static void wavecom_post_sim(struct ofono_modem *modem)
> +{
> +	wavecom_generic_post_sim(modem, OFONO_VENDOR_WAVECOM);
> +}
> +
> +static void wavecom_q2xxx_post_sim(struct ofono_modem *modem)
> +{
> +	wavecom_generic_post_sim(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
> +}
> +
>   static struct ofono_modem_driver wavecom_driver = {
>   	.name		= "wavecom",
>   	.probe		= wavecom_probe,
> @@ -173,15 +210,43 @@ static struct ofono_modem_driver wavecom_driver = {
>   	.post_sim	= wavecom_post_sim,
>   };
>
> +static struct ofono_modem_driver wavecom_q2xxx_driver = {
> +	.name		= "wavecom_q2xxx",
> +	.probe		= wavecom_probe,
> +	.remove		= wavecom_remove,
> +	.enable		= wavecom_q2xxx_enable,
> +	.disable	= wavecom_disable,
> +	.pre_sim	= wavecom_q2xxx_pre_sim,
> +	.post_sim	= wavecom_q2xxx_post_sim,
> +};
> +
>   static int wavecom_init(void)
>   {
> -	return ofono_modem_driver_register(&wavecom_driver);
> +	int ret;
> +
> +	ret = ofono_modem_driver_register(&wavecom_driver);
> +	if (ret<  0)
> +		goto err;
> +
> +	ret = ofono_modem_driver_register(&wavecom_q2xxx_driver);
> +	if (ret<  0)
> +		goto err_wavecom;
> +
> +	return ret;
> +
> +err_wavecom:
> +	ofono_modem_driver_unregister(&wavecom_driver);
> +err:
> +	return ret;
>   }
>
>   static void wavecom_exit(void)
>   {
>   	ofono_modem_driver_unregister(&wavecom_driver);
> +	ofono_modem_driver_unregister(&wavecom_q2xxx_driver);
>   }
>
>   OFONO_PLUGIN_DEFINE(wavecom, "Wavecom driver", VERSION,
>   		OFONO_PLUGIN_PRIORITY_DEFAULT, wavecom_init, wavecom_exit)
> +OFONO_PLUGIN_DEFINE(wavecom_q2xxx, "Wavecom Q2XXX driver", VERSION,
> +		OFONO_PLUGIN_PRIORITY_DEFAULT, wavecom_init, wavecom_exit)

The rest of this is really unnecessary, there is no need to register 
multiple modem drivers or any other fancy stuff ;)

Regards,
-Denis

[PATCH] wavecom: add support for Wavecom Q2403/Q2686 modems

[pablo]

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

From: Pablo Neira Ayuso <pablo@gnumonks.org>

This patch extends the existing wavecom plugin to include the new
wavecom 2xxx plugin.
---
 plugins/udev.c    |   15 ++++++++++++
 plugins/wavecom.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/plugins/udev.c b/plugins/udev.c
index 8cb87a5..6e3205e 100644
--- a/plugins/udev.c
+++ b/plugins/udev.c
@@ -167,6 +167,19 @@ static void add_calypso(struct ofono_modem *modem,
 	ofono_modem_register(modem);
 }
 
+static void add_wavecom_q2xxx(struct ofono_modem *modem,
+					struct udev_device *udev_device)
+{
+	const char *devnode;
+
+	DBG("modem %p", modem);
+
+	devnode = udev_device_get_devnode(udev_device);
+	ofono_modem_set_string(modem, "Device", devnode);
+
+	ofono_modem_register(modem);
+}
+
 static void add_tc65(struct ofono_modem *modem,
 			struct udev_device *udev_device)
 {
@@ -286,6 +299,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_q2xxx") == 0)
+		add_wavecom_q2xxx(modem, udev_device);
 }
 
 static gboolean devpath_remove(gpointer key, gpointer value, gpointer user_data)
diff --git a/plugins/wavecom.c b/plugins/wavecom.c
index 5d30f39..c8bd55b 100644
--- a/plugins/wavecom.c
+++ b/plugins/wavecom.c
@@ -66,7 +66,7 @@ static void wavecom_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }
 
-static int wavecom_enable(struct ofono_modem *modem)
+static int wavecom_generic_enable(struct ofono_modem *modem, int vendor)
 {
 	GAtChat *chat;
 	GIOChannel *channel;
@@ -104,6 +104,8 @@ static int wavecom_enable(struct ofono_modem *modem)
 	syntax = g_at_syntax_new_gsm_permissive();
 
 	chat = g_at_chat_new(channel, syntax);
+	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
+		g_at_chat_add_terminator(chat, "+CPIN: READY", -1, TRUE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
@@ -118,6 +120,16 @@ static int wavecom_enable(struct ofono_modem *modem)
 	return 0;
 }
 
+static int wavecom_enable(struct ofono_modem *modem)
+{
+	return wavecom_generic_enable(modem, OFONO_VENDOR_WAVECOM);
+}
+
+static int wavecom_q2xxx_enable(struct ofono_modem *modem)
+{
+	return wavecom_generic_enable(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
+}
+
 static int wavecom_disable(struct ofono_modem *modem)
 {
 	GAtChat *chat = ofono_modem_get_data(modem);
@@ -131,18 +143,32 @@ static int wavecom_disable(struct ofono_modem *modem)
 	return 0;
 }
 
-static void wavecom_pre_sim(struct ofono_modem *modem)
+static void wavecom_generic_pre_sim(struct ofono_modem *modem, int vendor)
 {
 	GAtChat *chat = ofono_modem_get_data(modem);
+	struct ofono_sim *sim;
 
 	DBG("%p", modem);
 
 	ofono_devinfo_create(modem, 0, "atmodem", chat);
-	ofono_sim_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat);
+	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
+	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
+		ofono_sim_inserted_notify(sim, TRUE);
+
 	ofono_voicecall_create(modem, 0, "atmodem", chat);
 }
 
-static void wavecom_post_sim(struct ofono_modem *modem)
+static void wavecom_pre_sim(struct ofono_modem *modem)
+{
+	wavecom_generic_pre_sim(modem, OFONO_VENDOR_WAVECOM);
+}
+
+static void wavecom_q2xxx_pre_sim(struct ofono_modem *modem)
+{
+	wavecom_generic_pre_sim(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
+}
+
+static void wavecom_generic_post_sim(struct ofono_modem *modem, int vendor)
 {
 	GAtChat *chat = ofono_modem_get_data(modem);
 	struct ofono_message_waiting *mw;
@@ -155,7 +181,8 @@ static void wavecom_post_sim(struct ofono_modem *modem)
 	ofono_netreg_create(modem, 0, "atmodem", chat);
 	ofono_call_meter_create(modem, 0, "atmodem", chat);
 	ofono_call_barring_create(modem, 0, "atmodem", chat);
-	ofono_sms_create(modem, 0, "atmodem", chat);
+	/* We have to specify the vendor to support AT+CPMS=? reply in Q2xxx */
+	ofono_sms_create(modem, vendor, "atmodem", chat);
 	ofono_phonebook_create(modem, 0, "atmodem", chat);
 
 	mw = ofono_message_waiting_create(modem);
@@ -163,6 +190,16 @@ static void wavecom_post_sim(struct ofono_modem *modem)
 		ofono_message_waiting_register(mw);
 }
 
+static void wavecom_post_sim(struct ofono_modem *modem)
+{
+	wavecom_generic_post_sim(modem, OFONO_VENDOR_WAVECOM);
+}
+
+static void wavecom_q2xxx_post_sim(struct ofono_modem *modem)
+{
+	wavecom_generic_post_sim(modem, OFONO_VENDOR_WAVECOM_Q2XXX);
+}
+
 static struct ofono_modem_driver wavecom_driver = {
 	.name		= "wavecom",
 	.probe		= wavecom_probe,
@@ -173,15 +210,43 @@ static struct ofono_modem_driver wavecom_driver = {
 	.post_sim	= wavecom_post_sim,
 };
 
+static struct ofono_modem_driver wavecom_q2xxx_driver = {
+	.name		= "wavecom_q2xxx",
+	.probe		= wavecom_probe,
+	.remove		= wavecom_remove,
+	.enable		= wavecom_q2xxx_enable,
+	.disable	= wavecom_disable,
+	.pre_sim	= wavecom_q2xxx_pre_sim,
+	.post_sim	= wavecom_q2xxx_post_sim,
+};
+
 static int wavecom_init(void)
 {
-	return ofono_modem_driver_register(&wavecom_driver);
+	int ret;
+
+	ret = ofono_modem_driver_register(&wavecom_driver);
+	if (ret < 0)
+		goto err;
+
+	ret = ofono_modem_driver_register(&wavecom_q2xxx_driver);
+	if (ret < 0)
+		goto err_wavecom;
+
+	return ret;
+
+err_wavecom:
+	ofono_modem_driver_unregister(&wavecom_driver);
+err:
+	return ret;
 }
 
 static void wavecom_exit(void)
 {
 	ofono_modem_driver_unregister(&wavecom_driver);
+	ofono_modem_driver_unregister(&wavecom_q2xxx_driver);
 }
 
 OFONO_PLUGIN_DEFINE(wavecom, "Wavecom driver", VERSION,
 		OFONO_PLUGIN_PRIORITY_DEFAULT, wavecom_init, wavecom_exit)
+OFONO_PLUGIN_DEFINE(wavecom_q2xxx, "Wavecom Q2XXX driver", VERSION,
+		OFONO_PLUGIN_PRIORITY_DEFAULT, wavecom_init, wavecom_exit)
-- 
1.7.10