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 methods to enable/disable the entire timer #183

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

justacec
Copy link

I am submitting this simple PR to start discussion on the recently opened issue #182. I have simply renamed the existing enable and disable methods to be channel_enable and channel_disable. I have also added two new methods, enable/disable, which would then be expected to be used to enable/disable the entire timer.

I think that this functionality is useful so that one does not need to temporarily store the current duty for each channel in order to "disable" the timer. The state would be maintained and it results in a single read/write to the timer control registers.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ryankurte ryankurte added this to the v1.0.0 milestone Apr 4, 2020
@ryankurte
Copy link
Contributor

ryankurte commented Apr 4, 2020

cc. @rust-embedded/hal, if this change is desirable perhaps we should break this in the v1 migration? (the caveat being this has not been demonstrated as per the new hal guidelines)

ryankurte
ryankurte previously approved these changes Apr 7, 2020
@therealprof
Copy link
Contributor

I think the desire to have this came up in stm32-rs/stm32f1xx-hal#176 .

@therealprof
Copy link
Contributor

@justacec Would you mind fixing the conflicts?

@therealprof
Copy link
Contributor

Ping @justacec.

@therealprof
Copy link
Contributor

@justacec Are you still around?

@justacec
Copy link
Author

@therealprof Working on it now.

@justacec
Copy link
Author

@therealprof Ok, I think I synced this up. As I was updating this PR, I was not sure if the other "channel" specific methods (such as try_get_duty) should be changed to try_get_duty_channel when called from the Pwm object. That name makes sense for the PwmPin, but maybe not intuitive enough for the Pwm object.

@therealprof
Copy link
Contributor

Looks good to me. Any objections @ryankurte?

@ryankurte
Copy link
Contributor

Looks good to me! Even though it is rather straightforward I think it would still be good to demonstrate an implementation / driver, as well as to add this change to the changelog.

@justacec
Copy link
Author

@ryankurte I just updated the changelog as requested. As for the implementation, I suppose that would need to be done in the stm32f1xx_hal crate.

@ryankurte
Copy link
Contributor

has anyone had a chance to implement this? would be excellent to get it merged ^_^

@eldruin eldruin removed this from the v1.0.0 milestone Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants