Cleaner .h/.cpp, overloaded for WiFi parameters #59

JanneKiiskila · 2017-12-08T18:28:44Z

A more pure .h/.cpp separation - implementation in the .cpp file
and the defines in the .h file.

Added also now the option to include WiFi SSID, Password as params
via an overloaded function. There is now logic to deduce if the
params were given and they were - those will be used.

If the paramers were note given (and we're using WiFi) we will
revert using the params given in the mbed_app.json -file.

That should sort out issue #56.

Rebased on top of the no-password commit by Andrew Chong, however
I think the the comparison should >1 rather than >2 - the password
could be just one character as well.

A more pure .h/.cpp separation - implementation in the .cpp file and the defines in the .h file. Added also now the option to include WiFi SSID, Password as params via an overloaded function. There is now logic to deduce if the params were given and they were - those will be used. If the paramers were note given (and we're using WiFi) we will revert using the params given in the `mbed_app.json` -file. That should sort out issue #56. Rebased on top of the no-password commit by Andrew Chong, however I think the the comparison should >1 rather than >2 - the password could be just one character as well.

bridadan

Good to see this being separated into .h/.cpp files, nice work. I have a few comments below.

bridadan · 2017-12-08T19:13:45Z

easy-connect.cpp

+        printf("[EasyConnect] Connecting to WiFi %s\n", _ssid);
+    }
+    network_interface = &wifi;
+    connect_success = wifi.connect(_ssid, _password, (strlen(_password) > 1) ? NSAPI_SECURITY_WPA_WPA2 : NSAPI_SECURITY_NONE);


Looks like all of the wifi connect calls are duplicated. would it be possible to just #ifdef for the log calls for each specific options, then just switch on the WIFI macro for the connect call?

So something like:

#elif defined(WIFI) if (log_messages) { #if MBED_CONF_APP_NETWORK_INTERFACE == WIFI_ESP8266 printf("[EasyConnect] Using WiFi (ESP8266)\n"); #elif MBED_CONF_APP_NETWORK_INTERFACE == WIFI_RTW printf("[EasyConnect] Using WiFi (RTW)\n"); #elif .... #endif printf("[EasyConnect] Connecting to WiFi %s\n", _ssid); } network_interface = &wifi; connect_success = wifi.connect(_ssid, _password, (strlen(_password) > 1) ? NSAPI_SECURITY_WPA_WPA2 : NSAPI_SECURITY_NONE);

You're right! Yes, it can be simplified. I'm however thinking of another way...

for each WIFI,
#define a WIFI_TYPE and
then just use printf("Using WiFi (%s)\n", WIFI_TYPE);

bridadan · 2017-12-08T20:38:19Z

easy-connect.cpp

+        if(strlen(MBED_CONF_APP_WIFI_PASSWORD) > WIFI_PASSWORD_MAX_LEN) {
+            printf("WARNING - MBED_CONF_APP_WIFI_PASSWORD is too long - it will be cut to %d chars.\n", WIFI_PASSWORD_MAX_LEN);
+        }
+        strncpy(_password, MBED_CONF_APP_WIFI_PASSWORD, WIFI_PASSWORD_MAX_LEN);


Normally I'm a big fan of avoiding dynamic allocation whenever possible, but this might be a good case to use the heap? Is there another requirement for using static allocation?

Well, me neither - most dynamic allocs will eventually fragment the heap. However, typically this one would be done just once at start... Passwords are rarely longer than say 8-14 characters, so there would be potential savings.

Simplify WiFi printing/logging, idea originated by Brian Daniels. - most of the code is just duplication and if we "abstract" the WiFi stack name via a #define, we can just use printf() with a parameter as the stack name. This shaves of quite a few repetitive lines of the implementation.

JanneKiiskila · 2017-12-08T22:06:42Z

easy-connect.cpp

+    // If they have not been set, use the ones we can gain from mbed_app.json.
+    if (strlen(_ssid) == 0) { 
+        if(strlen(MBED_CONF_APP_WIFI_SSID) > WIFI_SSID_MAX_LEN) {
+            printf("WARNING - MBED_CONF_APP_WIFI_SSID is too long - it will be cut to %d chars.\n", WIFI_SSID_MAX_LEN);


Or should we just return 0 here? Opinions? It would likely not be able to connect anyway.

Yeah it should just fail outright. But I think using dynamic allocation would be acceptable in this case, unless you made the requirement that the user is responsible for keeping the SSID and PASSWORD strings in scope during the lifetime of the Network Interface. Then you wouldn't have to copy the SSID at all and you could just use a pointer.

Print out the type of mesh (similar to as how we do it for WiFi as well). Simplify the Wifi-handling more - return with 0 if the SSID or password are too long. As we have now just one WiFi branch in the connection we can do it with one if. - No extra memory allocations/hard-coded buffers - No copying, just pass the pointer or use the mbed_app -defines. I considered doing a calloc() and then strncopy(), but that would leave us with dangling pointers we could never free.

JanneKiiskila · 2017-12-09T13:35:34Z

@bridadan - I think your concerns are now handled?

janjongboom · 2017-12-11T05:52:45Z

easy-connect.h


 #elif MBED_CONF_APP_NETWORK_INTERFACE == WIFI_ODIN
-#include "OdinWiFiInterface.h"
+#define WIFI


This macro seems too generic (same with MESH). Can we prefix this with EASY_CONNECT_ or something to avoid collisions in user applications?

Yes, we can do that (good point).

Fixed that now.

janjongboom · 2017-12-11T05:53:39Z

@JanneKiiskila This had to go with Andrew's PR but due to GH's weird review flow it was not submitted.

Rather change this to something like:

#ifndef MBED_CONF_APP_WIFI_SECURITY
#if MBED_CONF_APP_WIFI_PASSWORD == ""
#define MBED_CONF_APP_WIFI_SECURITY NSAPI_SECURITY_NONE
#else
#define MBED_CONF_APP_WIFI_SECURITY NSAPI_SECURITY_WPA_WPA2
#endif
#endif

So you can override the security setting from mbed config too (and to avoid code duplication).

JanneKiiskila · 2017-12-11T09:25:23Z

@janjongboom - that would not work with the 2nd overloaded API with the runtime params, so it needs to be like @andrewc-arm proposes.

SeppoTakalo · 2017-12-11T09:57:21Z

easy-connect.cpp

+#define WIFI_TYPE "ESP8266"
+
+#ifdef MBED_CONF_APP_ESP8266_DEBUG
+ESP8266Interface wifi(MBED_CONF_APP_ESP8266_TX, MBED_CONF_APP_ESP8266_RX, MBED_CONF_APP_ESP8266_DEBUG);


We are relying here on macros that require them to be defined in the applications mbed_app.json

Should we instead do this properly and have mbed_lib.json within this easy-connect library?

Same goes for all MBED_CONF_APP_* macros.

This module should be usable even when application does not have mbed_app.json at all.

They need to have the mbed_app.json anyway for choosing the network, so I'm not seeing the point necessarily in that.

The point is that these are configuration variables for Easy connect, not for the application.

Now your module does not compile if application is not defining these in mbed_app.json I don't think it is acceptable.

To avoid polluting the namespace with something too common. Ref: Jan Jongboom - review feedback.

JanneKiiskila · 2017-12-12T07:07:11Z

@SeppoTakalo - it's near impossible to put those parameters inside the mbed_lib.json, as the values vary so much. You can see if from here:

https://github.com/ARMmbed/mbed-os-example-client/blob/master/configs/wifi_idw01m1_v4.json#L15-L21

https://github.com/ARMmbed/mbed-os-example-client/blob/master/configs/wifi_odin_v4.json#L15-L21

or at least some creative work would be needed. They are now overlapping. Possibly we should re-factor some of the pins to be more module specific.. I'd be tempted to delay that to another PR, as the current status is the current status - there is no change for that in this PR.

SeppoTakalo · 2017-12-12T09:06:13Z

I'm not against it, if you don't want to change it. But in reality, those are configuration values for Easy connect, therefore they should be introduced in easy-connect repository.
This whole pattern seem wrong, easy connect is now depending on application defining some configuration values.
At least they seem to be documented in README.md so maybe that model works.

I would have assumed this repository to define the mbed_lib.json as

{
    "name": "easy-connect",
    "config": {
        "network-interface":{
            "help": "options are ETHERNET, WIFI_ESP8266, WIFI_IDW0XX1, WIFI_ODIN, WIFI_RTW, MESH_LOWPAN_ND, MESH_THREAD, CELLULAR_ONBOARD",
            "value": "WIFI_ESP8266"
        },
        "esp8266-tx": {
            "help": "Pin used as TX (connects to ESP8266 RX)",
            "value": "PTD3"
        },
        "esp8266-rx": {
            "help": "Pin used as RX (connects to ESP8266 TX)",
            "value": "PTD2"
        },
        "esp8266-debug": {
            "value": true
        },
        "wifi-ssid": {
            "value": "\"SSID\""
        },
        "wifi-password": {
            "value": "\"Password\""
        }
    }

Then on the application you would have following in the mbed_app.json

       "target_overrides": {
        "*": {
            "easy-connect.network-interface": "ETHERNET"
        }

JanneKiiskila · 2017-12-12T09:12:42Z

@janjongboom - thinking of your proposal for the 2nd time - I think it would work even with run-time.

TeroJaasko

+2. We really need to get rid of these all-the-code-in-one-header implementations.

JanneKiiskila · 2017-12-12T11:46:33Z

Ok, the #ifndef thing with the password will not work. It's just text pre-processing as it is macros, so it will not compute.

Compile [ 99.7%]: easy-connect.cpp
[Error] mbed_config.h@47,66: token ""Password"" is not valid in preprocessor expressions
[ERROR] In file included from <command-line>:0:0:
./.\BUILD\REALTEK_RTL8195AM\GCC_ARM\mbed_config.h:47:66: error: token ""Password"" is not valid in preprocessor expressions
 #define MBED_CONF_APP_WIFI_PASSWORD                              "Password"                     // set by application

We can't compare the string with a C macro, as that would require strcmp() or similar to work in C.

bridadan

So many good changes in here!

I still think the use of dynamic allocation in this case for the copying of strings for SSID and password is fine and makes for a better user experience. Having a max SSID and password length is unintuitive in my opinion.

bridadan · 2017-12-12T16:28:11Z

easy-connect.cpp

+    if (WiFiPassword != NULL) {
+        if(strlen(WiFiPassword) > WIFI_PASSWORD_MAX_LEN) {
+            printf("WARNING - WiFi Password is too long - it will be cut to %d chars.\n", WIFI_PASSWORD_MAX_LEN);
+            return 0;


This message is no longer true now that the return 0 is added.

I think it'd be an ok trade off to use dynamic allocation in this case to remove this failure mode. The amount memory should be very small and it just removes this edge case which would only be found at run time.

I think I've fixed it already in my branch (I've not pushed YET the very latest).

The size limiation is actually coming the WiFi specification, I looked up those numbers from various sources:

https://serverfault.com/questions/45439/what-is-the-maximum-length-of-a-wifi-access-points-ssid

https://security.stackexchange.com/questions/15653/recommend-length-for-wi-fi-psk

Ah ok, I didn't even think to look for that! I guess that's not bad then.

I'll keep an eye out for your update on the message 👍

It's fixed now.

Change the way we handle the default pinouts - not via mbed_app.json, but rather have them in mbed_lib.json. This is a non-backwards compatible change to certain extent, as the current pin defines potentially residing in mbed_app.json would not be valid anymore. Potential overrides need to be done differently, as the define one has to override is the easy-connect override, not the app override. Error handling was giving still wrong note in 2nd branch of easy_connect(), fixed that (ref: review feedback from Brian Daniels). README.md also updated a bit about the related changes.

JanneKiiskila · 2017-12-12T21:07:33Z

@bridadan - we don't need to allocate anything now with the approach done here.

if the user passes it, we just use the strings they already have.
if the string comes via mbed_app.json, we just use the #define as is.

The length checks are based on IEEE specs, I dug out the IEEE spec ref for one of those. Didn't find the WPA2, but https://security.stackexchange.com/questions/15653/recommend-length-for-wi-fi-psk says 63.

JanneKiiskila · 2017-12-12T21:10:59Z

@SeppoTakalo - going halfway where you want it. However, this one should be still quite compliant with our CI jobs, so should not wreak havoc there.

SeppoTakalo · 2017-12-13T10:35:08Z

Actually, driver should do the size checks. Most of them don't currently, but I will be adding testcases to force them to test. Therefore tests in this module are irrelevant.

JanneKiiskila · 2017-12-13T11:19:39Z

@SeppoTakalo - should I remove those then?

SeppoTakalo · 2017-12-13T12:44:40Z

Maybe in another PR. This is already exploding.
It was just a suggestion for improvement.

JanneKiiskila · 2017-12-13T14:28:53Z

Let's remove it then once you have the WiFi drivers sorted out, let me know.

JanneKiiskila · 2017-12-13T15:16:28Z

Test results are looking ok; ARMmbed/mbed-os-example-client#352

bridadan

Changes look good!

janjongboom · 2017-12-14T02:21:38Z

So why do we treat WiFi as special? https://github.com/ARMmbed/easy-connect/pull/59/files#diff-6a48c228794543db76758fdc80be436aR98 Why not cellular APN settings, or mesh channels. I'm not a big fan of this change.

janjongboom · 2017-12-14T02:22:38Z

I could live with some settings object, e.g.:

EasyConnectConfig config = { .wifi_ssid = "ssid", .wifi_password = "password" };
easy_connect(&config);

Would leave for flexibility.

JanneKiiskila · 2017-12-14T06:08:20Z

@janjongboom - that's another PR then. Feel free to submit. Though that config thing would be an "API" breaking change for easy-connect.

JanneKiiskila · 2017-12-14T07:13:41Z

We can @janjongboom create similar easy_connect overloads for all of the bearers, if need be. However, looking at the feedback so far (i.e. issues in this repo) -> wifi is the only one that's been requested. The same approach can be applied to all bearers, though.

SeppoTakalo · 2017-12-14T10:53:30Z

The moment you specify that it will be Cellular or Wifi, you are basically already away from the Easy connect's topic.
Also the example makes sense but is not valid C++, and in C it requires you to have RAM for all possible configuration parameter that this struct may have. Or maybe it is a union.

In reality, I would like to move part of the Easy Connect's job into the Mbed OS so that you could query default Wifi interface, default Cellular, or similarly to any device type.
Then applications would became portable between the same connectivity type.

Easy connect abstracts away all that. So because you are not required to specify between Wifi or Ethernet, you are either not supposed to provide configuration for those. In runtime.

For runtime configuration, easy connect is not very usable and probably not event the right choice.

JanneKiiskila · 2017-12-14T12:25:28Z

As long as there are 3 different wifi-stacks, you need something like this anyway. The whole easy-connect is a stop-gap-filler, as soon as there is something more sensible available in Mbed OS - we should move over to that and deprecate the easy-connect.

C/C++ discussion - well, Mbed OS choice of weapon is C++ - so any points of being pure C are invalid in that context.

andrewc-arm · 2017-12-19T09:19:13Z

Sorry. I was terribly busy recently.

Rebased on top of the no-password commit by Andrew Chong, however I think the the comparison should >1 rather than >2 - the password could be just one character as well.

@JanneKiiskila, it is 2 characters because it has quotes ("") for no password setting.

JanneKiiskila · 2017-12-19T09:30:44Z

@andrewc-arm - Did you try that out with what C actually sees in strlen()?

andrewc-arm · 2017-12-19T09:33:08Z

@JanneKiiskila. Nope. I just assumed. If I have time to test later, I will let you know. However, I am terribly busy onboarding new partner...

JanneKiiskila · 2017-12-19T09:45:42Z

I did test it... :) The mbed-app long define gets turned into a simple .h-file, if you look under BUILD/BOARD/COMPILER you will find an mbed_config.h and it there just a "", which is an empty string which has length of 0.

However, I doubt it to be very easy to set the password to anything shorter than 6, since at least my phone is simply letting me to put in anything shorter than 6 characters, so probably both 1 and 2 would work equally well.

Carmeloning · 2018-11-27T07:06:37Z

I have some problems and i don't konw how to slow them.my platfrom is L476rg

JanneKiiskila · 2018-11-27T09:44:02Z

@Carmeloning - near impossible to offer any help, as there are no details. Seems the WiFi cant' connect to the APN you are offering to it.

please check SW versions (there's been a lot of bug fixes to ESP8266), Mbed OS itself, too
verify your APN name and passphrase.

* cli work with K64F and ODIN * define to compile in linux * odin use updated sd repository * add documentation * add leds 2-5 to define

JanneKiiskila requested review from janjongboom, SeppoTakalo, bridadan, yogpan01, TeroJaasko, teetak01 and ashok-rao December 8, 2017 18:28

JanneKiiskila force-pushed the cpp-wifi-param branch from 7d4054a to e78560e Compare December 8, 2017 18:56

JanneKiiskila requested a review from andrewc-arm December 8, 2017 18:57

JanneKiiskila force-pushed the cpp-wifi-param branch from e78560e to ea8ba6f Compare December 8, 2017 18:59

JanneKiiskila force-pushed the cpp-wifi-param branch from ea8ba6f to 29421e6 Compare December 8, 2017 19:01

JanneKiiskila mentioned this pull request Dec 8, 2017

I have to pass WiFi credential (SSID/Passw) through NFC tag so w/o recompiling the .json file, how to do this with easy-connect? #56

Closed

bridadan reviewed Dec 8, 2017

View reviewed changes

JanneKiiskila commented Dec 8, 2017

View reviewed changes

JanneKiiskila mentioned this pull request Dec 9, 2017

Question: Why is everything defined in easy-connect.h? #49

Closed

janjongboom reviewed Dec 11, 2017

View reviewed changes

SeppoTakalo suggested changes Dec 11, 2017

View reviewed changes

WIFI and MESH pre-fixes with EASY_CONNECT

81685a6

To avoid polluting the namespace with something too common. Ref: Jan Jongboom - review feedback.

SeppoTakalo approved these changes Dec 12, 2017

View reviewed changes

TeroJaasko approved these changes Dec 12, 2017

View reviewed changes

bridadan suggested changes Dec 12, 2017

View reviewed changes

bridadan approved these changes Dec 13, 2017

View reviewed changes

JanneKiiskila merged commit 15f9b1c into master Dec 14, 2017

JanneKiiskila deleted the cpp-wifi-param branch December 14, 2017 06:08

bellashtiv pushed a commit that referenced this pull request Dec 27, 2018

Cli support (#59)

a28a11d

* cli work with K64F and ODIN * define to compile in linux * odin use updated sd repository * add documentation * add leds 2-5 to define

Cleaner .h/.cpp, overloaded for WiFi parameters #59

Cleaner .h/.cpp, overloaded for WiFi parameters #59

Conversation

JanneKiiskila commented Dec 8, 2017 • edited Loading

bridadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanneKiiskila Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanneKiiskila Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanneKiiskila commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janjongboom commented Dec 11, 2017

JanneKiiskila commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanneKiiskila commented Dec 12, 2017

SeppoTakalo commented Dec 12, 2017

JanneKiiskila commented Dec 12, 2017

TeroJaasko left a comment

Choose a reason for hiding this comment

JanneKiiskila commented Dec 12, 2017

bridadan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanneKiiskila commented Dec 12, 2017 • edited Loading

JanneKiiskila commented Dec 12, 2017

SeppoTakalo commented Dec 13, 2017

JanneKiiskila commented Dec 13, 2017

SeppoTakalo commented Dec 13, 2017

JanneKiiskila commented Dec 13, 2017

JanneKiiskila commented Dec 13, 2017

bridadan left a comment

Choose a reason for hiding this comment

janjongboom commented Dec 14, 2017

janjongboom commented Dec 14, 2017

JanneKiiskila commented Dec 14, 2017

JanneKiiskila commented Dec 14, 2017

SeppoTakalo commented Dec 14, 2017

JanneKiiskila commented Dec 14, 2017 • edited Loading

andrewc-arm commented Dec 19, 2017

JanneKiiskila commented Dec 19, 2017

andrewc-arm commented Dec 19, 2017

JanneKiiskila commented Dec 19, 2017 • edited Loading

Carmeloning commented Nov 27, 2018

JanneKiiskila commented Nov 27, 2018

JanneKiiskila commented Dec 8, 2017 •

edited

Loading

JanneKiiskila Dec 8, 2017 •

edited

Loading

JanneKiiskila Dec 8, 2017 •

edited

Loading

JanneKiiskila commented Dec 12, 2017 •

edited

Loading

JanneKiiskila commented Dec 14, 2017 •

edited

Loading

JanneKiiskila commented Dec 19, 2017 •

edited

Loading