-
Notifications
You must be signed in to change notification settings - Fork 206
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
base: master
Are you sure you want to change the base?
Add methods to enable/disable the entire timer #183
Conversation
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. |
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) |
I think the desire to have this came up in stm32-rs/stm32f1xx-hal#176 . |
@justacec Would you mind fixing the conflicts? |
Ping @justacec. |
@justacec Are you still around? |
@therealprof Working on it now. |
@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 |
Looks good to me. Any objections @ryankurte? |
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. |
@ryankurte I just updated the changelog as requested. As for the implementation, I suppose that would need to be done in the |
has anyone had a chance to implement this? would be excellent to get it merged ^_^ |
I am submitting this simple PR to start discussion on the recently opened issue #182. I have simply renamed the existing
enable
anddisable
methods to bechannel_enable
andchannel_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.