* [PATCH] mbmmodem: don't let chat open after fd is sent
@ 2011-03-03 22:23 Lucas De Marchi
2011-03-04 3:57 ` Denis Kenzior
0 siblings, 1 reply; 6+ messages in thread
From: Lucas De Marchi @ 2011-03-03 22:23 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]
Instead of using a GAtChat, just use a GIOChannel and close it as soon
as its fd is sent to core.
---
drivers/mbmmodem/location-reporting.c | 32 +++++++++++++++-----------------
1 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index 941fac4..b76a5c2 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -94,13 +94,13 @@ static void mbm_location_reporting_disable(struct ofono_location_reporting *lr,
g_free(cbd);
}
-static int mbm_create_data_chat(struct ofono_location_reporting *lr)
+static int enable_data_stream(struct ofono_location_reporting *lr)
{
- struct gps_data *gd = ofono_location_reporting_get_data(lr);
struct ofono_modem *modem;
const char *gps_dev;
- GAtSyntax *syntax;
GIOChannel *channel;
+ GIOStatus status;
+ gsize written;
int fd;
modem = ofono_location_reporting_get_modem(lr);
@@ -110,15 +110,18 @@ static int mbm_create_data_chat(struct ofono_location_reporting *lr)
if (channel == NULL)
return -1;
- syntax = g_at_syntax_new_gsm_permissive();
- gd->data_chat = g_at_chat_new(channel, syntax);
fd = g_io_channel_unix_get_fd(channel);
+ status = g_io_channel_write_chars(channel, "AT*E2GPSNPD\r\n", -1,
+ &written, NULL);
- g_at_syntax_unref(syntax);
+ g_io_channel_set_close_on_unref(channel, FALSE);
g_io_channel_unref(channel);
- if (gd->data_chat == NULL)
+ if (status != G_IO_STATUS_NORMAL || written != 13) {
+ close(fd);
+
return -1;
+ }
return fd;
}
@@ -129,7 +132,6 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
struct cb_data *cbd = user_data;
ofono_location_reporting_enable_cb_t cb = cbd->cb;
struct ofono_location_reporting *lr = cbd->user;
- struct gps_data *gd = ofono_location_reporting_get_data(lr);
struct ofono_error error;
int fd;
@@ -143,20 +145,16 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
return;
}
- fd = mbm_create_data_chat(lr);
+ fd = enable_data_stream(lr);
- if (fd < 0)
- goto out;
-
- if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
- NULL) > 0) {
- cb(&error, fd, cbd->data);
+ if (fd < 0) {
+ CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
return;
}
-out:
- CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+ cb(&error, fd, cbd->data);
+ close(fd);
}
static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] mbmmodem: close chat before sending fd
@ 2011-03-01 21:46 Denis Kenzior
2011-03-03 17:14 ` [PATCH] mbmmodem: don't let chat open after fd is sent Lucas De Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Denis Kenzior @ 2011-03-01 21:46 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]
Hi Lucas,
On 02/28/2011 07:43 AM, Lucas De Marchi wrote:
> ---
> drivers/mbmmodem/location-reporting.c | 44 ++++++++++++++++++---------------
> 1 files changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
> index 941fac4..b671d71 100644
> --- a/drivers/mbmmodem/location-reporting.c
> +++ b/drivers/mbmmodem/location-reporting.c
> @@ -48,7 +48,7 @@ static const char *e2gpsctl_prefix[] = { "*E2GPSCTL:", NULL };
>
> struct gps_data {
> GAtChat *chat;
> - GAtChat *data_chat;
> + GIOChannel *data_channel;
> };
>
> static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
> @@ -70,7 +70,7 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
> return;
> }
>
> - g_at_chat_unref(gd->data_chat);
> + g_io_channel_unref(gd->data_channel);
>
> CALLBACK_WITH_SUCCESS(cb, cbd->data);
> }
> @@ -94,33 +94,33 @@ static void mbm_location_reporting_disable(struct ofono_location_reporting *lr,
> g_free(cbd);
> }
>
> -static int mbm_create_data_chat(struct ofono_location_reporting *lr)
> +static GAtChat *mbm_create_data_chat(struct ofono_location_reporting *lr)
> {
> struct gps_data *gd = ofono_location_reporting_get_data(lr);
> struct ofono_modem *modem;
> const char *gps_dev;
> GAtSyntax *syntax;
> - GIOChannel *channel;
> - int fd;
> + GAtChat *chat;
>
> modem = ofono_location_reporting_get_modem(lr);
> gps_dev = ofono_modem_get_string(modem, "GPSDevice");
>
> - channel = g_at_tty_open(gps_dev, NULL);
> - if (channel == NULL)
> - return -1;
> + gd->data_channel = g_at_tty_open(gps_dev, NULL);
> + if (gd->data_channel == NULL)
> + return NULL;
>
> syntax = g_at_syntax_new_gsm_permissive();
> - gd->data_chat = g_at_chat_new(channel, syntax);
> - fd = g_io_channel_unix_get_fd(channel);
> + chat = g_at_chat_new(gd->data_channel, syntax);
>
> g_at_syntax_unref(syntax);
> - g_io_channel_unref(channel);
>
> - if (gd->data_chat == NULL)
> - return -1;
> + if (chat == NULL) {
> + g_io_channel_unref(gd->data_channel);
> +
> + return NULL;
> + }
>
> - return fd;
> + return chat;
> }
>
> static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
> @@ -131,7 +131,7 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
> struct ofono_location_reporting *lr = cbd->user;
> struct gps_data *gd = ofono_location_reporting_get_data(lr);
> struct ofono_error error;
> - int fd;
> + GAtChat *chat;
>
> DBG("lr=%p ok=%d", lr, ok);
>
> @@ -143,18 +143,22 @@ static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
> return;
> }
>
> - fd = mbm_create_data_chat(lr);
> -
> - if (fd < 0)
> + chat = mbm_create_data_chat(lr);
> + if (chat == NULL)
> goto out;
>
> - if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
> - NULL) > 0) {
> + if (g_at_chat_send(chat, "AT*E2GPSNPD", NULL, NULL, NULL, NULL) > 0) {
Are you sure this actually works as intended? By default chat uses
non-blocking writes and waits for the write watcher to fire. So just
because we succeed here does not mean anything has been written to the
channel. It just means we queued something up and created a write watch.
> + int fd = g_io_channel_unix_get_fd(gd->data_channel);
> +
> + g_at_chat_unref(chat);
And here you destroy the write queue and the write watch.
> cb(&error, fd, cbd->data);
>
> return;
> }
>
> + g_at_chat_unref(chat);
> + g_io_channel_unref(gd->data_channel);
> +
> out:
> CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
> }
Regards,
-Denis
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] mbmmodem: don't let chat open after fd is sent
2011-03-01 21:46 [PATCH 1/3] mbmmodem: close chat before sending fd Denis Kenzior
@ 2011-03-03 17:14 ` Lucas De Marchi
2011-03-03 17:21 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Lucas De Marchi @ 2011-03-03 17:14 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 6227 bytes --]
Instead of using a GAtChat, just use a GIOChannel and close it as soon
as its fd is sent to core.
Moreover, when oFono exits it's just a matter of checking if there's an
idle callback scheduled to control if we need to close the GIOChannel.
---
Hi Denis,
Following patch implements what the previous one were trying to accomplish, but
without the problem of closing the chat before the command was sent to tty. As
we talked on IRC, now I'm using the GIOChannel directly, without bothering with
creating the chat. Even if the command to send to this channel is very short, I
preferred to use async calls. Hence the use of an idle callback.
regards,
Lucas De Marchi
drivers/mbmmodem/location-reporting.c | 105 +++++++++++++++++++--------------
1 files changed, 60 insertions(+), 45 deletions(-)
diff --git a/drivers/mbmmodem/location-reporting.c b/drivers/mbmmodem/location-reporting.c
index 941fac4..30571fe 100644
--- a/drivers/mbmmodem/location-reporting.c
+++ b/drivers/mbmmodem/location-reporting.c
@@ -48,7 +48,14 @@ static const char *e2gpsctl_prefix[] = { "*E2GPSCTL:", NULL };
struct gps_data {
GAtChat *chat;
- GAtChat *data_chat;
+
+ struct {
+ GIOChannel *channel;
+ guint idle_source;
+ gsize written;
+ ofono_location_reporting_enable_cb_t cb;
+ void *cb_data;
+ } stream;
};
static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
@@ -57,7 +64,6 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
struct cb_data *cbd = user_data;
struct ofono_location_reporting *lr = cbd->user;
ofono_location_reporting_disable_cb_t cb = cbd->cb;
- struct gps_data *gd = ofono_location_reporting_get_data(lr);
DBG("lr=%p, ok=%d", lr, ok);
@@ -70,8 +76,6 @@ static void mbm_e2gpsctl_disable_cb(gboolean ok, GAtResult *result,
return;
}
- g_at_chat_unref(gd->data_chat);
-
CALLBACK_WITH_SUCCESS(cb, cbd->data);
}
@@ -94,69 +98,81 @@ static void mbm_location_reporting_disable(struct ofono_location_reporting *lr,
g_free(cbd);
}
-static int mbm_create_data_chat(struct ofono_location_reporting *lr)
+static gboolean enable_data_stream(gpointer data)
{
- struct gps_data *gd = ofono_location_reporting_get_data(lr);
- struct ofono_modem *modem;
- const char *gps_dev;
- GAtSyntax *syntax;
- GIOChannel *channel;
+ static const char *buf = "AT*E2GPSNPD\r\n";
+ gsize len = strlen(buf);
+ struct gps_data *gd = data;
+ GIOStatus status;
+ gsize written;
int fd;
- modem = ofono_location_reporting_get_modem(lr);
- gps_dev = ofono_modem_get_string(modem, "GPSDevice");
+ status = g_io_channel_write_chars(gd->stream.channel,
+ buf + gd->stream.written,
+ len - gd->stream.written,
+ &written, NULL);
+ if (status == G_IO_STATUS_AGAIN)
+ return TRUE;
- channel = g_at_tty_open(gps_dev, NULL);
- if (channel == NULL)
- return -1;
+ if (status != G_IO_STATUS_NORMAL) {
+ CALLBACK_WITH_FAILURE(gd->stream.cb, -1, gd->stream.cb_data);
- syntax = g_at_syntax_new_gsm_permissive();
- gd->data_chat = g_at_chat_new(channel, syntax);
- fd = g_io_channel_unix_get_fd(channel);
+ return FALSE;
+ }
+
+ gd->stream.written += written;
- g_at_syntax_unref(syntax);
- g_io_channel_unref(channel);
+ if (gd->stream.written != len)
+ return TRUE;
- if (gd->data_chat == NULL)
- return -1;
+ fd = g_io_channel_unix_get_fd(gd->stream.channel);
+ CALLBACK_WITH_SUCCESS(gd->stream.cb, fd, gd->stream.cb_data);
- return fd;
+ return FALSE;
+}
+
+static void destroy_data_stream(gpointer data)
+{
+ struct gps_data *gd = data;
+
+ g_io_channel_unref(gd->stream.channel);
+ gd->stream.idle_source = 0;
+ gd->stream.written = 0;
}
static void mbm_e2gpsctl_enable_cb(gboolean ok, GAtResult *result,
gpointer user_data)
{
- struct cb_data *cbd = user_data;
- ofono_location_reporting_enable_cb_t cb = cbd->cb;
- struct ofono_location_reporting *lr = cbd->user;
+ struct ofono_location_reporting *lr = user_data;
struct gps_data *gd = ofono_location_reporting_get_data(lr);
+ struct ofono_modem *modem;
+ const char *gps_dev;
struct ofono_error error;
- int fd;
DBG("lr=%p ok=%d", lr, ok);
decode_at_error(&error, g_at_result_final_response(result));
if (!ok) {
- cb(&error, -1, cbd->data);
+ gd->stream.cb(&error, -1, gd->stream.cb_data);
return;
}
- fd = mbm_create_data_chat(lr);
-
- if (fd < 0)
- goto out;
+ modem = ofono_location_reporting_get_modem(lr);
+ gps_dev = ofono_modem_get_string(modem, "GPSDevice");
+ gd->stream.channel = g_at_tty_open(gps_dev, NULL);
- if (g_at_chat_send(gd->data_chat, "AT*E2GPSNPD", NULL, NULL, NULL,
- NULL) > 0) {
- cb(&error, fd, cbd->data);
+ if (gd->stream.channel == NULL) {
+ ofono_error("Could select data GPS channel");
+ CALLBACK_WITH_FAILURE(gd->stream.cb, -1, gd->stream.cb_data);
return;
}
-out:
- CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+ gd->stream.idle_source = g_idle_add_full(G_PRIORITY_DEFAULT_IDLE,
+ enable_data_stream, gd,
+ destroy_data_stream);
}
static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
@@ -164,22 +180,17 @@ static void mbm_location_reporting_enable(struct ofono_location_reporting *lr,
void *data)
{
struct gps_data *gd = ofono_location_reporting_get_data(lr);
- struct cb_data *cbd = cb_data_new(cb, data);
DBG("lr=%p", lr);
- if (cbd == NULL)
- goto out;
-
- cbd->user = lr;
+ gd->stream.cb_data = data;
+ gd->stream.cb = cb;
if (g_at_chat_send(gd->chat, "AT*E2GPSCTL=1,5,1", none_prefix,
- mbm_e2gpsctl_enable_cb, cbd, g_free) > 0)
+ mbm_e2gpsctl_enable_cb, lr, NULL) > 0)
return;
-out:
CALLBACK_WITH_FAILURE(cb, -1, data);
- g_free(cbd);
}
static void mbm_location_reporting_support_cb(gboolean ok, GAtResult *result,
@@ -224,6 +235,10 @@ static void mbm_location_reporting_remove(struct ofono_location_reporting *lr)
ofono_location_reporting_set_data(lr, NULL);
g_at_chat_unref(gd->chat);
+
+ if (gd->stream.idle_source)
+ g_source_remove(gd->stream.idle_source);
+
g_free(gd);
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-04 3:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-03 22:23 [PATCH] mbmmodem: don't let chat open after fd is sent Lucas De Marchi
2011-03-04 3:57 ` Denis Kenzior
-- strict thread matches above, loose matches on Subject: below --
2011-03-01 21:46 [PATCH 1/3] mbmmodem: close chat before sending fd Denis Kenzior
2011-03-03 17:14 ` [PATCH] mbmmodem: don't let chat open after fd is sent Lucas De Marchi
2011-03-03 17:21 ` Marcel Holtmann
2011-03-03 17:49 ` Lucas De Marchi
2011-03-03 17:59 ` Marcel Holtmann
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.