Remove the embedded-nal / smoltcp-nal dependencies from `wifi_interface.rs` and esp-wifi about esp-wifi-sys HOT 17 CLOSED

Just as an FYI, I also saw they've added embedded-nal-async very recently.

6 hours ago, to be precise? :D

from esp-wifi-sys.

@ivmarkov yes, I agree. I meant to say I'm pretty sure it can be done for the client case.

from esp-wifi-sys.

My intention with embedded-nal was to just introduce some quality-of-life improvement - so I added it as a non-default feature.
I personally think that embedded-nal is the easier API when going the blocking way. But maybe it's just me.

Big thanks for pointing out the async feature of smoltcp - I somehow totally missed that. Really need to look into that. That's very exciting.

I don't have a strong opinion on embedded-nal but since it's a non-default feature it shouldn't hurt too much.
But I definitely like to have a similar way to configure smoltcp. And probably also re-add a smoltcp example.

from esp-wifi-sys.

My intention with embedded-nal was to just introduce some quality-of-life improvement - so I added it as a non-default feature. I personally think that embedded-nal is the easier API when going the blocking way. But maybe it's just me.

I completely agree that was a great first step, for a demo etc. The question is - how to continue from here. My problem with embedded-nal is that I actually don't see it as a solution, neither for blocking, nor for nonblocking access:

• Issues with blocking access
  • How can I even use a blocking networking API (embedded-nal or not), given that on "bare metal" I don't have multithreading, with a preemptive thread scheduler (i.e. FreeRTOS or similar)? This will likely not work even for toy projects
• Issues with non-blocking access
  • The poll based non-blocking access of embedded-nal suffers from the fact that it does not have the notion of a "waker". So the only choice users are having is to poll in a loop, with some sort of arbitrary sleep interval between the polls. I think this is what you are doing in the current examples. It is just that nowadays, with async wakers being built-in in smoltcp, and at least two executors to pick and port (embassy and my own little adjustment of smol's async-task), I don't see why we would consider the embedded-nal model
  • The nb-based embedded-nal polling model has usability issues as well, in that it does not compose and I cannot write regular sequential code; contrary to that, async await allows the illusion for sequential

I don't have a strong opinion on embedded-nal but since it's a non-default feature it shouldn't hurt too much. But I definitely like to have a similar way to configure smoltcp. And probably also re-add a smoltcp example.

But my point is - your embedded-nal configuration is in fact 99% percent smoltcp configuration actually. Only where you wrap the smoltcp interface in embedded-nal NetworkStack it becomes an embedded-nal configuration. Why don't we keep the existing smoltcp interface configuration, and just return it? Folks can then either use it with async, or wrap it in an embedded-nal NetworkStack if they wish so?

from esp-wifi-sys.

In other words, just delete this line, don't create a WifiClock and return the ethernet var.

from esp-wifi-sys.

I'm only really familiar with smoltcp, embedded-nal didn't exist when I was contributing to smol :D. I think you make some good points @ivmarkov. I suppose the only thing we can look at is what crate support will having embedded-nal implementations get us?

From the reverse dependencies, 17 crates use embedded-nal. Of those 17, 3 are implementations of thr traits so we can take that away. So having embedded-nal trait implementation gives us access to 14 crates of varying quality.

If it's easy to maintain embedded-nal behind a feature, then it probably makes sense to do so, but we should also offer helper methods for using smoltcp directly where appropriate.

from esp-wifi-sys.

If it's easy to maintain embedded-nal behind a feature, then it probably makes sense to do so, but we should also offer helper methods for using smoltcp directly where appropriate.

That's basically what I wanted to say 😄

from esp-wifi-sys.

Well, if we believe that the existing total of 14 lines (this one and this one) are worth a feature, then sure, let's keep it as some sort of utility code.

All I'm asking for is that the Wifi interface and struct not to depend on embedded-nal + smoltcp-nal and just to return a plain smoltcp interface. Users can always use the above 14 lines utility to wrap the returned smoltcp interface. Or not use them, as I would do.

from esp-wifi-sys.

So basically, that means that the feature embedded-svc shouldn't depend on embedded-nal?
I like the suggestion in general.

We only need to figure out how to deal with ClientIpStatus::Waiting and ClientIpStatus::Done - technically those relate to the networking stack, not the wifi interface. t.b.h. I don't like that much - but it's the way it is now

So, we need to add some code to handle DHCP (basically what smoltcp-nal does for us currently) there using smoltcp

Then I'm not sure if we can construct a smoltcp-nal instance with an already assigned IP address if we want to keep the feature

from esp-wifi-sys.

Just as an FYI, I also saw they've added embedded-nal-async very recently.

from esp-wifi-sys.

So basically, that means that the feature embedded-svc shouldn't depend on embedded-nal? I like the suggestion in general.

We only need to figure out how to deal with ClientIpStatus::Waiting and ClientIpStatus::Done - technically those relate to the networking stack, not the wifi interface. t.b.h. I don't like that much - but it's the way it is now

If you could clarify what is a networking stack - I have difficulties with this notion.
Isn't smoltcp also a networking stack?

By the way the only dependency on embedded-nal + smoltcp-nal in the existing implementation of embedded_svc::wifi::Wifi is this line right? (The network_stack variable to be precise, but it is only used indirectly - the Wifi trait implementation only cares about the wrapped smoltcp interface.)

So, we need to add some code to handle DHCP (basically what smoltcp-nal does for us currently) there using smoltcp

Indeed. But this would be outside of the embedded_svc::wifi::Wifi implementation, right?

Then I'm not sure if we can construct a smoltcp-nal instance with an already assigned IP address if we want to keep the feature

I imagine user would drive the DHCP protocol messages either via embedded-nal or via other means (async smoltcp). But the Wifi trait would be unaware who is driving the DHCP protocol. It will just - at some point - see the assigned IP on the smoltcp interface.

I think we are getting at a point where some prototyping / experimentation is necessary, English words become difficult. :)

