Skip to content

Set default network driver on Ublox target to ethernet #222

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

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

dgreen-arm
Copy link
Contributor

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 #26 (comment) now shows the change we wish to make here.

@simonbutcher
Copy link
Contributor

retest

@simonbutcher
Copy link
Contributor

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

@dgreen-arm
Copy link
Contributor Author

retest

@dgreen-arm
Copy link
Contributor Author

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

Copy link
Contributor

@simonbutcher simonbutcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

For our CI system, we probably will always prefer the ethernet, so should we document that in the README.md?

cc: @andresag01 / @k-stachowiak

@Patater
Copy link
Contributor

Patater commented Jan 2, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

Patater commented Jan 3, 2019

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
Copy link
Contributor Author

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
Copy link

kjbracey commented Jan 4, 2019

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants