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

embassy-stm32: Add SimplePwmChannel #3317

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

GrantM11235
Copy link
Contributor

No description provided.

/// This returns all four channels, including channels that
/// aren't configured with a [`PwmPin`].
// TODO: I hate the name "split"
pub fn split(&mut self) -> SimplePwmChannels<'_, T> {
Copy link
Member

@Dirbaio Dirbaio Sep 6, 2024

Choose a reason for hiding this comment

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

with this signature, the split channels still borrow from self, so you can't easily create a PWM in main then pass ownership of some channels to TaskA and others to TaskB. You'd have to put the PWM in a StaticCell to get the lifetimes right.

Instead the signature could be fn split(self) -> SimplePwmChannels<'d, T>. This way the unsplit PWM is simply gone, and the channels can live independently. It should be doable by unsafely duplicating the Timer, or making the channels contain a pointer to PAC regs.

(A split function that takes &mut self can still be useful if you want to only temporarily split, and regain access to the unsplit PWM once you drop the channels. We could have both, I've seen this "temporary split" called split_by_ref() in other crates.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to unsafely clone the timer PeripheralRef, there is a missing trait bound somewhere.

Aside from that, it is a bit unfortunate that splitting the channels means that the timer will never be disabled, even if all the channels are dropped.

Ideally, the channels would be refcounted with bit flags, which could also make it easier to hand out channels without accidentally duplicating channels. That sounds pretty complex though, so I am going to leave it as a "future possibility" for now.

Copy link
Member

Choose a reason for hiding this comment

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

yeah leaving it for future work sounds good to me.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

lgtm! can you fix CI?

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.

2 participants