[RSDK-10288] sta disconnect events log #452
Conversation
micro-rdk/src/esp32/conn/network.rs
Outdated
| let disconn: *const esp_idf_svc::sys::wifi_event_sta_disconnected_t = | ||
| unsafe { core::mem::transmute(ev_data) }; | ||
| let reason: super::wifi_error::WifiErrReason = (*disconn).reason.into(); | ||
| log::error!("sta disconnected: {:?}", reason); |
There was a problem hiding this comment.
I believe that the intention of this ticket (which was admittedly light on details) was to actually inspect the internal structure of the StaConnected and StaDisconnected events up around line 200 and report interesting context around these events (probably disconnection is generally more interesting).
There was a problem hiding this comment.
that's probably a better place than where I found originally, yeah.
I shifted the logging to there if you'd like to take a look.
So does that mean we want to look deeper into StaConnected as well for this ticket? It seems we would be able to log the ssid but not much else, https://docs.esp-rs.org/esp-idf-svc/esp_idf_svc/wifi/struct.StaConnectedRef.html.
I guess it could make sense given we now support multiple wifi, but need to test to see what the output is currently so we're not adding 'duplicate' logs
| sl_stack.subscribe::<WifiEvent, _>(move |event: WifiEvent| match event { | ||
| WifiEvent::StaDisconnected(disconnected) => { | ||
| let reason: super::wifi_error::WifiErrReason = disconnected.reason().into(); | ||
| log::info!("StaDisconnected event received: {:?}", reason); |
There was a problem hiding this comment.
I wouldn't bother writing StaDisconnected event here - it isn't really a meaningful bit of information for a consumer of the logs.
Instead, let's focus the log on user facing info: let's log the SSID, the reason, and, the RSSI. Something like:
log::info!("received a WiFi disconnection event for SSID {} (RSSI {}) with reason {}"
| } | ||
| })?; | ||
| WifiEvent::StaConnected(_) => { | ||
| log::info!("wifi connected event received"); |
There was a problem hiding this comment.
Similar thoughts. I think it is valuable to at least log the SSID here:
log::info!("received a WiFi connection event for SSID {}"
| sl_stack.subscribe::<WifiEvent, _>(move |event: WifiEvent| match event { | ||
| WifiEvent::StaDisconnected(disconnected) => { | ||
| let reason: super::wifi_error::WifiErrReason = disconnected.reason().into(); | ||
| log::info!("StaDisconnected event received: {:?}", reason); |
There was a problem hiding this comment.
From your slack message, I can see that logging the reason with {:?} is going to look like GroupKeyUpdateTimeout(16). However, the 16 isn't really helpful there, maybe. It makes it look like the 16 is somehow parameterizing the GroupKeyUpdateTimeout, but it is really just the underlying code. Maybe Debug on WifiErrReason isnt' the right way to log it.
There was a problem hiding this comment.
I can still see use for exposing the code itself, at least in the Unrecognized case I added as the catch-all. I can directly impl Display so we can still throw in a Debug statement if it's ever needed.
| let subscription = | ||
| sl_stack.subscribe::<WifiEvent, _>(move |event: WifiEvent| match event { | ||
| WifiEvent::StaDisconnected(disconnected) => { | ||
| let reason: super::wifi_error::WifiErrReason = disconnected.reason().into(); |
There was a problem hiding this comment.
Maybe import it, since it will also get used in the connected event below per my comment?
| use esp_idf_svc::sys::*; | ||
| use core::ffi::c_uint; | ||
|
|
||
| impl From<u16> for WifiErrReason { |
There was a problem hiding this comment.
I think I'd rather get a compilation error if we try to build on a platform where C's unsigned int isn't 32 bits. That's a really rare platform.
| wifi_err_reason_t_WIFI_REASON_NO_AP_FOUND_W_COMPATIBLE_SECURITY => { | ||
| Self::NoApFoundWithCompatibleSecurity(v) | ||
| } | ||
| wifi_err_reason_t_WIFI_REASON_NO_AP_FOUND_IN_AUTHMODE_THRESHOLD => { | ||
| Self::NoApFoundInAuthModeThreshold(v) | ||
| } | ||
| wifi_err_reason_t_WIFI_REASON_NO_AP_FOUND_IN_RSSI_THRESHOLD => { | ||
| Self::NoApFoundInRssiThreshold(v) | ||
| } | ||
| _ => Self::Unrecognized(v), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum WifiErrReason { | ||
| Unspecified(c_uint), | ||
| AuthExpire(c_uint), | ||
| AuthLeave(c_uint), | ||
| AssocExpire(c_uint), | ||
| AssocTooMany(c_uint), | ||
| NotAuthed(c_uint), | ||
| NotAssoced(c_uint), | ||
| AssocLeave(c_uint), | ||
| AssocNotAuthed(c_uint), |
There was a problem hiding this comment.
I have to wonder if there isn't some macro crate out there that can emit this kind of boilerplate for us given a C enum.
Also surprised that esp-idf-{svc,sys} doesn't do this for us.
I took this approach with the enum because:
esp_idf_sysc_uintwhich are typicallyu32however they can beu16depending on the platformi32s, I wasn't confident that setting the variants directly would be consistent between the bindings and the enum (enum WifiError { Roaming = esp_idf_sys::wifi_err_ROAMING ... }).wifi_event_sta_disconnected_t'sreasonfield is defined as au8in the bindingsMost of the time was spent hand-rolling the enum a few times as I realized the above.
still needs to be tested to see that the error messages are present and useful