Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Codecov Report

Merging #11219 (0f18337) into master (00855ee) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11219   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         158      158           
  Lines       20900    20900           
=======================================
  Hits        20563    20563           
  Misses        337      337           

@dpgeorge I hope I have implemented all of your suggestions right. The last commit tries to add variant support. Although it's SSL which weighs a lot.

@dpgeorge I reordered the commits into generic and samd-specific ones.

Rebased to the actual master with external flash support. There is just one variant now, wlan. At boards with external flash that supports SSL, using mbtls. At boards without external flash no SSL. SAMD21 boards with external flash could be supported for WiFi without SSL, but it is not enabled yet. Only the pins are defined for two boards.

Just a rebase and update after modules lost their "u".

Did you try the version with IRQ both at dataready and handshake?

Yes and with same results more or less, enabling IRQ on both pins at this point just results in more unnecessary polling.

OK, For MIMXRT and SAMD it makes a big difference, so I keep it there. The amount of unneccessary polling seems minor. But there are sequence where several SPI tansfers happen with dataready low. I kept the periodic polling at 128ms, since for any immediate response polling is triggered by the IRQ. I do not know if you changed the polling time.

#12490

Those are all of the changes I have here #12490

But there are sequence where several SPI tansfers happen with dataready low

If there's anything to read DATAREADY will be high already and spi_transfer will reschedule the poll as long as it's high.

With all relevant changes from #12490 to esp_hosted_hal.c applied, the figures are the same as with the dual IRQ, under the varying environmental situation. The traffic pattern at the wires looks comparable. I did not lower the polling rate.

I see that you still have support for a WiFi LED, if enabled.

With all relevant changes from #12490 to esp_hosted_hal.c applied, the figures are the same as with the dual IRQ,

Yes it should be the same, but this doesn't require configuring the second pin. I think this is the max it can do, I'm running the SPI clock at 25MHz, unless there's something to optimize at the firmware side.

I see that you still have support for a WiFi LED, if enabled.

Yes it's a very good indicator, it blinks much faster when running iperf, also the IRQ pin is now configured in board config files.

Updated the comparison of the boards.

After some testing I made a small change to esp_hosted_hal_spi_transfer(), which improved ping reliability. The changes consists of increasing the second delay after setting CS to 1 before the repeated poll from 10µs to 100µs, at least for SAMD51. Other boards may works with smaller values. Before, there were about 2% ping events lost. After, I had not drop in several thousand attempts. The total performance dropped, if at all, just a little bit.

    mp_hal_delay_us(100);  // Increased this one

    if (esp_hosted_hal_data_ready()) {
        void mod_network_poll_events(void);
        mod_network_poll_events();
    }

Yes I noticed it needs a small delay when doing repeated transfers, but I feel those minor fixes here and there will only get us so far, there's a bottleneck somewhere that's capping the TCP performance.

This change is not about speed, it is about reliability. In my local LAN, ping should always succeed with a small variation of response times. That's what troubled me. The other figures are fine.

10µs delay: Ping times ~10ms to sometimes >500ms, ~2% loss.
100µs delay: Ping times ~10ms to max ~30ms, 0 % loss.

No problem, I can add it to the PR.

On a related topic: reducing the wait time in the polling loops of nina_bsp_spi_slave_select() for the NINAW10 solution from 1ms to e.g. 100µs nearly doubles the speed and reduces average ping times from 40ms to 20ms. Still a little bit slower than the esp_hosted solution for TCP, but not much. Much slower for UDP.

On a related topic: reducing the wait time in the polling loops of nina_bsp_spi_slave_select() for the NINAW10 solution from 1ms to e.g. 100µs

Feel free to fix it in your PR 👍🏼

Of course. I'm still not sure whether I should keep the new NINAW10 interface or just require the wiring to be fixed in the respective mpconfigboard.h files.

Feel free to fix it in your PR 👍🏼

I did, but changed the approach using the esp_hosted style of esp_hosted_hal.c, by declaring the functions in nina_wifi_bsp.c as MP_WEAK and allowing a port to replace them. So in the template, the delay stays at 1ms, which works better for the arduino_nano_2040, and for SAMD it's reduced to 100us. That file implements as well the SAMD-specific set-up for SPI. I dropped the generic network.NINAW10 interface.

Those are good changes, thank you!

BTW was able to get a higher throughput by increasing lwip's memory (SND_BUF etc..) but it takes a lot of memory:

CLIENT MODE: TCP sending
Connecting to ('192.168.50.219', 5201)
Interval           Transfer     Bitrate
  0.00-1.00   sec   354 KBytes  2.90 Mbits/sec
  1.00-2.04   sec   421 KBytes  3.33 Mbits/sec
  2.04-3.04   sec   369 KBytes  3.02 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
  0.00-10.00  sec  3.94 MBytes  3.30 Mbits/sec  sender
  0.00-5.14   sec  3.94 MBytes  6.43 Mbits/sec  receiver
CLIENT MODE: UDP sending
Connecting to ('192.168.50.219', 5201)
Interval           Transfer     Bitrate         Total Datagrams
  0.00-1.00   sec   528 KBytes  4.32 Mbits/sec  371
  1.00-2.00   sec   530 KBytes  4.33 Mbits/sec  372
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
  0.00-10.00  sec  4.85 MBytes  4.07 Mbits/sec  3487  sender
  0.00-5.45   sec  4.85 MBytes  7.46 Mbits/sec  3487  receiver

The thing to note is, seems the cap is ~8 Mbits/sec.

I know that the LWIP settings are a dial to turn, but the increase is slow. You spend a lot of memory for a little bit higher speed. For the 8Mbit limit you should look into the net SPI transfer pattern. At the SAMD51 I see a short break between blocks of each 3 bytes, reducing already the net SPI transfer speed e.g. from 8Mbit to ~5.5MBit.

You spend a lot of memory for a little bit higher speed

Actually I see significant throughput increase with bigger SND_BUF + MTU etc.. even with CYW43, the defaults in MicroPython are too low and cause a lot of fragmentation (I guess the assumption is you wouldn't be sending large packets normally like iperf does).

BTW I also tested CYW43/RPI Pico_W (SPI mode) with the same LWIP config I use in my PR, and receiving is very good:

CLIENT MODE: TCP sending
Connecting to ('192.168.50.219', 5201)
Interval           Transfer     Bitrate
  0.00-10.00  sec  13.3 MBytes  11.1 Mbits/sec  sender
  0.00-10.25  sec  13.3 MBytes  10.9 Mbits/sec  receiver
CLIENT MODE: UDP sending
Connecting to ('192.168.50.219', 5201)
Interval           Transfer     Bitrate         Total Datagrams
  0.00-10.00  sec  10.6 MBytes  8.86 Mbits/sec  7598  sender
  0.00-10.50  sec  10.6 MBytes  8.44 Mbits/sec  7598  receiver 

For some reason the esp_hosted RX is always half the TX.

For some reason the esp_hosted RX is always half the TX.

From what I observed, it's a matter of the MCU type. For SAMD and MIMXRT I have ports with both NINAW10 and esp_hosted. With SAMD, RX is the same as TX, with MIMXRT RX is half of TX, no matter whether the NINA or esp_hosted variant runs. I do not know why.
Edit: At the ARDUINO_NANO_CONNECT_2040 board, RX and TX is the same as well.

Good Morning Ibrahim. Since your changes to esp_hosted are now merged, I try to pull up the respective SAMD and MIMXRT branches. There seems to be a problem with calling esp_hosted_hal_deinit(), which is called with esp_hosted_hal_init(), but not on a soft reset, unless bluetooth was enabled. But calling esp_hosted_hal_deinit() is required to call esp_hosted_wifi_deinit(). I used to have that in main.c, but it is not called in the C33 main.c.

Edit: b.t.w. At the end of your esp_hosted_hal.c, it still reads: #endif // MICROPY_PY_NETWORK_NINAW10

I'm not sure I fully understand the issue, in C33 WiFi is not deinitialized on soft-reset, unless Bluetooth was used, in that case mp_bluetooth_deinit calls hci_deinit, which is fine because you can't be using both BT + WiFi at the same time. If WiFi is used it's fine to leave it initialized (in fact I wanted that because it's faster this way).

BTW, did you manage to build the esp_hosted firmware ? If you did, can you please share steps to building the firmware ? I saw some recent changes that may improve the performance, but also wanted to try to increase SPI TX/RX buffers see if it helps. I have IDF 5+ and tried to build it, but I think our sdkconfig.defaults is not actually a defaults file, and the build just keeps reverting to default sdkconfig (with esp32 target).

I have to disable WiFi, because otherwise there are some dangling net events which cannot be handled since SPI is gone. No problem.

I have indeed built the esp_hosted firmware to change the pins for the interface signals. I went through the IDF-config session using idp.py menuconfig and ended up after a few attempts with a working sdkconfig. The default files did not make a lot to it. Attached is my sdkconfig. It's for a generic ESP32. Maybe it helps.

sdkconfig.zip

Will give it a try, thank you!

b.t.w: I made a small change to mp_bluetooth_hci_controller_init() to shorten the BLE start-up time. Nothing urgen or ciritcal t, so it can wait until the support for the respective port is merged, or if there are anyhow changes to be made.

diff --git a/drivers/esp-hosted/esp_hosted_bthci.c b/drivers/esp-hosted/esp_hosted_bthci.c
index 003054460..25876f039 100644
--- a/drivers/esp-hosted/esp_hosted_bthci.c
+++ b/drivers/esp-hosted/esp_hosted_bthci.c
@@ -122,9 +122,10 @@ int mp_bluetooth_hci_controller_init(void) {
 
     mp_uint_t start = mp_hal_ticks_ms();
     // Skip bootloader messages.
-    while ((mp_hal_ticks_ms() - start) < 2500) {
+    while ((mp_hal_ticks_ms() - start) < 500) {
         if (mp_bluetooth_hci_uart_any()) {
             mp_bluetooth_hci_uart_readchar();
+            start = mp_hal_ticks_ms();
         }
         MICROPY_EVENT_POLL_HOOK
     }

Actually I think this was required because the bootloader's verbose debugging is enabled (at least in the firmware that ships with C33), which is very annoying.

The esp_hosted variant works very well now. With the combination of a Teensy 4.0 and a Adafruit ESP32 breakout board I get with iperf3 3.4 MBit/s for TCP and 6.2 MBit/s for UDP, with SPI at 8Mbit/s!
And the BLE tests also perform well.

That's great news, close to the official benchmarks on ESP32.

I have a question about the macro MICROPY_HW_ESP_HOSTED_SHARED_PINS. Which signals are assumed to be shared when it's defined as 1?

The CS pin, you asked for this pin to not be changed on deinit.