Wait for esp now send callback to be called after sending

[M4tsuri]

This fixes #229. However, I think there might be a better way rather than a busy loop for the sync implementation.

Also, I noticed the send time (time between sending data and callback is called) after the fix is way too high.

Currently it takes about 10ms for a successful sending (measured with this example):

While it only takes about 1ms in my C implementation:

This PR is ready until these problems are settled:

• Find a better way for the sync implementation
• Find out the cause of high send time and try to fix it

[bjoernQ]

Just an idea: instead of waiting for the send_cb after sending and before returning from the function we could wait for it before the sending

Under normal conditions other code will run after sending and next time the function is called the callback probably already got called

It will not solve the general problem but might mitigate it

[M4tsuri]

Sorry I got something wrong, I'll fix it soon.

[M4tsuri]

I found I introduced a bug in 0e6037e, which can lead to repeated sending when using a busy-polling executor

[M4tsuri]

Just an idea: instead of waiting for the send_cb after sending and before returning from the function we could wait for it before the sending

Under normal conditions other code will run after sending and next time the function is called the callback probably already got called

It will not solve the general problem but might mitigate it

Thanks for your advice. I think I've fixed the implementation according to your observation, but don't know if this is what you expected.

I did more tests and unfortunately the problem is still there. I put all of my code in this repo. I wonder if you can reproduce my test result.

[bjoernQ]

I can reproduce your measurements and I guess it's mostly caused by the fact that our scheduler is a naive implementation - basically completely round robin with fixed time slices.

Given we switch the tasks at a quite low rate ( https://github.com/esp-rs/esp-wifi/blob/dd8765011ac2a8c65bda7c7d85205649499002ef/esp-wifi/src/timer_esp32c3.rs#L21 ) most of the delay is caused by waiting for the main task to get a chance to run

I can locally go up to 1000Hz - then it looks much better:

INFO - avg send time: 1752 us, err: 0
INFO - avg send time: 1754 us, err: 0
INFO - avg send time: 1793 us, err: 0
INFO - avg send time: 1745 us, err: 0
INFO - avg send time: 1753 us, err: 0
INFO - avg send time: 1756 us, err: 0

Still not the same performance (me might have a bit more overhead) but much better.

I guess with the default you won't be able to make the stress-test produce better numbers.

For a normal use-case however, I guess my comment in #232 (comment) should help a lot.

What I meant there is probably easiest to explain for the non-async case:

Changing the initial value of ESP_NOW_SEND_CB_INVOKED:
static ESP_NOW_SEND_CB_INVOKED: AtomicBool = AtomicBool::new(true);

And changing send like this:

    /// Send data to peer
    ///
    /// The peer needs to be added to the peer list first
    pub fn send(&mut self, dst_addr: &[u8; 6], data: &[u8]) -> Result<(), EspNowError> {
        while !ESP_NOW_SEND_CB_INVOKED.load(Ordering::Acquire) {}

        let mut addr = [0u8; 6];
        addr.copy_from_slice(dst_addr);
        ESP_NOW_SEND_CB_INVOKED.store(false, Ordering::Release);
        check_error!({ esp_now_send(addr.as_ptr(), data.as_ptr(), data.len() as u32) })?;

        if ESP_NOW_SEND_STATUS.load(Ordering::Relaxed) {
            Ok(())
        } else {
            Err(EspNowError::SendFailed)
        }
    }

Should help a lot. Given that in a usual application sending data is on a much lower frequency than 10ms there will be no wait needed.

Unfortunately applying the same for async is a bit tricky, I guess.

So, in the end (when applying the above pattern) it should be pretty much fine this way

[M4tsuri]

Got it. So we are actually waiting for the previous sending to complete instead of the current one.

[bjoernQ]

Got it. So we are actually waiting for the previous sending to complete instead of the current one.

Yes - that's the idea. Thanks for taking care of this

[M4tsuri]

But there is also a drawback where only in the next send can we know whether the current one has succeeded. The send and wait for callback method provides guaranteed delivery on mac layer when user calls it while wait for previous callback and send provides guarantee only when there is another send call.

Generally I come up with another idea based on yours. By returning a SendCallbackWaiter struct, user can choose where or whether to wait for the callback on their own choice:

struct SendCallbackWaiter;

impl SendCallbackWaiter {
    pub fn wait(self) -> Result<(), EspNowError> {
        while !ESP_NOW_SEND_CB_INVOKED.load(Ordering::Acquire) {}
        
        if ESP_NOW_SEND_STATUS.load(Ordering::Relaxed) {
            Ok(())
        } else {
            Err(EspNowError::SendFailed)
        }
    }
}

pub fn send(&mut self, dst_addr: &[u8; 6], data: &[u8]) -> Result<SendCallbackWaiter, EspNowError>;

And this is essentially the idea of returning a future in async, where users can just drop a future or wait for it. So it's also easier to implement for async.

Currently the send time is bounded by the scheduler, this also provides a temporary workaround to break the limit by dropping the waiter and future.

[bjoernQ]

True - haven't thought about the possible error.

I guess it's a good idea - we have similar approaches (for other reasons) in esp-hal, too

[M4tsuri]

I

• implemented the previous mentioned logic
• added docs
• tested on my hardware

I guess this is ready for review now.

[bjoernQ]

Looks good to me, thanks! Would you mind copying the adapted examples over to the other chips' directories, too? I guess you cannot test it but I can do that (and I don't expect any problems there)

[M4tsuri]

Done. All examples are updated.

[bjoernQ]

LGTM - Thank you!