feat: waiter and some small wrappers #13

ForsakenHarmony · 2021-10-26T02:23:36Z

The Waiter with a condvar can probably be used in the wifi module, saw some comments about it there

Didn't know where else to put the smaller changes

ivmarkov · 2021-10-31T06:40:21Z

src/misc.rs

@@ -0,0 +1,11 @@
+use esp_idf_sys::*;
+
+pub fn get_default_efuse_mac() -> Result<[u8; 6], EspError> {


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.

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)

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)

No it is not necessarily just an implementation of embedded-svc. Take a look at EspNetif. This is currently not having a corresponding embedded-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 the get_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 in esp_mac.h?

Are we expecting to expose all of these over time? If yes, would all of these be in misc, or do we need to dedicate a separate namespace (a.k.a. a Rust module) for them, rather than keeping them in misc? if so, how about we dedicate a module for all of these, from the get-go?

if we don't expect that this whole API would be necessary, and we'll only need 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?

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

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 for EspEth.

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 and EspEth driver wrappers themselves.

ivmarkov · 2021-10-31T06:41:56Z

src/time.rs

@@ -0,0 +1,7 @@
+use core::convert::TryInto;


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 embedded-svc or embedded-hal.

src/misc.rs

ivmarkov · 2021-11-04T07:47:48Z

src/misc.rs

@@ -0,0 +1,11 @@
+use esp_idf_sys::*;
+
+pub fn get_default_efuse_mac() -> Result<[u8; 6], EspError> {


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)

No it is not necessarily just an implementation of embedded-svc. Take a look at EspNetif. This is currently not having a corresponding embedded-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 the get_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 in esp_mac.h?

Are we expecting to expose all of these over time? If yes, would all of these be in misc, or do we need to dedicate a separate namespace (a.k.a. a Rust module) for them, rather than keeping them in misc? if so, how about we dedicate a module for all of these, from the get-go?

if we don't expect that this whole API would be necessary, and we'll only need 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?

ivmarkov · 2021-11-04T12:58:48Z

src/private/wait.rs

+    #[cfg(feature = "std")]
+    cvar: Condvar,
+    #[cfg(feature = "std")]
+    running: Mutex<bool>,


I'm trying to see what it would take to switch EspEth & EspWifi to use the Waiter. For that to happen, I think the Waiter needs to be generified so that running - instead of a mutex around a simple bool - becomes state - a mutex around some abstract state T. The idea is that you would like to wait on changes in that state, and get notified (when std is enabled) around changes to that state. The state might be as simple as running vs not running, or as complex as the Shared state of EspEth and EspWifi.

If you are still willing to drive e.g. the sntp implementation which needs (?) a "waiter", here's one that (ideally) should work for sntp & ping, but also for eth & wifi:
https://github.com/esp-rs/esp-idf-svc/compare/waitable

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.

If you are still willing to drive e.g. the sntp implementation which needs (?) a "waiter", here's one that (ideally) should work for sntp & ping, but also for eth & wifi:

Oh, you wrote your own now?
Should I close this then?

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. :)

ivmarkov · 2021-11-04T14:52:15Z

src/private/wait.rs

+        }
+    }
+
+    pub fn start(&self) {


In relation to the above generification, the start method should probably be retired, and the new method from above should take an initial state T, that is pushed into the mutex.

ivmarkov · 2021-11-04T14:52:55Z

src/private/wait.rs

+    }
+
+    #[cfg(feature = "std")]
+    pub fn wait(&self) {


This will have to take a Fn(&T) closure parameter that evaluates whether the desired state is good enough for the waiting to be over.

ivmarkov · 2021-11-04T14:53:04Z

src/private/wait.rs

+    }
+
+    #[cfg(not(feature = "std"))]
+    pub fn wait(&self) {


ivmarkov · 2021-11-04T14:53:21Z

src/private/wait.rs

+
+    /// return = !timeout (= success)
+    #[cfg(feature = "std")]
+    pub fn wait_timeout(&self, dur: Duration) -> bool {


ivmarkov · 2021-11-04T14:53:29Z

src/private/wait.rs

+
+    /// return = !timeout (= success)
+    #[cfg(not(feature = "std"))]
+    pub fn wait_timeout(&self, dur: Duration) {


ivmarkov · 2021-11-04T14:54:19Z

src/private/wait.rs

+    }
+
+    #[cfg(feature = "std")]
+    pub fn notify(&self) {


Needs to take a state T that is pushed to the mutex before notifying. We also need to probably rename this to set or something.

ivmarkov · 2021-11-04T14:54:28Z

src/private/wait.rs

+    }
+
+    #[cfg(not(feature = "std"))]
+    pub fn notify(&self) {


ivmarkov · 2021-11-13T13:41:07Z

49d66ed

feat: waiter and some small wrappers

599c9b0

This was referenced Oct 26, 2021

feat: sntp #14

Closed

feat: basic rtos task api #15

Closed

feat: double reset detector #16

Open

ForsakenHarmony marked this pull request as ready for review October 26, 2021 02:36

ForsakenHarmony mentioned this pull request Oct 26, 2021

feat: captive portal dns (+lwip apis) #20

Closed

ivmarkov added the hacktoberfest-accepted label Oct 31, 2021

ivmarkov requested changes Oct 31, 2021

View reviewed changes

ivmarkov requested changes Nov 4, 2021

View reviewed changes

ivmarkov closed this Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: waiter and some small wrappers #13

feat: waiter and some small wrappers #13

ForsakenHarmony commented Oct 26, 2021 •

edited

Loading

ivmarkov Oct 31, 2021

ForsakenHarmony Nov 4, 2021

ivmarkov Nov 4, 2021

ForsakenHarmony Nov 5, 2021

ivmarkov Nov 5, 2021 •

edited

Loading

ivmarkov Oct 31, 2021 •

edited

Loading

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021 •

edited

Loading

ivmarkov Nov 5, 2021 •

edited

Loading

ForsakenHarmony Nov 5, 2021

ivmarkov Nov 5, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov Nov 4, 2021

ivmarkov commented Nov 13, 2021

		@@ -0,0 +1,11 @@
		use esp_idf_sys::*;

		pub fn get_default_efuse_mac() -> Result<[u8; 6], EspError> {

feat: waiter and some small wrappers #13

feat: waiter and some small wrappers #13

Conversation

ForsakenHarmony commented Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

ivmarkov Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

ivmarkov Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivmarkov commented Nov 13, 2021

ForsakenHarmony commented Oct 26, 2021 •

edited

Loading

ivmarkov Nov 5, 2021 •

edited

Loading

ivmarkov Oct 31, 2021 •

edited

Loading

ivmarkov Nov 4, 2021 •

edited

Loading

ivmarkov Nov 5, 2021 •

edited

Loading