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&T 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 <state>/<aux> 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 <aux> 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
