georgerobotics / cyw43-driver

Other
79 stars 42 forks source link

src: mDns fixes. #30

Closed peterharperuk closed 2 years ago

peterharperuk commented 2 years ago

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

lurch commented 2 years ago

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 ? :man_shrugging:

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 commented 2 years ago

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):

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 commented 2 years ago

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.