Cinterion plugin patch

[Dan Williams]

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