Disable peripherals when unused #2544

bjoernQ · 2024-11-15T10:50:29Z

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

Closes #1382
Supercedes #2484

This disables most peripherals when their driver gets fully dropped.

There are some peripherals which don't get touched

UART0 (console logging would break)
serial-jtag (console logging would break)
systimer (we use it to get the time since boot)
TIMG (we use TIMG0-LACT to get the time on ESP32)
DMA (should be fine to do - but lets do it in a separate PR)
LEDC (that driver will see a refactoring or rewrite)

others which we currently don't explicitly enable (some of them for reasons, but not all)

Additionally, peripherals are disabled in init - gives us a ~ 5mA advantage when not using them

Testing

HIL-tests
manual testing with logging enabled

esp-hal/src/hmac.rs

esp-hal/src/system.rs

esp-hal/src/trace.rs

esp-hal/src/lcd_cam/mod.rs

esp-hal/src/system.rs

esp-hal/src/lcd_cam/cam.rs

esp-hal/src/system.rs

esp-hal/src/i2s/master.rs

bugadani · 2024-11-15T15:48:16Z

esp-hal/src/i2s/parallel.rs

@@ -220,8 +221,8 @@ where
        channel.runtime_ensure_compatible(&i2s);
        let channel = channel.degrade();

-        PeripheralClockControl::reset(i2s.peripheral());
-        PeripheralClockControl::enable(i2s.peripheral());
+        let guard = PeripheralGuard::new(i2s.peripheral());


It seems at one point the PeripheralGuard won over manually managing clocks. Would you consider updating AES/ADC, too?

esp-hal/src/mcpwm/mod.rs

esp-hal/src/parl_io.rs

esp-hal/src/rmt.rs

bugadani · 2024-11-15T16:02:55Z

In general, I like the idea of PeripheralGuard. I'd prefer it to be generic and zero-sized, but I don't see that happening with type erased peripherals. Maybe we could have two variants, one for ZST peripherals and one for erased ones.

Looking closely at the code, I can see that this PR was done mostly mechanically, there are a few places where a guard isn't necessary (RMT, and some others), or some where it looks not entirely correct to my eyes (PARL_IO).

MabezDev · 2024-11-19T17:53:26Z

examples/src/bin/spi_loopback.rs

-    .with_mosi(miso_mosi)
-    .with_miso(miso)
+    .with_miso(miso) // order matters
+    .with_mosi(miso_mosi) // order matters


Orthogonal to the PR but is this something we can fix @bugadani? Seems like would be hard to spot without knowing the internals of GPIO

This is actually addressed in #2557 I believe. Because with_miso in that PR only sets the input signal, this won't be a problem.

Well now that PR is closed, the problem is back with us 🥲

MabezDev · 2024-11-19T17:54:02Z

hil-test/tests/spi_half_duplex_write.rs

@@ -112,7 +112,8 @@ mod tests {
            )
            .map_err(|e| e.0)
            .unwrap();
-        transfer.wait();
+        // dropping SPI would make us see an additional edge - so let's keep SPI alive


When we reset the peripheral, we also reset CTRL.D_POL ( to 1) so MOSI is idle-high

So, it looks like this:

See how MOSI is set to high after resetting (near the marker)

BTW we can also see that effect on main before the initial transaction

There we see MOSI shows a "bump" between reset and setting D_POL to 0

esp-hal/src/trace.rs

jessebraham

Changes LGTM, but will wait for @MabezDev to approve before merging since he had requested changes

MabezDev

Thanks!

jessebraham reviewed Nov 15, 2024

View reviewed changes

esp-hal/src/hmac.rs Outdated Show resolved Hide resolved