Remove `EspWifiInitFor` & more granular init/deinit per driver #2301

MabezDev · 2024-10-08T10:24:59Z

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 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Rework EspWifiInit to EspWifiController - bikeshedding welcome
No longer require EspWifiInitFor on initialize
Add Drop impls for each driver, and add Drop for EspWifiController to fully deinit the stack
deinit no longer returns the used resources, instead the peripheral ref pattern will take care of this

I haven't updated all the examples yet, I'll do so after a bit of review.

Testing

Try this modified wifi_dhcp example: https://gist.github.com/MabezDev/78aac5133c482a56db9600697c179650

esp-wifi/src/ble/controller/mod.rs

esp-wifi/src/lib.rs

esp-wifi/src/timer/mod.rs

esp-wifi/src/timer/riscv.rs

examples/Cargo.toml

esp-wifi/src/timer/xtensa.rs

MabezDev · 2024-10-22T15:37:46Z

Nearly done updating the examples. esp-now is causing me a considerable headache right now as it can be initialized on its own or from an existing wifi driver (ish). This makes it a bit tricky to reason about when to deinit wifi - will need a bit longer to figure this out.

MabezDev · 2024-10-30T15:56:37Z

CI is green(ish), this is now ready for another review round.

esp-wifi/src/lib.rs

bjoernQ

Looks good and works fine - happy to see that awkward EspWifiInitialization and EspWifiInitFor go away

bugadani · 2024-10-31T14:39:18Z

esp-wifi/src/lib.rs

