From: Denis Kenzior <denkenz@gmail.com>
To: ofono@ofono.org
Subject: Re: [PATCH] ste: Add support for multiple AT channels
Date: Wed, 02 Mar 2011 23:48:09 -0600 [thread overview]
Message-ID: <4D6F2B99.7060203@gmail.com> (raw)
In-Reply-To: <1299058433-16980-1-git-send-email-lasse.kunnasluoto@tieto.com>
[-- Attachment #1: Type: text/plain, Size: 9979 bytes --]
Hi Lasse,
On 03/02/2011 03:33 AM, Lasse Kunnasluoto wrote:
> ---
> plugins/ste.c | 138 ++++++++++++++++++++++++++++++++++++---------------------
> 1 files changed, 87 insertions(+), 51 deletions(-)
>
> diff --git a/plugins/ste.c b/plugins/ste.c
> index b786571..9a1d3d3 100644
> --- a/plugins/ste.c
> +++ b/plugins/ste.c
> @@ -65,13 +65,19 @@
> #include <drivers/stemodem/caif_socket.h>
> #include <drivers/stemodem/if_caif.h>
>
> -#define NUM_CHAT 1
> +#define NUM_CHAT 5
> +#define AT_DEFAULT 0
> +#define AT_NET 1
> +#define AT_VOICE 2
> +#define AT_GPRS 3
> +#define AT_SIM 4
Can you cleanup the indentation of the channel numbers here to be more
consistent? And I'd actually like to have an empty line between these
and the MAX_PDP_CONTEXTS define.
> #define MAX_PDP_CONTEXTS 4
>
> -static char *chat_prefixes[NUM_CHAT] = { "Default: " };
> +static char *chat_prefixes[NUM_CHAT] = { "Default: ", "Net: ", "Voice: ",
> + "GPRS: ", "SIM: " };
>
> struct ste_data {
> - GAtChat *chat;
> + GAtChat *chat[NUM_CHAT];
> gboolean have_sim;
> struct ofono_sim *sim;
> };
> @@ -105,12 +111,14 @@ static int ste_probe(struct ofono_modem *modem)
> static void ste_remove(struct ofono_modem *modem)
> {
> struct ste_data *data = ofono_modem_get_data(modem);
> + int i;
>
> DBG("%p", modem);
>
> ofono_modem_set_data(modem, NULL);
>
> - g_at_chat_unref(data->chat);
> + for (i = 0; i < NUM_CHAT; i++)
> + g_at_chat_unref(data->chat[i]);
>
> g_free(data);
> }
> @@ -185,7 +193,7 @@ static gboolean init_sim_reporting(gpointer user_data)
> struct ofono_modem *modem = user_data;
> struct ste_data *data = ofono_modem_get_data(modem);
>
> - g_at_chat_send(data->chat, "AT*ESIMSR=1;*ESIMSR?", NULL,
> + g_at_chat_send(data->chat[AT_SIM], "AT*ESIMSR=1;*ESIMSR?", NULL,
> handle_sim_state,
> modem, NULL);
>
> @@ -195,11 +203,21 @@ static gboolean init_sim_reporting(gpointer user_data)
> static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> {
> struct ofono_modem *modem = user_data;
> + struct ste_data *data = ofono_modem_get_data(modem);
> + int i;
>
> DBG("");
>
> if (!ok) {
> ofono_modem_set_powered(modem, FALSE);
> +
> + for (i = 0; i < NUM_CHAT; i++) {
> + g_at_chat_cancel_all(data->chat[i]);
> + g_at_chat_unregister_all(data->chat[i]);
> + g_at_chat_unref(data->chat[i]);
> + data->chat[i] = NULL;
> + }
> +
> return;
> }
>
> @@ -295,35 +313,47 @@ static void esimsr_notify(GAtResult *result, gpointer user_data)
> static int ste_enable(struct ofono_modem *modem)
> {
> struct ste_data *data = ofono_modem_get_data(modem);
> - GIOChannel *channel;
> GAtSyntax *syntax;
> + int i;
>
> syntax = g_at_syntax_new_gsm_permissive();
>
Sharing a syntax between all channels isn't really going to work out how
you think ;) Each syntax has its own state, so you have to create a
syntax for every channel separately.
> - channel = ste_create_channel(modem);
> - if (!channel)
> - return -EIO;
> + for (i = 0; i < NUM_CHAT; i++) {
> + GIOChannel *channel = ste_create_channel(modem);
>
> - data->chat = g_at_chat_new_blocking(channel, syntax);
> - g_at_chat_send(data->chat, "AT&F E0 V1 X4 &C1 +CMEE=1",
> - NULL, NULL, NULL, NULL);
> + data->chat[i] = g_at_chat_new_blocking(channel, syntax);
> + if (data->chat[i] == NULL)
> + return -EIO;
You might want to handle these errors properly. If creation of one chat
fails, then you should unref all of them.
>
> - /* All STE modems support UTF-8 */
> - g_at_chat_send(data->chat, "AT+CSCS=\"UTF-8\"",
> - NULL, NULL, NULL, NULL);
> + g_at_chat_send(data->chat[i], "AT&F E0 V1 X4 &C1 +CMEE=1",
> + NULL, NULL, NULL, NULL);
>
> - g_io_channel_unref(channel);
> - g_at_syntax_unref(syntax);
> + /* All STE modems support UTF-8 */
> + g_at_chat_send(data->chat[i], "AT+CSCS=\"UTF-8\"",
> + NULL, NULL, NULL, NULL);
>
> - if (data->chat == NULL)
> - return -ENOMEM;
> + g_io_channel_unref(channel);
> + }
>
> - if (getenv("OFONO_AT_DEBUG"))
> - g_at_chat_set_debug(data->chat, ste_debug, chat_prefixes[0]);
> + g_at_syntax_unref(syntax);
>
> - g_at_chat_send(data->chat, "AT+CFUN=4", NULL, cfun_enable, modem, NULL);
> + if (getenv("OFONO_AT_DEBUG")) {
> + g_at_chat_set_debug(data->chat[AT_DEFAULT], ste_debug,
> + chat_prefixes[0]);
> + g_at_chat_set_debug(data->chat[AT_NET], ste_debug,
> + chat_prefixes[1]);
> + g_at_chat_set_debug(data->chat[AT_VOICE], ste_debug,
> + chat_prefixes[2]);
> + g_at_chat_set_debug(data->chat[AT_GPRS], ste_debug,
> + chat_prefixes[3]);
> + g_at_chat_set_debug(data->chat[AT_SIM], ste_debug,
> + chat_prefixes[4]);
> + }
Why is this being done here and not in a loop above?
>
> - g_at_chat_register(data->chat, "*ESIMSR:", esimsr_notify,
> + g_at_chat_send(data->chat[AT_DEFAULT], "AT+CFUN=4", NULL, cfun_enable,
> + modem, NULL);
> +
> + g_at_chat_register(data->chat[AT_SIM], "*ESIMSR:", esimsr_notify,
> FALSE, modem, NULL);
>
> return -EINPROGRESS;
> @@ -333,11 +363,14 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> {
> struct ofono_modem *modem = user_data;
> struct ste_data *data = ofono_modem_get_data(modem);
> + int i;
>
> DBG("");
>
> - g_at_chat_unref(data->chat);
> - data->chat = NULL;
> + for (i = 0; i < NUM_CHAT; i++) {
> + g_at_chat_unref(data->chat[i]);
> + data->chat[i] = NULL;
> + }
>
> if (ok)
> ofono_modem_set_powered(modem, FALSE);
> @@ -346,15 +379,15 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> static int ste_disable(struct ofono_modem *modem)
> {
> struct ste_data *data = ofono_modem_get_data(modem);
> + int i;
>
> DBG("%p", modem);
>
> - if (data->chat == NULL)
> - return 0;
> -
> - g_at_chat_cancel_all(data->chat);
> - g_at_chat_unregister_all(data->chat);
> - g_at_chat_send(data->chat, "AT+CFUN=4", NULL,
> + for (i = 0; i < NUM_CHAT; i++) {
> + g_at_chat_cancel_all(data->chat[i]);
> + g_at_chat_unregister_all(data->chat[i]);
> + }
Coding style M1
> + g_at_chat_send(data->chat[AT_DEFAULT], "AT+CFUN=4", NULL,
> cfun_disable, modem, NULL);
>
> return -EINPROGRESS;
> @@ -375,7 +408,7 @@ static void ste_set_online(struct ofono_modem *modem, ofono_bool_t online,
> ofono_modem_online_cb_t cb, void *user_data)
> {
> struct ste_data *data = ofono_modem_get_data(modem);
> - GAtChat *chat = data->chat;
> + GAtChat *chat = data->chat[AT_DEFAULT];
> struct cb_data *cbd = cb_data_new(cb, user_data);
> char const *command = online ? "AT+CFUN=1" : "AT+CFUN=4";
>
> @@ -395,10 +428,10 @@ static void ste_pre_sim(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> + ofono_devinfo_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> data->sim = ofono_sim_create(modem, OFONO_VENDOR_MBM, "atmodem",
> - data->chat);
> - ofono_voicecall_create(modem, 0, "stemodem", data->chat);
> + data->chat[AT_SIM]);
> + ofono_voicecall_create(modem, 0, "stemodem", data->chat[AT_VOICE]);
> }
>
> static void ste_post_sim(struct ofono_modem *modem)
> @@ -407,11 +440,11 @@ static void ste_post_sim(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_stk_create(modem, 0, "mbmmodem", data->chat);
> - ofono_phonebook_create(modem, 0, "atmodem", data->chat);
> - ofono_radio_settings_create(modem, 0, "stemodem", data->chat);
> + ofono_stk_create(modem, 0, "mbmmodem", data->chat[AT_SIM]);
> + ofono_phonebook_create(modem, 0, "atmodem", data->chat[AT_SIM]);
> + ofono_radio_settings_create(modem, 0, "stemodem", data->chat[AT_NET]);
>
> - ofono_sms_create(modem, 0, "atmodem", data->chat);
> + ofono_sms_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> }
>
> static void ste_post_online(struct ofono_modem *modem)
> @@ -424,22 +457,25 @@ static void ste_post_online(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_ussd_create(modem, 0, "atmodem", data->chat);
> - ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
> - ofono_call_settings_create(modem, 0, "atmodem", data->chat);
> - ofono_netreg_create(modem, OFONO_VENDOR_MBM, "atmodem", data->chat);
> - ofono_call_meter_create(modem, 0, "atmodem", data->chat);
> - ofono_call_barring_create(modem, 0, "atmodem", data->chat);
> - ofono_ssn_create(modem, 0, "atmodem", data->chat);
> - ofono_call_volume_create(modem, 0, "atmodem", data->chat);
> - ofono_cbs_create(modem, 0, "atmodem", data->chat);
> + ofono_ussd_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_call_forwarding_create(modem, 0,
> + "atmodem", data->chat[AT_DEFAULT]);
> + ofono_call_settings_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_netreg_create(modem, OFONO_VENDOR_MBM,
> + "atmodem", data->chat[AT_NET]);
> + ofono_call_meter_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_call_barring_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_ssn_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_call_volume_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
> + ofono_cbs_create(modem, 0, "atmodem", data->chat[AT_DEFAULT]);
>
> gprs = ofono_gprs_create(modem, OFONO_VENDOR_MBM,
> - "atmodem", data->chat);
> + "atmodem", data->chat[AT_GPRS]);
> +
> if (gprs) {
> for (i = 0; i < MAX_PDP_CONTEXTS; i++) {
> - gc = ofono_gprs_context_create(
> - modem, 0, "stemodem", data->chat);
> + gc = ofono_gprs_context_create(modem, 0, "stemodem",
> + data->chat[AT_GPRS]);
You have a whitespace violation here, mixing tabs and spaces.
> if (gc == NULL)
> break;
>
Regards,
-Denis
next prev parent reply other threads:[~2011-03-03 5:48 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-02 9:33 [PATCH] ste: Add support for multiple AT channels Lasse Kunnasluoto
2011-03-03 5:48 ` Denis Kenzior [this message]
2011-03-03 11:21 ` [PATCH v2] " Lasse Kunnasluoto
2011-03-03 19:43 ` Denis Kenzior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D6F2B99.7060203@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@ofono.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox