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: Implement LWIP's IGMP MAC filter callback. #25

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

jimmo
Copy link
Contributor

@jimmo jimmo commented Aug 26, 2022

This is used to register the multicast group address on the wifi
interface's filter, allowing frames to that group to be received.

This only applies to v4. See #24 for the equivalent v6 change.

Fixes micropython/micropython#9105

@jimmo
Copy link
Contributor Author

jimmo commented Aug 26, 2022

Note:

  • Unsure if the cyw43_delay_ms(50); is needed at the end of cyw43_ll_wifi_update_multicast_filter.
  • LWIP manages the group memberships, so in theory cyw43_ll_wifi_update_multicast_filter doesn't need to validate if a group is already registered. It's not much code though to ensure that they don't get out of sync.

@felixdoerre
Copy link
Contributor

this change and #24 have quite a bit in common. I've split my changes into two commits, and I think 2baa5c5 can be used directly here, I think we should agree on a common interface to set the mac addresses, so mcast registered for v4 does not conflict with addresses registered for v6. I left out the 50ms delay and it worked fine on my board, but that might be something that someone more familiar with the chip can decide. You also removed the hardcoded mcast-address for mdns. I am not sure if that is needed, but I imagine it was in there for a reason.

@jimmo
Copy link
Contributor Author

jimmo commented Aug 26, 2022

You also removed the hardcoded mcast-address for mdns. I am not sure if that is needed, but I imagine it was in there for a reason.

LWIP invokes the callback with the mdns group address when the mdns hostname is configured. The existing hard coded address was a workaround for not implementing this callback.

I think 2baa5c5 can be used directly here, I think we should agree on a common interface to set the mac addresses, so mcast registered for v4 does not conflict with addresses registered for v6.

This PR should replace that commit. It provides exactly the same functionality just without needing the extra 60 bytes of RAM to manage the list.

And to my first note above, using the same method for v6 is a good reason to keep the code to track add duplicate detection in cyw43_ll, as I think LWIP can't do this across v4&v6.

@felixdoerre
Copy link
Contributor

sounds good, I'll rebase the v6 PR on this one.

@peterharperuk
Copy link
Collaborator

This change is now in #24

This is used to register the multicast group address on the wifi
interface's filter, allowing frames to that group to be received.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@dpgeorge dpgeorge merged commit aae9939 into georgerobotics:main Dec 9, 2022
@dpgeorge
Copy link

dpgeorge commented Dec 9, 2022

Thanks, this is a good improvement, definitely needed!

@@ -100,13 +101,29 @@ STATIC err_t cyw43_netif_output(struct netif *netif, struct pbuf *p) {
return ERR_OK;
}

STATIC err_t cyw43_netif_update_igmp_mac_filter(struct netif *netif, const ip4_addr_t *group, enum netif_mac_filter_action action) {
Copy link

Choose a reason for hiding this comment

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

this was originally cyw32; fixed during merged

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.

Pico W: Joining multicast group doesn't receive messages.
4 participants