Adding a way to deinitialize the WiFi stack. #2187

playfulFence · 2024-09-18T13:57:07Z

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 📖

closes #1604

Description

⚠️ DRAFT
Problems to be fixed:

something is not being freed correctly for NPL BLE chips
C2 can be deinitialized while using BLE in coex mode, but not when using BLE alone.
User can't create more than 1 socket at this point

However, I decided to do at least a Draft PR to get some feedback or implementation suggestions

Testing

Some short examples (will
BLE:

let deinit = deinitialize_ble(ble).unwrap();
let init = reinitialize(EspWifiFor::Ble, deinit).unwrap();

Wi-Fi:

let deinit = deinitialize_wifi(controller, wifi_stack).unwrap();
let init = reinitialize(EspWifiFor::Wifi, deinit).unwrap();

Coex:

let deinit = deinitialize_all(controller, wifi_stack).unwrap();
let init = reinitialize(EspWifiFor::WifiBle, deinit).unwrap();

changelog entry

bjoernQ · 2024-09-18T14:45:47Z

esp-wifi/Cargo.toml

@@ -13,6 +13,7 @@ categories = ["embedded", "hardware-support", "no-std"]

 [dependencies]
 defmt = { version = "0.3.8", optional = true }
+bleps = { git = "https://github.com/bjoernQ/bleps", package = "bleps", rev = "a5148d8ae679e021b78f53fd33afb8bb35d0b62e", features = [ "macros", "async"] }


that's not good - we won't be able to release esp-wifi with a git dependency - and also users are free to use any BLE stack they want

MabezDev

Thanks for working on this! I'm excited to see this land!

I see the core functionality is in place (minus the c2 issues) but I think the API surface could do with some polish.

MabezDev · 2024-09-18T15:41:20Z

esp-wifi/src/lib.rs

+    info!("WiFi and BLE coexistence deinitialized");
+
+    // Drop the WiFi controller and stack
+    drop(controller);


No need to call drop manually here.

MabezDev · 2024-09-18T15:45:37Z

esp-wifi/src/lib.rs

+    drop(controller);
+    drop(stack);
+    drop(ble);
+


We also need to stop the timer responsible for task switching (this was @bugadani's main complaint iirc) so for both arch's we need an equivalent setup_timer to tear down the timer.

Correct, my stuff was really sensitive to the periodic interruptions caused by the scheduler.

Would be probably safer to also unlisten interrupts related to the radio peripherals

MabezDev · 2024-09-18T15:46:16Z

esp-wifi/src/lib.rs

+    ))
+}
+
+pub fn reinitialize(


Can we drop this function and just use initialize instead?

I would pretty much prefer that 👍

MabezDev · 2024-09-18T15:47:25Z

esp-wifi/src/wifi_interface.rs

@@ -282,7 +282,7 @@ impl<'a, MODE: WifiDeviceMode> WifiStack<'a, MODE> {
    /// Create a new [Socket]
    #[cfg(feature = "tcp")]
    pub fn get_socket<'s>(
-        &'s self,
+        &'s mut self,


@bjoernQ I'm not that familiar with the stack, but I think this should stay as &self? That's why we wrap things in refcell internally, right?

Yeah, agree, I've covered it in masked User can't create more than 1 socket at this point item of a TODO list above 😆

Will do something about that

Definitely - having only one socket is quite limiting 🤣

MabezDev · 2024-09-18T15:51:05Z

esp-wifi/src/ble/os_adapter_esp32c6.rs

    common_adapter::RADIO_CLOCKS,
    hal::system::{RadioClockController, RadioPeripherals},
 };

+// use crate::{


Remove these if they're not used

MabezDev · 2024-09-18T15:58:07Z

esp-wifi/src/ble/npl.rs

@@ -215,12 +215,21 @@ extern "C" {

    pub(crate) fn ble_controller_init(cfg: *const esp_bt_controller_config_t) -> i32;

+    // #[cfg(not(esp32c2))]


Remove these if they are useless

MabezDev · 2024-09-18T16:00:00Z

esp-wifi/src/lib.rs

+    esp_wifi_result!(unsafe { esp_wifi_stop() })?;
+    esp_wifi_result!(unsafe { esp_wifi_deinit_internal() })?;
+
+    // Deinitialize BLE


We should resuse the deinitialize_ble etc methods here in the deinit all

Ideally, we shouldn't care about what network or Bluetooth stack the user is using - e.g. they can use embassy-net instead and they can use Trouble instead of bleps. Users can even directly deal with HCI and use raw smoltcp

MabezDev · 2024-09-18T16:01:28Z

esp-wifi/src/ble/os_adapter_esp32h2.rs

+
+        bt_periph_module_disable();
+
+        // npl::esp_ble_unregister_bb_funcs();


MabezDev · 2024-09-18T16:08:47Z

esp-wifi/src/ble/npl.rs

+    let event = event.cast_mut();
+
+    if (*event).dummy == 0 {
+        panic!("Trying to deinitialize an uninitialized event");


This a bit sus, was this just test code or do the blobs really emit empty events?

bugadani · 2024-09-18T17:52:40Z

esp-wifi/src/ble/os_adapter_esp32c2.rs

+    unsafe {
+        // HCI deinit
+        npl::r_ble_hci_trans_cfg_hs(
+            Some(core::mem::transmute::<


This looks sus, these should be None when null, shouldn't they?

bugadani · 2024-09-18T17:53:31Z

esp-wifi/src/ble/os_adapter_esp32c6.rs

+            Some(core::mem::transmute::<
+                *const (),
+                unsafe extern "C" fn(*const u8, *const c_void),
+            >(core::ptr::null())),


bugadani · 2024-09-18T17:53:46Z

esp-wifi/src/ble/os_adapter_esp32h2.rs

+    unsafe {
+        // HCI deinit
+        npl::r_ble_hci_trans_cfg_hs(
+            Some(core::mem::transmute::<


bugadani · 2024-09-18T17:54:07Z

esp-wifi/src/lib.rs

+#[non_exhaustive]
+/// An internal struct designed to make [`EspWifiDeinitialization`] uncreatable
+/// outside of this crate.
+pub struct EspWifiDeinitializationInternal;


We shouldn't need multiple internal-but-externally-not-constructible types

bugadani · 2024-09-18T17:58:10Z

esp-wifi/src/lib.rs

@@ -290,7 +314,7 @@ impl EspWifiTimerSource for TimeBase {
 /// # }
 /// ```
 pub fn initialize(
-    init_for: EspWifiInitFor,
+    init_for: EspWifiFor,
    timer: impl EspWifiTimerSource,
    rng: hal::rng::Rng,
    radio_clocks: hal::peripherals::RADIO_CLK,


I'm guessing we'll need to pass peripherals as impl Peripheral and bind their lifetimes in EspWifiInitialization so that they aren't reused before deinited. Then we'd pass EspWifiInitialization to the deinit functions, which may return a different EspWifiInitialization if they only partially deinit, or () if everything was dropped.

bugadani · 2024-09-18T17:59:47Z

esp-wifi/src/lib.rs

+#[derive(Debug, PartialEq, PartialOrd)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+/// Represents the state of being deinitialized for WiFi, Bluetooth or both.
+pub enum EspWifiDeinitialization {


I don't get why this is needed. Just transform EspWifiInitialization if something changes.

bjoernQ · 2024-09-19T06:19:08Z

Awesome achievement and a great start!

I'm not a huge fan of renaming EspWifiInitFor since that introduces a lot of noise when reviewing but I see why the original naming doesn't suit things well anymore

playfulFence added 8 commits September 18, 2024 15:34

WIP

929f5d5

cleanup

ddad7f2

WIP (BLE)

e9afd2c

WIP (BLE)

7e6a70d

c2 ble deinit doesn't work with ble alone, works in coex (WIP)

b9fc6cc

fmt

1fe5c5f

cleanup

54ced45

changelog entry

89132fa

changelog entry

playfulFence force-pushed the feature/wifi_deinit branch from b6ee505 to 89132fa Compare September 18, 2024 14:04

bjoernQ reviewed Sep 18, 2024

View reviewed changes

MabezDev requested changes Sep 18, 2024

View reviewed changes

MabezDev reviewed Sep 18, 2024

View reviewed changes

bugadani reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a way to deinitialize the WiFi stack. #2187

Adding a way to deinitialize the WiFi stack. #2187

playfulFence commented Sep 18, 2024 •

edited

Loading

bjoernQ Sep 18, 2024

MabezDev left a comment

MabezDev Sep 18, 2024

MabezDev Sep 18, 2024

bugadani Sep 18, 2024

bjoernQ Sep 19, 2024

MabezDev Sep 18, 2024

bjoernQ Sep 19, 2024

MabezDev Sep 18, 2024

playfulFence Sep 18, 2024

bjoernQ Sep 19, 2024

MabezDev Sep 18, 2024

MabezDev Sep 18, 2024

MabezDev Sep 18, 2024

bjoernQ Sep 19, 2024

MabezDev Sep 18, 2024

MabezDev Sep 18, 2024

bugadani Sep 18, 2024

bugadani Sep 18, 2024

bugadani Sep 18, 2024

bugadani Sep 18, 2024

bugadani Sep 18, 2024

bugadani Sep 18, 2024

bjoernQ commented Sep 19, 2024

		@@ -215,12 +215,21 @@ extern "C" {

		pub(crate) fn ble_controller_init(cfg: *const esp_bt_controller_config_t) -> i32;

		// #[cfg(not(esp32c2))]


		bt_periph_module_disable();

		// npl::esp_ble_unregister_bb_funcs();

Adding a way to deinitialize the WiFi stack. #2187

Are you sure you want to change the base?

Adding a way to deinitialize the WiFi stack. #2187

Conversation

playfulFence commented Sep 18, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

Choose a reason for hiding this comment

MabezDev 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

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

bjoernQ commented Sep 19, 2024

playfulFence commented Sep 18, 2024 •

edited

Loading