Skip to content
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

src: mDns fixes. #30

Closed
wants to merge 1 commit into from
Closed

Conversation

peterharperuk
Copy link
Collaborator

The host name for mDns wasn't using CYW43_HOST_NAME. The mdns_resp_add_netif changed after lwip version 2.1.2. Avoid calling mdns_resp_remove_netif if it's not been added. Call mdns_resp_announce when STA connected.

Fixes #16 and #17

The host name for mDns wasn't using CYW43_HOST_NAME.
The mdns_resp_add_netif changed after lwip version 2.1.2.
Avoid calling mdns_resp_remove_netif if it's not been added.
Call mdns_resp_announce when STA connected.

Fixes georgerobotics#16 and georgerobotics#17
@lurch
Copy link
Contributor

lurch commented Sep 8, 2022

The mdns_resp_add_netif changed after lwip version 2.1.2

I don't think this is right?
https://github.com/lwip-tcpip/lwip/blob/STABLE-2_1_2_RELEASE/src/include/lwip/apps/mdns.h#L76 says

err_t mdns_resp_add_netif(struct netif *netif, const char *hostname, u32_t dns_ttl);

and https://github.com/lwip-tcpip/lwip/blob/STABLE-2_1_3_RELEASE/src/include/lwip/apps/mdns.h#L76 also says

err_t mdns_resp_add_netif(struct netif *netif, const char *hostname, u32_t dns_ttl);

It's only in https://github.com/lwip-tcpip/lwip/blob/master/src/include/lwip/apps/mdns.h#L110 where it changes to

err_t mdns_resp_add_netif(struct netif *netif, const char *hostname);

Looking at the branch names they have master and STABLE-2_1_x so perhaps the appropriate fix here is to check against LWIP_VERSION_MAJOR and LWIP_VERSION_MINOR from https://github.com/lwip-tcpip/lwip/blob/STABLE-2_1_x/src/include/lwip/init.h ? 🤷‍♂️

https://github.com/lwip-tcpip/lwip/blob/master/src/include/lwip/init.h seems to suggest that the current master will eventually become 2.2.x ?

@dpgeorge
Copy link

dpgeorge commented Sep 9, 2022

Thanks for the patch.

Honestly, I think it might be better to just remove all mDNS code from this driver and push that responsibility to the user of this driver. There are many ways that mDNS could be used/configured and it doesn't really make sense to add lots of configurability to this driver to support setting the mDNS in the way the application needs/wants.

We can instead have hooks/callbacks in this driver at various points (eg where the existing call to mdns_resp_add_netif is) to facilitate starting up lwIP (and other) components.

Would that work for pico-sdk? Pico-sdk could provide some convenience functions to enable mDNS, or the user could just do it all themselves.

Note also that there are subtly different functions to use to announce/restart mDNS (from the lwIP docs):

  • Call mdns_resp_announce() every time the IP address on the netif has changed.
  • Call mdns_resp_restart() every time the network interface comes up after being
    down, for example cable connected after being disconnected, administrative
    interface comes up after being down, or the device wakes up from sleep.

There's also the config option MDNS_RESP_USENETIF_EXTCALLBACK which seems very useful, it calls these announce/restart functions automatically. That's the kind of option the user could enable.

@peterharperuk
Copy link
Collaborator Author

Ok. I imagine it should be possible to do this in the sdk. I'll have to study it. Let me close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to specify hostname
3 participants