Make APN lookup the default behaviour for PPPCellularInterface #6166

RobMeades · 2018-02-22T15:28:36Z

Description

If the user does not provide an APN when using PPPCellularInterface, the driver currently uses the APN "internet" as a default. It includes the capability to look up the APN based on the IMSI of the SIM however this behaviour is only available under the compilation switch MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP.

With this change APN look-up is the default behaviour. If the user specifies an APN then that still takes precedence and if the look-up fails then, finally, "internet" is used, so this should have no adverse effect on existing users.

All of the cellular interface tests in mbed-os-features-netsocket-cellular-generic_modem_driver-tests-unit_tests-default pass after making this change.

Pull request type

Enhancement to Feature

Dependencies

This change depends upon the fix in #6160.

RobMeades · 2018-02-22T15:29:35Z

Note that if this change is accepted then we can delete the UbloxPPPCellularInterface driver which we otherwise maintain to provide this functionality.

kjbracey · 2018-02-23T08:28:03Z

I think all you would need to do here for this change is flip the default for "apn-lookup" to true in https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/cellular/generic_modem_driver/mbed_lib.json . That's where MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP is coming from.

Or you could set that to "true" in your own board's targets.json if you want.

But this feels like an "application" thing, rather than a driver/board thing - isn't it a matter for the application whether or not it wants this? That's why the comments tell the app to turn this on. It's not a board/modem-specific feature.

I guess the reason for the flag is to save the space for the database. If the database is in, it will be consuming ROM pointlessly if the app is calling set_credentials anyway.

Thinking about it, I would support having the database there by default - people would be free to save RAM by flipping the flag to false in their mbed_app.json. I'm generally in favour of defaults that work, rather than are "space-optimised". Given the API, I can't see a way to automatically linker-exclude it without a JSON flag, but why not default to true.

Are you following #6082, by the way? Don't believe those new drivers are currently supporting the APN database at all. If the APN database functionality is useful to you, you should probably comment there - (the intent is currently to deprecate PPPCellularInterface in favour of the framework-based board-specific drivers there.)

RobMeades · 2018-02-23T09:34:33Z

APNs are terrible, terrible things and no user should need to be aware of them unless they want a special service of some kind. The APN_db does this and we musn't lose it; I'll go see what's happening in #6082.

I also think that, rather than making everyone add a #define, I think it's always better to make it blindingly obvious what the code is intended to do by taking the #define out and, putting in the "#ifndef DISABLE" pattern on the off chance that anyone should need to make an exception. It probably costs a kbyte of ROM, which is not the scarce resource in the case of platforms that have enough room for a cellular modem.

So will you approve this @kjbracey-arm :-)?

kjbracey · 2018-02-23T09:37:17Z

I'll support flipping the default to true, but that should just be changing that mbed_lib.json file.

