Add sdm #2371
Add sdm #2371
Conversation
katyo
force-pushed
the
add-sdm
branch
5 times, most recently
from
2147280
to
03edc6a
Compare
katyo
force-pushed
the
add-sdm
branch
2 times, most recently
from
16e74f9
to
87a85a3
Compare
|
@bugadani @Dominaezzz I have rework API just now but I still have some doubt about the new design. |
| ]; | ||
|
|
||
| #[doc(hidden)] | ||
| pub trait RegisterAccess { |
There was a problem hiding this comment.
This trait isn't really being used, the methods should just be inlined.
| impl<'d> Drop for Sdm<'d> { | ||
| fn drop(&mut self) { | ||
| GPIO_SD::enable_clock(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
Looks like this can't fail or is there something planned for the Error?
There was a problem hiding this comment.
This may fail when prescale which calculated from frequency is out of range.
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 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
Almost all ESP SoCs has Sigma-delta modulation periphery which can be used as an alternative for DAC.
SDM on Wikipedia
IDF API documentation
Testing
Describe how you tested your changes.