Skip to content
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

esp-hal-smartled: Calculate RMT cycles from runtime clock config #1154

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

wetheredge
Copy link
Contributor

This replaces the hard-coded RMT cycle counts in the smartled driver with runtime calculations from the clock config. It resolves this TODO.

I've maintained the assumption that the RMT clock source is set to the APB, as it is by default (verified using the patch at the bottom of this PR). The only change to the calculation from the original is the final divisor (500 → 1000) as the original constants were half of the actual APB clock.

All my testing was done exclusively on an ESP32-S3 since that's what I've got on hand. The numbers do work out for the other chips, though.

I have not added anything to the changelog since it's not part of esp-hal-common/esp-hal and is a pretty small change anyway. Happy to add it if that's wanted.

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI
diff --git a/esp32s3-hal/examples/hello_rgb.rs b/esp32s3-hal/examples/hello_rgb.rs
index 1ef7bea..74c46d5 100644
--- a/esp32s3-hal/examples/hello_rgb.rs
+++ b/esp32s3-hal/examples/hello_rgb.rs
@@ -37,6 +37,18 @@ fn main() -> ! {
     let rmt_buffer = smartLedBuffer!(1);
     let mut led = SmartLedsAdapter::new(rmt.channel0, io.pins.gpio48, rmt_buffer, &clocks);
 
+    let rmt_base_addr = 0x6001_6000;
+    let rmt_sys_conf_reg = unsafe { core::ptr::read_volatile((rmt_base_addr + 0x00C0) as *const u32) };
+    let rmt_sclk_sel = (rmt_sys_conf_reg >> 24) & 0b11;
+    let rmt_sclk_sel = match rmt_sclk_sel {
+        0 => "invalid",
+        1 => "APB_CLK",
+        2 => "RC_FAST_CLK",
+        3 => "XTAL_CLK",
+        _ => unreachable!(),
+    };
+    esp_println::println!("RMT_SCLK_SEL = {rmt_sclk_sel}");
+
     // Initialize the Delay peripheral, and use it to toggle the LED state in a
     // loop.
     let mut delay = Delay::new(&clocks);

@jessebraham jessebraham added the skip-changelog No changelog modification needed label Feb 10, 2024
@jessebraham
Copy link
Member

Thanks for taking care of this! I will give this a proper review and test it on hardware for all supported chips on Monday when I'm back to work.

@playfulFence
Copy link
Contributor

playfulFence commented Feb 12, 2024

esp32 (tried on esp-buddy and esp32-wrover) - ❌ :

IMPORTANT: current main branch does the same error

!! A panic occured in 'examples/hello_rgb.rs', at line 71, column 18

PanicInfo {
    payload: Any { .. },
    message: Some(
        called `Result::unwrap()` on an `Err` value: BufferSizeExceeded,
    ),
    location: Location {
        file: "examples/hello_rgb.rs",
        line: 71,
        col: 18,
    },
    can_unwind: true,
    force_no_backtrace: false,
}

Backtrace:

0x400d0875
0x400d0875 - core::result::Result<T,E>::unwrap
    at /Users/playfulfence/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/result.rs:1077
0x400d37cb
0x400d37cb - Reset
    at /Users/playfulfence/.cargo/registry/src/index.crates.io-6f17d22bba15001f/xtensa-lx-rt-0.16.0/src/lib.rs:70
0x400d2e09
0x400d2e09 - ESP32Reset
    at /Users/playfulfence/esp/reviews/esp-hal/esp-hal/src/soc/esp32/mod.rs:71
0x40000000
0x40000000 - _ESP_HAL_DEVICE_PERIPHERALS
    at ??:??
0x40079a84
0x40079a84 - _ESP_HAL_DEVICE_PERIPHERALS
    at ??:??
0x400806b8
0x400806b8 - core::fmt::Arguments::new_v1
    at /Users/playfulfence/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:335
0x40007c34
0x40007c34 - _ESP_HAL_DEVICE_PERIPHERALS
    at ??:??
0x40000740
0x40000740 - _ESP_HAL_DEVICE_PERIPHERALS
    at ??:??
0x40000000
0x40000000 - _ESP_HAL_DEVICE_PERIPHERALS
    at ??:??

esp32s2/s3 - ✅
esp32h2 - ✅
esp32c6 - ✅
esp32c3 - ✅ (rust-board, devkitM and devkitC)

@MabezDev
Copy link
Member

@playfulFence thanks for testing, I guess if it happens on main then we can go ahead and merge this. I'll create an issue tracking the other bug.

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My ESP32 devkit does not have a smart LED unfortunately, so I'm unable to confirm for this device. The remaining chips seems to be working, however, so LGTM.

I will open an issue regarding the ESP32. As @MabezDev said, if this already happens in main then I won't block this PR based on that.

Thanks for the contribution! And thanks for testing @playfulFence!

@jessebraham jessebraham added this pull request to the merge queue Feb 12, 2024
Merged via the queue into esp-rs:main with commit c92d69c Feb 12, 2024
17 of 18 checks passed
@wetheredge wetheredge deleted the smartled-runtime-clocks branch February 12, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants