UdpClient/UdpServer may be confusing to users

[Sympatron]

Looking at the whole UdpClient/UdpServer thing from #21 I think the idea behind the trait it kind of changed.

Before it was called UdpStack and represented the whole network interface that creates sockets on demand for new connections, but now the same suddenly a UdpServer that creates either UdpSockets in server or client mode depending on the function you call. I think it is kind of confusing that your network interface has to implement UdpServer now.
I have no definitive answer how to make the API clearer, but maybe the socket types should be different in client and server mode instead of the stack. The stack traits could be called UdpStack/UdpServerStack or something like UdpSimpleStack/UdpAdvancedStack or UdpStackCore/... Maybe somebody has a good idea for names.

The point I am trying to make is, that it may be confusing to mix semantics of network interfaces and their traits and actual sockets. Similar things apply to TCP (#30) of course.

[ryan-summers]

To my knowledge, a UDP socket is the same (a simple data pipe with RX/TX) regardless of client/server roles, so there's no point in differentiating between a socket created by a UDP server and a socket created by a UDP client once it's connected to the peer.

It's a bit of a misnomer that we had UdpStack previously because it wasn't actually a stack. Instead, it only exposed client-specific APIs and had no support for the server role at all. If we wanted to bring back UdpStack, it would likely be exactly what is currently UdpServer.

[Sympatron]

What I meant was that the UdpStack/UdpServer trait represents the whole network interface, or am I getting something wrong here? The connection is represented by UdpSocket, but UdpStack was basically supposed to be a "global" entity, or not?
Therefore the current naming scheme is confusing, because UdpClient/UdpServer implies a connection in my opinion.

With UDP you are correct, that server and client are quite similar, but we could still represent the current mode with the type model to ensure proper usage at compile time. At the moment you could call send() on a server socket which doesn't make sense, because then you can't specify the recipient. And you can call send_to() on a client that is already connected to a (potentially different) remote host. These errors can easily be detected at compile time with a stricter type system.

[ryan-summers]

What I meant was that the UdpStack/UdpServer trait represents the whole network interface, or am I getting something wrong here? The connection is represented by UdpSocket, but UdpStack was basically supposed to be a "global" entity, or not?
Therefore the current naming scheme is confusing, because UdpClient/UdpServer implies a connection in my opinion.

I see what you're saying here now and this definitely makes sense. Do you have an alternative naming scheme in mind? I agree that naming the network stack as Client and Server is likely a bit of a misnomer here, since it can implement sockets that are either clients or servers.

With UDP you are correct, that server and client are quite similar, but we could still represent the current mode with the type model to ensure proper usage at compile time. At the moment you could call send() on a server socket which doesn't make sense, because then you can't specify the recipient. And you can call send_to() on a client that is already connected to a (potentially different) remote host. These errors can easily be detected at compile time with a stricter type system.

In general, I agree. Feel free to propose a PR with type updates if this is something you'd like to see! As currently implemented, this would be checked by the network stack implementing these traits.

My only concern on using type-state programming is that it can make programming models awkward. For example, if you have a driver that internally stores the socket it uses, now it has to have two variables - one for an unconnected socket and one for a connected socket - instead of just one, but this is application-specific. It largely depends on how type-state programming is implemented in this crate.

At the moment you could call send() on a server socket which doesn't make sense, because then you can't specify the recipient.

When you say "server socket", are you implying a TCP socket in the listening state or a UDP socket in an unconnected state? For all other socket states, this seems acceptable unless I'm missing something

[Sympatron]

Do you have an alternative naming scheme in mind?

In the first post I made a few suggestions. None of them are perfect IMO. I was hoping someone would help out here. 😅

In general, I agree. Feel free to propose a PR with type updates if this is something you'd like to see! As currently implemented, this would be checked by the network stack implementing these traits.

Ok, I'll think about how this could be implemented without making the API awkward to use.

When you say "server socket", are you implying a TCP socket in the listening state or a UDP socket in an unconnected state? For all other socket states, this seems acceptable unless I'm missing something

I was talking about a UDP socket in bind() mode.

[jonahd-g]

One solution would be to simply join them all back into a single {Udp|Tcp}Stack trait.. I'd be fully on-board with having a single trait that requires all of the necessary Berkeley socket functions. This would definitely provide a lot of interface simplicity. While there are benefits in breaking it up into subsets so that developers don't have to implement all features, it's not necessarily more important than other priorities. Developers could always implement noop functions if they can't complete the implementation. We could recommend having FeatureNotSupported kind in their Error enum.

The other extreme would be a trait for every function. Then a NAL user could indicate exactly the functions it needs.

[MathiasKoch]

One solution would be to simply join them all back into a single {Udp|Tcp}Stack trait.

I don't think that would solve the problem @Sympatron is describing, as that would still leave you able to call send() on a bind()'ed socket etc?

I think, if that issue is one we want to solve in this abstraction layer, and not leave up to every implementer, we need to introduce typestates, but that would add quite a bit of complexity to this crate, which i would be fine with if we can come up with a pattern that works elegantly..?

The other extreme would be a trait for every function. Then a NAL user could indicate exactly the functions it needs.

I would personally be very much opposed to that solution.

[jonahd-g]

I don't think that would solve the problem @Sympatron is describing, as that would still leave you able to call send() on a bind()'ed socket etc?

You're correct, "solve" wasn't quite the right word there. It would offload the problem onto the implementer (as you said). All this crate would do is offer the list of functions, with no guarantees about behavior.

[jonahd-g]

I've begun considering a typestate solution for these traits. Since the Stack might be working with multiple sockets all in different states, it doesn't make sense for the type variation to exist at that level. We're going to need different socket type variations for different modes of behavior.

For simplicity's sake, I'm starting with the UdpStack. My first attempt is using generics on the UdpSocket and zero sized types.

struct Udp;
struct Closed;
struct Connected;
struct Bound;

trait OpenMode {}
impl OpenMode for Connected {}
impl OpenMode for Bound {}

pub trait UdpStack {
    type UdpSocket<T, M>;
    type Error: core::fmt::Debug;

    fn socket(&self) -> Result<Self::UdpSocket<Udp, Closed>, Self::Error>;

    fn connect(&self, socket: Self::UdpSocket<Udp, Closed>, remote: SocketAddr) -> Result<Self::UdpSocket<Udp, Connected>, Self::Error>;

    fn bind(&self, socket: Self::UdpSocket<Udp, Closed>, local_port: u16) -> Result<Self::UdpSocket<Udp, Bound>, Self::Error>;

    fn send(&self, socket: &mut Self::UdpSocket<Udp, Connected>, buffer: &[u8]) -> nb::Result<(), Self::Error>;

    fn send_to(
        &self,
        socket: &mut Self::UdpSocket<Udp, impl OpenMode>,
        remote: SocketAddr,
        buffer: &[u8],
    ) -> nb::Result<(), Self::Error>;

    fn receive(
        &self,
        socket: &mut Self::UdpSocket<Udp, impl OpenMode>,
        buffer: &mut [u8],
    ) -> nb::Result<(usize, SocketAddr), Self::Error>;

    fn close<M>(&self, socket: Self::UdpSocket<Udp, M>) -> Result<(), Self::Error>;
}

Of course this doesn't build, because Rust doesn't (yet?) support generic associated types. I'm having trouble figuring out how to require UdpSocket to accept the generic parameters that are needed here. Any ideas, or tweaks to make something similar work?

[jonahd-g]

Further digging has revealed this issue:

rust-lang/rust#44265

It seems that generic associated types (GAT) are available in nightly, although possibly unstable according to the warnings. Using that feature and compiling with nightly, this version works:

pub struct Udp;
pub struct Closed;
pub struct Connected;
pub struct Bound;

pub trait Mode {}
impl Mode for Udp {}

pub trait Status {}
impl Status for Closed {}
impl Status for Connected {}
impl Status for Bound {}

pub trait OpenStatus: Status {}
impl OpenStatus for Connected {}
impl OpenStatus for Bound {}

pub trait UdpStack {]
    type UdpSocket<M: Mode, S: Status>;]
    type Error: core::fmt::Debug;
]
    fn socket(&self) -> Result<Self::UdpSocket<Udp, Closed>, Self::Error> where <Self as UdpStack>::UdpSocket<Udp, Closed>: core::marker::Sized;

    fn connect(&self, socket: Self::UdpSocket<Udp, Closed>, remote: SocketAddr) -> Result<Self::UdpSocket<Udp, Connected>, Self::Error> where <Self as UdpStack>::UdpSocket<Udp, Connected>: core::marker::Sized;

    fn bind(&self, socket: Self::UdpSocket<Udp, Closed>, local_port: u16) -> Result<Self::UdpSocket<Udp, Bound>, Self::Error> where <Self as UdpStack>::UdpSocket<Udp, Bound>: core::marker::Sized;

    fn send(&self, socket: &mut Self::UdpSocket<Udp, Connected>, buffer: &[u8]) -> nb::Result<(), Self::Error>;

    fn send_to(
        &self,
        socket: &mut Self::UdpSocket<Udp, impl OpenStatus>,
        remote: SocketAddr,
        buffer: &[u8],
    ) -> nb::Result<(), Self::Error>;

    fn receive(
        &self,
        socket: &mut Self::UdpSocket<Udp, impl OpenStatus>,
        buffer: &mut [u8],
    ) -> nb::Result<(usize, SocketAddr), Self::Error>;                                                                                                                                          

    fn close<S: Status>(&self, socket: Self::UdpSocket<Udp, S>) -> Result<(), Self::Error>;
}

