8000 wiznet: Reset mDNS when interface is brought up. by greezybacon · Pull Request #16011 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

wiznet: Reset mDNS when interface is brought up. #16011

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

Merged

Conversation

greezybacon
Copy link
Contributor

Summary

The LwIP interface is removed in wiznet5k_deinit() which is called as part of the init sequence. Therefore, if using mDNS, then the interface will need to be re-added when bringing the interface up.

Additionally, this allows to set the hostname from Micropython code prior to bringing the interface up and mDNS responding to the (new) hostname. This allows the hostname to be configured and saved on the flash or be based on dynamic information such as the MAC or unique_id().

Testing

This was tested with the W5100S_EVB_PICO board.

Trade-offs and Alternatives

@greezybacon greezybacon force-pushed the fix/wiznet-lwip-hostname-on-reset branch from 6f44da4 to e3a68f9 Compare October 14, 2024 15:47
Copy link

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

Copy link
codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (3f54e5d) to head (078ead2).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #16011   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21341    21341           
=======================================
  Hits        21036    21036           
  Misses        305      305           

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

@tpwrules
Copy link
Contributor

Does mdns_resp_remove_netif need to be called at any point? I think it should be called in wiznet5k_deinit, otherwise it will be initialized twice and leak things. I have had it set up like that for a bit and everything seems to be working.

@tpwrules
Copy link
Contributor

This is my patch on top of 1.23.0 for reference:

diff --git a/extmod/network_wiznet5k.c b/extmod/network_wiznet5k.c
index 1eaabe9eb..5dc1e93e8 100644
--- a/extmod/network_wiznet5k.c
+++ b/extmod/network_wiznet5k.c
@@ -61,6 +61,7 @@
 #include "lwip/err.h"
 #include "lwip/dns.h"
 #include "lwip/dhcp.h"
+#include "lwip/apps/mdns.h"
 #include "netif/etharp.h"
 
 #define TRACE_ETH_TX (0x0002)
@@ -198,6 +199,10 @@ static void wiznet5k_config_interrupt(bool enabled) {
 }
 
 void wiznet5k_deinit(void) {
+#if LWIP_MDNS_RESPONDER
+    mdns_resp_remove_netif(&wiznet5k_obj.netif);
+#endif
+
     for (struct netif *netif = netif_list; netif != NULL; netif = netif->next) {
         if (netif == &wiznet5k_obj.netif) {
             netif_remove(netif);
@@ -243,6 +248,11 @@ static void wiznet5k_init(void) {
 
     // register with network module
     mod_network_register_nic(&wiznet5k_obj);
+
+#if LWIP_MDNS_RESPONDER
+    mdns_resp_add_netif(&wiznet5k_obj.netif, mod_network_hostname_data, 60);
+    // no need to call anything further as LWIP_NETIF_EXT_STATUS_CALLBACK == 1
+#endif
 }
 
 static void wiznet5k_send_ethernet(wiznet5k_obj_t *self, size_t len, const uint8_t *buf) {

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Oct 16, 2024
@greezybacon greezybacon force-pushed the fix/wiznet-lwip-hostname-on-reset branch from e3a68f9 to eae17a5 Compare October 21, 2024 14:08
@greezybacon
Copy link
Contributor Author
greezybacon commented Oct 21, 2024

@tpwrules thank you for your review. I really appreciate it.

I added the call to remove it as you suggested.

@greezybacon greezybacon force-pushed the fix/wiznet-lwip-hostname-on-reset branch from eae17a5 to e8d9562 Compare October 21, 2024 14:13
@@ -199,6 +200,10 @@ static void wiznet5k_config_interrupt(bool enabled) {
}

void wiznet5k_deinit(void) {
#if LWIP_MDNS_RESPONDER
mdns_resp_remove_netif(&wiznet5k_obj.netif);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be moved down to just before the netif_remove(netif) line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, @dpgeorge. I moved it as requested.

@greezybacon greezybacon force-pushed the fix/wiznet-lwip-hostname-on-reset branch from e8d9562 to 05da4c4 Compare October 22, 2024 12:42
The LwIP interface is removed in wiznet5k_deinit() which is called as part
of the init sequence.  Therefore, if using mDNS, then the interface will
need to be re-added when bringing the interface up.

Additionally, this allows to set the hostname from MicroPython code prior
to bringing the interface up and mDNS responding to the (new) hostname.
This allows the hostname to be configured and saved on the flash or be
based on dynamic information such as the MAC or unique_id().

Signed-off-by: Jared Hancock <jared.hancock@centeredsolutions.com>
@dpgeorge dpgeorge force-pushed the fix/wiznet-lwip-hostname-on-reset branch from 05da4c4 to 078ead2 Compare October 23, 2024 05:30
@dpgeorge dpgeorge merged commit 078ead2 into micropython:master Oct 23, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0