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

Expose IEEE802.15.4 address in Driver #1704

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Conversation

rubdos
Copy link
Contributor

@rubdos rubdos commented Jul 28, 2023

(I'm sorry for the PR-spam. I'm trying to get the "finished" things we have cleaned up, atomic and mergeable.)

This allows a net::Driver to be an IEEE802.15.4 device.

I wrote this in two stages.

  1. Added ieee802154_address() to the trait
  2. Merged ethernet_address() and ieee802154_address into hardware_address().

This allows the community (mainly @Dirbaio, cc @xoviat) to decide which way is better. HardwareAddress means we have a bunch of match-statements at runtime, for a property that's typically known at compile time (but that's the case anyway with smoltcp, currently), but it's a lot more compact and future proof. It's a (big-ish) breaking change, however.

CI will probably complain, but I'll clean that up after a decision on the above is taken :-)

@xoviat
Copy link
Contributor

xoviat commented Jul 28, 2023

I'll take a look at this later if someone else doesn't. I want to clarify that the main thing I care about is that the implementation remains zero-copy, as is currently done in the runner implementation. I don't really have a strong opinion on changes that aren't performance-relevant.

@rubdos
Copy link
Contributor Author

rubdos commented Jul 28, 2023

the implementation remains zero-copy

That's also a requirement for us, although not strictly in an initial phase. I don't think this change specifically is really concerning regarding zero-copy. Thanks for weighing in!

@xoviat
Copy link
Contributor

xoviat commented Jul 28, 2023

Yeah I don't think this is performance relevant, I was just discussing in general.

@xoviat
Copy link
Contributor

xoviat commented Jul 28, 2023

I just looked and I agree that hardware address is the away forward. Make the CI pass and we can merge.

@xoviat
Copy link
Contributor

xoviat commented Jul 28, 2023

Re-export the hardware address type though

defmt = { version = "0.3", optional = true }
smoltcp = { version = "0.10", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

embassy-net-driver should not depend on smoltcp. This is so smoltcp can be upgraded without requiring every driver crate to update. Can you copypaste the HardwareAddress enum instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you copypaste the HardwareAddress enum instead?

IMO that loses part of the meaning of using HardwareAddress, since the whole purpose is to be able to use it as-is with smoltcp. It would be cool if we could just depend on smoltcp_wire instead, I suppose, although even that doesn't alleviate the concern.

I get the concern however, it felt pretty awkward to put smoltcp as a dependency.

Another option that I didn't mention in my OP, is to fix the hardware address type at compile time through a generic.

If after my plea here you still rather have the definition copied, I'll happily do that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I slept on it, and I think I agree. So I'll rewrite it and fix CI. If we're happy with the result, I'll squash the results such that Cargo.toml remains unchanged.

@rubdos
Copy link
Contributor Author

rubdos commented Jul 29, 2023

Re-export the hardware address type though

I could reuse the one from embassy_net, I suppose, since it's reexported there already.

@xoviat
Copy link
Contributor

xoviat commented Jul 29, 2023

You can make hardware address an enum with ethernet and ieee802154 variants. In any case, a driver should only need to have embassy-net-driver as a dependency in cargo.toml, nothing else.

@rubdos rubdos force-pushed the ieee802154-fixes branch 9 times, most recently from 339bdb9 to 31b0ac1 Compare July 31, 2023 09:52
/// Representation of an hardware address, such as an Ethernet address or an IEEE802.15.4 address.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum HardwareAddress {
Copy link
Member

Choose a reason for hiding this comment

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

could you add an Ip variant with no contents, like in smoltcp? otherwise drivers with IP medium have to return a "dummy" hardware address which is a bit awkward.

(I know this was already "wrong" before, but now that you're changing this it would be the ideal time to fix it. Thank you! 🙏 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/embassy-net/src/lib.rs b/embassy-net/src/lib.rs
index 746acae4..dbefb0ef 100644
--- a/embassy-net/src/lib.rs
+++ b/embassy-net/src/lib.rs
@@ -252,23 +252,7 @@ impl<D: Driver + 'static> Stack<D> {
         resources: &'static mut StackResources<SOCK>,
         random_seed: u64,
     ) -> Self {
-        #[cfg(any(feature = "medium-ethernet", feature = "medium-ieee802154"))]
-        let medium = device.capabilities().medium;
-
-        let hardware_addr = match medium {
-            #[cfg(feature = "medium-ethernet")]
-            Medium::Ethernet => to_smoltcp_hardware_address(device.hardware_address()),
-            #[cfg(feature = "medium-ip")]
-            Medium::Ip => HardwareAddress::Ip,
-            #[cfg(feature = "medium-ieee802154")]
-            Medium::Ieee802154 => to_smoltcp_hardware_address(device.hardware_address()),
-            #[allow(unreachable_patterns)]
-            _ => panic!(
-                "Unsupported medium {:?}. Make sure to enable it in embassy-net's Cargo features.",
-                medium
-            ),
-        };
-        let mut iface_cfg = smoltcp::iface::Config::new(hardware_addr);
+        let mut iface_cfg = smoltcp::iface::Config::new(to_smoltcp_hardware_address(device.hardware_address()));
         iface_cfg.random_seed = random_seed;

         let iface = Interface::new(

Including this, I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

yup! :D

@rubdos
Copy link
Contributor Author

rubdos commented Jul 31, 2023

I suggest I still squash some things up here, but this is the general idea, right? :-)

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thank you!

@Dirbaio Dirbaio added this pull request to the merge queue Jul 31, 2023
Merged via the queue into embassy-rs:main with commit 6caf627 Jul 31, 2023
@rubdos rubdos deleted the ieee802154-fixes branch July 31, 2023 12:40
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.

3 participants