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

Add the up/down Device and Interface methods #502

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dlrobertson
Copy link
Collaborator

Add up/down methods to Device and Interface for placing the underlying
device in a up or down state. This interface will also be handy when
adding support for MLDv2.

Copy link
Collaborator Author

@dlrobertson dlrobertson left a comment

Choose a reason for hiding this comment

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

I was lazy and combined some compile errors (utils.rs) and some non-matching indents in the feature commit.

@@ -43,7 +43,7 @@ mod mock {
mod mock {
use std::sync::Arc;
use std::sync::atomic::{Ordering, AtomicUsize};
use smoltcp::time::{Duration, Instant};
use smoltcp::time::{Duration, Instant};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just an indent fix.

@@ -20,7 +20,7 @@ mod utils {
mod mock {
use std::sync::Arc;
use std::sync::atomic::{Ordering, AtomicUsize};
use smoltcp::time::{Duration, Instant};
use smoltcp::time::{Duration, Instant};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same.

@Dirbaio
Copy link
Member

Dirbaio commented Jun 27, 2021

Thanks for the PR! What's the intended use case?

It seems the new functionality is not yet used for anything, method calls are threaded through and exposed to the user. You can call custom methods on your particular Device with iface.device_mut().up() without needing to add them to the Device trait.

IMO we should add methods to Device when smoltcp itself needs them. Does MLDv2 require up/down?

@dlrobertson
Copy link
Collaborator Author

Does MLDv2 require up/down?

Not explicitly, but there will be requests that we might want to send when we start/stop.

It seems the new functionality is not yet used for anything, method calls are threaded through and exposed to the user. You can
call custom methods on your particular Device with iface.device_mut().up() without needing to add them to the Device trait.

IMO we should add methods to Device when smoltcp itself needs them. Does MLDv2 require up/down?

Yeah, currently this isn't really much different than iface.device_mut.up(). You're 100% right about that. This doesn't have to complicate any existing device implementations, but is something that users would likely have to hand implement. I can't think of a use case where you would not want to implement a up/down method for your device.

Add up/down methods to Device and Interface for placing the underlying
device in a up or down state. This interface will also be handy when
adding support for MLDv2.

Signed-off-by: Dan Robertson <[email protected]>
@Dirbaio
Copy link
Member

Dirbaio commented Sep 8, 2021

Thinking about it, there are 2 related but different things we want:

  • Actively set the device in an up/down state.
  • Ask whether the device is up/down.

A device may come up/down "on its own". For example, an Ethernet being plugged/unplugged, WiFi connecting/disconnecting.... Or it may come up/down due to us manually setting it up/down. I imagine an Ethernet driver may report as "up" when we've set it as up AND there's a cable plugged in.

so perhaps all smoltcp needs is the second? Just an fn is_up(&self) -> bool in the trait. This would be useful for DHCP, MLD, IGMP and others to know when the network comes up, to react accordingly.

The first can be done with device-specific methods, calling them through iface.device_mut(). Smoltcp will never need to call them, just the user, so it makes sense for them not to be in the trait.

@korken89
Copy link
Contributor

We discussed yesterday in the chat about the DHCP always needing 5s.
It turned out to be that smoltcp tried to do DHCP DISCOVER while the link was down, and the 5s came from the retry cooldown.

Something like this would give smoltcp opportunity to act accordingly to the link state, as @Dirbaio pointed out.
As it is not the user need to track the up/down state of the link and reset DHCP when the link goes down -> up.

Not sure if it's smoltcp's job to track this though, I'm not that familiar with its design.

@Dirbaio Dirbaio marked this pull request as draft May 19, 2022 16:46
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.

3 participants