Re: [PATCH 6/6] Add PPP option to gsmdial
Hi Gustavo,
+static void connect_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ GIOChannel *channel;
+
+ if (!ok) {
+ g_print(Unable to define context\n);
+ exit(1);
I guess we should not call exit here. Just return.
it is a test program. So it doesn't really matter. We will never install
this program. It is just for the developers ;)
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 08/10] Fix do not emit error if extended error has emitted
Hi Denis, On 03/23/2010 02:27 AM, Denis Kenzior wrote: Hi Zhenhua, --- gatchat/gatserver.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c index 1ffc2c3..9683fa3 100644 --- a/gatchat/gatserver.c +++ b/gatchat/gatserver.c @@ -165,6 +165,10 @@ static void g_at_server_send_final(GAtServer *server, GAtServerResult result) char r = v250.s4; unsigned int len; + /* Do not emit error if extended error has already been emitted */ + if (result == G_AT_SERVER_RESULT_EXT_ERROR) + return; + I really don't get it, why would anyone call g_at_server_send_final with EXT_ERROR? Don't we have g_at_server_send_ext_final or something for that? I understand your point. My thinking is that callback should return GAtServrResult so that the main logic know whether we should parse the next command or abort parsing. But if the function call is asynchronized, I don't know, do you want send_final able to abort the parsing iteration? if (v250.quiet) return; Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] Update patch for parsing basic command
Rebase with latest upstream and resent it. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] Add basic command parsing
---
gatchat/gatserver.c | 110 ++-
1 files changed, 109 insertions(+), 1 deletions(-)
diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index 6579b38..c207bd8 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -308,9 +308,117 @@ next:
return i + 1;
}
+static gboolean get_basic_prefix(const char *buf, char *prefix)
+{
+ char c = *buf;
+
+ if (!g_ascii_isalpha(c) c != '')
+ return FALSE;
+
+ if (g_ascii_isalpha(c)) {
+ c = g_ascii_toupper(c);
+ if (c == 'S') {
+ int i = 0;
+
+ prefix[0] = 'S';
+
+ /* V.250 5.3.2 'S' command follows with
+* a parameter number.
+*/
+ while (g_ascii_isdigit(buf[++i]))
+ prefix[i] = buf[i];
+
+ prefix[i] = '\0';
+ } else {
+ prefix[0] = c;
+ prefix[1] = '\0';
+ }
+ }
+
+ if (c == '') {
+ prefix[0] = '';
+ prefix[1] = g_ascii_toupper(buf[1]);
+ prefix[2] = '\0';
+ }
+
+ return TRUE;
+}
+
static unsigned int parse_basic_command(GAtServer *server, char *buf)
{
- return 0;
+ char *command = NULL;
+ char prefix[3];
+ unsigned int i;
+ GAtServerRequestType type;
+ gboolean seen_equals = FALSE;
+
+ if (!get_basic_prefix(buf, prefix))
+ return 0;
+
+ i = strlen(prefix);
+
+ if (*prefix == 'D') {
+ type = G_AT_SERVER_REQUEST_TYPE_SET;
+
+ /* All following characters are the part of the call */
+ while (buf[i] != '\0')
+ i += 1;
+
+ goto done;
+ }
+
+ if (buf[i] == '\0' || buf[i] == ';') {
+ type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
+ goto done;
+ }
+
+ /* Additional commands may follow a command without any character
+* required for separation.
+*/
+ if (is_basic_command_prefix(buf[i])) {
+ type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
+ goto done;
+ }
+
+ /* Match '?', '=', '=?' and '=xxx' */
+ if (buf[i] == '=') {
+ seen_equals = TRUE;
+ i += 1;
+ }
+
+ if (buf[i] == '?') {
+ i += 1;
+
+ if (seen_equals)
+ type = G_AT_SERVER_REQUEST_TYPE_SUPPORT;
+ else
+ type = G_AT_SERVER_REQUEST_TYPE_QUERY;
+ } else {
+ /* V.250 5.3.1 The subparameter (if any) are all digits */
+ while (g_ascii_isdigit(buf[i]))
+ i++;
+
+ type = G_AT_SERVER_REQUEST_TYPE_SET;
+ }
+
+done:
+ command = g_strndup(buf, i);
+
+ at_command_notify(server, command, prefix, type);
+
+ g_free(command);
+
+ /* Commands like ATA, ATD, ATZ cause the remainder line
+* to be ignored.
+*/
+ if (*prefix == 'A' || *prefix == 'D' || *prefix == 'Z')
+ return 0;
+
+ /* Consumed the seperator ';' */
+ if (buf[i] == ';')
+ i += 1;
+
+ return i;
}
static void server_parse_line(GAtServer *server, char *line)
--
1.6.6.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH v2] Add parser for file list objects
Hi Yang, --- src/stkutil.c | 88 + src/stkutil.h | 11 +++ 2 files changed, 99 insertions(+), 0 deletions(-) Your patch was fine as is and I applied it, however I still managed to nitpick some style issues which I fixed up afterwards :) Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] Fix: simplify isimodem call-barring driver.
From: Pekka Pessi pekka.pe...@nokia.com
Add support for lock codes AG and AC (MMI codes 333 and 353. respectively).
---
drivers/isimodem/call-barring.c | 153 ---
drivers/isimodem/ss.h | 28
2 files changed, 63 insertions(+), 118 deletions(-)
diff --git a/drivers/isimodem/call-barring.c b/drivers/isimodem/call-barring.c
index 6487ae8..b0246fa 100644
--- a/drivers/isimodem/call-barring.c
+++ b/drivers/isimodem/call-barring.c
@@ -48,6 +48,28 @@ struct barr_data {
GIsiClient *client;
};
+static int lock_code_to_mmi(char const *lock)
+{
+ if (strcmp(lock, AO) == 0)
+ return SS_GSM_BARR_ALL_OUT;
+ else if (strcmp(lock, OI) == 0)
+ return SS_GSM_BARR_OUT_INTER;
+ else if (strcmp(lock, OX) == 0)
+ return SS_GSM_BARR_OUT_INTER_EXC_HOME;
+ else if (strcmp(lock, AI) == 0)
+ return SS_GSM_BARR_ALL_IN;
+ else if (strcmp(lock, IR) == 0)
+ return SS_GSM_BARR_ALL_IN_ROAM;
+ else if (strcmp(lock, AB) == 0)
+ return SS_GSM_ALL_BARRINGS;
+ else if (strcmp(lock, AG) == 0)
+ return SS_GSM_OUTGOING_BARR_SERV;
+ else if (strcmp(lock, AC) == 0)
+ return SS_GSM_INCOMING_BARR_SERV;
+ else
+ return 0;
+}
+
static bool set_resp_cb(GIsiClient *client, const void *restrict data,
size_t len, uint16_t object, void *opaque)
{
@@ -84,19 +106,19 @@ static void isi_set(struct ofono_call_barring *barr, const
char *lock,
{
struct barr_data *bd = ofono_call_barring_get_data(barr);
struct isi_cb_data *cbd = isi_cb_data_new(barr, cb, data);
- int ss_code;
- char *ucs2 = NULL;
+ int ss_code = lock_code_to_mmi(lock);
unsigned char msg[] = {
SS_SERVICE_REQ,
enable ? SS_ACTIVATION : SS_DEACTIVATION,
SS_ALL_TELE_AND_BEARER,
- 0, 0, /* Supplementary services code
*/
- SS_SEND_ADDITIONAL_INFO,
+ ss_code 8, ss_code 0xFF, /* Supplementary services code
*/
+ SS_SEND_ADDITIONAL_INFO,/* ? */
1, /* Subblock count */
SS_GSM_PASSWORD,
28, /* Subblock length */
- 0, 0, 0, 0, 0, 0, 0, 0, /* Password */
+ 0, passwd[0], 0, passwd[1], /* Password */
+ 0, passwd[2], 0, passwd[3],
0, 0, 0, 0, 0, 0, 0, 0, /* Filler */
0, 0, 0, 0, 0, 0, 0, 0, /* Filler */
0, 0/* Filler */
@@ -105,43 +127,10 @@ static void isi_set(struct ofono_call_barring *barr,
const char *lock,
DBG(lock code %s enable %d class %d password %s\n,
lock, enable, cls, passwd);
- if (!cbd || !passwd || strlen(passwd) 4 || cls != 7)
- goto error;
-
- if (strcmp(lock, AO) == 0)
- ss_code = SS_GSM_BARR_ALL_OUT;
- else if (strcmp(lock, OI) == 0)
- ss_code = SS_GSM_BARR_OUT_INTER;
- else if (strcmp(lock, OX) == 0)
- ss_code = SS_GSM_BARR_OUT_INTER_EXC_HOME;
- else if (strcmp(lock, AI) == 0)
- ss_code = SS_GSM_BARR_ALL_IN;
- else if (strcmp(lock, IR) == 0)
- ss_code = SS_GSM_BARR_ALL_IN_ROAM;
- else if (strcmp(lock, AB) == 0)
- ss_code = SS_GSM_ALL_BARRINGS;
- else if (strcmp(lock, AG) == 0)
- ss_code = SS_GSM_BARR_ALL_OUT;
- else if (strcmp(lock, AC) == 0)
- ss_code = SS_GSM_BARR_ALL_IN;
- else
- goto error;
-
- msg[3] = ss_code 8;
- msg[4] = ss_code 0xFF;
-
- ucs2 = g_convert(passwd, 4, UCS-2BE, UTF-8//TRANSLIT,
- NULL, NULL, NULL);
- if (ucs2 == NULL)
- goto error;
-
- memcpy((char *)msg + 9, ucs2, 8);
- g_free(ucs2);
-
- if (g_isi_request_make(bd-client, msg, sizeof(msg), SS_TIMEOUT,
- set_resp_cb, cbd))
+ if (cbd g_isi_request_make(bd-client, msg, sizeof(msg), SS_TIMEOUT,
+ set_resp_cb, cbd))
return;
-error:
+
CALLBACK_WITH_FAILURE(cb, data);
g_free(cbd);
}
@@ -267,43 +256,23 @@ static void isi_query(struct ofono_call_barring *barr,
const char *lock, int cls
{
struct barr_data *bd = ofono_call_barring_get_data(barr);
struct isi_cb_data *cbd = isi_cb_data_new(barr, cb, data);
- int ss_code;
+ int ss_code = lock_code_to_mmi(lock);
unsigned char msg[] = {
SS_SERVICE_REQ,
SS_INTERROGATION,
SS_ALL_TELE_AND_BEARER,
- 0, 0, /* Supplementary services code
*/
-
Re: [PATCH 0/6] PPP Patches v3
Hi Kristen, And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type. Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned. I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 0/6] PPP Patches v3
On Tue, 23 Mar 2010 10:56:00 -0700 Marcel Holtmann mar...@holtmann.org wrote: Hi Kristen, And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type. Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned. I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions. Regards Marcel so is_proto_handler was casting a 16 bit value to an 8 bit value? Or are you just complaining about having g_list_find_custom in the routines. It would really be easier on me to understand what you are trying to tell me if you you posted your patches to the list next time. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 0/6] PPP Patches v3
Hi Kristen, On Tue, 23 Mar 2010 10:56:00 -0700 Marcel Holtmann mar...@holtmann.org wrote: Hi Kristen, And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type. Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned. I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions. Regards Marcel so is_proto_handler was casting a 16 bit value to an 8 bit value? Or are you just complaining about having g_list_find_custom in the routines. It would really be easier on me to understand what you are trying to tell me if you you posted your patches to the list next time. If you have questions or concerns then the best place to discuss this is on IRC. I don't expect maintainers to post re-factoring patches on the mailing list. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 0/6] PPP Patches v3
Hi Kristen, And there was another problem with one variable being guint16, but the is_option casts it back to guint8. We can't really have that. Once you start casting on that scale the compiler will not warn you about type mismatches or if the value of argument is too big for its type. Can you please let me know which git commit this fix you made was? I want to review it because all of the option types should only be a byte anyway, so I am trying to figure out if there was a mistake somewhere where we were using is_option to examine a 16 bit value. I searched through the git log and can't figure out where this change was. I can see where you changed the option type we are comparing to a regular guint to avoid compiler problems, but not the other issue you mentioned. I had a second look at these. It is the is_proto_handler actually and not the is_option. However that thing applies here. A helper function to find that handle would be better then manually coding g_list_find_custom in the functions. so is_proto_handler was casting a 16 bit value to an 8 bit value? Or are you just complaining about having g_list_find_custom in the routines. It would really be easier on me to understand what you are trying to tell me if you you posted your patches to the list next time. I expect you to ensure that your patches compile on 32-bit systems. And also that they work on 32-bit and 64-bit systems. In addition I expect that little endian and big endian works smoothly. In general I don't wanna have casting in the source since it is a potential trap for errors. Without casting the compiler will warn us if we make mistakes, without we run into troubles. Especially with the different pointer sizes it is important to not cast. So in this specific cases, I think the usage of g_list_find_custom is not a good idea. Manually walking this in a helper function and ensuring that the variable and parameters types are enforced. In short summary: 1) No casting if not really really needed. Think twice before adding a casting to the source code. 2) Think about your optimization and understand what you are trying to optimize. Simple code flow is preferred. Only in hot path we should attempt to optimize manually and then it needs to be documented in the source code itself. Otherwise lets GCC do it for us. Walking the GLib list manually is sometimes a lot simpler to read than trying to use foreach type functions. 3) No variable initialization on declaration. There are a few exceptions when it makes sense. However the general rule is not to do it. We want the compiler to warn us about variables that we might use uninitialized. In most cases this indicates a mistake in the code flow and that the code is too complex. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] Add basic command parsing
Hi Zhenhua,
---
gatchat/gatserver.c | 110
++- 1 files changed,
109
insertions(+), 1 deletions(-)
diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
index 6579b38..c207bd8 100644
--- a/gatchat/gatserver.c
+++ b/gatchat/gatserver.c
@@ -308,9 +308,117 @@ next:
return i + 1;
}
+static gboolean get_basic_prefix(const char *buf, char *prefix)
+{
+ char c = *buf;
+
+ if (!g_ascii_isalpha(c) c != '')
+ return FALSE;
+
+ if (g_ascii_isalpha(c)) {
+ c = g_ascii_toupper(c);
+ if (c == 'S') {
+ int i = 0;
+
+ prefix[0] = 'S';
+
+ /* V.250 5.3.2 'S' command follows with
+ * a parameter number.
+ */
+ while (g_ascii_isdigit(buf[++i]))
+ prefix[i] = buf[i];
Is there some limit on the number? Can we limit it to max int or something?
+
+ prefix[i] = '\0';
+ } else {
+ prefix[0] = c;
+ prefix[1] = '\0';
+ }
Use a return here or an else if clause, no sense checking the if twice.
+ }
+
+ if (c == '') {
+ prefix[0] = '';
+ prefix[1] = g_ascii_toupper(buf[1]);
+ prefix[2] = '\0';
+ }
+
+ return TRUE;
+}
+
static unsigned int parse_basic_command(GAtServer *server, char *buf)
{
- return 0;
+ char *command = NULL;
Why do you initialize this? Doesn't seem to be a point.
+ char prefix[3];
+ unsigned int i;
+ GAtServerRequestType type;
+ gboolean seen_equals = FALSE;
+
+ if (!get_basic_prefix(buf, prefix))
+ return 0;
+
+ i = strlen(prefix);
+
+ if (*prefix == 'D') {
+ type = G_AT_SERVER_REQUEST_TYPE_SET;
+
+ /* All following characters are the part of the call */
+ while (buf[i] != '\0')
+ i += 1;
Quoting V.250:
All characters appearing on the same command line after the D are considered
part of the call addressing information to be signalled to the network, or
modifiers used to control the signalling process (collectively known as a dial
string), up to a semicolon character (IA5 3/11) or the end of the command
line.
So this logic seems incorrect.
+
+ goto done;
+ }
+
+ if (buf[i] == '\0' || buf[i] == ';') {
+ type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
+ goto done;
+ }
+
+ /* Additional commands may follow a command without any character
+ * required for separation.
+ */
+ if (is_basic_command_prefix(buf[i])) {
+ type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
+ goto done;
+ }
+
+ /* Match '?', '=', '=?' and '=xxx' */
+ if (buf[i] == '=') {
+ seen_equals = TRUE;
+ i += 1;
+ }
+
+ if (buf[i] == '?') {
+ i += 1;
+
+ if (seen_equals)
+ type = G_AT_SERVER_REQUEST_TYPE_SUPPORT;
+ else
+ type = G_AT_SERVER_REQUEST_TYPE_QUERY;
+ } else {
+ /* V.250 5.3.1 The subparameter (if any) are all digits */
+ while (g_ascii_isdigit(buf[i]))
+ i++;
+
+ type = G_AT_SERVER_REQUEST_TYPE_SET;
+ }
+
+done:
+ command = g_strndup(buf, i);
+
+ at_command_notify(server, command, prefix, type);
+
+ g_free(command);
+
+ /* Commands like ATA, ATD, ATZ cause the remainder line
+ * to be ignored.
+ */
+ if (*prefix == 'A' || *prefix == 'D' || *prefix == 'Z')
+ return 0;
Returning 0 means this command was not properly formatted. Not something you
want. Also, as we saw before this is true of ATA and possibly ATZ, but not
ATD.
+
+ /* Consumed the seperator ';' */
+ if (buf[i] == ';')
+ i += 1;
+
+ return i;
}
static void server_parse_line(GAtServer *server, char *line)
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH] Fix: simplify isimodem call-barring driver.
Hi Pekka,
From: Pekka Pessi pekka.pe...@nokia.com
Add support for lock codes AG and AC (MMI codes 333 and 353. respectively).
First of all, please keep your commit messages less than 72 characters long
and headings less than 50 characters long.
Secondly, please keep your patches whitespace clean, there are several cases
of spaces being used instead of tabs for indentation.
---
drivers/isimodem/call-barring.c | 153
--- drivers/isimodem/ss.h |
28
2 files changed, 63 insertions(+), 118 deletions(-)
diff --git a/drivers/isimodem/call-barring.c
b/drivers/isimodem/call-barring.c index 6487ae8..b0246fa 100644
--- a/drivers/isimodem/call-barring.c
+++ b/drivers/isimodem/call-barring.c
@@ -48,6 +48,28 @@ struct barr_data {
GIsiClient *client;
};
+ DBG(lock code %s (%u) old password %s new password %s\n,
+ lock, ss_code, old_passwd, new_passwd);
Here is an instance of whitespace error.
- if (g_isi_request_make(bd-client, msg, sizeof(msg), SS_TIMEOUT,
+ if (cbd
+ g_isi_request_make(bd-client, msg, sizeof(msg), SS_TIMEOUT,
set_passwd_resp_cb, cbd))
return;
Here's another one..
enum ss_codes {
- SS_GSM_ALL_FORWARDINGS = 0x02,
- SS_GSM_ALL_COND_FORWARDINGS = 0x04,
- SS_GSM_FORW_UNCONDITIONAL = 0x15,
- SS_GSM_BARR_ALL_OUT = 0x21,
- SS_GSM_BARR_ALL_IN = 0x23,
- SS_GSM_CALL_WAITING = 0x2B,
- SS_GSM_FORW_NO_REPLY = 0x3D,
- SS_GSM_FORW_NO_REACH = 0x3E,
- SS_GSM_FORW_BUSY = 0x43,
- SS_GSM_ALL_BARRINGS = 0x014A,
- SS_GSM_BARR_OUT_INTER = 0x014B,
- SS_GSM_BARR_OUT_INTER_EXC_HOME = 0x014C,
- SS_GSM_BARR_ALL_IN_ROAM = 0x015F
+ SS_GSM_ALL_FORWARDINGS = 002,
+ SS_GSM_ALL_COND_FORWARDINGS = 004,
Are you sure this isn't octal syntax? It still works, but you might simply
use 2 and 4 here.
+ SS_GSM_FORW_UNCONDITIONAL = 21,
+ SS_GSM_BARR_ALL_OUT = 33,
+ SS_GSM_OUTGOING_BARR_SERV = 333,
+ SS_GSM_INCOMING_BARR_SERV = 353,
+ SS_GSM_BARR_ALL_IN = 35,
+ SS_GSM_CALL_WAITING = 43,
+ SS_GSM_FORW_NO_REPLY = 61,
+ SS_GSM_FORW_NO_REACH = 62,
+ SS_GSM_FORW_BUSY = 67,
+ SS_GSM_ALL_BARRINGS = 330,
+ SS_GSM_BARR_OUT_INTER = 331,
+ SS_GSM_BARR_OUT_INTER_EXC_HOME = 332,
+ SS_GSM_BARR_ALL_IN_ROAM = 351,
};
From now on we'll require all enum values to be tab aligned so they're a bit
easier to read. See src/stkutil.h for reference.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