@@ -361,7 +385,7 @@ impl EspWifiTimerSource for TimeBase {
 /// ```
 pub fn init<'d, T: EspWifiTimerSource>(
    timer: impl Peripheral<P = T> + 'd,
-    _rng: hal::rng::Rng,
+    _rng: impl EspWifiRngSource,


This could be impl Peripheral if Trng implements Peripheral without any additional traits.

It could be, but the problem is that we don't want to completely own the Rng or Trng here, users will still want to use it in their own applications see: #2372

And how does this (a trait that requires passing by value) help in not completely owning the rng?

Rng driver is clone, Trng might be clone soon. RNG (the raw peripheral) will take exclusive access whether its via &mut Self or Self and not allow any other part of the system to use Rng.

I think you misunderstand what I want. I'm not proposing requiring using the raw peripheral. I'm proposing accepting drivers that work with the peripheral. Making Rng and Trng implement Peripheral allows us to pass both of those, or a mutable reference to them (or just the RNG peripheral directly) and esp-wifi wouldn't know the difference.

I.e. I'm suggesting that instead of a custom trait we have:

impl Peripheral for Rng { type P = RNG; ... } impl Peripheral for Trng { type P = RNG; ... }

In that case we may still go this way, just using Peripheral. But I'll not force the issue.

With Trng also impl Peripheral? That could work. Just wondering if there a case where someone might need to write Peripheral, because they wouldn't be able to in this case.

Peripheral

That is an interesting question, drivers absolutely requiring true randomness. I'm not sure, but we're not supporting it right now, either.

The "funny" thing is that RNG will give true random numbers when the RF peripherals are enabled (but we still need #2443)

bugadani · 2024-10-31T14:40:53Z

esp-wifi/src/lib.rs

@@ -287,7 +288,7 @@ impl<'d> Drop for EspWifiController<'d> {
 ///
 /// This trait is meant to be used only for the `init` function.
 /// Calling `timers()` multiple times may panic.
-pub trait EspWifiTimerSource {
+pub trait EspWifiTimerSource: private::Sealed {


Isn't this a bit of an overkill?

Maybe, but is there a down side to doing it?

More code :)

I suppose, but I can't see a reason to keep this trait open of downstream crates to impl.

I'm happy to remove it, but I'd like a more compelling argument than removing a few lines of hidden boiler plate impls.

MabezDev · 2024-10-31T15:14:05Z

Skip changelog for esp-hal changes, this is now ready for another round of review.

esp-wifi/src/ble/controller/mod.rs

esp-wifi/src/esp_now/mod.rs

- Rework EspWifiInit - No longer require EspWifiInitFor - Add Drop impls for each driver, and add Drop for EspWifiController to fully deinit the stack

MabezDev commented Oct 8, 2024

View reviewed changes

jessebraham reviewed Oct 21, 2024

View reviewed changes

esp-wifi/src/timer/xtensa.rs Outdated Show resolved Hide resolved

MabezDev force-pushed the esp-wifi-peripheral-ref branch from df63636 to 5f16e35 Compare October 22, 2024 14:19

MabezDev mentioned this pull request Oct 29, 2024

Check all option combinations in CI esp-rs/esp-generate#26

Merged

MabezDev force-pushed the esp-wifi-peripheral-ref branch 3 times, most recently from ef966f2 to c845caa Compare October 29, 2024 15:32

MabezDev marked this pull request as ready for review October 30, 2024 15:56

bjoernQ reviewed Oct 31, 2024

View reviewed changes

esp-wifi/src/lib.rs Outdated Show resolved Hide resolved

bjoernQ approved these changes Oct 31, 2024

View reviewed changes

MabezDev mentioned this pull request Oct 31, 2024

Esp wifi/more dynamic allocations #2396

Merged

6 tasks

MabezDev force-pushed the esp-wifi-peripheral-ref branch from c845caa to 78cf67c Compare October 31, 2024 13:52

bugadani reviewed Oct 31, 2024

View reviewed changes

MabezDev added the skip-changelog No changelog modification needed label Oct 31, 2024

jessebraham reviewed Nov 1, 2024

View reviewed changes

esp-wifi/src/ble/controller/mod.rs Show resolved Hide resolved

esp-wifi/src/esp_now/mod.rs Outdated Show resolved Hide resolved

MabezDev added 8 commits November 1, 2024 10:49

More granular init and deinit per driver

47af511

- Rework EspWifiInit - No longer require EspWifiInitFor - Add Drop impls for each driver, and add Drop for EspWifiController to fully deinit the stack

unwrap! more stuff

4282cdc

fixup examples and esp-now

aff9668

unwrap less stuff

59734b2

review feedback

0404cb9

seal wifi traits, allow rng or trng to init esp-wifi

0133b22

changelog and migration guide

3c4aa61

return wifi error in esp now constructor instead of panic

8edb88a

MabezDev force-pushed the esp-wifi-peripheral-ref branch from 6bbca3e to 8edb88a Compare November 1, 2024 10:49

jessebraham approved these changes Nov 2, 2024

View reviewed changes

jessebraham added this pull request to the merge queue Nov 4, 2024

Merged via the queue into esp-rs:main with commit c717f04 Nov 4, 2024
28 checks passed

MabezDev mentioned this pull request Nov 6, 2024

esp-wifi: use Peripheral in init APIs #2244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `EspWifiInitFor` & more granular init/deinit per driver #2301

Remove `EspWifiInitFor` & more granular init/deinit per driver #2301

MabezDev commented Oct 8, 2024 •

edited

Loading

MabezDev commented Oct 22, 2024

MabezDev commented Oct 30, 2024

bjoernQ left a comment

bugadani Oct 31, 2024

MabezDev Oct 31, 2024

bugadani Nov 2, 2024

MabezDev Nov 4, 2024

bugadani Nov 4, 2024

MabezDev Nov 4, 2024

bugadani Nov 4, 2024

MabezDev Nov 4, 2024

bugadani Nov 4, 2024

bjoernQ Nov 4, 2024

bugadani Oct 31, 2024

MabezDev Oct 31, 2024

bugadani Oct 31, 2024

MabezDev Oct 31, 2024

MabezDev commented Oct 31, 2024

Remove EspWifiInitFor & more granular init/deinit per driver #2301

Remove EspWifiInitFor & more granular init/deinit per driver #2301

Conversation

MabezDev commented Oct 8, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

MabezDev commented Oct 22, 2024

MabezDev commented Oct 30, 2024

bjoernQ left a 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

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

MabezDev commented Oct 31, 2024

Remove `EspWifiInitFor` & more granular init/deinit per driver #2301

Remove `EspWifiInitFor` & more granular init/deinit per driver #2301

MabezDev commented Oct 8, 2024 •

edited

Loading