Added more features to timer

[David-OConnor]

The intent is to add more features (Including, but not exclusively for PWM) to the timer.rs module, to expand functionality, and avoid ownership issues of dp.TIMx. WIP, in that it's missing features. Non-breaking, but exposes a second API for PWM, that's not compatible with the EH trait. The current approach to PWM gives the pwm.rs module ownership of the dp.TIMx peripheral. If you want to modify timer features, you must use raw pointers, or timer::Timer and pwm::PwmChannel will have a conflict.

This exposes more features from the hardware, and follows the Ref Man and register action more closely. It avoids using raw pointers, which would otherwise be required in your code, and are currently used in pwm.rs. Note that the previous version only included features shared by all timers. I think we should have 3 categories, per the ref man, since their features are different. The current approach only includes the set of features they all share. I added General Purpose timers. Haven't added Advanced Timers, and Basic Timers.

Have only implemented these for the 3 general purpose timers. Expanding to the others would be easy by copying the macro and modifying according to the datasheet. Haven't done it yet. Doesn't have full functionality.

My intent is to make this explicit by making heavy use of enums corresponding to register entries.

Example, using 3 channels in asymmetric mode:

let mut timer = Timer::tim2(dp.TIM2, 94.hz(), &clocks);

timer.set_resolution(85_106);

timer.set_preload(timer::Channel::One, true);

timer.set_output_compare(Channel::One, OutputCompare::Pwm1);
timer.set_output_compare(Channel::Two, OutputCompare::Pwm1);
timer.set_output_compare(Channel::Three, OutputCompare::AsymmetricPwm1);
timer.set_output_compare(Channel::Four, OutputCompare::AsymmetricPwm1);

timer.set_duty(Channel::One, timer.get_max_duty() / 2);
timer.set_duty(Channel::Two, timer.get_max_duty() / 5);
// Set a phase offset for Channel 3/4, using asymmetric mode.
timer.set_duty(Channel::Three, timer.get_max_duty() * 3 / 5);
timer.set_duty(Channel::Four, timer.get_max_duty() * 4 / 5);

timer.set_alignment(Alignment::Center1);

timer.enable(Channel::One);
timer.enable(Channel::Two);
timer.enable(Channel::Three);
timer.enable(Channel::Four);

This is missing features and could use more documentation / ref man quotes, but is non-breaking, and I'd be happy merging even without the features. (And adding them later as required). The main thing that needs to be done is borrow clocks instead of own.

Tangent: Does anyone know whypwm.rs

[David-OConnor]

Broke the macros into 4 categories, but the only effective changes thus far are for timers 2, 3, 4.

Support for the unique timers to f373 and f378 are currently broken.

[IamfromSpace]

This is a nice way to get around the shared register ownership problem, but sacrificing the ability to impl traits seems like a high cost :( I feel like the ability to write code that just says "I just need a timer/PWM/etc, I don't care which chip" is valuable.

Somewhat loosely related and maybe of interest, this comment goes over a recent attempt to offer safe flexibility in regard to the shared-global-register problem. Users choose their exclusivity type, allowing zero-overhead if a user only needs one channel, and potentially Critical Sections if they need more than one.

I suppose an advantage of the strategy in this PR has no runtime penalty at all no matter how many channels are used. Doesn't seem like we're gonna be able to have our cake and eat it too with common traits and zero overhead in all cases.

It may be worth using typed channel enums so that timers that don't support four don't have invalid inputs.

Tangent: Does anyone know why pwm.rs broke the macros into 4 categories, but the only effective changes thus far are for timers 2, 3, 4.

There's a couple of levels of macros in the timers, because some needed to set break registers and others didn't. This split was just trying to reduce duplication.

[David-OConnor]

I completely agree on the traits being important. I'm starting to think offering both APIs is the way to go: Use the existing approach for the general use case, and the new one for when you need to customize.

pwm.rs API is good for supporting embedded-hal traits, eg writing generic code, which is probably majority use case. timer.rs API is good for exposing more functionality in high-level code, and documenting it. Bonus of avoiding raw pointers and using the ownership-of-registers model.

I'm starting to worry less about embedded-hal traits, since they by nature only support the most generic traits for a class of hardware. Eg I gave up on using them in the DAC pr, since evidently, functionality other than setting a word is not generic enough. As soon as you need a feature not part of the eh spec, you lose the ability to take advantage of the traits anyway. Then you have to consider if you need to use raw pointers to get around ownership issues.

We have a specific challenge of making the eh traits work here, since much of the functionality used for PWM uses timer registers, while the trait expects it to be channel-based. I think it makes sense to have PWM in 2 locations.

I love your idea about choosing exclusivity. Thoughts on putting up the PR from your fork?

Tangent: (What I meant to post before - oops!) what does the pin selection in the pwm module do? AFAIK, the MCU knows to output to the correct pin if it's set up as an output on the right AF.

[IamfromSpace]

I completely agree on the traits being important. I'm starting to think offering both APIs is the way to go... I think it makes sense to have PWM in 2 locations.

This also makes sense! I think abstraction in layers is a good way to go. PAC -> Ergonomic but Low level access -> Generic Traits does seem reasonable.

I love your idea about choosing exclusivity. Thoughts on putting up the PR from your fork?

Hey, great! I had added it to the issue, but hadn't got feedback yet (and been quite busy!), but I do think it's essentially ready for review. Would also be interested to get broader feedback from the community and see if it helps address some of the RTIC folks concerns about CSes (in that hopefully they can supply their own Mutex impl). It mostly just needs some polish/suggestions/tweaks at this stage, which a PR can do!

It notably would also break the interface, so if could be serendipitous to potentially change the frequency/resolution interfacing around the same time to just have one break.

Tangent: (What I meant to post before - oops!) what does the pin selection in the pwm module do? AFAIK, the MCU knows to output to the correct pin if it's set up as an output on the right AF.

Ah, the pin selection in to try and hide the AFx settings all together. So a user doesn't have to lookup how to configure the pin, they just pass it in and everything else is wired for them. That particular bit of abstraction I do think worked out nicely!