embedded-hal icon indicating copy to clipboard operation
embedded-hal copied to clipboard

Published 20 hours ago • verified publisher icon rust-embedded

Add methods to enable/disable the entire timer

Open justacec opened this issue 5 years ago • 12 comments

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.

justacec avatar Jan 26 '20 14:01 justacec

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.

rust-highfive avatar Jan 26 '20 14:01 rust-highfive

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 avatar Apr 04 '20 22:04 ryankurte

I think the desire to have this came up in https://github.com/stm32-rs/stm32f1xx-hal/pull/176 .

therealprof avatar Apr 07 '20 21:04 therealprof

@justacec Would you mind fixing the conflicts?

therealprof avatar Apr 07 '20 21:04 therealprof

Ping @justacec.

therealprof avatar Apr 19 '20 10:04 therealprof

@justacec Are you still around?

therealprof avatar Jun 20 '20 13:06 therealprof

@therealprof Working on it now.

justacec avatar Jun 21 '20 18:06 justacec

@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.

justacec avatar Jun 21 '20 18:06 justacec

Looks good to me. Any objections @ryankurte?

therealprof avatar Jun 21 '20 19:06 therealprof

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 avatar Jun 22 '20 02:06 ryankurte

@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.

justacec avatar Jun 22 '20 10:06 justacec

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

ryankurte avatar Aug 31 '20 01:08 ryankurte