<div dir="ltr"><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">matthew stanger</b> <span dir="ltr"><<a href="mailto:stangerm2@gmail.com">stangerm2@gmail.com</a>></span><br>Date: Mon, Sep 5, 2016 at 7:16 PM<br>Subject: Re: Cinterion plugin patch<br>To: Dan Williams <<a href="mailto:dcbw@redhat.com">dcbw@redhat.com</a>>, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es">aleksander@aleksander.es</a>><br><br><br><div dir="ltr">Hey guys,<div><br></div><div>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.</div><div><br></div><div>The attached patch consist of 5 commits since I branched from '<wbr>59f57befa4be81b562f9adfbac39fb<wbr>e19d88c111'. 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 <span style="font-size:12.8px">PDP_CID to </span><span style="font-size:12.8px">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_<wbr>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.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">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.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">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.</span></div><div> </div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 29, 2016 at 1:27 PM, matthew stanger <span dir="ltr"><<a href="mailto:stangerm2@gmail.com" target="_blank">stangerm2@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.</div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 29, 2016 at 1:19 PM, Dan Williams <span dir="ltr"><<a href="mailto:dcbw@redhat.com" target="_blank">dcbw@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On Mon, 2016-08-29 at 09:44 -0600, matthew stanger wrote:<br>
> Attached is the patch, gmail is the culprit.<br>
<br>
</span>Thanks!<br>
<br>
A couple general comments.<br>
<br>
If it's possible, could the GPS stuff be split out into a separate<br>
patch?  Makes for easier review of both that and the SWWAN stuff.<br>
<br>
For feature detection, maybe instead of assuming everything with a net<br>
port supports SWWAN, just send ^SWWAN? and if you get an error then<br>
obviously it's unsupported, if you get a "yes" response *and* we know<br>
of a net port already, then assume supported.  I'd do this at modem<br>
initialization time and do the usb0/usb1 :: PDP# at the same time,<br>
rather than on bearer creation.<br>
<br>
Next, I would actually separate the connect and disconnect state<br>
machine functions.  I feel like piling multiple cases into<br>
get_swwan_response() and cinterion_3gpp_state_machine_l<wbr>ogic() confuses<br>
the functions.  I feel like it would be easier to follow to move the<br>
actual state machine logic out of the command-parsing functions, and<br>
into smaller functions that just call the command parsing functions and<br>
handle their result.  I know that's probably what you're trying to do<br>
in get_swwan_response() but I think it could be more generalized to<br>
make the flow clearer.  This also lets you pass the specific connect or<br>
disconnect context to the _ready() functions that process the<br>
responses, which will make some of the "connect_pending_context" stuff<br>
simpler too.  In other modems we only use those when we get unsolicited<br>
responses since there's no caller function to pass a context from.<br>
<br>
For connect cancellation (like if the user manually disconnects while<br>
you're already connecting), I believe you'll get a<br>
report_connection_status() call when the connect is canceled, so you<br>
can handle that instead of checking manually for<br>
in connect_3gpp_context_step()<wbr>.  Although it does seem like the Huawei<br>
plugin does the same thing your patch does, so maybe I'm off-base here.<br>
<br>
Instead of doing a bunch of stuff in STEP_INIT, I'd just do that in<br>
connect_3gpp() when creating the Control3gppContext.  I'd probably also<br>
defer creating the ipv4_config until we actually read the bearer<br>
details.<br>
<br>
Then at the end of INIT in connect_3gpp_step() instead of<br>
calling cinterion_3gpp_state_m<wbr>achine_logic() you can just drop down to<br>
the next case statement, eg no 'break'.  But put an explicit comment<br>
there to show that's intended.<br>
<br>
Then for VERIFY_SWWAN, I can't see where it waits with a timeout before<br>
polling ^SWWAN again?  I think the Huawei bearer is the model to follow<br>
here, since that sends the ^NDISDUP command that is like ^SWWAN to<br>
start the initial connection, then periodically polls ^NDISSTATQRY to<br>
see if the attempt succeeded.  Note that the response<br>
handler connect_ndisstatqry_ch<wbr>eck_ready() parses the response with a<br>
generic helper function, and if the modem still isn't connected it<br>
starts a 1s timeout before trying again.  In your case, that timeout<br>
would trigger another call to ^SWWAN to check the status.<br>
<br>
Basically, the flow should go from step-to-step and typically not back<br>
up steps.  I think your model can follow this too.  Basically:<br>
<br>
INIT: do the ip type stuff, drop down to next step<br>
AUTH: check if auth is required, if so send SGAUTH, otherwise drop down<br>
CONNECT: send ^SWWAN=1,X,Y and when that's complete advance to next<br>
step<br>
WAIT: poll the connection status with SWWAN? and if not yet connected,<br>
g_timeout_add_seconds() to poll again.  When connected, advance step.<br>
IP_CONFIG: pull bearer properties and set ip4_config<br>
FINISH: complete the context<br>
<br>
Disconnect would follow a similar process, but of course you don't need<br>
AUTH or IP_CONFIG steps.<br>
<br>
Lastly, is the PDP CID -> usb interface mapping hardcoded in the<br>
firmware?  I can't find anything about that in PLS8-E_ATC_V02.011<br>
or PLS8-E_ATC_V01.000.  ie, can't we activate any arbitrary PDP context<br>
for an arbitary WWAN adapter with ^SWWAN=1,X,Y where X is a PDP context<br>
already determined by the MM core?  The +CGDCONT logic in the Cinterion<br>
plugin is overriding that and and may conflict with MM core logic.<br>
<span><font color="#888888"><br>
Dan<br>
<br>
</font></span></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></div><br></div>