SDM driver support

[ttzytt]

Add initial Sigma-Delta Modulation driver

This is inspired by the old SDM PR: #2371 and to resolve the issue #2370. I used that PR as the main reference and tried to address many of the design comments raised there. Thanks to @katyo for the original implementation and all the discussion around it.

Overview

This PR adds an initial SDM driver. The current goal is to settle the driver structure and API first, before expanding the documentation and polishing all details.

The driver exposes SDM as a peripheral-wide collection that owns GPIO_SD and provides individual channel fields. Users can move individual channels out of the collection, for example:

let sdm = Sdm::new(peripherals.GPIO_SD);
let mut channel0 = sdm.channel0;

This follows the direction discussed in the old PR: the collection itself does not shut the peripheral down on Drop, because the intended usage is to split channels out and potentially move them into different tasks.

Resource Management

Each channel manages its own active peripheral usage through guards. When a channel is connected, it acquires an SDM clock guard. When the channel is dropped, the guard is dropped too, and the clock reference count is released.

The implementation uses GenericPeripheralGuard for the actual peripheral enable/disable reference counting. I used this because it is already the common pattern in other drivers. To make that possible for SDM, the metadata now also adds Peripheral::GpioSd entries for the chips that have SDM hardware.

However, GenericPeripheralGuard only tracks whether the peripheral clock is in use. It does not store SDM-specific state, such as which source clock is currently selected. SDM has a shared source clock for all channels, so this PR also adds a small SDM-specific clock state guarded by NonReentrantMutex.

That state tracks:

• how many SDM channels currently use the shared SDM clock,
• which clock source is currently selected,
• whether a newly connected channel is trying to use a conflicting source.

The public API only allows selecting the clock source at Sdm construction time. Individual channels receive that source internally, but they do not expose a per-channel source selection API. This is intentional because the underlying clock source is shared.

Channel Generation

Different chips expose different numbers of SDM channels. Some have 4 channels, while ESP32/ESP32-S2/ESP32-S3/ESP32-P4 have 8.

Writing all channel fields and match arms manually with cfg attributes gets complicated quickly. To avoid that, this PR follows the style used by dedicated GPIO: the channel count comes from metadata, and metadata generates a for_each_sdm_channel! macro. The SDM driver then uses that macro to generate the Sdm channel fields and the channel-to-output-signal mapping.

Clock Source Metadata

The metadata currently includes the default SDM clock source and the list of available SDM clock sources for each supported chip.

At the moment, the driver still has a handwritten ClockSource enum and handwritten per-chip clock source configuration code. I am not fully sure yet what the best way is to consume these metadata fields directly from driver code.

This part is definitely open for improvement. Feedback on how this should fit with the existing clock-related code would be very helpful.

Example/Testing

This PR also adds an SDM example under examples/peripheral/sdm.

I tested the example on my ESP32-C5 board and the output worked there. I have not tested the other supported chips on hardware yet.

Regarding HIL testing, I'm not sure what to put there for this driver. It does not seem very practical to test the actual SDM waveform. Maybe the HIL test should instead focus on the digital side of the driver, such as connecting a channel, reading back the raw prescaler and pulse density registers, checking prescaler error handling, and verifying that dropping one channel releases the guard state so another channel can still be used. I will add some HIL testing later if that seems like a good idea.

Documentation

The documentation and comment in code is still minimal. I plan to improve it after the overall API shape and resource-management structure have been reviewed.

[bugadani]

