rust-embedded-community / embedded-nal

An Embedded Network Abstraction Layer
Apache License 2.0
177 stars 25 forks source link

SocketAddr associated types #92

Open chrysn opened 10 months ago

chrysn commented 10 months ago

Implementing embeded-hal(-async), it seems I'm always converting SocketAddr back and forth with the platform's own socket addr type.

Would it make sense to add an associated SocketAddr type (probably constrained to Into<SocketAddr> + TryFrom<SocketAddr, Error=Unavailable> + PartialEq<Self::SocketAddr> + Hash)? In all places where we previously pass SocketAddr, we'd now pass Self::SocketAddr. Wherever users store addresses, they'd use their stack's SocketAddr type, and only convert through a SocketAddr when entering or leaving the stack (or even not at all if we require Display and FromStr).

Some reasons why a stack would not like working with SocketAddr all the time:

This is mainly a concern for users of unconnected UDP sockets, where recvfrom-style use is predominant, and addresses are handled in every single packet. For TCP or connected UDP sockets, it's probably a secondary concern.

ryan-summers commented 10 months ago

I am very okay with a generic socketaddr type myself, as I've had these exact same issues during implementation. I don't see any specific benefit to using no-std-net, since we still have versioning issues, and there's not very convenient methods to convert between common types.

ryan-summers commented 10 months ago

However, how would a library that is agnostic to the underlying stack know how to construct and use these addresses? I.e. https://github.com/quartiq/minimq/blob/master/src/network_manager.rs, where we don't know what stack we're running on. Would this be the TryFrom impl bound?

chrysn commented 10 months ago

The versioning issues probably go away as we might use the opportunity to switch to core::net::SocketAddr as a reference type. Indeed I'd think that TryFrom<core::net::SocketAddr> and Into<core::net::SocketAddr> (Or From<Self> for SocketAddr, I always mix up which is the good one to implement) should be part of the trait bounds -- in particular, core::net::SocketAddr would stay a perfectly valid choice there.

In code such as the network managers case, there's the TcpStack: TcpClientStack generic type around, so options are (as a matter of taste, partially) to either put TcpStack::SocketAddr in the own object's signtures, or to put SocketAddr there and go through the conversions (or to go half way and put impl TryInto<TcpStack::SocketAddr> in there).

ryan-summers commented 10 months ago

I would love to use core::net, but it's still nightly-only behind feature(ip_in_core)

chrysn commented 10 months ago

Oh -- indeed, sorry, I didn't check all the way up on the documentation tree (and the types don't get the warning if their module is gated). In that case, the proposed associated types are not helping the versioning situation (but don't don't make it worse either, for where we used to use no-std-net's SocketAddr we'd now use TryFrom/Into it).

chrysn commented 7 months ago

The trouble of core::net stabilization is just about to go away, and https://github.com/rust-embedded-community/embedded-nal/pull/102 already does the breaking API change. As there is renewed interest in this from https://github.com/rust-embedded-community/embedded-nal/pull/106 (CC'ing @ivmarkov), I propose we give this a try.

Processing input from #106, it may make sense (but may need discussion) to

ryan-summers commented 5 months ago

Closing this as resolved by https://github.com/rust-embedded-community/embedded-nal/pull/102, as we now use the freshly-stabilized core::net :)

If my assessment is wrong here, feel free to reopen!

chrysn commented 5 months ago

Please reopen (I don't have the permssions to); 102 does not fix this: the core socket addresses are a more-easily-to-agree-on type, but socket implementations may still have a local better type.

The easiest example to give is an IPv4-only stack that has thus has a smaller socket address type (yikes, but it is the simplest example); a better example are 6LoWPAN implementations that only communicate locally and thus use compressed addresses, or where addresses IP addresses are thin pointers into refcounted neighbor table entries.

chrysn commented 5 months ago

The core transition was mainly mentioned here because it'd be a breaking change anyway, and would thus be an opportunity to introduce the associated types (where indeed many stacks would then put core::net::SockAddr as the AT, because it is TryFrom and Into itself).