8000 RP2: Add ublox Nina-W10 WiFi/BT module driver. by iabdalkader · Pull Request #7668 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

RP2: Add ublox Nina-W10 WiFi/BT module driver. #7668

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

Closed
wants to merge 2 commits into from

Conversation

iabdalkader
Copy link
Contributor

No description provided.

@iabdalkader iabdalkader force-pushed the ninaw10 branch 2 times, most recently from d37baa4 to 840bae9 Compare August 15, 2021 21:15
@dpgeorge
Copy link
Member

Thanks for this, there's a lot of work here and it's a really good addition.

Some initial questions:

  1. Did you write this code from scratch, or are parts obtained from elsewhere (eg an Arduino driver)?
  2. I was hoping it'd be possible to configure the Nina-W10 in raw ETH mode so that lwIP can run on the rp2. Do you know if this is possible?

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Aug 16, 2021
1. Did you write this code from scratch, or are parts obtained from elsewhere (eg an Arduino driver)?

Yes this is 100% written from scratch all of it, but I copied the command constants enum in nina_wifi_drv.c from the Arduino library. Does this cause an issue/violate the license ? If so I can rename these constants, and probably should anyway because they are poorly named.

2. I was hoping it'd be possible to configure the Nina-W10 in raw ETH mode so that lwIP can run on the rp2.  Do you know if this is possible?

I thought about that initially too, it may be able to support something like a bypass mode but I think it will require a different firmware to so. That said, this driver is mainly for the Nano RP2040 which ships with this firmware which is also used by other Arduino boards (and I think some of Adafruits boards use it as well not sure) so this driver is needed for these boards to support WiFi out of the box. Maybe at some point in the future we can add support for lwip firmware too and make it configurable.

@iabdalkader
Copy link
Contributor Author

Renamed all constants in question to their actual respective functions, and removed some unused ones.

@codecov-commenter
Copy link

Codecov Report

Merging #7668 (51090b8) into master (b51e7e9) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7668      +/-   ##
==========================================
- Coverage   98.25%   98.25%   -0.01%     
==========================================
  Files         154      154              
  Lines       20071    20071              
==========================================
- Hits        19721    19720       -1     
- Misses        350      351       +1     
Impacted Files Coverage Δ
py/bc.c 88.65% <0.00%> (-1.04%) ⬇️
py/objlist.c 99.23% <0.00%> (-0.39%) ⬇️
py/obj.c 97.22% <0.00%> (ø)
py/runtime.c 99.39% <0.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b51e7e9...51090b8. Read the comment docs.

@iabdalkader iabdalkader changed the title Add ublox Nina-W10 WiFi/BT module driver. RP2: Add ublox Nina-W10 WiFi/BT module driver. Sep 12, 2021
@iabdalkader iabdalkader force-pushed the ninaw10 branch 5 times, most recently from ff7cff7 to cb8a125 Compare September 19, 2021 16:21
@iabdalkader
Copy link
Contributor Author

Cleaned up and rebased also moved RP2 port support to this PR since it was labeled port-rp2.

@iabdalkader
Copy link
Contributor Author

@dpgeorge Hello, this been ready for a while.

@dpgeorge
Copy link
Member

It would be good to add the new driver code introduced here to the code formatting, like this:

