ARMmbed / wifi-x-nucleo-idw01m1

X-NUCLEO-IDW0xx1 Wi-Fi Driver
3 stars 8 forks source link

Review of wifi driver for mbed OS 5 support #1

Closed MarceloSalazar closed 7 years ago

MarceloSalazar commented 7 years ago

Find here relevant feedback on the current driver implementation and suggestions to make it compatible with mbed OS 5: Branch: https://github.com/ARMmbed/wifi-x-nucleo-idw01m1/tree/betzw_wb

General requirements for the driver

Features & enhancements

Testing

@betzw cc @geky @screamerbg

betzw commented 7 years ago

@MarceloSalazar what is the advantage of reimplementing ::gethostbyname() rather than resorting to NetworkStack::gethostbyname()

betzw commented 7 years ago

General requirements for the driver

  • Driver should run on its own thread for TX of AT commands

Does this mean, that mbed's netsockets are not thread-safe?!? :zap:

  • RX of AT commands happens on IRQ context.

Is this a requirement? If yes, why? Isn't it enough to receive incoming data via Interrupt?

geky commented 7 years ago

@MarceloSalazar what is the advantage of reimplementing ::gethostbyname() rather than resorting to NetworkStack::gethostbyname()

There is a small code size improvement if you can offload DNS logic to an external chip (ESP8266 for example). But using mbed's gethostbyname is probably easier and less error prone. I think you're right with sticking to NetworkStack::gethostbyname().

Driver should run on its own thread for TX of AT commands

I don't think this should be a requirement. There may be a performance benefit to buffering outgoing packets, but functionality should come first and sending TX data in the callers thread will probably work just fine.

RX of AT commands happens on IRQ context.

I don't think I understand this bullet point. RX of AT commands can't happen in IRQ context, since waiting for a serial may block. The driver does need to signal the socket_attach callback when data comes in, but the RX of AT commands needs to either occur during the top-level call of socket_recv or in a seperate thread.

betzw commented 7 years ago

Pls. find attached the test results of an adapted version of the ESP8266 tests for release v0.1.1 of the wifi-x-nucleo-idw01m1 driver.

Please note:

cc @nikapov-ST

trace_non_verbose_1_250717.txt

MarceloSalazar commented 7 years ago

@betzw thanks a lot for updating the driver. We'll have a look into this soon.

nikapov-ST commented 7 years ago

@MarceloSalazar @betzw I made a trial with mbed OS example client using a NUCLEO_L476RG + X-NUCLEO-IDW01M1 and it seems it correctly connects and provides the virtual button data to the mbed device connector.

You can check it yourself here: https://github.com/nikapov-ST/mbed-os-example-client

To enable the Wi-Fi:

easy-connect lib was also modified to include support to STM Wi-Fi: https://github.com/nikapov-ST/easy-connect

MarceloSalazar commented 7 years ago

@nikapov-ST this is great news! thanks for confirming, I'll give it a try soon. FYI @screamerbg

MarceloSalazar commented 7 years ago

Thanks @betzw and @nikapov-ST for working on the driver and sharing this detailed information.

I confirm I've been able to successfully use your Client example application on the NUCLEO_L476RG + X-NUCLEO-IDW01M1 :) Some CLI tests are failing (UDP/DTLS), but suspect it's caused by my environment - I need to review this again.

In order to move forward and have the Wifi driver/module supported, I'd suggest working on the following areas (probably in this order):

Let me know if you have any question.

betzw commented 7 years ago

ATParser: understand what changes are strictly required. Please submit a PR to the official repo. Then update the ATParser.lib

Pls. refer to ATParser PR #11.

betzw commented 7 years ago

@MarceloSalazar & @nikapov-ST, Just to inform you that PR #11 has been merged.

betzw commented 7 years ago

Btw, I have made this repo public.

MarceloSalazar commented 7 years ago

@betzw that's great! Could you please test with the mbed-os-example-wifi and send a PR to the docs once you see it working?

betzw commented 7 years ago

@MarceloSalazar, pls. refer to https://github.com/ARMmbed/mbed-os-example-wifi/pull/50

betzw commented 7 years ago

@MarceloSalazar, can we close this issue?

marcuschangarm commented 7 years ago

Marcelo is currently on vacation.

MarceloSalazar commented 7 years ago

@betzw let's keep this item open a bit more until we have the driver supported in mbed-os-example-client.

betzw commented 7 years ago

@nikapov-ST, at what point are we there?

nikapov-ST commented 7 years ago

@betzw @MarceloSalazar In fact I was waiting for having mbed-os-example-wifi (simpler example) integrated first :-) . So, will try to progress with the client-example.

betzw commented 7 years ago

@MarceloSalazar, could you pls. take a look at mbed-os-example-client issue #303?

betzw commented 7 years ago

@MarceloSalazar, should I try to proceed with the PR for integrating X-NUCLEO-IDW01M1 into mbed-os-example-client despite of issue https://github.com/ARMmbed/mbed-os-example-client/issues/303?

MarceloSalazar commented 7 years ago

@betzw yes, you could start testing and preparing a PR for mbed-os-example-client and easy-connect, that could get merged as soon as ARMmbed/mbed-os#4406 gets released in 5.6 (and client app updated to point at 5.6).

betzw commented 7 years ago

@MarceloSalazar, pls. take a look at https://github.com/ARMmbed/easy-connect/pull/42 regarding easy-connect. Btw, using the current master branch of mbed-os does not yet solve the issue on my side even if https://github.com/ARMmbed/mbed-os/pull/4406 is already merged in ...