from esp-wifi-sys.

Isn't smoltcp also a networking stack?

It is!

I think we are getting at a point where some prototyping / experimentation is necessary, English words become difficult. :)

Hope to get to this next week - I think I got your points ;) ... if anyone gets bored meanwhile - feel free to jump in

from esp-wifi-sys.

I was taking a quick look at the embedded-nal-async traits, and they are an exact replica of the older nb ones, except done with async futures based on GATs (just like embedded-hal-async and everything async-related in embedded-svc).

Nothing wrong with that and quite the opposite - this is a great start! And a way for us to implement e.g. an async mqtt client in Rust which works on bare-metal (esp-wifi + esp-hal) as well as in ESP-IDF (esp-idf-sys + esp-idf-hal + esp-idf-svc), as I can (I think) quite easily map the embedded-nal-async traits to ESP-IDF LWIP via Rust STD and smol's async reactor.

But this new development actually reinforces my view that embedded-nal and especially smoltcp-nal's NetworkStack should probably not be part of the Wifi trait impl, because NetworkStack is all about the old nb based embedded-nal and not the new async one.

That is, unless someone comes with a clever PR to smoltcp-nal, where the existing NetworkStack struct is extended to also implement the new async traits, by utilizing the async feature of smoltcp!

from esp-wifi-sys.

FYI a summary of a discussion yesterday on the embassy Matrix channel:

@Dirbaio pointed out the obvious (in retrospective) - the new embedded-nal-async traits have a design flaw where you can't send/recv on multiple sockets simultaneously, because the api takes the "stack" as a &mut self.

There are other design issues as well, as in accept returning a fresh socket instance where to await on it and on the previously accepted sockets you have to either use FuturesUnordered (as I do), or have a trait abstraction for a "spawner" (like this one) so that you can spawn a new task which would process the newly-accepted socket.

Both of these (FuturesUnordered and traitifying the notion of a Spawner) are incompatible with embassy-rs's executor as they do require allocations, while embassy is a completely static no-alloc thing. There's also an issue with the wakers of FuturesUnordered being a special thing incompatible with the embassy ones, but that's a whole other can of worms.

Perhaps an easier approach which is also future-compatible with embassy would be the one discussed a few days ago on the Matrix channel - standardize at least the notion of asynchronous no_std Reader/Writer, then write e.g. async MQTT client, HTTP client / server only with these traits, where it is up to the user of e.g. the HTTP server to spin the socket accept loop in whatever platform-specific way that happens. Effort to have a common set of no_std async Reader/Writer (and sync ones too) here.

So IMO we should continue for now with the original plan of removing the dependency on embedded-nal from the Wifi trait implementation and instead having it depend directly on the smoltcp interface notion. We can still have utilities for e.g. asynchronous work with smoltcp, as in opening a socket, but these likely will be executor-specific (here's the embassy one), as I don't yet see a way to come up with a set of networking traits than can abstract over the no-alloc Embassy executor on one hand, and the alloc ones like my own creations + the standard ones that do work on the ESP-IDF (futures-rs, smol, etcetera) on the other.

from esp-wifi-sys.

@lulf ^^^ I'm very interested in your comments / suggestions/ corrections as well. Shall we move this discussion to embedded-nal?

from esp-wifi-sys.

@ivmarkov I think your comment sums it up well! I think there is probably some changes we can make to the embedded-nal-async traits to have the 'shared stack' situation work in some situations, but I've not played enough with that concept yet and focused on 'client usage' for now. No matter what is done with embedded-nal-async, I think it's good to keep any http, mqtt, tls client work with just the bare Read/Write async traits from embassy (soon moved to embedded_io). Then, embedded-nal-async support might be a nice convenience for some, but not 'forced' on anyone if made 'optional'.

from esp-wifi-sys.

@lulf I think we have to solve the "shared stack" issue for clients as well, or else how can I have an mqtt client and an http client simultaneously, each spinning its own async loop and its own tcp socket?

from esp-wifi-sys.