Conversation
There was a problem hiding this comment.
Pull request overview
Updates TIMG clock/peripheral handling and metadata to better model timer-group clocking and remove unconditional “keep enabled” behavior for TIMG0 on several chips.
Changes:
- Remove
keep_enabled = truefromTimg0peripheral clock entries in multipleesp-metadata/devices/*.tomlfiles. - Refactor
esp-hal’s TIMG driver to use aPeripheralaccessor per instance and inline clock-tree interactions for WDT clock configuration/gating.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| esp-metadata/devices/esp32s3.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32s2.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32h2.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32c61.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32c6.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32c5.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32c3.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-metadata/devices/esp32c2.toml | Drop keep_enabled from Timg0 peripheral clock entry. |
| esp-hal/src/timer/timg.rs | Update TIMG instance trait to expose Peripheral, adjust enable/reset logic, and rework WDT clock configuration/gating calls. |
Comments suppressed due to low confidence (1)
esp-hal/src/timer/timg.rs:86
PeripheralGuardis imported but not used anywhere in this module. Withcargo clippy -D warningsthis will fail CI; either usePeripheralGuard(e.g., for the refcounted enable/reset path) or remove the import until it’s needed.
system::PeripheralClockControl,
time::{Duration, Instant, Rate},
|
Tests look broken |
4db887d to
694d28a
Compare
|
I'll have to come back to the idea of not keeping TIMG0 running. There is some funky behaviour going on on S2 and S3 around that... |
| } | ||
|
|
||
| if !enable { | ||
| unsafe { Self::reset_racey(peripheral) }; |
There was a problem hiding this comment.
This goes because resetting TIMG0 enables its WDT, and that's just a source of frustration.
|
/hil full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
esp-hal/src/system.rs:205
PeripheralClockControl::disableis documented as resetting the peripheral before disabling, but the reset-on-disable logic was removed fromenable_forced_with_counts. Either update the doc comment to match the new behavior, or reintroduce a reset if callers rely on peripherals being reset when the refcount reaches 0.
true
}
|
Triggered full HIL run for #5256. Run: https://github.com/esp-rs/esp-hal/actions/runs/23605168776 Status update: ❌ HIL (full) run failed (conclusion: failure). |
Closes #1366 - everything is modelled, and maybe the default clock source could be changed from XTAL to PLL/APB where possible.
Besides converting the driver to use State/Info, to use PeripheralGuard, to allow actually configuring the clock sources, there isn't much to do. The active source clock frequency should be cached because a bunch of operations (like
now!) take a critical section.