Is it possible to make ESP-NOW duplex about esp-wifi HOT 11 CLOSED

Thanks! Generally, this sounds like a good idea to me

Moreover, they mentioned that we must wait for the send callback of esp_now_send. I noticed that current implementation does not count this. Maybe there should be another issue for this.

Didn't knew that t.b.h. - I agree adding an issue for this is a good thing

from esp-wifi.

Ok. I will open a pull request for the previous one after testing my implementation on the hardware. As for the latter one, I will write a demo to check the potential impact of a fix and open another issue later.

from esp-wifi.

I asked the developers of esp-now and they said that esp-now send is thread safe (espressif/esp-now#87). So I think my idea is safe to implement.

Moreover, they mentioned that we must wait for the send callback of esp_now_send. I noticed that current implementation does not count this. Maybe there should be another issue for this.

from esp-wifi.

esp-idf-svc implemented esp now in a way where all config, send and receive methods do not take mutable reference of self. Why not do what they did so we can just share references of EspNow struct between threads.

from esp-wifi.

esp-idf-svc implemented esp now in a way where all config, send and receive methods do not take mutable reference of self. Why not do what they did so we can just share references of EspNow struct between threads.

To be honest I never looked into that but I think it's an easy and good enough solution for the problem. I don't see why we shouldn't do the same 👍

from esp-wifi.

Closed in favor of #237

from esp-wifi.

@bjoernQ Sorry I just realized that I've made a fundamentally wrong design in #232, which makes EspNow totally unusable when performing data sending in multiple tasks (Sorry for not having it tested thoroughly).

Here I used an AtomicWaker to wake the future when callback is invoked.

https://github.com/esp-rs/esp-wifi/blob/e685dfcb08f249f5c6402b2d8ebb5a6a152f40bd/esp-wifi/src/esp_now/mod.rs#L660

However, what I didn't considered is the register method overwrites the previous waker. So when two tasks task1 and task2 are performing data sending, the following event series can occur:

[task1] esp_now_send
[task1] SendFuture::poll -> ESP_NOW_TX_WAKER = task1.waker
[task2] esp_now_send
[task2] SendFuture::poll -> ESP_NOW_TX_WAKER = task2.waker
[task1] callback -> wakes task2
[task2] Future returns with status of sending in task1 

Finally, task1 will never get waked while task2 gets wrong status.

I believe your idea of wait before sending can help solving this issue. I'm currently working on this.

from esp-wifi.

Ah I see. Sorry I haven't noticed that before.

Maybe the easiest fix would be to combine what we have (awaitable result) and wait before sending. That way it's always just one "in-flight" send and the user can check the result when needed

from esp-wifi.

I agree that this is a good enough solution for the sync version. However, after having tried several different approaches for the async version, I realized that it's way more difficult than I've expected.

My approach

Initially I used a simple global flag to indicate whether the previous sending is completed, i.e. the send callback is invoked. Let's call it PREV_SEND_COMPLETE, we may implement the future like this:

fn poll(...) -> ... {
    if !PREV_SEND_COMPLETE {
        return Poll::Pending
    }
    ...
    esp_now_send(...)
    ...
} 

However, when the future is firstly polled with Poll::Pending returned, it will probably never get polled again since the future will never be awoke by a waker.

So we should register a waker for current future before returning Poll::Pending, and the waker should be called when the previous sending completes. Finally a chain-like relationship will be built between all spawned future:

Note that we need to add future to this chain at its first poll.

This idea still have drawbacks:

1. It's pretty complex. I'm not sure if it's suitable to implement it in this crate.
2. It requires dynamic allocation since the length of that chain is unknown. (or static sized queue with additional checks)

I have a feeling that this idea is over designed. But I cannot find a simpler design that really works.

Another Idea

If send_async takes mutable borrowed receiver and we adopt to the type state design in my first comment. User will need a mutex to send data in multiple tasks, thus making the futures naturally non-overlapping. I think this would be better than synchronizing the futures inside the API implementation and also provides more flexibility.

from esp-wifi.

I agree, the async implementation shouldn't be over engineered.

Your suggestion sounds good to me

from esp-wifi.

We can finally closed this as #240 got merged.

from esp-wifi.