Re: New 'u-blox' plugin (v2)

2016-10-12 Thread Aleksander Morgado 
On Tue, Oct 11, 2016 at 10:13 AM, Aleksander Morgado
 wrote:
>>> ublox: new +UBANDSEL? response parser
>>
>> +dupstr = g_strdup (mm_strip_tag (response, "+UBANDSEL:"));
>> +i = strlen (dupstr);
>> +while (i > 0 && (dupstr[i - 1] == '\r' || dupstr[i - 1] == '\n' || 
>> dupstr[i - 1] == ' ')) {
>> +dupstr[i - 1] = '\0';
>> +i--;
>> +}
>>
>> dupstr = g_strchomp (g_strdup (mm_strip_tag (response, "+UBANDSEL:")));
>>
>> Maybe?
>>
>
> But g_strchomp() doesn't remove LFs and CRs, right?

Oh, wait, it does remove them because CR and LF are considered
whitespaces :) Will change that.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: New 'u-blox' plugin (v2)

2016-10-12 Thread Aleksander Morgado 
On Tue, Oct 11, 2016 at 10:13 AM, Aleksander Morgado
 wrote:
>>> modem-helpers: new helper to parse +CESQ response into MMSignal
>> objects
>>
>> There's some small differences from the way I implemented it in the
>> Huawei signal interface, at least for get_rsrq_db() (-19 instead of
>> -20).  Not sure which one is right.
>>
>
> I'll give it a look.

Yes, it was -20 not -19; so this should be right:
*out_rsrq = -20.0 + (((gdouble) rsrq_level) * 0.5);

So that we get -19.5 when level is 1 and < -19.5 (e.g. -20) when level is 0.

Will be updated in v3.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: New 'u-blox' plugin (v2)

2016-10-11 Thread Aleksander Morgado 
Hey hey,

Thanks again for the review :)

>>
>>  * Added support for the new CSFB related registration state values.
>
> The DENIED handling looks like it changed:
>
> +/* Denied? */
> +if (ctx->cs  == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED ||
> +ctx->ps  == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED ||
> +ctx->eps == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED)
> + return MM_MODEM_3GPP_REGISTRATION_STATE_DENIED;
>
> -/* If one state is DENIED and the others are UNKNOWN, use DENIED
> */
> -if (ctx->cs == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED &&
> -ctx->ps == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN &&
> -ctx->eps == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN)
>
> This effectively reverts part of
> 9186e2aa39668c0d071f52d47d6a1aea087bc8a7 which was discussed on the
> list on 2015-02-23.  The background for that commit was that I had a
> Verizon SIM in my EM7345, the device doesn't have Verizon LTE bands
> (B13), and other visible LTE providers (AT and T-Mobile) denied LTE
> registration to the modem.  So the modem should report DENIED in that
> case.
>
> But is there any scenario where a device could be CS/PS
> registered/searching/etc and EPS be DENIED, or the other way around?
>  In that case if the modem can still be used on some domain (CS/PS/EPS)
> I would expect the modem state to not be DENIED.
>

Ah! You're totally right, the logic of DENIED ended up changing, I'll
recover the old one as you're right, we should only show DENIED if the
others aren't in any other known state.

>
>>  * I didn't do the +CFUN parsing generic, because the +CFUN state
>> numbers reported are mostly (except for the first 5) vendor-specific.
>
> Though couldn't we  move the actual CFUN parsing to the core, and leave
> the / interpretation up to the plugin itself?  Linktop,
> MBM, generic, and now Ublox all send "CFUN?" and could all have
> consolidated code.  Note that some devices may return only one number
> there, so the regex would have to consider  optional.
>

I understand what you wanted now, yes, we can definitely do that.

>> You can find the updated ublox plugin in the 'aleksander/ublox'
>> branch
>> in the upstream git repository:
>> https://cgit.freedesktop.org/ModemManager/ModemManager/log/?h=aleksan
>> der/ublox
>>
>> Reviews of the implementation are more than welcome.
>
>> ublox: implement PIN retry count loading
>> ublox: new +UPINCNT response parser
>
> The last hunk of "implement PI retry count loading" shouljd be in the
> +UPINCNT patch.
>

Ok.

>> modem-helpers: new helper to parse +CESQ response into MMSignal
> objects
>
> There's some small differences from the way I implemented it in the
> Huawei signal interface, at least for get_rsrq_db() (-19 instead of
> -20).  Not sure which one is right.
>

I'll give it a look.

>> ublox: new +UBANDSEL? response parser
>
> +dupstr = g_strdup (mm_strip_tag (response, "+UBANDSEL:"));
> +i = strlen (dupstr);
> +while (i > 0 && (dupstr[i - 1] == '\r' || dupstr[i - 1] == '\n' || 
> dupstr[i - 1] == ' ')) {
> +dupstr[i - 1] = '\0';
> +i--;
> +}
>
> dupstr = g_strchomp (g_strdup (mm_strip_tag (response, "+UBANDSEL:")));
>
> Maybe?
>

But g_strchomp() doesn't remove LFs and CRs, right?

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: New 'u-blox' plugin (v2)

