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

Split up edge-net into smaller crates #10

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 30, 2023

I'm putting this up early because I want to hear feedback about the general direction. As it is, the PR probably won't pass CI.

embassy-net is now a thin facade crate reexporting the others, and also containing some std implementations. std is a bit of a mystery still, as some parts can be implemented easily in their respective crates (captive's server, TcpAcceptor, etc.), but StdRawMutex for example is a bit of a misfit. Maybe it would make sense to move all std-related code (except error type impls...) into edge-std or something similar.

Compared to the original plans I had to create edge-tcp which only contains traits (and may contain the std implementation, except then I don't know where to put the std UDP impl).

Some crates only make sense with nightly enabled because they only contain async implementations. Current code doesn't perfectly reflect this - I still want to put implementations behind feature gates just like in the current edge-net, so that they remain consistent and we don't take place away from possible blocking impls.

websocket's http-related additions are now in an extension trait so there's no http --> websocket dependency.

I also found a bunch of unused code in an unreachable utils module. io was maybe usable, but unreachable and I didn't find much reason to keep it.

@ivmarkov
Copy link
Owner

I'm putting this up early because I want to hear feedback about the general direction. As it is, the PR probably won't pass CI.

Fair enough.

embassy-net is now a thin facade crate reexporting the others

Fair enough.

and also containing some std implementations. std is a bit of a mystery still, as some parts can be implemented easily in their respective crates (captive's server, TcpAcceptor, etc.), but StdRawMutex for example is a bit of a misfit.

All std types except the StdRawMutex (not sure we still need that one - need to check) are - in fact - implementations of the embedded-nal-async traits on top of async-io. As such, they need to live in their own crate which might not even follow the edge- naming convention. Also, in an ideal world, the edge- crates should not even depend in the "std" crate, but only on the traits in embedded-nal-async so that you can reuse these with any embedded-nal-async implementation.

A detail but still: all crates should also have a usable subset which only relies on embedded-io-async, or that's how I've tried to arrange stuff. Some crates (captive portal, dhcp) should even have a "compute-only" aspect that does not need any async, but with async-fn-in-trait hitting stable in a few weeks, I don't think this is super important.

Maybe it would make sense to move all std-related code (except error type impls...) into edge-std or something similar.

As per above, though with a different name maybe.

A little but annoying detail: embedded-nal-async does not (yet) have traits to model "server" TCP sockets. You know, the whole "bind"/"accept" circus from std.

This is necessary for the http server only, yet kinda important. So the "std" code also has such sample traits and their implementation on top of async-io, but I consider this situation temporary, as ultimately, these need to be merged in the nal crate - in one form or another.

Compared to the original plans I had to create edge-tcp which only contains traits (and may contain the std implementation, except then I don't know where to put the std UDP impl).

I think it does not make sense to have a crate for that. We should rather fork/PR the embedded-nal-async crate, and then poke Dario, Lulf and the others until the additional traits are accepted there.

Some crates only make sense with nightly enabled because they only contain async implementations. Current code doesn't perfectly reflect this - I still want to put implementations behind feature gates just like in the current edge-net, so that they remain consistent and we don't take place away from possible blocking impls.

This is a short term, tactical problem, fortunately. Let me look at the PR...

websocket's http-related additions are now in an extension trait so there's no http --> websocket dependency.

Ditto - let me look at the PR.

I also found a bunch of unused code in an unreachable utils module. io was maybe usable, but unreachable and I didn't find much reason to keep it.

Fair enough.
Later today or tomorrow I'll spend time on the actual PR. The above is just top of mind, high level.

@ivmarkov
Copy link
Owner

Regarding the "std" stuff - and putting aside the serverside TCP socket traits - it is essentially identical in spirit to this crate:
https://gitlab.com/chrysn/std-embedded-nal/-/tree/master/std-embedded-nal-async?ref_type=heads

I'm not sure why the crate from above ^^^ has dependencies on all of async-std and not just on async-io (you see, the smol echosystem and the async-std echosystem are in a weird relationship, where async-std re-uses some (but not all) off the crates from smol like e.g. async-io. async-std is an effort pre-dating smol btw. With smol - AFAIK - Stjepan tried to create an async echosystem which has a smaller API surface and does not try to re-implement all of Rust STD, with an "async" prefix in front of everything - hence the Async adaptor for STD types).

Anyway. Perhaps we can even convince the maintainer of the above stuff to (optionally) remove the async-stddependency and only keep the async-io one as a mandatory. Then we don't even need the one in edge-net anymore.

@bugadani
Copy link
Contributor Author

@chrysn if I may ask for your opinion on this (i.e. trying to make std-embedded-nal-async usable without async-std) question?

@chrysn
Copy link

chrysn commented Nov 30, 2023

I'm not sure I get the question. std-embedded-nal-async is just a provider of implementations of embedded-nal-async, and all that's in here is building on embedded-nal-async. That it uses async-std should be purely an imlpementation detail, and should have no bearing on its users.

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 1, 2023

I'm not sure I get the question. std-embedded-nal-async is just a provider of implementations of embedded-nal-async, and all that's in here is building on embedded-nal-async. That it uses async-std should be purely an imlpementation detail, and should have no bearing on its users.

@chrysn The question is: are you open to changes of std-embedded-nal-async that would make it depend only on async-io and not on all of async-std?

There are good reasons for that. As in some STD environments only having support for async-io but not for all of async-std. And I don't think you need all of async-std in your crate anyway.

@chrysn
Copy link

chrysn commented Dec 1, 2023

I'm certainly open to altering the implementation -- and that UnconnectedUdp is already implemented w/o async_std shows that it can be done.

What I'm curious about is:

  • Which environments have a std but no async-std. I know that there is for example wasm32-unknown-unknown, but the std there barely deserves the name (frankly I consider it a mistake; if there is any shim it should be in wasm32-unknown-emscripten or wasm32-unknown-ourownmakebelievestd)? And why can't support for that platform just be added to async_std? (In other words: What's wrong with async_std? Granted, its executor is opinionated, but the futures should all work independently of the executor.)
  • Why does a crate that primarily implements protocols on top of embedded-nal-async (which is great to have BTW, if you want to add CoAP keep an eye out for embedded-nal-coap that's currently maturing a bit) need any particular implementation of embedded-nal-async in the first place? (Sure, one for the tests, but what's the problem then?)

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 1, 2023

I'm certainly open to altering the implementation -- and that UnconnectedUdp is already implemented w/o async_std shows that it can be done.

What I'm curious about is:

  • Which environments have a std but no async-std. I know that there is for example wasm32-unknown-unknown, but the std there barely deserves the name (frankly I consider it a mistake; if there is any shim it should be in wasm32-unknown-emscripten or wasm32-unknown-ourownmakebelievestd)? And why can't support for that platform just be added to async_std? (In other words: What's wrong with async_std? Granted, its executor is opinionated, but the futures should all work independently of the executor.)

The Rust STD support for ESP IDF (i.e. #[cfg(all(unix, target_os = "espidf"))]:

  • It is upstreamed into rust-lang since a few years
  • Additionally, async-io V2.X does support ESP IDF (via poll/select + ESP-IDF's weird version of event_fd)
  • Additionally, all of socket2, rustix and errno (and a bunch of others) support ESP IDF as well

I'm digressing, but next tokio will support it too, but this version of tokio is not yet released at crates.io.
For async-std - I'm not sure it is compatible with ESP IDF. For one, it is still on async-io 1.X, but even putting that aside, I'm not seeing that much interest w.r.t. async-std in the Esp-Rust non-baremetal community. Folks either want "everything and the kitchen sink" (= tokio), or something lean and small (= async-io, or like smol in general).

  • Why does a crate that primarily implements protocols on top of embedded-nal-async (which is great to have BTW, if you want to add CoAP keep an eye out for embedded-nal-coap that's currently maturing a bit) need any particular implementation of embedded-nal-async in the first place? (Sure, one for the tests, but what's the problem then?)

It doesn't. And it won't do any assumptions about the underlying implementation. However, If I want to run any crate that depends on embedded-nal-async on top of ESP IDF, I need an embedded-nal-async implementation that would work there, in the first place. This is not "tests", it is real production. Rust STD on top of ESP IDF is production stuff, kind of.

So in a way, if you reduce the dependencies of your crate to the bare minimum (say, async-io and then one of libc/nix/rustix/socket2 for the "custom" stuff that you cannot express via the async-io safe APIs), you are covering more std platforms.

Also, I - for one - am interested in using these tiny crates that come from the MCU Rust world in "STD" scenarios like embedded Linux as well. Why not?

And it is not just a case of "this MCU guy knows his MCU echosystem and just wants to re-use it with STD", I hope. For one, rs-matter - once I get back to it - might switch to embedded-nal-async where it currently uses hard-coded STD+embassy-net implementations for the UDP stack (with feature toggles). And I don't think rs-matter would have an MCU-only baremetal story.

@chrysn
Copy link

chrysn commented Dec 1, 2023

Ah, this is about ESP. As for that:

As std has no async implementation of sockets, std-embedded-nal-async (where it is not using async-std) is using the nix crate to have async sockets based on POSIX sockets. (Eventually going down to socket(), setsockaddr() and recvmsg() -- no matter whether through nix or async-std). Is ESP POSIX compatible enough that those work?

  • If so, all fine, let's do more of embedded-nal-async in the style of its current UnconnectedUdp socket.
  • If not, what of std-embedded-nal-async can be shared with ESP? In that case, which APIs could it be built on? Then, what portion of std-embedded-nal-async would be shared between a POSIX nix-based and the ESP based part in the first place?

(Long story short: Yes, let's do it, but not if 90% of the code become ifdef'd -- in that case, it'd be better to have a dedicated ESP implementation).

As for using this on Linux: Yes, that's what std-embedded-nal is for, and can support well either way.

And it is not just a case of "this MCU guy knows his MCU echosystem and just wants to re-use it with STD", I hope.

Full agree. The embedded-nal-async UDP is how I'd wish UDP sockets behaved everywhere. For example, that's how I'm using it for CoAP even on large systems.

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 1, 2023

Ah, this is about ESP. As for that:

As std has no async implementation of sockets, std-embedded-nal-async (where it is not using async-std) is using the nix crate to have async sockets based on POSIX sockets. (Eventually going down to socket(), setsockaddr() and recvmsg() -- no matter whether through nix or async-std). Is ESP POSIX compatible enough that those work?

Just to be crystal clear though I suspect it is obvious: one cannot have "async" sockets with only something like nix. nix gives one access to more BSD socket functions that one might need, but that's it. One would still - in addition - need a Reactor (like async-io) or else the non-blocking BSD sockets cannot be "linked" to the Waker mechanism of the Rust futures. Or to put it simply, we need a reactor so as to "bind" poll/epoll/kqueue to wakers.

(Mr Obvious again, but "STD" sockets are posix sockets of course, as you can always get their "raw fd" and call unsafe posix functions on these.)

W.r.t. nix, haven't checked this crate specifically, but I doubt it would compile with ESP IDF without patches. In the meantime we have alternatives though. Namely:

  • socket2
  • rustix
  • libc (obviously)

...all do compile fine with target_os = "espidf.

If a future incarnation of std-embedded-nal-async is based squarely on async-io (without async-std) we might want to take a look at rustix, as rustix replaced socket2 in async-io V2 internally, for the socket posixy stuff which is not available in STD, yet async-io needs it.

rustix has the benefit of having a safe API.

One small problem with rustix (and socket2, and - I suspect - with nix too) is that these do not support the sockaddr_ll C struct in any shape or form.This is only supported by libc. Why do we need it, you might ask? Raw sockets. Sure, I'm pushing it (I need it for the Rust dhcpc client I'm tinkering with), but wouldn't it be nice if in future embedded-nal-async also supports something in the lines of RawStack? But I digress.

  • If so, all fine, let's do more of embedded-nal-async in the style of its current UnconnectedUdp socket.

That should be possible. But I think your current approach is solid - drop to raw Posix API's only where you cannot go via the async-io APIs and only for the rare cases where you can't.

  • If not, what of std-embedded-nal-async can be shared with ESP? In that case, which APIs could it be built on? Then, what portion of std-embedded-nal-async would be shared between a POSIX nix-based and the ESP based part in the first place?

As I mentioned, safe bets are: async-io itself, and then any of libc/socket2/rustix (or a combination of these thereof) for where async-io and STD do not expose the APIs we need.

(Long story short: Yes, let's do it, but not if 90% of the code become ifdef'd -- in that case, it'd be better to have a dedicated ESP implementation).

Hopefully not.

As for using this on Linux: Yes, that's what std-embedded-nal is for, and can support well either way.

And it is not just a case of "this MCU guy knows his MCU echosystem and just wants to re-use it with STD", I hope.

Full agree. The embedded-nal-async UDP is how I'd wish UDP sockets behaved everywhere. For example, that's how I'm using it for CoAP even on large systems.

We are aligned here.

@chrysn
Copy link

chrysn commented Dec 1, 2023

one cannot have "async" sockets with only something link nix

No, it's async-io plus nix.

alternatives thoug. Namely

  • socket2: doesn't have the PKTINFO that async-embedded-nal needs (they have some kind of recvmsg AIU, but not the PKTINFO)
  • rustix: haven't looked at it before, might be an option
  • libc (obviously): that may compile fine with target_os = "espidf", but if it doesn't have the PKTINFO opt, it's a very incomplete implementation of regular POSIX sockets

Raw sockets

Nice, I'd love to have some of them (link-layer? IP? both?) in embedded-nal-async!


To go forward, looks like the best path would be to go async-io plus rustix. As I don't have any experience with that, do you think you could check whether the parts of std-embedded-nal-async's src/udp.rs that currently use nix could be replaced with rustix, while still on Linux (because ripping out the rest of async-std would be tedious and replacing it only makes sense if it works for the nix parts in the first place). If that works, replacing async-std with async-io and some rustix where needed will be the slightly more tedious part, but at least I don't expect any more surprises there.

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 1, 2023

one cannot have "async" sockets with only something link nix

No, it's async-io plus nix.

alternatives thoug. Namely

  • libc (obviously): that may compile fine with target_os = "espidf", but if it doesn't have the PKTINFO opt, it's a very incomplete implementation of regular POSIX sockets

I think it actually does have the pktinfo stuff?
Moreover, the pktinfo stuff in nix - at least in terms of raw C structs - seems to just be a re-export of the pktinfo in libc?

In fact, I would dare say libc is the most complete implementation of them all (it even has the sockaddr_ll raw sockets stuff). It is just the oldest of all, and the most unsafe of all (just a bunch of raw SYS-like bindings). We can also stick to it instead of to e.g. rustix, as unsafe as it is, because Rust STD itself is just a bunch of wrappers on top of the libc crate, so - in a way - if you depend on Rust STD, you already depend on libc as well, indirectly.

Raw sockets

Nice, I'd love to have some of them (link-layer? IP? both?) in embedded-nal-async!

I don't see why not both. In a way, what you get is specified when you do the socket syscall and then off you go, isn't it? (Might be wrong, I'm new to all of this.) I need only IP (SOCK_DGRAM + ETH_P_IP) for DHCP of course.

To go forward, looks like the best path would be to go async-io plus rustix. As I don't have any experience with that, do you think you could check whether the parts of std-embedded-nal-async's src/udp.rs that currently use nix could be replaced with rustix, while still on Linux (because ripping out the rest of async-std would be tedious and replacing it only makes sense if it works for the nix parts in the first place). If that works, replacing async-std with async-io and some rustix where needed will be the slightly more tedious part, but at least I don't expect any more surprises there.

I'll put some effort into this. Might take a few days or even a week, so please bear with me. Need to finish this DHCP stuff first...

@chrysn
Copy link

chrysn commented Dec 2, 2023

libc has pktinfo alright, but only on platforms that really have it. (For example, Windows does have some kind of recvmsg / pktinfo, but it's weird, very unlike the POSIX way, and therefore currently unsupported in socket2).

I'll put some effort into this. Might take a few days or even a week, so please bear with me.

Much appreciated!

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 3, 2023

@bugadani I'll be pushing a lot of changes to the dhcp module. Most likely these would make your PR unmergeable. Do you think you can update it post-factum?

Once you do, I'll merge it in mainline and we'll continue fixing / ironing out from there.

@bugadani bugadani marked this pull request as ready for review December 4, 2023 08:39
@ivmarkov ivmarkov merged commit 4a80e47 into ivmarkov:master Dec 5, 2023
1 check passed
@bugadani bugadani deleted the reorg branch December 5, 2023 09:53
@chrysn
Copy link

chrysn commented Dec 5, 2023

Seeing this closed, where do we continue the side discussion on std-embedded-nal? Would you mind creating an issue over at https://gitlab.com/chrysn/std-embedded-nal?

@ivmarkov
Copy link
Owner

ivmarkov commented Dec 5, 2023

Seeing this closed, where do we continue the side discussion on std-embedded-nal? Would you mind creating an issue over at https://gitlab.com/chrysn/std-embedded-nal?

Just did.

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