:(

[ttzytt]

should I delete these metadata if not currently using them in the code?

[bugadani]

It can stay, but I'd prefer implementing the relevant clock tree bits properly. We don't document them, so I won't press you on it.

[ttzytt]

Sure I'll take a look at the clock tree, probably after I fix other more basic things like the API change you proposed.

[bugadani]

There should only be apply_config, and maybe set_duty. Duplicaing the entire config API with independent methods isn't something we would like to do.

[ttzytt]

The reason I kept this separate is that apply_config always updates both the timing and the pulse density, while some use cases only need to update one of them.

For SDM I think changing the pulse density is likely the more common runtime operation, and it may happen frequently. Re-applying the whole config just to change density feels a bit awkward and wasteful, as the prescaler usually only needs to be configured once during initialization.

If we want to avoid duplicating API, maybe the better direction is to keep only the set_* functions and drop apply_config instead (since that apply_config is really used once in connect). What do you think?

[bugadani]

Actually, because prescale and the duty cycle are in the same register, a common update function can be more efficient than a partial one - it could use a straight register write instead of a modify which would need to read, mask, update and then write.

We can conceptualise this as something that is not configuration, but an output value, but I'd prefer keeping the API small and without redundant methods.

[ttzytt]

I reworked the SDM channel API again based on the feedback.

apply_config is back. I avoided the earlier idea of storing either a frequency or a raw prescaler in the config, because applying that would still need to branch and convert at runtime. Instead, the config is prepared before it reaches the channel.

One awkward part was frequency conversion: with_frequency needs to know the SDM clock source, but the clock source is selected at the Sdm level rather than per channel. To avoid asking users to pass the same clock source again, I added a builder on Sdm:

let config = sdm
    .channel_config()
    .with_frequency(Rate::from_khz(500))?
    .with_duty(128);

This shape is a little different from the previous channel-builder API, but it keeps the shared clock-source selection in one place. The final ChannelConfig only contains the prepared channel values: the raw prescaler value and the pulse density.

This also follows the point that prescaler and pulse density live in the same register. If we want apply_config to be a single register write, both values need to be known at the same time. The implementation now writes the whole SDM channel register directly instead of using modify, so it avoids the read-mask-write sequence.

For the convenience methods like set_duty / set_pulse_density, there is still a tradeoff. A partial update only receives the new density, but the straight register write also needs the current prescaler. To avoid reading the register back, the connected Channel currently keeps a cached ChannelConfig and updates that cache before applying it.

I'm not completely sure whether caching the config in Channel is the best API design. One alternative would be to make this more explicit: if users want to avoid read-modify-write behavior, they should keep their own ChannelConfig and call apply_config. In that model, set_duty / set_pulse_density could either be removed or implemented as partial updates using modify.

[bugadani]

It's pretty easy to query the source frequency from the clock tree, it doesn't need to be part of the configuration, but for now I guess your solution works.

To avoid reading the register back, the connected Channel currently keeps a cached ChannelConfig and updates that cache before applying it.

That seems to overcomplicate matters a bit, you are allowed to call modify, it's just not necessary when you apply a complete config.

[ttzytt]

The config cache is removed now.

[MabezDev]

Overall this looks pretty nice! I was hoping that maybe we could do what we did the DMA channels, and make these SDM channels part of Peripherals, but the clock source selection kills that unfortunately.

This needs some HIL tests, I think you can probably take a look at how we use rmt to count pulses in other tests for this purpose.

[github-actions]

New commits in main have made this PR unmergeable. Please resolve the conflicts.

[ttzytt]

I have C5/C6/S3, the HIL test worked well for C6 and S3. However, it always seems to time out for C5, with outputs like the following:

running 2 tests
test tests::duties_have_expected_high_ratios      ... Core 0
    Frame 0: R<esp32c5::rmt::int_raw::INT_RAW_SPEC>::ch_rx_end @ 0x4200805a inline
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\esp32c5-0.2.2\src\rmt\int_raw.rs:66:41
    Frame 1: DynChannelAccess<esp_hal::rmt::Rx>::get_rx_status @ 0x4200805a inline
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:2704:20
    Frame 2: RxTransaction::poll_internal @ 0x42008056 inline
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:1852:26
    Frame 3: RxTransaction::wait @ 0x42008052
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:1899:24
    Frame 4: measure_high_ratio @ 0x42004dd0
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:245:47
    Frame 5: duties_have_expected_high_ratios @ 0x420051ac
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:274:31
    Frame 6: __duties_have_expected_high_ratios_entrypoint @ 0x42005714
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:250:1
    Frame 7: __embedded_test_start @ 0x42006606
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:83:13
    Frame 8: ensure_linker_file_was_added_to_rustflags @ 0x420062de
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:46:14
    Frame 9: __embedded_test_entry @ 0x42006710
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:36:5
    Frame 10: hal_main @ 0x4200babc
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\soc\mod.rs:84:9
    Frame 11: start_rust @ 0x4200495e
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\riscv-rt-0.16.0\src\lib.rs:705:5
    Frame 12: .Lpcrel_hi2 @ 0x420048c8
    Frame 13: .Lpcrel_hi2 @ 0x420048c8
FAILED
test tests::higher_duties_have_higher_high_ratios ... Core 0
    Frame 0: R<esp32c5::rmt::int_raw::INT_RAW_SPEC>::ch_rx_end @ 0x42008062 inline
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\esp32c5-0.2.2\src\rmt\int_raw.rs:66:26
    Frame 1: DynChannelAccess<esp_hal::rmt::Rx>::get_rx_status @ 0x4200805a inline
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:2704:20
    Frame 2: RxTransaction::poll_internal @ 0x42008056 inline
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:1852:26
    Frame 3: RxTransaction::wait @ 0x42008052
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\rmt.rs:1899:24
    Frame 4: measure_high_ratio @ 0x42004dd0
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:245:47
    Frame 5: higher_duties_have_higher_high_ratios @ 0x42005314
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:306:27
    Frame 6: __higher_duties_have_higher_high_ratios_entrypoint @ 0x42005798
        D:\shared_1\prog\esp-hal-tzyt\hil-test\src\bin\sdm.rs:250:1
    Frame 7: __embedded_test_start @ 0x42006606
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:83:13
    Frame 8: ensure_linker_file_was_added_to_rustflags @ 0x420062de
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:46:14
    Frame 9: __embedded_test_entry @ 0x42006710
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\embedded-test-0.7.1\src\export.rs:36:5
    Frame 10: hal_main @ 0x4200babc
        D:\shared_1\prog\esp-hal-tzyt\esp-hal\src\soc\mod.rs:84:9
    Frame 11: start_rust @ 0x4200495e
        C:\Users\tzyt\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\riscv-rt-0.16.0\src\lib.rs:705:5
    Frame 12: .Lpcrel_hi2 @ 0x420048c8
    Frame 13: .Lpcrel_hi2 @ 0x420048c8
FAILED

failures:

---- tests::duties_have_expected_high_ratios ----
Test timed out after 3s

---- tests::higher_duties_have_higher_high_ratios ----
Test timed out after 3s


failures:
    tests::duties_have_expected_high_ratios
    tests::higher_duties_have_higher_high_ratios

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.52s

Seems like the issue is that it can't detect the idle signal in rx_transaction.wait().unwrap();.

But if I run other HIL tests like GPIO on C5, it will work. So I'm quite confused now.

Would appreciate it if someone could help here.

[github-actions]

New commits in main have made this PR unmergeable. Please resolve the conflicts.

[github-actions]

New commits in main have made this PR unmergeable. Please resolve the conflicts.

[ttzytt]

Hi @MabezDev and @bugadani, just a quick follow-up on this PR since it has been around a week without further comments.

I’ve tried to address the recent feedback by removing the example, adding an initial HIL test using RMT RX to measure the SDM output, removing the config cache, and updating the branch after the merge conflicts.

Could you let me know if there is anything else I should change or investigate? I’m also still not completely sure about the ESP32-C5 HIL timeout I mentioned above, so any guidance there would be appreciated.

Thanks!

[bugadani]

There are several issues with this PR that don't align with esp-hal.

• Channel is generic when it really doesn't need to be
• Channel doesn't carry a lifetime, so you need that weird disconnect method that gives you back the ChannelCreator. Instead, ChannelCreator shouldn't be consumed to create a Channel.
• Clock source handling basically reimplements parts of the auto-generated clock tree code we have
• Channels carry a clock source for whatever reason
• SdmConfig and ChannelConfig basically manually reimplements the BuilderLite procmacro instead of using it

As for your C5 difficulty, for me the HIL test doesn't build.

[github-actions]

New commits in main have made this PR unmergeable. Please resolve the conflicts.

retracted

[MabezDev]

Hey @ttzytt, do you plan on finishing up this PR? SDM would be a great addition to finally land :)

[ttzytt]

Hi @MabezDev, yes, I do plan to finish this up. It's been a bit busy for me recently, so I haven't really had time to work on this.

But I have some time this weekend, so I'll try to address the problems mentioned by @bugadani in the previous comment.

Thanks for the reminder.