Adding initial TCP server traits by ryan-summers · Pull Request #30 · rust-embedded-community/embedded-nal (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation32 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR refactors the TCP traits to align with #21.
This PR fixes #27 by adding a new TcpServer trait.
This PR fixes #16 by clarifying the life cycle of a TCP socket. (socket() -> connect() -> close()).
This PR fixes #10 by updating connect() to return a nb::Result so that connect can be completed asynchronously.
TODO:
- Implement a test use case of a TCP server using
std::netfor demonstration purposes.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about the TCP side of things. Is it possible for a single socket to communicate with multiple clients at once? Or would you need multiple sockets listening on the same port? Or something else?
I don't know much about the TCP side of things. Is it possible for a single socket to communicate with multiple clients at once?
With a TCP server, you typically have a single socket listening on a given local port. It then receives connection requests from various remote clients and creates a new connection for each request. Each connection is assigned a unique socket for the communication stream, but the original listening socket remains operational to facilitate more connections. Thus, a single TCP server can serve any number of TCP clients that it wants to support simultaneously.
Or would you need multiple sockets listening on the same port? Or something else?
Based on my reading of berkley-sockets:
accept() is used on the server side. It accepts a received incoming attempt to create a new TCP connection from the remote client, and creates a new socket associated with the socket address pair of this connection.
There is a single listening socket that creates new sockets for each accepted connection.
There is a single listening socket that creates new sockets for each accepted connection.
That makes sense, thanks for explaining.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, once the stuff from @jonahd-g has been resolved 👍
Great work!
I have now written a small TCP echo server using the traits to verify that everything works as I would expect to - there was one modification to update accept() to return the connected client's address, but other than that, everything worked nicely. This should be ready for merge I believe @MathiasKoch, @Sympatron and @jonahd-g.
If we'd like to add type-state control of the TCP listening socket, I would advocate we wait for another PR. There's some difficulties because of the open() and close() APIs having to get re-implemented.
If we'd like to add type-state control of the TCP listening socket, I would advocate we wait for another PR. There's some difficulties because of the open() and close() APIs having to get re-implemented.
I agree on this, as it seems like a super nice to have approach, but kind of out-of-reach at this point, unless one of you guys have the time to draft up such an API?
So from my side of things, LGTM
I'm willing to start working on a typestate solution, but I'd really like to see this current approach merged and released in the mean-time, considering the currently-released version does not support binding.
LGTM. I think we can merge it the way it is and add type states in a new PR with a separate discussion.
CC: @eldruin - this should now be ready for merge
Thanks @ryan-summers! GitHub complains about conflicts, though. Could you rebase this?
Alright, should now be fixed up after a rebase (there was indeed a conflict)
eldruin added a commit to eldruin/embedded-nal that referenced this pull request
eldruin added a commit that referenced this pull request