extmod: Create a common cyw43 config header, apply mDNS fix in it. #17092

projectgus · 2025-04-08T02:01:48Z

Summary

First commit creates a new extmod/cyw43_config_common.h header with the common macros from ports which use CYW43 Wi-fi driver. There's more refactoring that could be done here (for example a cyw43_sdio_common.h header) but I've limited myself to macros that are the same, or that appear effectively the same, on every port.
Second commit moves the rp2 LWIP mDNS resolver fix added in ports/rp2: Fix rp2 mDNS issue via new cyw43 driver hooks. #17057 into the new header, so it's now also applied on stm32 and mimxrt ports which use the CYW43 driver.

Testing

Connected a PYBD_SF2 to my local network, verified I could look up the host via mDNS.
Ran the multi_wlan tests with a RPI_PICO_W board and a PYBD_SF2 as the two nodes. This uncovered two pre-existing issues (not added in this PR):
- The stm32 port logs DHCPS: updates when running as the AP, so the test fails on the additional output (at least easy to manually check for).
- On both ports, the cyw43 driver doesn't like being a STA first and then switching to become an AP (even though the unit tests de-activate the unused interface). To run the two tests in sequence then I needed to trigger a hard reset in between. Will investigate more when time permits.
I have not been able to test mimxrt as I have no suitable hardware. If I understand ports/mimxrt: Add CYW43 WiFi/BT support. #11397 correctly then no such widely available board exists, the only mimxrt+cyw43 users have added a CYW43 module to custom hardware or an eval board.

Trade-offs and Alternatives

Could keep the per-port config headers but it makes it harder to verify consistent behaviour between the different ports, and to apply non-port-specific changes like the mDNS resolver fix.

projectgus · 2025-04-08T02:02:26Z

@dpgeorge BTW I haven't forgotten our discussion about network_cyw43.h. I have a proposed refactor that covers this, but it turned out independent from this one.

projectgus · 2025-04-08T02:06:16Z

ports/rp2/cyw43_configport.h


 #define CYW43_INCLUDE_LEGACY_F1_OVERFLOW_WORKAROUND_VARIABLES (1)
 #define CYW43_WIFI_NVRAM_INCLUDE_FILE   "wifi_nvram_43439.h"
-#define CYW43_IOCTL_TIMEOUT_US          (1000000)
 #define CYW43_SLEEP_MAX                 (10)


I haven't moved this macro into the common header because it's 50 in the mimxrt & stm32 ports, and 10 here in rp2. I'm unsure if this difference is necessary or a historical accident.

It just controls how often the the cyw43 poll gets scheduled via pendsv (at least in stm32, mimxrt, alif). I'd say on a slower chip maybe 50 is better.

Oh! Perhaps just have the default at 50 and in RP2 undef and redefine it.

Right! What I'm unsure is whether it was set to 10 on rp2 to fix some problem (or improve performance), or whether it's simply an accident that it's been set to 10 here. I haven't done any tests to compare 50 vs 10 on rp2.

Oh! Perhaps just have the default at 50 and in RP2 undef and redefine it.

Good idea. I've gone even further and removed it from all the ports except rp2, as 50 is the default in the driver repo. I suspect it could also be set to 50 in the rp2 port without any impact (maybe even lower sleep consumption), but haven't tested this.

codecov · 2025-04-08T02:10:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.53%. Comparing base (e53f262) to head (2e687fe).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17092      +/-   ##
==========================================
- Coverage   98.54%   98.53%   -0.01%     
==========================================
  Files         169      169              
  Lines       21890    21890              
==========================================
- Hits        21572    21570       -2     
- Misses        318      320       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-04-08T02:14:19Z

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% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

iabdalkader · 2025-04-09T07:17:52Z

extmod/cyw43_config_common.h

+    }
+}
+
+#define CYW43_EVENT_POLL_HOOK mp_event_handle_nowait()


For stm32 and mimxrt this was MICROPY_EVENT_POLL_HOOK which does something different, especially in the stm32 port (see the old one in stm32 port). The same for the delay functions which called this macro. I know that eventually we'll use the new functions, but I'd suggest you leave this macro and the delay functions in the port config files for now, to avoid changing the behavior, unless it's time to remove this macro.

Oh good catch, I mis-remembered the macro compatibility as being different! Will fix.

I've swapped this around so stm32 should be using the same hook as before.

mimxrt should be using mostly the same hooks, although it previously had cyw43_delay_us and cyw43_delay_ms aliased directly to mp_hal_delay_us and mp_hal_delay_ms and now it uses the inline function definitions instead. This seems like either a no-change or a small improvement, but I'm not able to test it to verify.

iabdalkader · 2025-04-16T13:41:36Z

Note that the old cywbt.c driver will be replaced with a new BTHCI Uart backend that's been recently added to cyw43 driver, which requires some additional config option (no big changes, see #16848). The new Alif port already uses the new BTHCI backend, and it could be switch to the common config: https://github.com/micropython/micropython/blob/master/ports/alif/cyw43_configport.h