2016-10-07 Thread Dan Williams 
On Wed, 2016-10-05 at 16:27 +0200, Aleksander Morgado wrote:
> Hey Dan & everyone,
> 
> After Dan's review, I've updated the branch implementing support for
> the ublox devices with the following changes:
> 
>  * +CGCONTRDP parser is generic, and now supports both formats (pre
> and post TS 27.007 v9.4.0)
> 
>  * Added some additional comments to explain how the static IP
> addressing is retrieved when in bridge mode. Note also that there is
> a
> long README explaining most of the plugin details under
> plugins/ublox.
> 
>  * Removed logic implementing the automatic connection to the default
> LTE bearer when the empty apn ("") was given.
> 
>  * Moved AT+CGACT? polling for connection status monitoring to the
> generic modem object.
> 
>  * Added support for the new CSFB related registration state values.

The DENIED handling looks like it changed:

+/* Denied? */
+if (ctx->cs  == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED ||
+ctx->ps  == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED ||
+ctx->eps == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED)
+ return MM_MODEM_3GPP_REGISTRATION_STATE_DENIED;
 
-/* If one state is DENIED and the others are UNKNOWN, use DENIED
*/
-if (ctx->cs == MM_MODEM_3GPP_REGISTRATION_STATE_DENIED &&
-ctx->ps == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN &&
-ctx->eps == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN)

This effectively reverts part of
9186e2aa39668c0d071f52d47d6a1aea087bc8a7 which was discussed on the
list on 2015-02-23.  The background for that commit was that I had a
Verizon SIM in my EM7345, the device doesn't have Verizon LTE bands
(B13), and other visible LTE providers (AT and T-Mobile) denied LTE
registration to the modem.  So the modem should report DENIED in that
case.

But is there any scenario where a device could be CS/PS
registered/searching/etc and EPS be DENIED, or the other way around?
 In that case if the modem can still be used on some domain (CS/PS/EPS)
I would expect the modem state to not be DENIED.

>  * Moved the AT+CESQ based extended signal interface support to the
> generic modem.
> 
>  * Updated the AT+UBANDSEL? parser to use mm_parse_uint_list().
> 
> What I didn't do:
> 
>  * No specific hex-encoded operator name parsing in AT+COPS. Dan,
> there is already a mm_3gpp_normalize_operator_name() call doing this
> processing after mm_3gpp_parse_cops_read_response(), don't think we
> should merge both.

Yeah, I see that now.  Sure.

>  * I didn't do the +CFUN parsing generic, because the +CFUN state
> numbers reported are mostly (except for the first 5) vendor-specific.

Though couldn't we  move the actual CFUN parsing to the core, and leave
the / interpretation up to the plugin itself?  Linktop,
MBM, generic, and now Ublox all send "CFUN?" and could all have
consolidated code.  Note that some devices may return only one number
there, so the regex would have to consider  optional.

> You can find the updated ublox plugin in the 'aleksander/ublox'
> branch
> in the upstream git repository:
> https://cgit.freedesktop.org/ModemManager/ModemManager/log/?h=aleksan
> der/ublox
> 
> Reviews of the implementation are more than welcome.

> ublox: implement PIN retry count loading
> ublox: new +UPINCNT response parser

The last hunk of "implement PI retry count loading" shouljd be in the
+UPINCNT patch.

> modem-helpers: new helper to parse +CESQ response into MMSignal
objects

There's some small differences from the way I implemented it in the
Huawei signal interface, at least for get_rsrq_db() (-19 instead of
-20).  Not sure which one is right.

> ublox: new +UBANDSEL? response parser

+dupstr = g_strdup (mm_strip_tag (response, "+UBANDSEL:"));
+i = strlen (dupstr);
+while (i > 0 && (dupstr[i - 1] == '\r' || dupstr[i - 1] == '\n' || 
dupstr[i - 1] == ' ')) {
+dupstr[i - 1] = '\0';
+i--;
+}

dupstr = g_strchomp (g_strdup (mm_strip_tag (response, "+UBANDSEL:")));

Maybe?

Rest LGTM.

Dan

___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

New 'u-blox' plugin (v2)

2016-10-05 Thread Aleksander Morgado 
Hey Dan & everyone,

After Dan's review, I've updated the branch implementing support for
the ublox devices with the following changes:

 * +CGCONTRDP parser is generic, and now supports both formats (pre
and post TS 27.007 v9.4.0)

 * Added some additional comments to explain how the static IP
addressing is retrieved when in bridge mode. Note also that there is a
long README explaining most of the plugin details under plugins/ublox.

 * Removed logic implementing the automatic connection to the default
LTE bearer when the empty apn ("") was given.

 * Moved AT+CGACT? polling for connection status monitoring to the
generic modem object.

 * Added support for the new CSFB related registration state values.

 * Moved the AT+CESQ based extended signal interface support to the
generic modem.

 * Updated the AT+UBANDSEL? parser to use mm_parse_uint_list().

What I didn't do:

 * No specific hex-encoded operator name parsing in AT+COPS. Dan,
there is already a mm_3gpp_normalize_operator_name() call doing this
processing after mm_3gpp_parse_cops_read_response(), don't think we
should merge both.

 * I didn't do the +CFUN parsing generic, because the +CFUN state
numbers reported are mostly (except for the first 5) vendor-specific.

You can find the updated ublox plugin in the 'aleksander/ublox' branch
in the upstream git repository:
https://cgit.freedesktop.org/ModemManager/ModemManager/log/?h=aleksander/ublox

Reviews of the implementation are more than welcome.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel