implement wifi event handling with data #2453
Merged
Conversation
This file contains 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
add to wifi_access_point example
|
By the way, I get a linker error trying to build the example on the latest commit, even before changing anything, and had to fix it with: - esp-wifi-sys = { version = "0.6.0", git = "https://github.com/esp-rs/esp-wifi-sys.git", rev = "a3e2ed64e9095f120bbcebe6287ddf62760774db"}
+ esp-wifi-sys = { version = "0.6.0" }Error: |
bugadani
requested changes
Nov 3, 2024
esp-wifi/src/wifi/event.rs
Outdated
Comment on lines
5
to
7
| mod sealed { | ||
| pub trait Sealed {} | ||
| } |
There was a problem hiding this comment.
Please move this to esp_wifi::private::Sealed. We're adding this trait there in a different PR and we don't need two.
There was a problem hiding this comment.
I was going to move the get_handler method into the Sealed trait so it can't be called by user-code and can be changed later. Should I still move the sealed trait?
esp-wifi/src/wifi/event.rs
Outdated
| pub type Handler<T> = dyn FnMut(&T) + Sync + Send; | ||
| // fn default_handler<'a, T>(_: &'a T) {} | ||
| /// Data for a wifi event which can be handled by an event handler. | ||
| pub trait WifiEventData: sealed::Sealed + Sized + 'static { |
There was a problem hiding this comment.
This trait is completely unnecessary. You can, in your macro, define a newtype around the event types. That way we could avoid the extremely confusing "static method on C type" situation we have here.
There was a problem hiding this comment.
I would still need the trait though? How else do you make a static per type? Are you suggesting making a newtype in rust for each c type and making that the public interface? I must not understand what you mean, could you clarify?
|
This isn't going in a direction I'm comfortable with, but I'll wait for you to mark this as ready before passing judgement. |
- match simpler api from `std::panic::update_hook` - do not assume size_of in prelude - make all events handleable - box static instead of leak
|
bugadani
reviewed
Nov 5, 2024
bugadani
reviewed
Nov 5, 2024
bugadani
reviewed
Nov 5, 2024
- pass critical section to event handlers - comment on perf of Box<ZST> - don't pass `prev` in `update_handler`, instead call previous handler first unconditionally
bugadani
approved these changes
Nov 5, 2024
Maybe your local Cargo cache is damaged? |
|
One concern I have is that currently we are burning cycles in the (most common) case when no one is interested in the events (that's on C6 - I guess it's more expensive on multi-core chips) It's not the end of the world given events are not emitted at a high frequency (not sure about things like Probably the best thing to do would be to have a way to mask out events no one is interested in as early as possible (before doing anything with them) ... but that would be out of scope for this PR |
How did you measure the cycles? I'd be interested in trying this myself. Is this a comparison of before/after the pr? It would be strange to me that this be a significant performance drop with no registered handler. |
|
I've changed the critical section to be passed in to |
|
The numbers were just the measured cycles calling the dispatch function w/o any registered callbacks. On our RISC-V chips the code to do that is unsafe {
asm!(
"
csrrwi x0, 0x7e0, 1 #what to count, for cycles write 1 for instructions write 2
csrrwi x0, 0x7e1, 0 #disable counter
csrrwi x0, 0x7e2, 0 #reset counter
csrrwi x0, 0x7e1, 1 #enable counter
"
);
}
// WHAT WE WANT TO MEASURE HERE!!!! e.g. calling `dispatch`
let mut perf_counter: u32 = 0;
unsafe {
asm!(
"
csrr {x}, 0x7e2
",
options(nostack),
x = inout(reg) perf_counter,
)
};
info!("took = {} cycles", perf_counter); |
|
Numbers I got from testing on esp32:
no registered event handler dispatch asmdispatch same handler logdispatch different handler log |
|
Ok this definitely looks like cache misses - not really something that should be touched in this PR. I'm personally not very convinced by the chained callbacks but I'm fine with it - will leave the last word to someone else in the team to decide (i.e. approve) |
If it helps, using |
|
@bjoernQ Is there something I can do to move this along at this point? I'd like to avoid having to merge main to keep the branch up-to-date. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Fixes: #2412
Implement event handling for apsta{,dis}connect and probe
I kind of guessed which events are associated with which data. Is there a better place for this?
Added to wifi_access_point example, I don't know it would be preferred to copy-paste it as a new example.
Currently requires
Boxing and somewhat uglyupdatefunction. I'm going to work on this a bit more before undrafting the request, but would like feedback on:If the approach should be modified completely instead of following the
std::panic::hookapis. The implementation here doesn't support removing a single handler, but I'm not sure when that would be preferred over checking a flag in the handler itself.Testing
Ran the modified example on
esp32and connected to it. Gotprintlnsfor thestaconnected and disconnected events as expected. Didn't test probe event.