dpgeorge · 2025-04-22T03:16:26Z

Sorry @projectgus but this now needs rebasing after merging #16848.

Gadgetoid · 2025-04-22T08:36:27Z

Some possible overlap between this and d6cfdca from #16057 ?

projectgus · 2025-05-01T03:29:00Z

The new Alif port already uses the new BTHCI backend, and it could be switch to the common config

Sorry @projectgus but this now needs rebasing after merging #16848.

I've rebased on top the BTHCI changes, and migrated the Alif port to the common cyw43 config. All ports build after updating, but I only have hardware to test stm32 PYBD_SF2 (confirmed Wi-Fi & BLE both work but didn't run complete tests).

I think the BTHCI macros could also be put into the common config but have left it out of this PR for now, same as SDIO.

projectgus · 2025-05-01T03:29:27Z

Some possible overlap between this and d6cfdca from #16057 ?

I think some overlap, but as those changes are scoped to rp2 I think they're more or less a subset of these.

EDIT: Oops it was a while since I looked, I think that change is mostly breaking out the mpconfig layer of CYW43 config. Whereas this change is refactoring the driver-facing layer of CYW43 config (i.e. the layer that translates the mpconfig settings to the driver). So mostly orthogonal.

This is only a surface level refactor, some deeper refactoring would be possible with (for example) the SDIO interface in mimxrt and stm32, or the BTHCI interface which is is similar on supported ports. But sticking to cases where the macros are the same across all ports. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>

This means the fix from dd1465e will also apply to stm32 and mimxrt ports that use CYW43. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>

projectgus added port-stm32 extmod Relates to extmod/ directory in source port-mimxrt port-rp2 labels Apr 8, 2025

projectgus commented Apr 8, 2025

View reviewed changes

projectgus added this to the release-1.26.0 milestone Apr 8, 2025

iabdalkader reviewed Apr 9, 2025

View reviewed changes

projectgus force-pushed the refactor/cyw43_common_config branch from 83718b1 to df7dfc0 Compare April 15, 2025 01:35

projectgus force-pushed the refactor/cyw43_common_config branch 2 times, most recently from 12be4d6 to 09f64bb Compare May 1, 2025 02:24

projectgus requested review from iabdalkader and dpgeorge and removed request for iabdalkader May 1, 2025 06:33

projectgus added 2 commits May 2, 2025 12:33

rp2,extmod/cyw43: Move the LWIP responder fix into common CYW43 config.

2e687fe

This means the fix from dd1465e will also apply to stm32 and mimxrt ports that use CYW43. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>

projectgus force-pushed the refactor/cyw43_common_config branch from 09f64bb to 2e687fe Compare May 2, 2025 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extmod: Create a common cyw43 config header, apply mDNS fix in it. #17092

extmod: Create a common cyw43 config header, apply mDNS fix in it. #17092

projectgus commented Apr 8, 2025 •

edited

Loading

projectgus commented Apr 8, 2025

projectgus Apr 8, 2025

iabdalkader Apr 16, 2025

iabdalkader Apr 16, 2025

projectgus Apr 16, 2025

projectgus May 1, 2025

codecov bot commented Apr 8, 2025 •

edited

Loading

github-actions bot commented Apr 8, 2025

iabdalkader Apr 9, 2025

projectgus Apr 9, 2025

projectgus Apr 15, 2025 •

edited

Loading

iabdalkader commented Apr 16, 2025

dpgeorge commented Apr 22, 2025

Gadgetoid commented Apr 22, 2025

projectgus commented May 1, 2025

projectgus commented May 1, 2025 •

edited

Loading

extmod: Create a common cyw43 config header, apply mDNS fix in it. #17092

Are you sure you want to change the base?

extmod: Create a common cyw43 config header, apply mDNS fix in it. #17092

Conversation

projectgus commented Apr 8, 2025 • edited Loading

Summary

Testing

Trade-offs and Alternatives

projectgus commented Apr 8, 2025

projectgus Apr 8, 2025

Choose a reason for hiding this comment

iabdalkader Apr 16, 2025

Choose a reason for hiding this comment

iabdalkader Apr 16, 2025

Choose a reason for hiding this comment

projectgus Apr 16, 2025

Choose a reason for hiding this comment

projectgus May 1, 2025

Choose a reason for hiding this comment

codecov bot commented Apr 8, 2025 • edited Loading

Codecov Report

github-actions bot commented Apr 8, 2025

iabdalkader Apr 9, 2025

Choose a reason for hiding this comment

projectgus Apr 9, 2025

Choose a reason for hiding this comment

projectgus Apr 15, 2025 • edited Loading

Choose a reason for hiding this comment

iabdalkader commented Apr 16, 2025

dpgeorge commented Apr 22, 2025

Gadgetoid commented Apr 22, 2025

projectgus commented May 1, 2025

projectgus commented May 1, 2025 • edited Loading

projectgus commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 8, 2025 •

edited

Loading

projectgus Apr 15, 2025 •

edited

Loading

projectgus commented May 1, 2025 •

edited

Loading