ARMmbed / mbed-os-example-tls

mbed TLS Sample application
Apache License 2.0
30 stars 52 forks source link

Set default network driver on Ublox target to ethernet #222

Closed dgreen-arm closed 5 years ago

dgreen-arm commented 5 years ago

The tls-client example needs updating for the UBLOX_EVK_ODIN_W2 platform to correctly set the default network interface to be ethernet.

The previous fix was discussed in #26, however the suggested fix in https://github.com/ARMmbed/mbed-os-example-tls/issues/26#issuecomment-275036468 now shows the change we wish to make here.

simonbutcher commented 5 years ago

retest

simonbutcher commented 5 years ago

CI passes for gcc but fails on IAR and Arm C Compiler. Can you please look into it @dgreen-arm?

dgreen-arm commented 5 years ago

retest

dgreen-arm commented 5 years ago

It looks like the failures were due to infrastructure issues, all the tests have successfully run in the last two test runs.

Patater commented 5 years ago

This change is making an implicit preference for ethernet over Wifi on a specific platform.

I believe we were already trying to make that preference before this PR. The issue is that how one elects to prefer Ethernet over WiFi has changed, and this PR updates it for the (currently) correct way.

Maybe we don't need to pick Ethernet anymore? I'd expect our examples to work fine on WiFi, and CI can do WiFi.

dgreen-arm commented 5 years ago

Preference for ethernet for the Ublox platform is specifically done as its the only target that we currently test that has the default interface being wifi, all the other targets have ethernet as their default interface.

simonbutcher commented 5 years ago

@patater - Because we test this sample with a variety of boards in the CI, the boards we test with are always configured for ethernet.

To fix this board in CI, we're making it explicit. That was my point.

So do we want to document this? I guess there's a bunch of mbed boards we're also not configured for.

Patater commented 5 years ago

I believe we are making it explicit already for all targets our example runs on via this line to select Ethernet as the interface to use: https://github.com/ARMmbed/mbed-os-example-tls/blob/master/tls-client/mbed_app.json#L9

It's probably worth documenting in the README that the example requires Ethernet. We had issues in the past on the Odin board because a selection of Ethernet meant WiFi at some point in the past (2016), as we didn't have WiFi credentials present. It looks like Odin understands Ethernet means Ethernet these days, and perhaps the workaround isn't necessary anymore.

dgreen-arm commented 5 years ago

The README for the TLS client does state that ethernet is required.

I've tried removing the lines changing the default driver, the Odin board still fails as the example tries to get the default network interface, which is Wifi. There's two ways to fix this I know of: changing the default driver to ethernet, or changing the example to specifically get an ethernet interface.

Given that the TLS example currently requires ethernet, which would be a better solution? Change the example to specifically get an ethernet interface, or set the default driver to ethernet for all boards?

kjbracey commented 5 years ago

If you always want this to be Ethernet, then you can unconditionally set that default to ETHERNET, regardless of target.

Or you could change the code to ask for EthInterface::get_default_instance(), not NetworkInterface::get_default_instance().

I would suggest that in an example it's better for the default mbed_app.json to use the target default network interface. Maybe you could have a separate mbed_app.json for CI that forces ETHERNET for all targets - I believe various CI jobs do select alternative JSON files.

simonbutcher commented 5 years ago

I think this issue of default network interface is out of scope of this PR.

The example should always work with the default network interface for the device, except when on the CI, when it should always be ethernet.

Thanks for the pointers @kjbracey-arm - I'll raise this as bug (or maybe better described as a work item), and we'll adapt the example to prefer ethernet on the CI, but otherwise go with the default interface.

As such, I'm approving the PR.