-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: waiter and some small wrappers #13
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
use esp_idf_sys::*; | ||
|
||
pub fn get_default_efuse_mac() -> Result<[u8; 6], EspError> { | ||
let mut mac = [0; 6]; | ||
unsafe { esp!(esp_efuse_mac_get_default(mac.as_mut_ptr()))? } | ||
Ok(mac) | ||
} | ||
|
||
pub fn restart() { | ||
ivmarkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
unsafe { esp_restart() }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
pub mod common; | ||
pub mod cstr; | ||
pub mod net; | ||
pub mod wait; | ||
|
||
mod stubs; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
use core::time::Duration; | ||
#[cfg(feature = "std")] | ||
use std::sync::{Condvar, Mutex}; | ||
|
||
use embedded_svc::mutex::Mutex as _; | ||
#[cfg(not(feature = "std"))] | ||
use esp_idf_sys::*; | ||
|
||
#[cfg(not(feature = "std"))] | ||
use super::time::micros_since_boot; | ||
|
||
pub struct Waiter { | ||
#[cfg(feature = "std")] | ||
cvar: Condvar, | ||
#[cfg(feature = "std")] | ||
running: Mutex<bool>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to see what it would take to switch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are still willing to drive e.g. the Not really tested but at least compiles and passed fmt and clippy checks. And used in all of ping, eth and wifi. Hopefully that implementation makes clearer some of the changes I'm asking for here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, you wrote your own now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Up to you. You can also take bits and pieces of mine and update yours with these. The reason I took the effort to actually write something is because you might feel that I'm nitpicking on everything which is definitely not the impression I want to leave - so I thought - I can at least take the effort to justify -in code - including how it will be used in eth and wifi code - why the waiter needs to be generified, you know. :) |
||
#[cfg(not(feature = "std"))] | ||
running: EspMutex<bool>, | ||
} | ||
|
||
impl Waiter { | ||
pub fn new() -> Self { | ||
Waiter { | ||
#[cfg(feature = "std")] | ||
cvar: Condvar::new(), | ||
#[cfg(feature = "std")] | ||
running: Mutex::new(false), | ||
#[cfg(not(feature = "std"))] | ||
running: EspMutex::new(false), | ||
} | ||
} | ||
|
||
pub fn start(&self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In relation to the above generification, the |
||
self.running.with_lock(|running| *running = true); | ||
} | ||
|
||
#[cfg(feature = "std")] | ||
pub fn wait(&self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will have to take a |
||
if !self.running.with_lock(|running| *running) { | ||
return; | ||
} | ||
|
||
let _running = self | ||
.cvar | ||
.wait_while(self.running.lock().unwrap(), |running| *running) | ||
.unwrap(); | ||
} | ||
|
||
#[cfg(not(feature = "std"))] | ||
pub fn wait(&self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
while self.running.with_lock(|running| *running) { | ||
unsafe { vTaskDelay(500) }; | ||
} | ||
} | ||
|
||
/// return = !timeout (= success) | ||
#[cfg(feature = "std")] | ||
pub fn wait_timeout(&self, dur: Duration) -> bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
if !self.running.with_lock(|running| *running) { | ||
return true; | ||
} | ||
|
||
let (_running, res) = self | ||
.cvar | ||
.wait_timeout_while(self.running.lock().unwrap(), dur, |running| *running) | ||
.unwrap(); | ||
|
||
return !res.timed_out(); | ||
} | ||
|
||
/// return = !timeout (= success) | ||
#[cfg(not(feature = "std"))] | ||
pub fn wait_timeout(&self, dur: Duration) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
let now = micros_since_boot(); | ||
let end = now + dur.as_micros(); | ||
|
||
while self.running.with_lock(|running| *running) { | ||
if micros_since_boot() > end { | ||
return false; | ||
} | ||
unsafe { vTaskDelay(500) }; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
#[cfg(feature = "std")] | ||
pub fn notify(&self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to take a state |
||
*self.running.lock().unwrap() = false; | ||
self.cvar.notify_all(); | ||
} | ||
|
||
#[cfg(not(feature = "std"))] | ||
pub fn notify(&self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
self.running.with_lock(|running| *running = false); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
use core::convert::TryInto; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's be careful when extending the API surface with all sorts of utilities, until we have good use cases for those. And ideally - traits in |
||
|
||
use esp_idf_sys::*; | ||
|
||
pub fn micros_since_boot() -> u64 { | ||
unsafe { esp_timer_get_time() }.try_into().unwrap() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel very comfortable wrapping this function, unless there are multiple - as in many - many use-cases where we need it.
If you are just going to use it in e.g. the smtp implementation and a couple of others - then I rather say - we pass on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, like the nvs api is mostly to have safe wrappers around esp idf functions, so I don't have to have unsafe code in my project
I suppose if this is mostly meant as an implementation of
embedded-svc
there could be another crate just for safe wrappers (which this crate could make use of)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not necessarily just an implementation of
embedded-svc
. Take a look atEspNetif
. This is currently not having a correspondingembedded-svc
trait, as I'm not sure that the notion of a network interface can be easily exposed everywhere (as in on all platforms) as a top level notion.But
esp-idf-svc
IS about exposing self-contained pieces of functionality, rather than individual functions on "as needed" basis, because the moment you expose an individual function "because I just need it for this one use case", you have to maintain and evolve it, which is not easy and results in breaking changes if you don't think upfront how this is going to evolve. If I have to give you example precisely about theget_default_efuse_mac
function you've exposed: this is just one API call from a whole set of APIs:esp_mac.h
. So, how do we think about the whole set of APIs which are out there inesp_mac.h
?misc
, or do we need to dedicate a separate namespace (a.k.a. a Rust module) for them, rather than keeping them inmisc
? if so, how about we dedicate a module for all of these, from the get-go?get_default_efuse_mac
- for whatever reasons - at one place, perhaps it is not worth it exposing it as a public, safe API in the first place, as it won't be having that many usages anyway?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yes I understand your concerns
The reason I exposed it was to generate a unique Wi-Fi name (which seems to be a common use case)
But exposing it like this is probably too low level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by
unique Wi-Fi name
? A MAC (as in OSI layer 2 ethernet MAC)?Huh. I was expecting that this is somehow handled by the ESP WiFi driver itself. Now, if it is not, we should fix the
EspWifi
code itself to derive a useful MAC or something. Ditto forEspEth
.But, if you don't need this MAC in the captive DNS portal, or in the SNTP implementation (and I'm curious why would you need a layer 2 thing in layer 3+ protocols anyway), then I say let's open a new issue describing the problem, and then fix it in the
EspWifi
andEspEth
driver wrappers themselves.