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

iface: do not require device and timestamp for multicast join/leave. #985

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Sep 11, 2024

Instead of eagerly sending the join/leave packet when the user calls join/leave,
we update internal state and send the packet when the interface is polled.

Advantages:

  • If the device is exhausted, the packet gets sent later instead of just failing and returning an error to the user.
  • Makes the API consistent with everything else in smoltcp: operations only update internal state, poll is what sends/receives packets.
  • Enables wrappers to offer simpler APIs with less generics. See net: refactor to simplify lifetimes/generics. embassy-rs/embassy#3329 for an example, which is my original motivation.

Comment on lines +85 to +94
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum MulticastGroupState {
/// Joining group, we have to send the join packet.
Joining,
/// We've already sent the join packet, we have nothing to do.
Joined,
/// We want to leave the group, we have to send a leave packet.
Leaving,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we move this to interface/igmp.rs?

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't work because multicast_groups exists if any(feature = "proto-igmp", feature = "proto-ipv6"), but mod igmp exists if feature = "proto-igmp" only. So it would break if you enable ipv6 but not igmp.

(fwiw I think these feature flags are a bit confusing. I'd like to refactor this so there's a single multicast Cargo feature that enables multicast for ipv4 and/or ipv6 depending on what you have enabled. Then rename mod igmp to mod multicast and move all multicast-related stuff there. I'd rather do this in a next PR tho)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

got nerdsniped, just did it in #988

Instead of eagerly sending the join/leave packet when the user calls join/leave,
we update internal state and send the packet when the interface is polled.

Advantages:
- If the device is exhausted, the packet gets sent later instead of just failing and returning an error to the user.
- Makes the API consistent with everything else in smoltcp: operations only update internal state, poll is what sends/receives packets.
- Enables wrappers to offer simpler APIs with less generics. See embassy-rs/embassy#3329 for an example, which is my original motivation.
@Dirbaio Dirbaio force-pushed the multicast-no-device branch from f0593e4 to 34b1fa5 Compare September 12, 2024 13:53
@Dirbaio
Copy link
Member Author

Dirbaio commented Sep 12, 2024

(pushed small update, removing MulticastError::Exhausted that's unused now)

@thvdveld thvdveld added this pull request to the merge queue Sep 12, 2024
Merged via the queue into main with commit 649bce4 Sep 12, 2024
9 checks passed
@thvdveld thvdveld deleted the multicast-no-device branch September 12, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants