From: Denis Kenzior <denkenz@gmail.com>
To: ofono@ofono.org
Subject: Re: [PATCH] Add GPRS, mux, volume control support in SIMCOM sim900 modem module
Date: Thu, 28 Feb 2013 10:42:57 -0600 [thread overview]
Message-ID: <512F8911.3030100@gmail.com> (raw)
In-Reply-To: <1362066215-27452-1-git-send-email-r.r.zaripov@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10015 bytes --]
Hi Renat,
On 02/28/2013 09:43 AM, r.r.zaripov(a)gmail.com wrote:
> From: Renat Zaripov<r.r.zaripov@gmail.com>
>
> ---
> plugins/sim900.c | 162 +++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 137 insertions(+), 25 deletions(-)
>
Watch those trailing whitespaces:
Applying: Add GPRS, mux, volume control support in SIMCOM sim900 modem
module
/home/denkenz/ofono-master/.git/rebase-apply/patch:118: trailing whitespace.
if(!g_at_mux_start(data->mux))
/home/denkenz/ofono-master/.git/rebase-apply/patch:153: trailing whitespace.
/home/denkenz/ofono-master/.git/rebase-apply/patch:155: trailing whitespace.
/home/denkenz/ofono-master/.git/rebase-apply/patch:186: trailing whitespace.
if (data->dlcs[SETUP_DLC] == NULL)
/home/denkenz/ofono-master/.git/rebase-apply/patch:202: trailing whitespace.
g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CMUX=0,0,5,128,10,3,30,10,2",
warning: squelched 1 whitespace error
fatal: 6 lines add whitespace errors.
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index 496faa6..f228c0e 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -29,6 +29,7 @@
> #include<glib.h>
> #include<gatchat.h>
> #include<gattty.h>
> +#include<gatmux.h>
>
> #define OFONO_API_SUBJECT_TO_CHANGE
> #include<ofono/plugin.h>
> @@ -44,13 +45,28 @@
> #include<ofono/history.h>
> #include<ofono/log.h>
> #include<ofono/voicecall.h>
> +#include<ofono/call-volume.h>
>
> #include<drivers/atmodem/vendor.h>
>
> +#define NUM_DLC 5
> +
> +#define SETUP_DLC 0
> +#define VOICE_DLC 1
> +#define NETREG_DLC 2
> +#define SMS_DLC 3
> +#define GPRS_DLC 4
> +
> +
No double empty lines please
> +static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ", "GPRS: " , "Setup: "};
This line is over 80 characters
> +
> static const char *none_prefix[] = { NULL };
>
> struct sim900_data {
> - GAtChat *modem;
> + GIOChannel *device;
> + GAtMux *mux;
> + GAtChat *dlcs[NUM_DLC];
> + guint frame_size;
> };
>
> static int sim900_probe(struct ofono_modem *modem)
> @@ -76,7 +92,7 @@ static void sim900_remove(struct ofono_modem *modem)
>
> ofono_modem_set_data(modem, NULL);
>
> - g_at_chat_unref(data->modem);
> + g_at_chat_unref(data->dlcs[SETUP_DLC]);
Why do you only take care of 1 DLC here? You need to clean up all of
them and the mux as well.
>
> g_free(data);
> }
> @@ -91,6 +107,7 @@ static void sim900_debug(const char *str, void *user_data)
> static GAtChat *open_device(struct ofono_modem *modem,
> const char *key, char *debug)
> {
> + struct sim900_data *data = ofono_modem_get_data(modem);
> const char *device;
> GAtSyntax *syntax;
> GIOChannel *channel;
> @@ -119,10 +136,33 @@ static GAtChat *open_device(struct ofono_modem *modem,
> if (channel == NULL)
> return NULL;
>
> + data->device = channel;
> syntax = g_at_syntax_new_gsm_permissive();
> chat = g_at_chat_new(channel, syntax);
> g_at_syntax_unref(syntax);
>
> + if (chat == NULL)
> + return NULL;
> +
> + if (getenv("OFONO_AT_DEBUG"))
> + g_at_chat_set_debug(chat, sim900_debug, debug);
> +
> + return chat;
> +}
> +
> +
Again please no double-empty lines
> +static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *modem,
> + char *debug)
> +{
> + GAtSyntax *syntax;
> + GAtChat *chat;
> +
> + if (channel == NULL)
> + return NULL;
> +
> + syntax = g_at_syntax_new_gsmv1();
> + chat = g_at_chat_new(channel, syntax);
> + g_at_syntax_unref(syntax);
> g_io_channel_unref(channel);
>
> if (chat == NULL)
> @@ -134,6 +174,65 @@ static GAtChat *open_device(struct ofono_modem *modem,
> return chat;
> }
>
> +static void setup_internal_mux(struct ofono_modem *modem)
> +{
> + struct sim900_data *data = ofono_modem_get_data(modem);
> + int i;
> +
> + DBG("");
> +
> + data->frame_size = 128;
> +
> + data->mux = g_at_mux_new_gsm0710_basic(data->device, data->frame_size);
> + if (data->mux == NULL)
> + goto error;
> +
> + if (getenv("OFONO_MUX_DEBUG"))
> + g_at_mux_set_debug(data->mux, sim900_debug, "MUX: ");
> +
> + if(!g_at_mux_start(data->mux))
You're not cleaning up the mux here
> + goto error;
> +
> + for (i = 0; i< NUM_DLC; i++) {
> + GIOChannel *channel = g_at_mux_create_channel(data->mux);
> +
> + data->dlcs[i] = create_chat(channel, modem, dlc_prefixes[i]);
> + if (data->dlcs[i] == NULL) {
> + ofono_error("Failed to create channel");
> + goto error;
You are not cleaning up channels or the mux here
> + }
> + }
> +
> + ofono_modem_set_powered(modem, TRUE);
> +
> + return;
> +
> +error:
> + ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct sim900_data *data = ofono_modem_get_data(modem);
> +
> + DBG("");
> +
> + g_at_chat_unref(data->dlcs[SETUP_DLC]);
> + data->dlcs[SETUP_DLC] = NULL;
> +
> + if (!ok)
> + goto error;
> +
> + setup_internal_mux(modem);
> +
> + return;
> +
> +error:
> + ofono_modem_set_powered(modem, FALSE);
You also need to close the device IO channel.
> +}
> +
> +
And again, double empty lines :)
> static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> {
> struct ofono_modem *modem = user_data;
> @@ -142,11 +241,9 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> DBG("");
>
> if (!ok) {
> - g_at_chat_unref(data->modem);
> - data->modem = NULL;
> + g_at_chat_unref(data->dlcs[SETUP_DLC]);
> + data->dlcs[SETUP_DLC] = NULL;
> }
> -
> - ofono_modem_set_powered(modem, ok);
> }
>
> static int sim900_enable(struct ofono_modem *modem)
> @@ -155,21 +252,22 @@ static int sim900_enable(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - data->modem = open_device(modem, "Device", "Device: ");
> - if (data->modem == NULL) {
> - DBG("return -EINVAL");
> + data->dlcs[SETUP_DLC] = open_device(modem, "Device", "Setup: ");
> + if (data->dlcs[SETUP_DLC] == NULL)
> return -EINVAL;
> - }
>
> - g_at_chat_send(data->modem, "ATE0", NULL, NULL, NULL, NULL);
> + g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
>
> /* For obtain correct sms service number */
> - g_at_chat_send(data->modem, "AT+CSCS=\"GSM\"", NULL,
> + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"", NULL,
> NULL, NULL, NULL);
>
> - g_at_chat_send(data->modem, "AT+CFUN=1", none_prefix,
> + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=1", none_prefix,
> cfun_enable, modem, NULL);
>
> + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CMUX=0,0,5,128,10,3,30,10,2",
> + NULL, mux_setup_cb, modem, NULL);
Maybe you should do this from cfun_enable, and call
ofono_modem_set_powered once the mux is setup, not before.
> +
> return -EINPROGRESS;
> }
>
> @@ -181,8 +279,8 @@ static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
>
> DBG("");
>
> - g_at_chat_unref(data->modem);
> - data->modem = NULL;
> + g_at_chat_unref(data->dlcs[SETUP_DLC]);
> + data->dlcs[SETUP_DLC] = NULL;
You need to clean up properly here, including all chats, muxer, and
device channel.
>
> if (ok)
> ofono_modem_set_powered(modem, FALSE);
> @@ -194,10 +292,10 @@ static int sim900_disable(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - g_at_chat_cancel_all(data->modem);
> - g_at_chat_unregister_all(data->modem);
> + g_at_chat_cancel_all(data->dlcs[SETUP_DLC]);
> + g_at_chat_unregister_all(data->dlcs[SETUP_DLC]);
>
> - g_at_chat_send(data->modem, "AT+CFUN=4", none_prefix,
> + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=4", none_prefix,
> cfun_disable, modem, NULL);
>
> return -EINPROGRESS;
> @@ -210,9 +308,9 @@ static void sim900_pre_sim(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_devinfo_create(modem, 0, "atmodem", data->modem);
> + ofono_devinfo_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> sim = ofono_sim_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> - data->modem);
> + data->dlcs[VOICE_DLC]);
>
> if (sim)
> ofono_sim_inserted_notify(sim, TRUE);
> @@ -221,12 +319,25 @@ static void sim900_pre_sim(struct ofono_modem *modem)
> static void sim900_post_sim(struct ofono_modem *modem)
> {
> struct sim900_data *data = ofono_modem_get_data(modem);
> + struct ofono_gprs *gprs;
> + struct ofono_gprs_context *gc;
>
> DBG("%p", modem);
>
> - ofono_phonebook_create(modem, 0, "atmodem", data->modem);
> + ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> - data->modem);
> + data->dlcs[SMS_DLC]);
> +
> + gprs = ofono_gprs_create(modem, 0,
> + "atmodem", data->dlcs[GPRS_DLC]);
> + if (gprs == NULL)
> + return;
> +
> + gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> + "atmodem", data->dlcs[GPRS_DLC]);
> + if (gc)
> + ofono_gprs_add_context(gprs, gc);
> +
> }
>
> static void sim900_post_online(struct ofono_modem *modem)
> @@ -235,9 +346,10 @@ static void sim900_post_online(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_netreg_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", data->modem);
> - ofono_ussd_create(modem, 0, "atmodem", data->modem);
> - ofono_voicecall_create(modem, 0, "atmodem", data->modem);
> + ofono_netreg_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", data->dlcs[VOICE_DLC]);
> + ofono_ussd_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> + ofono_voicecall_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> + ofono_call_volume_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
> }
>
> static struct ofono_modem_driver sim900_driver = {
Regards,
-Denis
next prev parent reply other threads:[~2013-02-28 16:42 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-28 15:43 [PATCH] Add GPRS, mux, volume control support in SIMCOM sim900 modem module r.r.zaripov
2013-02-28 15:43 ` [PATCH] Add vendor quirk for SIMCOM r.r.zaripov
2013-02-28 16:44 ` Denis Kenzior
2013-02-28 16:42 ` Denis Kenzior [this message]
-- strict thread matches above, loose matches on Subject: below --
2013-03-12 14:37 [PATCH] Add GPRS, mux, volume control support in SIMCOM sim900 modem module r.r.zaripov
2013-03-26 15:54 ` 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=512F8911.3030100@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.