rust-embedded-community / embedded-nal

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

[Question] No way to set UDP port? #20

Closed jonahbron closed 3 years ago

jonahbron commented 3 years ago

I see that SocketAddr supports an IP address and a remote port number. However, I see no way to specify a local port number. How would a program specify the port to listen on/send from without that ability?

MathiasKoch commented 3 years ago

I am not sure this is that thoroughly used and tested wrt. UDP yet.

If you have any elegant ideas to fix your issues, i am very sure a PR would be welcomed, such that we can bring this up to a level where it gets the attention and usage it deserves.

jonahbron commented 3 years ago

Okay. I think simply adding a port arg to the open function would do the job. I'll create a PR.

ryan-summers commented 3 years ago

In response to the discussions in #21, it has become apparent that in general, the embedded-nal traits do not support server-based embedded implementations and the crate is geared more towards embedded TCP/UDP clients. Hence, that's why there's no current way to specify a local UDP port (bind-like behavior). Similarly, this is not exposed for TCP either.

As part of a generic interface, I believe our design goal at this point is to aim for high adoption rates in using the embedded-nal, and I think the way we accomplish that is by keeping implementation bare-bones and simple.

In this case, an external library is likely operating as either a server or a client, but is unlikely to be both at the same time. As discussed in #21, it may make sense to create new traits for server-side behavior for the embedded-nal as well, as this means that network stacks that are either unable to or do not want to operate as a server do not need to implement the necessary traits.

ryan-summers commented 3 years ago

Disclaimer: After writing this, I've noticed that the code in https://github.com/rust-embedded-community/embedded-nal/pull/21 shows a nice approach for UDP as well.


Currently, it looks like the following functions are missing from the embedded-nal:

While on the UDP-side, the server only exposes 1 additional function, this gets more complex on the TCP-side. I think we should aim for symmetry in the implementations, so we should add all server-related functions in a symmetric manner for the TCP and UDP stacks.

UDP and TCP servers are generally a super-set of their client components (with the exception of TCP-connect), thus it would be desirable for having common core functionality for the shared components (e.g. open(), read(), write()), but then expose separate traits for TCP clients and servers.

One approach here is to have a core-trait, shown here:

pub trait UdpClient {
   fn open();
   fn close();
   fn read();
   fn write();
}

pub trait UdpServerCore {
    fn bind();
}

pub trait UdpServer {};
impl<T: UdpServerCore + UdpClient> UdpServer for T {}

Then, a user would implement the UdpServerCore as follows and have the UdpServer trait implemented for them (because them:

pub struct UdpStack {
};

impl UdpClient for UdpStack {
    // ....
}

impl UdpServerCore for UdpStack {
    // ...
}

And library could could require a UdpServer as follows:

pub fn my_function(stack: impl UdpServer) {
}

If your library only needed the client, it could do:


pub fn my_function(stack: impl UdpClient) {
}
jonahd-g commented 3 years ago

@ryan-summers I'm a bit confused on the benefit of having three separate traits, one of which is automatically implemented. How is that better than having a {Udp/Tcp}Client and a {Udp/Tcp}Server that extends it? It seems like your example code contradicts your explanation: explanation prescribes a shared "core" trait, but the example code instead has a "servercore" trait.

MathiasKoch commented 3 years ago

I would personally much rather see the approach with server extending client, as the server without the rest of the functions doesn't make any sense at all.

I like the approach you have in your pr!

ryan-summers commented 3 years ago

@ryan-summers I'm a bit confused on the benefit of having three separate traits, one of which is automatically implemented. How is that better than having a {Udp/Tcp}Client and a {Udp/Tcp}Server that extends it?

After I saw your implementation, I was like "Oh right, that is exactly what I was trying to do" - I forgot the Rust syntax to express it, so I added the disclaimer to the top. I definitely think the PR has a better approach here as well after review.

I would personally much rather see the approach with server extending client, as the server without the rest of the functions doesn't make any sense at all.

I agree that the approach works well for UDP, but the TCP side of things gets a bit trickier because TCP servers aren't necessarily an extension of TCP clients. For example, a TCP server does not need to use connect(), but rather accept() and listen(). connect() is a client-only API. I'm curious to hear thoughts on the TCP side of things, but I'll open a new issue to track that, since it's out of scope of the UDP side of things.