Skip to content

ports/rp2: Fix rp2 mdns responder issue. #16641

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 211 commits into from

Conversation

mseminatore
Copy link

@mseminatore mseminatore commented Jan 25, 2025

Summary

As a number of users have reported via issues and discussions, the RP2 port of MicroPython has an incomplete mDNS implementation. The code in main.c calls mdns_resp_init() which opens the UDP socket for mDNS. However, no code in the cyw43 driver of the RP2 port makes the proper calls to mdns_resp_add_netif() and mdns_resp_remove_netif() to send the announce packets. The wiznet5k driver does make these calls and was used as a model for these changes.

This PR attempts to address this by very small changes to the extmod/network_cyw43.c driver file. As far as I can tell this driver is currently used only by the RP2 port.

Testing

This was tested by building the firmware and deploying to a PicoW, starting a WLAN connection and pinging PicoW.local from several devices on the local network. I also tested changing the hostname from the default.

Trade-offs and Alternatives

Alternatively, if this PR is not accepted we should disable the partial implementation by setting LWIP_MDNS_RESPONDER to 0 in lwipopts.h so that users can implement their own mDNS solutions without E_ADDRINUSE failures. A user should not have to build their own firmware to disable a partial/broken mDNS solution.

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

Attention: Patch coverage is 99.22481% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.54%. Comparing base (a4ab847) to head (22f6cb0).
Report is 207 commits behind head on master.

Files with missing lines Patch % Lines
extmod/modtls_mbedtls.c 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #16641      +/-   ##
==========================================
- Coverage   98.58%   98.54%   -0.05%     
==========================================
  Files         167      169       +2     
  Lines       21596    21890     +294     
==========================================
+ Hits        21291    21571     +280     
- Misses        305      319      +14     

☔ 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.

Copy link

github-actions bot commented Jan 25, 2025

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

@mseminatore
Copy link
Author

@dpgeorge @omogenot @felixdoerre My apologies for the formatting issues. I am a first time contributor and am not certain what I did incorrectly.

@@ -96,8 +97,20 @@ static void network_cyw43_print(const mp_print_t *print, mp_obj_t self_in, mp_pr
static mp_obj_t network_cyw43_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args) {
mp_arg_check_num(n_args, n_kw, 0, 1, false);
if (n_args == 0 || mp_obj_get_int(args[0]) == MOD_NETWORK_STA_IF) {
#if LWIP_MDNS_RESPONDER
// NOTE: interface is removed in network_cyw43_deinit
Copy link
Member

Choose a reason for hiding this comment

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

You need to dedent these lines to line up with the # (or maybe you used tabs?).

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I've updated the PR to fix the dedent formatting issue.

@dpgeorge
Copy link
Member

Thanks for the contribution. Did you also see #16623? Maybe that actually fixes the same issue this PR is trying to fix.

@mseminatore
Copy link
Author

Thanks for the contribution. Did you also see #16623? Maybe that actually fixes the same issue this PR is trying to fix.

This PR is trying to address some of the issues mentioned in #15297, #10957 and #8946

I just looked at that PR. It appears that it only changes .h configuration for timers but does not add any missing mDNS functionality. The lwIP documentation seems clear that a call to mdns_resp_add_netif() is required for mDNS announce packets to be sent.

Looking over the code in extmod/network_wiznet5k.c it demonstrates the same calling pattern.

@tpwrules
Copy link
Contributor

A PR recently went in (#16011) to correct those calls for the wiznet5k driver. With that, plus the timer PR, mDNS seems to work well on RP2 using wiznet5k Ethernet.

I don't have wireless devices to test but this seems like a similar fix for a similar issue to the wiznet5k PR. There is some discussion in that PR about the calling pattern which might be a good reference; I have not cross-checked the API usage.

@mseminatore
Copy link
Author

mseminatore commented Jan 25, 2025

A PR recently went in (#16011) to correct those calls for the wiznet5k driver. With that, plus the timer PR, mDNS seems to work well on RP2 using wiznet5k Ethernet.

I don't have wireless devices to test but this seems like a similar fix for a similar issue to the wiznet5k PR. There is some discussion in that PR about the calling pattern which might be a good reference; I have not cross-checked the API usage.

Yes, I think it makes sense that the wiznet5K and cyw34 drivers for the RP2 should be similar. And that's why I tried to follow the pattern. I am new to the MicroPython codebase and it looks some time to peruse the code and get a feel for the patterns. Not certain my solution is 100% correct but it does function.

Regarding the timers, I do find that the RP2 port, at least for PicoW, sometimes gets into a bad state where the WiFi is not responding at startup. Maybe the timers resolve that?

@mseminatore
Copy link
Author

@dpgeorge @omogenot @felixdoerre My apologies for the incorrectly formatted commit messages. Would you prefer that I yank this PR and resubmit? Doing so will lose the comments on this PR.

@robert-hh
Copy link
Contributor

You do not have to close the PR. Just fix the commit message locally with git commit --amend in combination with git rebase -i commit-id and do a force push to your branch. Then, the PR will be updated.

The cyw43 driver in the RP2 port has an apparently
incomplete mDNS implementation. The code in main.c
calls mdns_resp_init() but there is no call to add
a responder for the NIC via mdns_resp_add_netif().

Signed-off-by: Mark Seminatore <nebula_peeps4t@icloud.com>
This checkin corrects the improper indentation to comply with the
coding standards.

Signed-off-by: Mark Seminatore <nebula_peeps4t@icloud.com>
@mseminatore
Copy link
Author

You do not have to close the PR. Just fix the commit message locally with git commit --amend in combination with git rebase -i commit-id and do a force push to your branch. Then, the PR will be updated.

Thanks, I did that and it appears to have worked.

@mseminatore mseminatore changed the title fix rp2 mdns responder issue ports/rp2: Fix rp2 mdns responder issue. Jan 28, 2025
@projectgus
Copy link
Contributor

I'm a little surprised this works, as the cyw43-driver only initialises the netif and adds it to LWIP in cyw43_cb_tcpip_init() (implemented in lib/cyw43-driver/src/cyw43_lwip.c), which is a callback when the low-level network interface goes "up".

At the time the object is constructed in Python, I don't think this has happened yet - so although the argument to mdns_resp_add_netif() points to the correct structure, its contents will be all zeroes (I think). In particular, the calls to add the interface to a multicast group for mDNS probably fail (unless I'm misunderstanding the context here). But maybe that doesn't really matter, if basic mDNS is working...

(If this is a problem, I think the only way around it with the current driver design would be to add/remove the interface to/from mdns inside the cyw43-driver callbacks instead of from micropython code. But maybe it's not actually a problem.)

@mseminatore
Copy link
Author

I'm a little surprised this works, as the cyw43-driver only initialises the netif and adds it to LWIP in cyw43_cb_tcpip_init() (implemented in lib/cyw43-driver/src/cyw43_lwip.c), which is a callback when the low-level network interface goes "up".

At the time the object is constructed in Python, I don't think this has happened yet - so although the argument to mdns_resp_add_netif() points to the correct structure, its contents will be all zeroes (I think). In particular, the calls to add the interface to a multicast group for mDNS probably fail (unless I'm misunderstanding the context here). But maybe that doesn't really matter, if basic mDNS is working...

(If this is a problem, I think the only way around it with the current driver design would be to add/remove the interface to/from mdns inside the cyw43-driver callbacks instead of from micropython code. But maybe it's not actually a problem.)

Thanks. I will write some test code to see what the netif state is at the time of the mdns_resp_add_netif() call. If netif is uninitialized, then perhaps lwIP defaults to the default_netif? I will have a look at the lwIP code to see how it behaves and what validation it performs.

That said, in my testing the mDNS for the device was properly advertised to my local network as PicoW.local and it was responding properly to pings.

@projectgus
Copy link
Contributor

projectgus commented Jan 29, 2025

Sounds great @mseminatore, sorry I posted that concern without doing any follow-up myself.

The part that I suspect won't work is sending the announcement to add the interface to the IGMP group (for IPv4) or MLD (for IPv6). This probably won't matter when everything is on a single network with no routing, but it may matter if the mDNS multicast group is also routed between networks. That said, having mDNS work on a single local network is probably what most people are looking for so it might be fine to simply ignore this.

(That's assuming nothing else gets corrupted by adding an incomplete netif to lwip mdns. I don't know about this part.)

@mseminatore
Copy link
Author

Sounds great @mseminatore, sorry I posted that concern without doing any follow-up myself.

The part that I suspect won't work is sending the announcement to add the interface to the IGMP group (for IPv4) or MLD (for IPv6). This probably won't matter when everything is on a single network with no routing, but it may matter if the mDNS multicast group is also routed between networks. That said, having mDNS work on a single local network is probably what most people are looking for so it might be fine to simply ignore this.

(That's assuming nothing else gets corrupted by adding an incomplete netif to lwip mdns. I don't know about this part.)

@projectgus let me know if you'd prefer to see the approach you called out in #15297 instead.

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2025

I also have similar concerns as @projectgus that mDNS needs to be started after the netif is configured.

There are three ways we could do that:

  1. Modify cyw43-driver to init/deinit the mDNS itself in the correct location, as @projectgus suggested.
  2. Modify cyw43-driver to add callback hooks after a netif_add and before a netif_remove so that users of this library can do whatever they need at that point. Then in this repo, implement those callback hooks to init/deinit mDNS.
  3. Do the init here but put it in network_cyw43_active(): before the call to cyw43_wifi_set_up() one would need to deinit mDNS, then after the call to cyw43_wifi_set_up()` one would init mDNS (but only in the case that it was setting the interface up when it was previously down).

Option (3) only requires changes in this repo but it's a little tricky to get right. I think option (2) is the right way forward.

@mseminatore
Copy link
Author

Thanks for the review @dpgeorge I agree that option 2 seems the most robust solution. Do you want me to draft a PR for option 2 or should someone else do that? Happy to pursue either path.

If you'd like me to draft a cyw43 lib PR can you let me know whether you'd prefer to see support for a single callback, a list of callbacks, or chainable callbacks?

Thanks!

@andrewleech
Copy link
Contributor

On a related note, not meaning to make things more complicated but I think there should also be a call made to

    #if LWIP_MDNS_RESPONDER
    mdns_resp_netif_settings_changed(&netif);
    #endif

any time an ip address is changed, eg in extmod/network_lwip.c in ipconfig/ifconfig and any time the ip is set/changed by the dhcp client.

As I write this I realise it's really a new feature request which should probably be tracked in its own issue? I'll write one up if preferred.

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2025

@mseminatore I made a PR for cyw43-driver here: georgerobotics/cyw43-driver#138

Hopefully that's enough to do what you need with mDNS. If you could test that out with the required mDNS fixes, that would be great!

@dpgeorge
Copy link
Member

dpgeorge commented Mar 3, 2025

On a related note, not meaning to make things more complicated but I think there should also be a call made to
mdns_resp_netif_settings_changed

I don't think we need to do this because:

/**
 * @ingroup mdns
 * Announce IP settings have changed on netif.
 * Call this in your callback registered by netif_set_status_callback().
 * No need to call this function when LWIP_NETIF_EXT_STATUS_CALLBACK==1,
 * this handled automatically for you.
 * @param netif The network interface where settings have changed.
 */
#define mdns_resp_netif_settings_changed(netif) mdns_resp_announce(netif)

and we have LWIP_NETIF_EXT_STATUS_CALLBACK enabled.

@dpgeorge
Copy link
Member

I tested this PR as-is, and it doesn't work for me. The problem does look exactly like what @projectgus mentioned above, that the IGMP (multicast) mDNS group is not joined because the netif is not ready when mDNS is started on it.

@mseminatore will you get a chance to update this PR using the new hooks in cyw43-driver?

@dpgeorge
Copy link
Member

The original (first version) cyw43-driver did actually handle mDNS correctly, see 7b70ab7 . It called mdns_resp_add_netif and mdns_resp_remove_netif in precisely the locations that georgerobotics/cyw43-driver#138 added hooks for.

Gadgetoid and others added 25 commits March 31, 2025 13:45
When PICO_DEFAULT_I2C is not set require an I2C bus ID instead of
using -1 as a default, which would fail with a cryptic:

    "I2C(-1) doesn't exist"

Signed-off-by: Phil Howard <github@gadgetoid.com>
If the "spi_id" arg is not supplied and then the board default specified by
PICO_DEFAULT_SPI will be used.

Signed-off-by: Phil Howard <github@gadgetoid.com>
It's common with write-only SPI displays for MISO to be repurposed as a
register select or data/command pin.

While that was possible by setting up the pin after a call to
`machine.SPI()` this change makes `machine.SPI(miso=None)` explicit.

Signed-off-by: Phil Howard <github@gadgetoid.com>
There's a very odd but predictable sequence of events that breaks Wi-Fi
when using both cores:

1) CPU1 calls pendsv_suspend() - for example sleep() causes
   a softtimer node to be inserted, which calls pendsv_suspend().
2) CYW43 sends wakeup IRQ. CPU0 GPIO IRQ handler schedules PendSV
   and disables the GPIO IRQ on CPU0, to re-enable after
   cyw43_poll() runs and completes.
3) CPU0 PendSV_Handler runs, sees pendsv is suspended, exits.
4) CPU1 calls pendsv_resume() and pendsv_resume() sees PendSV
   is pending and triggers it on CPU1.
5) CPU1 runs PendSV_Handler, runs cyw43_poll(), and at the end
   it re-enables the IRQ *but now on CPU1*.

However CPU1 has GPIO IRQs disabled, so the CYW43 interrupt never runs
again...

The fix in this commit is to always enable/disable the interrupt on CPU0.
This isn't supported by the pico-sdk, but it is supported by the hardware.

Fixes issue micropython#16779.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
A better indication of whether a cyw43 event is pending is the actual flag
in the PendSV handler table. (If this fails, could also use the GPIO
interrupt enabled register bit).

This commit was needed of a previous version of the fix in the parent
commit, but it turned out not strictly necessary for the current version.
However, it's still a good clean up.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Changes:
- Move setting of PendSV priority to pendsv_init().
- Call pendsv_init() from CPU1 as well, to ensure priority is the same.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
The buffer was be reset on every call to uart.init().  If no sizes were
given, the buffer was set to the default size 256.  That made problems e.g.
with PPP.

This commit fixes it, keeping the buffer size if not deliberately changed
and allocating new buffers only if the size was changed.

Signed-off-by: robert-hh <robert@hammelrath.com>
The buffer was be reset on every call to uart.init().  If no sizes were
given, the buffer was set to the default size 256.  That made problems
e.g. with PPP.

This commit fixes it, keeping the buffer size if not deliberately changed
and allocating new buffers only if the size was changed.  Cater for changes
of the bits value, which requires a change to the buffer size.

Signed-off-by: robert-hh <robert@hammelrath.com>
Signed-off-by: robert-hh <robert@hammelrath.com>
This is a pretty fundamental built-in and having CPython-compatible
behaviour is beneficial.  The code size increase is not much, and
ports/boards can still disable it if needed to save space.

Addresses issue micropython#5384.

Signed-off-by: Damien George <damien@micropython.org>
Because 2-arg `next()` is implemented, and now enabled at the basic feature
level.

Signed-off-by: Damien George <damien@micropython.org>
Addresses some TODOs in this file.

Signed-off-by: Damien George <damien@micropython.org>
Allocation of a large compression window may fail, and in that case keep
the `DeflateIO` state consistent so its other methods (such as `close()`)
still work.  Consistency is kept by only updating the `self->write` member
if the window allocation succeeds.

Thanks to @jimmo for finding the bug.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
This extends the existing `vfs.mount()` function to accept zero arguments,
in which case it returns a list of tuples of mounted filesystem objects
and their mount location.

Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
Signed-off-by: Anson Mansfield <amansfield@mantaro.com>
This is needed if you chdir to a ROMFS and want to query your current
directory.

Prior to this change, using `os.getcwd()` when in a ROMFS would raise:

    AttributeError: 'VfsRom' object has no attribute 'getcwd'

Signed-off-by: Damien George <damien@micropython.org>
If a ROMFS is mounted then "/rom/lib" is usually in `sys.path` before the
writable filesystem's "lib" entry.  The ROMFS directory cannot be installed
to, so skip it if found.

Signed-off-by: Damien George <damien@micropython.org>
Rather than having Make calling CMake to generate a list of submodules and
then run a Make target (which is complex and prone to masking other
errors), implement the submodule update logic in CMake itself.

Internal CMake-side changes are that GIT_SUBMODULES is now a CMake list,
and the trigger variable name is changed from ECHO_SUBMODULES to
UPDATE_SUBMODULES.

The run is otherwise 100% a normal CMake run now, so most of the other
special casing can be removed.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
This prevents printing the lengthy command and makes the build output a
little cleaner.

Signed-off-by: Damien George <damien@micropython.org>
This prevents printing the lengthy command and makes the build output a
little cleaner.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Mark Seminatore <nebula_peeps4t@icloud.com>
Signed-off-by: Mark Seminatore <nebula_peeps4t@icloud.com>
@mseminatore
Copy link
Author

@dpgeorge If OTOH you are OK with this PR as it currently stands that is fine as well. The other PR is the same code changes sans the path of messy checkins and improperly formatted log messages.

@mseminatore
Copy link
Author

See #17057 for new, cleaner version of this PR

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.