Cinterion plugin patch

[matthew stanger]

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/20160829/6eecdaed/attachment.html>