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

Add sdm #2371

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add sdm #2371

wants to merge 5 commits into from

Conversation

katyo
Copy link
Contributor

@katyo katyo commented Oct 20, 2024

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:

Pull Request Details 📖

Description

Almost all ESP SoCs has Sigma-delta modulation periphery which can be used as an alternative for DAC.

ESP32 SoC Number of SigmaDelta channels
ESP32 8
ESP32-S2 8
ESP32-S3 8
ESP32-C3 4
ESP32-C6 4
ESP32-H2 4

SDM on Wikipedia
IDF API documentation

Testing

Describe how you tested your changes.

@katyo katyo marked this pull request as draft October 20, 2024 22:05
@katyo katyo changed the title [WIP] Add sdm Add sdm Oct 20, 2024
@katyo katyo force-pushed the add-sdm branch 5 times, most recently from 2147280 to 03edc6a Compare October 20, 2024 22:33
esp-hal/src/sdm.rs Outdated Show resolved Hide resolved
esp-hal/src/sdm.rs Outdated Show resolved Hide resolved
esp-hal/src/sdm.rs Outdated Show resolved Hide resolved
@SergioGasquez SergioGasquez linked an issue Oct 21, 2024 that may be closed by this pull request
esp-hal/src/sdm.rs Outdated Show resolved Hide resolved
@katyo katyo force-pushed the add-sdm branch 2 times, most recently from 16e74f9 to 87a85a3 Compare October 21, 2024 18:22
@katyo
Copy link
Contributor Author

katyo commented Oct 21, 2024

@bugadani @Dominaezzz I have rework API just now but I still have some doubt about the new design.

@katyo katyo marked this pull request as ready for review October 21, 2024 18:51
@katyo katyo marked this pull request as draft October 21, 2024 18:51
@katyo katyo requested review from bugadani and Dominaezzz October 21, 2024 18:51
];

#[doc(hidden)]
pub trait RegisterAccess {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This trait isn't really being used, the methods should just be inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Comment on lines +80 to +84
impl<'d> Drop for Sdm<'d> {
fn drop(&mut self) {
GPIO_SD::enable_clock(false);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, having a Drop implementation here means that the channels can't be sent off to other threads/tasks/cores/interrupts.

I'm wondering if there are any downsides of leaving it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the channels themselves should track how many are still enabled (with an AtomicUsize for example) and disable the clocks if all are gone. The "collection" peripheral shouldn't be Drop because the usual intent is to move out of it. We are putting work into establishing these patterns in this dev cycle, along with implementing Drop for peripherals so that they clean up after themselves. I'm not sure it's worth doing similar work in parallel until we arrive at something we like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally I'd like an explicit drop. The user can recreate the "collection" peripheral from the parts that were split, and then call some release() method to safely get the peripheral back (which would disable clocks, reset, etc.).
Only two issues are it doesn't quite play nicely with the PeripheralRef pattern (but it works better for statics) and it's somewhat tedious (though I reckon few applications will actually want to release their drivers anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dominaezzz When I tried to share LEDC channel with interrupt handler I cannot do that. Seems current design lacks support for such usage scenarios.

@bugadani I think that ability to deinit periphery is matter especially for applications which needs power management.

@Dominaezzz In my opinion explicit drop via API looks like a low level thing and we should avoid it as much as possible.


/// Sigma-Delta modulation channel handle.
pub struct Channel<'d, const N: u8, O: OutputPin> {
_ref: &'d ChannelRef<'d, N>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using 'd for both the borrow and the ChannelRef will have some awkward consequences around static.

The Channel should just take ownership of the ChannelRef, at least then the Channels can be sent off to other threads.

Copy link
Contributor Author

@katyo katyo Oct 22, 2024

Choose a reason for hiding this comment

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

Of course, I trying to redesign that.
But seems taking ownership leads to necessity in explicit drop.

&'d self,
output: impl Peripheral<P = O> + 'd,
frequency: HertzU32,
) -> Result<Channel<'d, N, O>, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this can't fail or is there something planned for the Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may fail when prescale which calculated from frequency is out of range.

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.

Sigma-delta modulation driver
3 participants