Would love to hear others opinions on this. Are we willing to (for now) depend on an (unstabel) nightly feature? Personally, I'm already using nightly for my projects because that's the only place the AVR compiler is available, so this wouldn't be an issue for me. Plus, we're still in 0.x version. If someone has another solution to this issue besides GAT, please post it.

I wish we knew how long before GAT hit stable :(

[jonahd-g]

Another approach would be to publish our own Socket struct with the needed generics. This would have the user down-side that they'd likely need to wrap it in another object, and unwrap it whenever they need to use it.

[Sympatron]

I like the approach. This is kind of what I had in mind. We only need to figure out how to do this without GAT as requiring nightly is probably not a good idea. I'll ask someone very familiar with type state programming for some insight.
I suppose we continue further discussion about this in #33.

[jonahd-g]

@Sympatron sg, let's close this issue then.

[ryan-summers]

Let's leave this issue open for potential renaming of UdpClient/UdpServer and TcpClient/TcpServer

Perhaps renames could be:

• xxxClientStack - for stacks that implement just client APIs
• xxxFullStack - for stacks that implement client and server APIs

I think the key here is to indicate that these are network stacks and not clients or servers themselves.

[Sympatron]

+1 for ClientStack/FullStack

[MathiasKoch]

+1 from here,

I think that makes great sense 👍

[ryan-summers]

Fixed in #36