--- a/tools/codeformat.py
+++ b/tools/codeformat.py
@@ -35,6 +35,7 @@ import subprocess
 # Relative to top-level repo dir.
 PATHS = [
     # C
+    "drivers/ninaw10/*.[ch]",
     "extmod/*.[ch]",
     "extmod/btstack/*.[ch]",
     "extmod/nimble/*.[ch]",

@dpgeorge
Copy link
Member

I see that the Nina-W10 contains an ESP32 for the WiFi and BT. As such, could the driver here be considered as a generic ESP32 WiFi-BT-coprocessor driver? Eg if I took an ESP32 board and ins 8000 talled the arduino/nina-fw on it, this driver would probably work out-of-the box with that ESP32 connected to, say, an STM32?

Well, I don't really want to suggest to rename this driver to esp32-wifi, but just wanted to point out that it could be used with other ESP32-based WiFi modules, not only Nina W-10.

@dpgeorge
Copy link
Member

Regarding licensing: I see that https://github.com/arduino/nina-fw is LGPL. So it's really important that no code at all has been taken/copied/adapted from that driver to make the driver here.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Nov 12, 2021

@dpgeorge updated the PR to address all the requested changes.

if I took an ESP32 board and installed the arduino/nina-fw on it, this driver would probably work out-of-the box with that ESP32 connected to, say, an STM32?

Yes it should, STM or any other MCU just need to implement the BSP layer. The firmware just turns the ESP32 into an SPI WIFI module (like the WINC1500).

Regarding licensing: I see that https://github.com/arduino/nina-fw is LGPL. So it's really important that no code at all has been taken/copied/adapted from that driver to make the driver here.

No it's written completely from scratch and has nothing to do with the LGPL driver (which is C++ btw) but it's also not a port in fact it takes a very different approach to how the protocol and var args/values are handled, and it's much more efficient and maintains a higher datarate than the LGPL driver.

It would be good to add the new driver code introduced here to the code formatting, like this:

Done.

@iabdalkader iabdalkader force-pushed the ninaw10 branch 2 times, most recently from afb76ae to e8b65c2 Compare November 12, 2021 23:19
* Add WiFi/BT drivers for ublox Nina-W10 (esp32 based) module.
* Add ublox Nina-W10 Python module in extmod.
@dpgeorge
Copy link
Member

Thanks for the fast turn around on updating the PR, and also for confirming the rewrite (ie licensing).

Merged in 43079aa and 3745c39 with very minor formatting changes.

@dpgeorge dpgeorge closed this Nov 13, 2021
@iabdalkader iabdalkader deleted the ninaw10 branch November 13, 2021 12:26
@iabdalkader iabdalkader restored the ninaw10 branch November 13, 2021 12:26
@iabdalkader iabdalkader deleted the ninaw10 branch November 13, 2021 12:26
@iabdalkader iabdalkader restored the ninaw10 branch November 13, 2021 12:26
@iabdalkader iabdalkader deleted the ninaw10 branch November 13, 2021 12:26
@robert-hh
Copy link
Contributor

Eg if I took an ESP32 board and installed the arduino/nina-fw on it, this driver would probably work out-of-the box with that ESP32 connected to, say, an STM32?

I loaded the Arduino NINA.bin to a generic ESP32, connected it (for simplicity) to a RP2040 Pico with networking enabled, tried WiFi and it works. Thanks to @iabdalkader for making the driver so clean. There is a little bit of confusion between versions. The one from Arduino is 1.4.8, Adafruit points at a version 1.7.4. Both variants are available as source code (GPL 2.1) and binaries.

@iabdalkader
Copy link
Contributor Author

There is a little bit of confusion between versions. The one from Arduino is 1.4.8, Adafruit points at a version 1.7.4

I think the firmware from Adafruit is a fork with a (likely) compatible API/protocol. What I know for sure is 1.4.8 fixes a bug in send/recv and that's what you should be using or higher.

@robert-hh
Copy link
Contributor

The Arduino Version 1.4.8 works in the same way as with the Arduino Nano connect, which runs art 1.4.5. But using it with my standard set of test programs fails. socket.settimeout(n) responds with [Errno 128] ENOTCONN, option socket.SOL_SOCKET is not known, ... Pretty disappointing.
Which tests did you run?

@iabdalkader
Copy link
Contributor Author

The Arduino Version 1.4.8 works in the same way as with the Arduino Nano connect, which runs art 1.4.5.

Yes it may look the same but 1.4.8 fixes a very subtle issue that breaks internal socket state when used in a tight send/recv loop.

socket.settimeout(n) responds with [Errno 128] ENOTCONN, option socket.SOL_SOCKET is not known

The settimeout() function is implemented in my drivers/modules (it just sets a variable in the socket state) it shouldn't fail, but setsocketopt() is not, it just returns -1 the firmware doesn't provide that.

Which tests did you run?

I run standard basic tests, send/recv, http client, ntp etc...

@robert-hh
Copy link
Contributor

I made all further tests with 1.4.8.

Some of the trouble comes from trying to use a SOCK_RAW type socket, which seems to be supported by the lwip stack in the NINA firmware, but not in the driver. I tried timeout on a SOCK_DGRAM type socket, which so far worked fine on other ports. Maybe it was simply ignored.
Another unexpected behavior:

  • connect to a AP. That's e.g. done at startup in a script, which sets wlan.active(True)
  • use later wlan=network.WLAN(). That should return an object with the settings of the established wlan, but wlan.active() returns False.

@iabdalkader
Copy link
Contributor Author

SOCK_RAW type socket, which seems to be supported by the lwip stack in the NINA firmware

Yes lwip stack supports it, but does the firmware allow creating raw sockets ? I'd have to check, but if there's a command to create them we can add support for it, if needed.

That should return an object with the settings of the established wlan, but wlan.active() returns False.

I think this behavior is not documented anywhere (I could be wrong), the CYW driver works like that, but right now in my driver in make_new I reset the active flag, so this is intended. I could fix if it's the expected behavior.

I tried timeout on a SOCK_DGRAM type socket, which so far worked fine on other ports. Maybe it was simply ignored.

This is actually a bug, I can fix it, I return a different error number for time so upper layers fail to detect it, please open issues for this or any other bug/feature mention me and I may have some time to update the driver.

@robert-hh
Copy link
Contributor

RAW sockets can be used in all other ports with socket support.

I think this behavior is not documented anywhere (I could be wrong), the CYW driver works like that, but right now in my driver in make_new I reset the active flag, so this is intended. I could fix if it's the expected behavior.

The ESP drivers work that way as well. It may be documented there.

I will make further tests and report. Yesterday I had trouble with that litte script below for getting ntptime, which I use since early ESP8266 time on all ports. It fails at least at three steps:

  • line 20: getaddrinfo() fails, if the first argument is a symbolic name, not an IP address.
  • line 23: settimeout() on a DGRAM type socket (see above)
  • line 25: s.recv() fails, s.recvfrom() succeeds. Both work on other ports an should work.

I shortened the initial script a little bit, leaving out the part which sets the RTC. The error comes up when calling ntptime.time()


import socket
import struct

# (date(1970, 1, 1) - date(1900, 1, 1)).days * 24*60*60
NTP_DELTA = 2208985200

# The NTP host can be configured at runtime by doing: ntptime.host = 'myhost.org'
host = "pool.ntp.org"

def time():
    NTP_QUERY = bytearray(48)
    NTP_QUERY[0] = 0x1B
    addr = socket.getaddrinfo(host, 123)[0][-1]
    s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
    try:
        s.settimeout(1)
        res = s.sendto(NTP_QUERY, addr)
        msg = s.recv(48)
    finally:
        s.close()
    val = struct.unpack("!I", msg[40:44])[0]
    return val - NTP_DELTA

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Jan 4, 2022

RAW sockets can be used in all other ports with socket support.

Again to be very clear, in order for me to support RAW socks in this driver, it must be supported by the firmware running on the Nina/ESP32, which does Not seem to provide any commands to handle raw socks. I'd raise this issue with Arduino/Adafruit.

The ESP drivers work that way as well. It may be documented there.

I'm not sure if it's intended that WLAN maintains its state or they just assume you'd call it once, either way is fine by me. You can send a PR to fix that if you like and we'll discuss it there. This line just needs to be removed: https://github.com/micropython/micropython/blob/master/extmod/network_ninaw10.c#L66 and line 74 where autobind state is reset.

I had trouble with that litte script below for getting ntptime,

Try this script, it works flawlessly, and it uses a domain name with getaddrinfo(). As for the recv() issue, it should be used with TCP sockets because recv() and recvfrom() issue different commands to read/write from sockets depending on the socket type. Again this is something that's just forced on the driver by firmware design, we may be able to get around it by detecting the socket type, calling the right function, and returning the expected results to the user seamlessly. If you open an issue I'll look into it.

@robert-hh
Copy link
Contributor

Try this script, it works flawlessly, and it uses a domain name with getaddrinfo().

This script works only if the wifi connection happens within the script. If I take that out and connect in a separate script, it fails. So it is related to the other aspect of handling the state of the WiFi connection by instance and not global. In my opinion it is a global property. The need to connect in each module which wants to use WiFi seems strange. And, b.t.w. connecting more than once failed sometimes, sometimes it works. You can try by putting the content of your sample script into a function and calling that repeatedly.

As for recv() and recvfrom(). I looked up at various sources. Even if is seems suggested to use recvfrom() with UDP sockets, I found no place mandating it. Since recvfrom() works, it is more a matter of inconsistency between the various MicroPython ports.

@iabdalkader
Copy link
Contributor Author

Alright I'll send a PR to address all these issue.

@robert-hh
Copy link
Contributor

Thank you very much.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Jan 4, 2022

Keep in mind that my NTP script in a loop will fail anyway due to this issue: arduino/nina-fw#72

EDIT: The workaround is to not bind the socket at all, and just let the driver auto-bind it for you.

@robert-hh
Copy link
Contributor

Keep in mind that my NTP script in a loop will fail anyway due to this issue: arduino/nina-fw#72

That's fine. The connection to an AP should happen only once and should be usable until a disconnect.

@iabdalkader iabdalkader mentioned this pull request Jan 4, 2022
@iabdalkader
Copy link
Contributor Author

#8138

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Mar 13, 2023
document going directly to display with ondiskgif
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0