The option is specified by the JSON file, and any documentation should be added there. (By adding a "help" section - cf #6092 ) Some documentation is autogenerated from the JSON.

RobMeades · 2018-02-23T09:57:25Z

I think your implementation is less clear and obvious that way, but if you insist I will make that change instead.

kjbracey · 2018-02-23T10:18:24Z

That seems like a universal comment on the config system, rather than something about this specific option. Given the config system in use (https://docs.mbed.com/docs/mbed-os-handbook/en/latest/advanced/config_system/ ) , and that we already have this existing option, just changing its default value to true is the natural thing to do.

Your C change added a new define without any corresponding JSON option, and also dropped support for the existing JSON option.

If you aligned them, you'd have a new option "disable-apn-lookup": false, but double-negatives like that just get weird in config systems. Having an enable option with a default of true that can be turned off works better. (Similar options: lwip.ipv4-enabled and lwip.ethernet-enabled)

RobMeades · 2018-02-23T10:25:50Z

OK, I'm happy to go with whatever the convention is.

kjbracey · 2018-02-23T10:32:24Z

#6160 has been merged, so please rebase on master to lose the duplicate commit here.

RobMeades · 2018-02-23T10:47:27Z

Arg, sorry, fixing...

kjbracey

Actually, going to have to unapprove this, because I see that this actually "forces" APN database lookup - it doesn't seem like this is implemented right in PPPCellularInterface- the "set_credentials_used" flag isn't read.

I think "connect-with-parameters" needs to set that flag if any of APN, uname or pwd are set, and the lookup and use of apnconfig in "connect()" needs to be conditional on that being false.

RobMeades · 2018-02-23T11:28:45Z

I think the implementation is a bit confusing: I tested it and I believe that if you specify the APN with set_credentials() it does use the credentials you supply, 'cos I deliberately specified the wrong APN for my SIM ("internet") in mbed_app.json and the connection failed.

RobMeades · 2018-02-23T13:11:26Z

No, you're right, I just tried it again and it does force the look-up, must have been a fluke when I tried that the first time. I will propose a modification to PPPCellularInterface...

RobMeades · 2018-02-23T15:05:49Z

Revised version now committed. The following four situations were tested against the tests in mbed-os-features-netsocket-cellular-generic_modem_driver-tests-unit_tests-default:

MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP set to true in mbed_lib.json, APN present in mbed_app.json: APN from mbed_app.json is used, all tests pass.
MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP set to false in mbed_lib.json, APN present in mbed_app.json: APN from mbed_app.json is used, all tests pass.
MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP set to true in mbed_lib.json, no APN present in mbed_app.json and IMSI has an APN match in APN_db.h: APN from APN_db.h is used, all tests pass.
MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP set to true in mbed_lib.json, no APN present in mbed_app.json and IMSI does not have an APN match in APN_db.h (I hacked the entry for my SIM out of APN_db.h manually for this test): "internet" is used for the APN.

Test results for all four cases are attached (bouncing off my own echo server so that I can see what's going on), noting that the correct APN for my SIM is "jtm2m" and the user APN I have set is "users_apn"; setting the wrong APN in this way will work for a few connection attempts until the cellular network realises what you're up to. You can, in any case, see the +CGDCONT=1,"blah" line setting the APN in the modem for the tests. Note that only the first test really tests setting the APN as the call to setup_context_and_credentials() is only made if initialise is set to false and initialise is only set once for a given instantiation of the class.

case_1_test_results.txt
case_2_test_results.txt
case_3_test_results.txt
case_4_test_results.txt

kjbracey · 2018-02-26T08:56:17Z

features/netsocket/cellular/generic_modem_driver/PPPCellularInterface.cpp

@@ -565,7 +563,21 @@ nsapi_error_t PPPCellularInterface::connect()
    bool success;
    bool did_init = false;
    const char *apn_config = NULL;
-
+    bool user_specified_apn = false;


Can you have connect(credentials) actually call set_credentials() itself. Just to make sure that call is functionally equivalent to the set_credentials/connect pair, as appears to be the intent.

kjbracey · 2018-02-26T09:04:54Z

features/netsocket/cellular/generic_modem_driver/PPPCellularInterface.cpp

+     * "internet" as a best guess
+     */
+    if (_apn != NULL) {
+        user_specified_apn = true;


This now has a different problem, in that this will have user_specified_apn true on the second connect with the database, because the _apn was set from the lookup. So the apn chosen for the first network seen becomes "sticky" and reused on the next network.

I think for clarity I would rename the members user_apn, etc. Then use local variables / parameters for the bringup, not those members directly. connect() should fill in local apn etc variables from lookup or copy them from the "user" members (or default to "internet").

setup_context_and_credentials should take "apn" as a parameter, not read the "user_apn" member.

The APN you use is not based on the registered network, it is based on the IMSI of the SIM (i.e. the network providing your service, rather than the one you happen to be registered on), so unless the SIM is hot-swapped (which we have no concept of here) the choice will have already been made correctly (hence why it is inside the if (!initialized) {... block just below).

I'll look into the other changes.

Ah, okay, that changes things. Misunderstanding on my part. Given that, I guess this is reasonable, if we assume SIM swap means reboot.

Yes, unless special arrangement have been made (very rare, and we don't have the code to deal with them here anyway), recognising a change of SIM requires a reboot.

I tried the renaming/parameter passing as suggested but it ends up looking wrong to me since, in the case of database look-up, it is not really the user's APN, it is the default APN for the operator that provides their service.

There is a bigger issue, which I've been avoiding: setup_context_and_credentials() is only ever called once (inside the if (!initialized) {... block), so I don't think it matters if a user passes a different APN into connect(credentials) on a subsequent call, that new APN would not be passed to the modem (with AT+CGDCONT) in any case.

Right, okay, leave that change then. But if you could do the set_credentials inside connect at least.

ensuring that, if there is a user-specified APN, it is still used.

RobMeades · 2018-02-26T11:34:57Z

Changes made and retested.

case_1_test_results_1.txt
case_2_test_results_1.txt
case_3_test_results_1.txt
case_4_test_results_1.txt

cmonr · 2018-02-27T05:57:45Z

/morph build

cmonr · 2018-02-27T05:58:34Z

Thoughts @hasnainvirk?

mbed-ci · 2018-02-27T06:31:25Z

Build : SUCCESS

Build number : 1268
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6166/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

hasnainvirk

Looks good to me.

mbed-ci · 2018-02-27T09:49:26Z

Exporter Build : SUCCESS

Build number : 934
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6166/

mbed-ci · 2018-02-27T10:22:53Z

Test : SUCCESS

Build number : 1065
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6166/1065

0xc0170 requested review from kjbracey and hasnainvirk February 22, 2018 16:50

0xc0170 added the needs: review label Feb 22, 2018

RobMeades force-pushed the apn_default branch from f17196a to 64077ae Compare February 23, 2018 10:24

RobMeades force-pushed the apn_default branch from 64077ae to 6341e0a Compare February 23, 2018 10:46

RobMeades force-pushed the apn_default branch 2 times, most recently from ae0f3c5 to 835b4d5 Compare February 23, 2018 11:00

kjbracey approved these changes Feb 23, 2018

View reviewed changes

kjbracey suggested changes Feb 23, 2018

View reviewed changes

RobMeades mentioned this pull request Feb 23, 2018

Mbed OS cellular connectivity #6082

Merged

RobMeades force-pushed the apn_default branch from 835b4d5 to e5f7990 Compare February 23, 2018 15:04

kjbracey suggested changes Feb 26, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 26, 2018

Make MBED_CONF_PPP_CELL_IFACE_APN_LOOKUP default in mbed_lib.json while

2fe3223

ensuring that, if there is a user-specified APN, it is still used.

RobMeades force-pushed the apn_default branch from e5f7990 to 2fe3223 Compare February 26, 2018 11:36

kjbracey approved these changes Feb 26, 2018

View reviewed changes

cmonr added needs: CI and removed needs: work labels Feb 27, 2018

hasnainvirk approved these changes Feb 27, 2018

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Feb 27, 2018

cmonr added the release-version: 5.7.7 label Feb 27, 2018

cmonr merged commit 478fda7 into ARMmbed:master Feb 27, 2018

cmonr removed the ready for merge label Feb 27, 2018

adbridge added release-version: 5.8.0-rc1 and removed release-version: 5.7.7 labels Feb 28, 2018

Make APN lookup the default behaviour for PPPCellularInterface #6166

Make APN lookup the default behaviour for PPPCellularInterface #6166

Uh oh!

Conversation

RobMeades commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Dependencies

Uh oh!

RobMeades commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobMeades commented Feb 23, 2018

Uh oh!

kjbracey commented Feb 23, 2018

Uh oh!

RobMeades commented Feb 23, 2018

Uh oh!

kjbracey commented Feb 23, 2018

Uh oh!

RobMeades commented Feb 23, 2018

Uh oh!

kjbracey commented Feb 23, 2018

Uh oh!

RobMeades commented Feb 23, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

RobMeades commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RobMeades commented Feb 23, 2018

Uh oh!

RobMeades commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobMeades Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobMeades commented Feb 26, 2018

Uh oh!

cmonr commented Feb 27, 2018

Uh oh!

cmonr commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbed-ci commented Feb 27, 2018

Triggering tests

Uh oh!

hasnainvirk left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Feb 27, 2018

Uh oh!

mbed-ci commented Feb 27, 2018

Uh oh!

Uh oh!

RobMeades commented Feb 22, 2018 •

edited

Loading

RobMeades commented Feb 22, 2018 •

edited

Loading

kjbracey commented Feb 23, 2018 •

edited

Loading

RobMeades commented Feb 23, 2018 •

edited

Loading

RobMeades commented Feb 23, 2018 •

edited

Loading

RobMeades Feb 26, 2018 •

edited

Loading

cmonr commented Feb 27, 2018 •

edited

Loading