Re: Patch on unsupported AT command
Hi Yang, > If some unsupported AT command is issued, different modem may have > their own response. Now at my hand is a Huawei modem (EM770W), and it returns > "COMMAND NOT SUPPORT". In my case, this modem doesn't support "AT+CGAUTO=0" > in atmodem/gprs.c. Current oFono will hang there for it's not a valid return. > We may have some quirk to handle this problem, the same way as current > code in network-registration.c with CALYPSO. But I wonder if it's better to > add the response string into "terminator table", so that we don't need this > kind of quirk here and there. I'm not sure if this is the better/best way to > handle this problem. After all, the table may become larger and larger is > more and more specific terminator like this are added. > Comments are welcome! this lovely broken Huawei modem where the firmware developers were incapable of reading the specification and just made up a new response. I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Denis, or do you want this quirked in every plugin or modem driver? Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Marcel, > I think this might need a GAtChat quirk function where we can add extra > terminator responses that will be recognized. And maybe even translated > into something meaningful. Originally I had the terminators freely definable on the GAtChat + some hardcoded ones, but abandoned that approach since it didn't seem useful. Perhaps this needs to be resurrected. > > Denis, or do you want this quirked in every plugin or modem driver? I do prefer non-standard terminators to be setup in the plugin/driver. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, > > I think this might need a GAtChat quirk function where we can add extra > > terminator responses that will be recognized. And maybe even translated > > into something meaningful. > > Originally I had the terminators freely definable on the GAtChat + some > hardcoded ones, but abandoned that approach since it didn't seem useful. > Perhaps this needs to be resurrected. > > > > > Denis, or do you want this quirked in every plugin or modem driver? > > I do prefer non-standard terminators to be setup in the plugin/driver. I meant adding something like g_at_chat_add_terminator() or so. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Marcel, > Hi Denis, > > > > I think this might need a GAtChat quirk function where we can add extra > > > terminator responses that will be recognized. And maybe even translated > > > into something meaningful. > > > > Originally I had the terminators freely definable on the GAtChat + some > > hardcoded ones, but abandoned that approach since it didn't seem useful. > > Perhaps this needs to be resurrected. > > > > > Denis, or do you want this quirked in every plugin or modem driver? > > > > I do prefer non-standard terminators to be setup in the plugin/driver. > > I meant adding something like g_at_chat_add_terminator() or so. Yes, that is what I meant as well. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
>-Original Message- >From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of >Denis Kenzior >Sent: Saturday, November 14, 2009 1:06 AM >To: ofono@ofono.org >Subject: Re: Patch on unsupported AT command > >Hi Marcel, > >> Hi Denis, >> >> > > I think this might need a GAtChat quirk function where we can add extra >> > > terminator responses that will be recognized. And maybe even translated >> > > into something meaningful. >> > >> > Originally I had the terminators freely definable on the GAtChat + some >> > hardcoded ones, but abandoned that approach since it didn't seem useful. >> > Perhaps this needs to be resurrected. >> > >> > > Denis, or do you want this quirked in every plugin or modem driver? >> > >> > I do prefer non-standard terminators to be setup in the plugin/driver. >> >> I meant adding something like g_at_chat_add_terminator() or so. > >Yes, that is what I meant as well. Attached is the patch to support plugin specific terminator, and it supports Huawei's special one "COMMAND NOT SUPPORT". >Regards, >-Denis >___ >ofono mailing list >ofono@ofono.org >http://lists.ofono.org/listinfo/ofono 0001-Support-vendor-specific-terminator.patch Description: 0001-Support-vendor-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, > >> > > I think this might need a GAtChat quirk function where we can add extra > >> > > terminator responses that will be recognized. And maybe even translated > >> > > into something meaningful. > >> > > >> > Originally I had the terminators freely definable on the GAtChat + some > >> > hardcoded ones, but abandoned that approach since it didn't seem useful. > >> > Perhaps this needs to be resurrected. > >> > > >> > > Denis, or do you want this quirked in every plugin or modem driver? > >> > > >> > I do prefer non-standard terminators to be setup in the plugin/driver. > >> > >> I meant adding something like g_at_chat_add_terminator() or so. > > > >Yes, that is what I meant as well. > > Attached is the patch to support plugin specific terminator, and it supports > Huawei's special one "COMMAND NOT SUPPORT". you need to split this into a GAtChat specific patch and oFono plugin patch. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi, >> Attached is the patch to support plugin specific terminator, and it supports >> Huawei's >special one "COMMAND NOT SUPPORT". > >you need to split this into a GAtChat specific patch and oFono plugin >patch. Thanks for the comments. I have split the patch to two separate ones. > >Regards > >Marcel > > >___ >ofono mailing list >ofono@ofono.org >http://lists.ofono.org/listinfo/ofono Regards, -Yang 0001-Framework-to-support-standard-and-non-standard-termi.patch Description: 0001-Framework-to-support-standard-and-non-standard-termi.patch 0002-Support-Huawei-specific-terminator.patch Description: 0002-Support-Huawei-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Yang,
> Thanks for the comments. I have split the patch to two separate ones.
>
Two problems:
>+void g_at_chat_add_terminator(GAtChat *chat, const char *terminator,
>+ int len, gboolean success)
>+{
>+ struct terminator_info *ti = g_new0(struct terminator_info, 1);
>+ ti->terminator = terminator;
>+ ti->len = len;
>+ ti->success = success;
>+ chat->terminator_table = g_slist_prepend(chat->terminator_table, ti);
>+}
You're not strdup'ing the terminator string here. For safety reasons I
suggest this be done.
>+ g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE);
>+ g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE);
>+ g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE);
>+ g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE);
>+ g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE);
>+ g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE);
>+ g_at_chat_add_terminator(chat, "BUSY", -1, FALSE);
>+ g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE);
>+ g_at_chat_add_terminator(chat, "ERROR", -1, FALSE);
>+ g_at_chat_add_terminator(chat, "OK", -1, TRUE);
I really don't like this. Lets keep the non-standard terminators in a
separate list. I don't want the vast majority of the drivers incurring the
cost of multiple g_new/g_frees.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, > >+g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); > >+g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); > >+g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); > >+g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); > >+g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); > >+g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); > >+g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); > >+g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); > >+g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); > >+g_at_chat_add_terminator(chat, "OK", -1, TRUE); > > I really don't like this. Lets keep the non-standard terminators in a > separate list. I don't want the vast majority of the drivers incurring the > cost of multiple g_new/g_frees. I have to agree on this. We should keep the penalty for well behaving cards as small as possible. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
>-Original Message- >From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of >Marcel Holtmann >Sent: Wednesday, November 18, 2009 4:36 AM >To: ofono@ofono.org >Subject: Re: Patch on unsupported AT command > >Hi Denis, > >> >+ g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); >> >+ g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); >> >+ g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); >> >+ g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); >> >+ g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); >> >+ g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); >> >+ g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); >> >+ g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); >> >+ g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); >> >+ g_at_chat_add_terminator(chat, "OK", -1, TRUE); >> >> I really don't like this. Lets keep the non-standard terminators in a >> separate list. I don't want the vast majority of the drivers incurring the >> cost of multiple g_new/g_frees. > >I have to agree on this. We should keep the penalty for well behaving >cards as small as possible. Thank you for the comments. Modified patches are attached! >Regards > >Marcel > > >___ >ofono mailing list >ofono@ofono.org >http://lists.ofono.org/listinfo/ofono Regards, -Yang 0001-Framework-to-support-non-standard-terminator.patch Description: 0001-Framework-to-support-non-standard-terminator.patch 0002-Support-Huawei-specific-terminator.patch Description: 0002-Support-Huawei-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, > >> >+ g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); > >> >+ g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); > >> >+ g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); > >> >+ g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); > >> >+ g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); > >> >+ g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); > >> >+ g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); > >> >+ g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); > >> >+ g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); > >> >+ g_at_chat_add_terminator(chat, "OK", -1, TRUE); > >> > >> I really don't like this. Lets keep the non-standard terminators in a > >> separate list. I don't want the vast majority of the drivers incurring the > >> cost of multiple g_new/g_frees. > > > >I have to agree on this. We should keep the penalty for well behaving > >cards as small as possible. > > Thank you for the comments. Modified patches are attached! please do casts with a space between. Like (char *) terminator etc. Also why do you bother with making it const. Just leave that out. Since you do actually copy the string anyway. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
>-Original Message- >From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of >Marcel Holtmann >Sent: Friday, November 20, 2009 9:31 PM >To: ofono@ofono.org >Subject: RE: Patch on unsupported AT command > >Hi Yang, > >> >> >+g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); >> >> >+g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); >> >> >+g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); >> >> >+g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); >> >> >+g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); >> >> >+g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); >> >> >+g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); >> >> >+g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); >> >> >+g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); >> >> >+g_at_chat_add_terminator(chat, "OK", -1, TRUE); >> >> >> >> I really don't like this. Lets keep the non-standard terminators in a >> >> separate list. I don't want the vast majority of the drivers incurring >> >> the >> >> cost of multiple g_new/g_frees. >> > >> >I have to agree on this. We should keep the penalty for well behaving >> >cards as small as possible. >> >> Thank you for the comments. Modified patches are attached! > >please do casts with a space between. Like (char *) terminator etc. Also >why do you bother with making it const. Just leave that out. Since you >do actually copy the string anyway. Fixed! > >Regards > >Marcel > > >___ >ofono mailing list >ofono@ofono.org >http://lists.ofono.org/listinfo/ofono 0001-Framework-to-support-non-standard-terminator.patch Description: 0001-Framework-to-support-non-standard-terminator.patch 0002-Support-Huawei-specific-terminator.patch Description: 0002-Support-Huawei-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, > >> >> >+ g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); > >> >> >+ g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); > >> >> >+ g_at_chat_add_terminator(chat, "OK", -1, TRUE); > >> >> > >> >> I really don't like this. Lets keep the non-standard terminators in a > >> >> separate list. I don't want the vast majority of the drivers incurring > >> >> the > >> >> cost of multiple g_new/g_frees. > >> > > >> >I have to agree on this. We should keep the penalty for well behaving > >> >cards as small as possible. > >> > >> Thank you for the comments. Modified patches are attached! > > > >please do casts with a space between. Like (char *) terminator etc. Also > >why do you bother with making it const. Just leave that out. Since you > >do actually copy the string anyway. > > Fixed! I don't like this casting at all actually. Please just store the terminator as char and not const char. That is internal code anyway and you do allocate it. So no need to mark it as read-only. Also please use g_strdup since you are using g_free to free it. +static gboolean meet_terminator(GAtChat *chat, + struct terminator_info *info, char *line) About this function name, I don't really like it since it confuses the hell out of me what it is meant do be doing. I do get what you are trying to do here, but I would just name it check_terminator and then leave the g_at_chat_finish_command() call in the original code. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
>-Original Message- >From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of >Marcel Holtmann >Sent: Saturday, November 21, 2009 6:42 PM >To: ofono@ofono.org >Subject: RE: Patch on unsupported AT command > >Hi Yang, > >> >> >> >+ g_at_chat_add_terminator(chat, "+EXT ERROR:", 11, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "+CME ERROR:", 11, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "+CMS ERROR:", 11, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "NO ANSWER", -1, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "CONNECT", -1, TRUE); >> >> >> >+ g_at_chat_add_terminator(chat, "NO CARRIER", -1, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "BUSY", -1, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "NO DIALTONE", -1, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "ERROR", -1, FALSE); >> >> >> >+ g_at_chat_add_terminator(chat, "OK", -1, TRUE); >> >> >> >> >> >> I really don't like this. Lets keep the non-standard terminators in a >> >> >> separate list. I don't want the vast majority of the drivers >> >> >> incurring the >> >> >> cost of multiple g_new/g_frees. >> >> > >> >> >I have to agree on this. We should keep the penalty for well behaving >> >> >cards as small as possible. >> >> >> >> Thank you for the comments. Modified patches are attached! >> > >> >please do casts with a space between. Like (char *) terminator etc. Also >> >why do you bother with making it const. Just leave that out. Since you >> >do actually copy the string anyway. >> >> Fixed! > >I don't like this casting at all actually. Please just store the >terminator as char and not const char. That is internal code anyway and >you do allocate it. So no need to mark it as read-only. Also please use >g_strdup since you are using g_free to free it. > >+static gboolean meet_terminator(GAtChat *chat, >+ struct terminator_info *info, char *line) > >About this function name, I don't really like it since it confuses the >hell out of me what it is meant do be doing. I do get what you are >trying to do here, but I would just name it check_terminator and then >leave the g_at_chat_finish_command() call in the original code. Code is modified according to your comment. Please have a review! >Regards > >Marcel > > >___ >ofono mailing list >ofono@ofono.org >http://lists.ofono.org/listinfo/ofono 0001-Framework-to-support-non-standard-terminator.patch Description: 0001-Framework-to-support-non-standard-terminator.patch 0002-Support-Huawei-specific-terminator.patch Description: 0002-Support-Huawei-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang,
looks good so far, but ...
> +static gboolean check_terminator(struct terminator_info *info, char
> *line)
> +{
> + if ((info->len == -1 && !strcmp(line, info->terminator)) ||
> + (info->len > 0 && !strncmp(line, info->terminator,
> info->len)))
> + return TRUE;
> + else
> + return FALSE;
> +}
> +
This is first of all violating the coding style with the indentation on
the second line of the if, but it is also way too complicated.
if (info->len == -1 && !strcmp(line, info->terminator)
return TRUE;
if (info->len > 0 && !strncmp(line, info->terminator, ...))
return TRUE;
return FALSE;
Maybe it is too early in the morning to do code review, but this should
be doing the same, but be a lot simpler to read ;)
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Marcel, > This is first of all violating the coding style with the indentation on > the second line of the if, but it is also way too complicated. There is actually a reason for this. > > if (info->len == -1 && !strcmp(line, info->terminator) > return TRUE; This part checks for static terminators, like "OK" or "BUSY" or ERROR. We do whole string comparison here. > > if (info->len > 0 && !strncmp(line, info->terminator, ...)) > return TRUE; This part checks for variable terminators. E.g. +CMS ERROR: XXX. These are well defined by the standard so we only do a prefix comparison for these. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, > > This is first of all violating the coding style with the indentation on > > the second line of the if, but it is also way too complicated. > > There is actually a reason for this. > > > > > if (info->len == -1 && !strcmp(line, info->terminator) > > return TRUE; > > This part checks for static terminators, like "OK" or "BUSY" or ERROR. We do > whole string comparison here. > > > > > if (info->len > 0 && !strncmp(line, info->terminator, ...)) > > return TRUE; > > This part checks for variable terminators. E.g. +CMS ERROR: XXX. These are > well defined by the standard so we only do a prefix comparison for these. and your point is? I was just going by the pure algorithmic of the if statement to make it actually readable without getting a headache ;) Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
>-Original Message-
>From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of
>Marcel Holtmann
>Sent: Monday, November 23, 2009 2:21 PM
>To: ofono@ofono.org
>Subject: RE: Patch on unsupported AT command
>
>Hi Yang,
>
>looks good so far, but ...
>
>> +static gboolean check_terminator(struct terminator_info *info, char
>> *line)
>> +{
>> + if ((info->len == -1 && !strcmp(line, info->terminator)) ||
>> + (info->len > 0 && !strncmp(line, info->terminator,
>> info->len)))
>> + return TRUE;
>> + else
>> + return FALSE;
>> +}
>> +
>
>This is first of all violating the coding style with the indentation on
>the second line of the if, but it is also way too complicated.
>
> if (info->len == -1 && !strcmp(line, info->terminator)
> return TRUE;
>
> if (info->len > 0 && !strncmp(line, info->terminator, ...))
> return TRUE;
>
> return FALSE;
>
>Maybe it is too early in the morning to do code review, but this should
>be doing the same, but be a lot simpler to read ;)
You're absolutely right. In this way, the code is more readable. Please review
again.
>
>Regards
>
>Marcel
>
>
>___
>ofono mailing list
>ofono@ofono.org
>http://lists.ofono.org/listinfo/ofono
0001-Framework-to-support-non-standard-terminator.patch
Description: 0001-Framework-to-support-non-standard-terminator.patch
0002-Support-Huawei-specific-terminator.patch
Description: 0002-Support-Huawei-specific-terminator.patch
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, > >Maybe it is too early in the morning to do code review, but this should > >be doing the same, but be a lot simpler to read ;) > > You're absolutely right. In this way, the code is more readable. Please > review again. both patches have been applied. Thanks. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
