-
Notifications
You must be signed in to change notification settings - Fork 3k
Cellular: Make AT_CellularContext::get_context() virtual #10442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@ARMmbed/team-embeddedplanet |
@ARMmbed/mbed-os-wan |
@trowbridgec, thank you for your changes. |
@trowbridgec do you have a log with AT debug to see why the first APN is not selected? I'm guessing your problem might be due to the automatic APN lookup, which you could try to disable in
|
I'm trying out your suggestion on the APN lookup now. In the mean time, below is an AT log from when I see this issue. It looks like it asks the module what contexts are configured, doesn't match to any of them, decides to create a new one (a 7th one), and then gets rejected because the module only supports 6 contexts. I would expect/hope that it would realize the first context has an empty APN and use with that one. BTW, the APN we're using is "11583.mcs".
|
After adding the suggested settings to
If I tweak the Any other ideas? |
@trowbridgec you need to clear existing contexts, e.g. add temporarily this code to make cleanup:
|
@AriParkkila Adding the tweak to clear out the contexts before choosing one seems to have worked; however, in order to add this change, wouldn't I still need to be able to write my own implementation of the |
@trowbridgec The problem was likely that there were too many existing contexts. After that cleanup tweak was once executed those extra contexts were removed. It should work fine without that cleanup tweak now due to Mbed cellular framework should clear any new contexts that it creates. |
I'm not up to speed with this topic. But I believe we were seeing the issue with a number of ME910 modules. I wasn't sure if they shipped that way or if the driver had been adding the extra profiles. |
@loverdeg-ep could you implement some recovery code at your application, e.g. for APN cleanup:
|
@AriParkilla it would need to be part of our BSP or factory code. Being a solution & design services provider we do everything we can not to push any workarounds upto the application level. This would burden the application writer otherwise. Not necessarily saying this MR needs to make it into the OS. Maybe a better solution will appear as we gain deeper understanding of the module. |
@AriParkkila I think you're right that the issue was just that there were too many contexts already populated, but as @loverdeg-ep pointed out, we are seeing this on some ME910s and not others (it isn't just an isolated incident on one board). I agree that this might not be the best solution moving forward; how do you feel about a change where the
|
@trowbridgec It doesn't sound good to ignore APN if given, and you probably shouldn't define APN at all in that case. |
@AriParkkila So, my argument is that if a context exists but has an APN of an empty string ( |
@AriParkkila any further thoughts? |
@ARMmbed/mbed-os-wan Can we progress here? |
Yes please. Embedded Planet is a partner and is attempting to get an Mbed based Pelion ready product launched. |
@AriParkkila Can you comment on this PR and/or the proposed alternative workaround? It seems that if this function was virtual and allowed to be overridden, that could allow flexibility for specific cellular module drivers. Also, please comment about why overwriting an APN with length of 0 could be a problem. |
@maclobdell Do you mean that when APN is "", that is accepted match to any APN in json setup? |
From my understanding, the purpose of the If yes, then I'd vote we make that change instead. If no, then I'd vote we make the |
@AriParkkila are you happy with @trowbridgec comments ? |
@adbridge your issue bumps have been sorely needed. Thank you. |
@trowbridgec I'm not quite following here... If you don't require any named APN then |
@AriParkkila maybe I'm not explaining the issue properly. When the mbed ATHandler calls the
The APN that I am trying to use is The current logic from the |
@AriParkkila @AnttiKauppila Please let us know if this request is clear to you. Can the logic change to get an unused context? |
@trowbridgec I'm not sure... Would it be better to have getter for a list of APNs from which you could select one? Another (not so optimal) way would be first try to connect with a named APN and in case unsuccessful with a void APN? |
@AriParkkila Would the list of APNs be generated from the APN database (i.e. I think the 2nd method might be ok. I have seen that some of our modules will somtimes come up with the correct APN name in one of the contexts (I'm assuming it's because the module itself remembers the last used APN?), so I think the logic to first attempt to connect via a named APN is valid. Do you want me to update my PR to meet your second suggestion? |
@trowbridgec That sounds good, but now as I see what you try to achieve |
I updated "Release Notes" to contain bit more details. CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
@AnttiKauppila Please review this functionality change |
Description
I have observed that some cell modules (namely the Telit ME910C1) pre-fill the contexts with dummy values. When the current
get_context()
function calls theAT+CGDCONT?
command, the module responds with 6 already populated dummy contexts. For example:I would expect the
get_context()
function to select and overwrite the 1st context, but instead cycles through all of the contexts, rejects them all (none of them match the target APN for my SIM), and tries to create a 7th. This then throws an error since the ME910C1 only supports 6 contexts.Therefore, I propose making the
get_context()
functionvirtual
and overwrite-able by the individual cell module driver authors (similar to what is available for otherAT_CellularContext
functions, likedo_connect()
), which is what this PR enables.Pull request type
Reviewers
Release Notes
Makes
AT_CellularContext::get_context()
virtual - overwrite-able by the individual cell module driver authors (similar to what is available for other AT_CellularContext functions, like do_connect()).