Skip to content

Conversation

@bugadani
Copy link
Contributor

cc #4502


/// RTC Clocks.
#[instability::unstable]
pub struct RtcClock;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet removing RtcClock because there is no public alternative

@bugadani bugadani marked this pull request as ready for review January 27, 2026 15:45
Copilot AI review requested due to automatic review settings January 27, 2026 15:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR continues the clock tree rework by removing legacy RTC clock abstractions and wiring RTC slow clock usage into the new generated clock tree, while updating tests and documentation to match the new APIs.

Changes:

  • Remove the legacy Clock trait along with the RtcFastClock and RtcSlowClock enums, and reimplement RtcClock::slow_freq() in terms of the new ClockTree-based frequency getters.
  • Remove Rtc::estimate_xtal_frequency() and its underlying calibration implementation, updating the HIL clock monitor test to validate the detected XTAL frequency via Clocks::get().xtal_clock instead.
  • Adjust RTC sleep timer code on all supported SoCs to use the new RtcClock::slow_freq() Rate API, and document the removals in CHANGELOG.md.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
hil-test/src/bin/misc_non_drivers/clock_monitor.rs Simplifies the XTAL frequency test to rely on esp_hal::init and Clocks::get().xtal_clock.as_mhz() instead of the removed Rtc::estimate_xtal_frequency().
esp-hal/src/rtc_cntl/sleep/esp32s3.rs Updates RTC sleep timer wake source to use RtcClock::slow_freq().as_hz() and drops the now-removed Clock import.
esp-hal/src/rtc_cntl/sleep/esp32s2.rs Same RTC sleep timer update as above for ESP32-S2.
esp-hal/src/rtc_cntl/sleep/esp32h2.rs Same RTC sleep timer update, using RtcClock::slow_freq().as_hz() and removing unused clock::Clock import.
esp-hal/src/rtc_cntl/sleep/esp32c6.rs Same RTC sleep timer update for ESP32-C6, still using the LP timer path.
esp-hal/src/rtc_cntl/sleep/esp32c3.rs Same RTC sleep timer update for ESP32-C3.
esp-hal/src/rtc_cntl/sleep/esp32c2.rs Same RTC sleep timer update for ESP32-C2.
esp-hal/src/rtc_cntl/sleep/esp32.rs Same RTC sleep timer update for ESP32 classic.
esp-hal/src/rtc_cntl/mod.rs Removes Rtc::estimate_xtal_frequency() and its dependency on the old Clock trait, and adapts time_since_power_up() to the new RtcClock::slow_freq() -> Rate API.
esp-hal/src/clock/mod.rs Deletes the legacy Clock trait and RtcFastClock/RtcSlowClock enums, redefines RtcClock::slow_freq() via ClockTree slow-clock frequency getters, and keeps RTC slow clock calibration logic intact.
esp-hal/CHANGELOG.md Records the removal of Rtc::estimate_xtal_frequency(), RtcFastClock, and RtcSlowClock under the Unreleased “Removed” section.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings January 28, 2026 04:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

esp-hal/src/ledc/timer.rs:261

  • use_ref_tick is only ever set to true when the APB-based divisor exceeds LEDC_TIMER_DIV_NUM_MAX, but it is never reset to false on subsequent configure calls. This means that if a timer is first configured to use REF_TICK and later reconfigured with a frequency where the APB divisor would fit, divisor will be computed assuming APB as the source while the hardware still uses REF_TICK, leading to an incorrect effective frequency. To avoid this mismatch, use_ref_tick should be reset to false at the start of configure (under #[cfg(soc_has_clock_node_ref_tick)]) and only set back to true in the fallback path where REF_TICK is actually selected.
    fn configure(&mut self, config: config::Config<S::ClockSourceType>) -> Result<(), Error> {
        self.duty = Some(config.duty);
        self.clock_source = Some(config.clock_source);

        let src_freq: u32 = self.freq().ok_or(Error::FrequencyUnset)?.as_hz();
        let precision = 1 << config.duty as u32;
        let frequency: u32 = config.frequency.as_hz();
        self.frequency = frequency;

        #[cfg_attr(not(soc_has_clock_node_ref_tick), expect(unused_mut))]
        let mut divisor = ((src_freq as u64) << 8) / frequency as u64 / precision as u64;

        #[cfg(soc_has_clock_node_ref_tick)]
        if divisor > LEDC_TIMER_DIV_NUM_MAX {
            // APB_CLK results in divisor which too high. Try using REF_TICK as clock
            // source.
            self.use_ref_tick = true;
            divisor = (1_000_000u64 << 8) / frequency as u64 / precision as u64;
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant