New 'u-blox' plugin (review)

[Aleksander Morgado]

Hey!

>
> I'll put in another recommendation for g_clear_object() :)
>

Yeah... I'll try to do that from now on :)

>> ublox: new +UIPADDR=N response parser
>> ublox: new +CGCONTRDP=N response parser
>> ublox: implement connection using the 2G/3G logic in router or bridge
>
> It seems like the +CGCONTRDP parsing code could be generic and not
> limited to ublox.  However, it looks like the ublox you have (and thus
> the code) uses a newer ETSI 27.007 verison of +CGCONTRDP responses.
>  Somewhere between v9 and v11 they changed the format (surprise!) and
>
> Could you add an expected failure testcase for the old format that
> ensures we don't succeed in parsing the old format until we explicitly
> add support for it?
>

Ok, will try to do that.

> I got really confused about UIPADDR vs. CGCONTRDP until I read some
> +UIPADDR documentation (AT-CommandsExamples_AppNote_(UBX-
> 13001820).pdf).  That's probably on my list of dumbest things I've
> seen.  The CGCONTRDP is supposed to supply the gateway address, but
> from the examples it looks like that's always the same as the reported
> IP address, while the completely different address from UIPADDR is the
> acutal gateway address?  Wow.  This at least deserves some
> documentation in the mm-broadband-bearer-ublox.c.
>

Yeah, the logic required is dumb, and the fact that the documentation
uses the same IP address in both commands while it shouldn't is also
totally misleading. I'll try to add some comments explaining the whole
thing.

>
>> modem-helpers: new COPS? response parser
>
> Would be nice to have a testcase for the Option hex-encoded COPS case,
> here's some sample output for you:
>
> AT+CSCS="IRA"
> OK
> AT+COPS=3,0;+COPS?
> +COPS: 0,0,"T-Mobile",2
>
> OK
> AT+CSCS="UCS2"
> OK
> AT+COPS=3,0;+COPS?
> +COPS: 0,0,"0054002d004d006f00620069006c0065",2
>
> OK
>

Ah, good one, thanks, will add it.

>
>> ublox: if on LTE and empty APN given, use the default LTE bearer
>
> The disconnect code reads oddly here; LTE never gets an explicit
> disconnect?  That doesn't match the commit message.  My read is that if
> an APN was given and the access tech is LTE that APN won't get
> disconnected, which doesn't seem correct.
>

I believe that was fixed? Maybe in a follow-up commit; not sure, will
need to check. But yes, I know what you mean.

> Also, we need better handling of the default bearer in general.  Any
> idea how other devices handle this?  My Huawei E3276 doesn't auto-
> activate PDP context 0 (shown via +CGACT) when it's registered to LTE.
>  Is the ublox special in this regard?
>
> Also, I'm not really sure about this in general.  Usually a blank APN
> means "subscription value is requested", eg let the network figure it
> out.  That isn't necessarily the Default EPS Bearer AFAICT.
>
> Maybe for LTE if we can detect there is a Default EPS Bearer active we
> just create an MMBearer for it by default?  27.007 is a bit confusing
> on the relationship between the "initial PDP context" and the "Default
> EPS Bearer"; I don't think the two are the same thing, but they both
> get activated automatically on network attach.
>

I thought about that, yes, but looked like a big change in the whole
logic cycle of MM, as now the modem would directly transition to
"connected" state without any user interaction. There is also the
possibility of requesting an update of the APN used in the default EPS
bearer, and that can only be done with the modem in low-power mode in
this case (kind of like the allowed modes and bands update), and
therefore we cannot easily insert that logic within e.g. the
Simple.Connect() logic (as the modem would get unregistered from the
network during the connection attempt).

Anyway, maybe it's time we support "auto-connected" modems, not just
this u-blox device with the default EPS bearer connection, but other
modems as well, like QMI modems (e.g. --wds-get-autoconnect-settings /
--wds-set-autoconnect-settings). And for that we would need to have
the notion of a "default bearer" with its own settings. What do you
think?

>> ublox: subclass the connection monitoring logic
>
> How about we do this monitoring for all modems?  I was playing around
> the other day with a Sierra MC8781 and the only thing that reported
> disconnect when I removed antennas was AT+CGACT polling.  It seems
> useful for all modems really.
>

Yes, could move this logic to mm-broadband-modem.c and see what
happens, but I'd like to do that after having worked on all the other
u-blox related things, because this change alone will require
retesting with all the pile of modems we have around.

>
>> ublox: new +CFUN? response parser
>
> Seems hard to believe we don't already have a CFUN parser somewhere?
>  But we don't... I'd put this into modem helpers and then use it in
> linktop, mbm, ublox, and core.
>

Ok, yep.

>
>> ublox: custom parsing for CREG/CGREG/CEREG registration state values
>
> These CEREG values are standard ETSI 27.007 CEREG <stat> values, so
> can't this just be done in the common parser?
>

Oh, I totally assumed these were u-blox specific things, will need to
check ETSI 27.007 more carefully.

>
>> ublox: new +CESQ response parser
>> ublox: new helper to parse +CESQ response into MMSignal objects
>
> Also standard 27.007, can it be generic for all modems?
>

Yes, we could do that, will try to include it in mm-broadband-modem.c.

>
>> ublox: do not try to authenticate or activate the default LTE bearer
> context
>
> There's gotta be a way to figure out which PDP context the default
> bearer is using instead of assuming this...
>

According to the documentation this will be PDP context #4. But not
sure up to which point we can assume that with all u-blox modem
types...

>> ublox: implement supported bands loading
>
> I guess +UBANDSEL=? doesn't report anything useful?
>

It reports the allowed bands in the current access tech. If we update
access tech, the list in +UBANDSEL=? would change; and we don't expect
that.

>
>> ublox: new +UBANDSEL? response parser
>
> Maybe a use-case for the new mm_parse_uint_list()?
>

Ah, will look that, thanks.


-- 
Aleksander
https://aleksander.es