Fwd: Cinterion plugin patch

Tue Sep 6 01:47:59 UTC 2016

---------- Forwarded message ----------
From: matthew stanger <stangerm2 at gmail.com>
Date: Mon, Sep 5, 2016 at 7:16 PM
Subject: Re: Cinterion plugin patch
To: Dan Williams <dcbw at redhat.com>, Aleksander Morgado <
aleksander at aleksander.es>

Hey guys,

Here is try two. First off if you are uncomfortable with the attachment let
me know and I'll send the patch in the body. It's just 4k lines and Chrome
almost crashes when I paste it in. I won't take offence if that is needed
though. I can also squash it down to a single commit if that makes viewing
it easier.

The attached patch consist of 5 commits since I branched from '
59f57befa4be81b562f9adfbac39fbe19d88c111'. The first commit being the
original I sent and the next 4 address suggestions made by Dan. I tried to
get the GPS separated but there is some hardship in doing that with our
commit history. I was able to make all of the changes except, I don't think
I can move PDP_CID to modem initialization as it is tied to the net
interface being used by the bearer.  Leading in to answer your last
question, it's not hard coded technically but our tec rep's at Cinterion
told me to map these contexts to these interface to avoid issues, though it
does seem to work with any context to any interface. I could not
get report_connection_status() to trigger off a manual connect cancellation
either so I left that logic in. As far as using the MM core for CGDCONT and
PDP ctx I thought that would only work if I was using
the MM_BROADBAND_MODEM class instead of the MM_BROADBAND_MODEM_CINTERION
class when creating a bearer. I may be super wrong on that though. I just
say that as I wasn't able to trace how it was called inside
of find_cid_ready in mm-bb-bearer.c. I though because of that class I had
to handle our own CGD & PDP ctx but if that's not the case then please let
me know how I can do that. I made all the other changes & to be honest it
was quite a mess so thanks for the feedback.

I also see you are using an old AT command ref for this modem. Cinterions
newest is 3.017. I can't give it to you due to NDA but if you want it I'd
be happy to lobby for you to Cinterion so they will give you a new copy. I
have already been lobbying MM to them just FYI, hoping they might take some
of the programming burden but they seemed really only focused on windows
atm. I'm not sure if you guys have PLS8 modems either but if you want ones
that's another thing I'm sure I could help you get. Having you test
anything I push even for 5 mins is well worth it so if you need anything
like that please let me know.

Let me know how this looks. We have lots more to add as far as features go.
I'm working on updating URC's right now so after we get a base line you'll
be seeing some more patches from me.

On Mon, Aug 29, 2016 at 1:27 PM, matthew stanger <stangerm2 at gmail.com>
wrote:

> Thanks Dan for the very helpful feedback. I will get update the patch with
> the changes. It may take me some time to squeeze it in as our dev sprints
> allow but I'll get it back up here.
>
> On Mon, Aug 29, 2016 at 1:19 PM, Dan Williams <dcbw at redhat.com> wrote:
>
>> On Mon, 2016-08-29 at 09:44 -0600, matthew stanger wrote:
>> > Attached is the patch, gmail is the culprit.
>>
>> Thanks!
>>
>> A couple general comments.
>>
>> If it's possible, could the GPS stuff be split out into a separate
>> patch?  Makes for easier review of both that and the SWWAN stuff.
>>
>> For feature detection, maybe instead of assuming everything with a net
>> port supports SWWAN, just send ^SWWAN? and if you get an error then
>> obviously it's unsupported, if you get a "yes" response *and* we know
>> of a net port already, then assume supported.  I'd do this at modem
>> initialization time and do the usb0/usb1 :: PDP# at the same time,
>> rather than on bearer creation.
>>
>> Next, I would actually separate the connect and disconnect state
>> machine functions.  I feel like piling multiple cases into
>> get_swwan_response() and cinterion_3gpp_state_machine_logic() confuses
>> the functions.  I feel like it would be easier to follow to move the
>> actual state machine logic out of the command-parsing functions, and
>> into smaller functions that just call the command parsing functions and
>> handle their result.  I know that's probably what you're trying to do
>> in get_swwan_response() but I think it could be more generalized to
>> make the flow clearer.  This also lets you pass the specific connect or
>> disconnect context to the _ready() functions that process the
>> responses, which will make some of the "connect_pending_context" stuff
>> simpler too.  In other modems we only use those when we get unsolicited
>> responses since there's no caller function to pass a context from.
>>
>> For connect cancellation (like if the user manually disconnects while
>> you're already connecting), I believe you'll get a
>> report_connection_status() call when the connect is canceled, so you
>> can handle that instead of checking manually for
>> in connect_3gpp_context_step().  Although it does seem like the Huawei
>> plugin does the same thing your patch does, so maybe I'm off-base here.
>>
>> Instead of doing a bunch of stuff in STEP_INIT, I'd just do that in
>> connect_3gpp() when creating the Control3gppContext.  I'd probably also
>> defer creating the ipv4_config until we actually read the bearer
>> details.
>>
>> Then at the end of INIT in connect_3gpp_step() instead of
>> calling cinterion_3gpp_state_machine_logic() you can just drop down to
>> the next case statement, eg no 'break'.  But put an explicit comment
>> there to show that's intended.
>>
>> Then for VERIFY_SWWAN, I can't see where it waits with a timeout before
>> polling ^SWWAN again?  I think the Huawei bearer is the model to follow
>> here, since that sends the ^NDISDUP command that is like ^SWWAN to
>> start the initial connection, then periodically polls ^NDISSTATQRY to
>> see if the attempt succeeded.  Note that the response
>> handler connect_ndisstatqry_check_ready() parses the response with a
>> generic helper function, and if the modem still isn't connected it
>> starts a 1s timeout before trying again.  In your case, that timeout
>> would trigger another call to ^SWWAN to check the status.
>>
>> Basically, the flow should go from step-to-step and typically not back
>> up steps.  I think your model can follow this too.  Basically:
>>
>> INIT: do the ip type stuff, drop down to next step
>> AUTH: check if auth is required, if so send SGAUTH, otherwise drop down
>> CONNECT: send ^SWWAN=1,X,Y and when that's complete advance to next
>> step
>> WAIT: poll the connection status with SWWAN? and if not yet connected,
>> g_timeout_add_seconds() to poll again.  When connected, advance step.
>> IP_CONFIG: pull bearer properties and set ip4_config
>> FINISH: complete the context
>>
>> Disconnect would follow a similar process, but of course you don't need
>> AUTH or IP_CONFIG steps.
>>
>> Lastly, is the PDP CID -> usb interface mapping hardcoded in the
>> firmware?  I can't find anything about that in PLS8-E_ATC_V02.011
>> or PLS8-E_ATC_V01.000.  ie, can't we activate any arbitrary PDP context
>> for an arbitary WWAN adapter with ^SWWAN=1,X,Y where X is a PDP context
>> already determined by the MM core?  The +CGDCONT logic in the Cinterion
>> plugin is overriding that and and may conflict with MM core logic.
>>
>> Dan
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160905/5fd5a566/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mm_cinterion_patch2
Type: application/octet-stream
Size: 188506 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160905/5fd5a566/attachment-0001.obj>