-
Notifications
You must be signed in to change notification settings - Fork 48
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
WIP: HRTIM #96
WIP: HRTIM #96
Conversation
examples/hrtim/eev.rs
Outdated
@@ -0,0 +1,113 @@ | |||
//This example puts the timer in PWM mode using the specified pin with a frequency of 100Hz and a duty cycle of 50%. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These descriptions should be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost all of the hrtim examples are just copy pasted from examples/hrtim/hrtim.rs or examples/hrtim/simple.rs and modified without having their comments updated.
|
||
out1.enable_rst_event(&cr1); // Set low on compare match with cr1 | ||
out1.enable_rst_event(eev_input4); | ||
out1.enable_set_event(&timer); // Set high at new period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@no111u3 sort of related to #98...
I have been experimenting a bit with different ways to specify different types of event sources.
Here an event is specified by passing a reference to the thing to listen to. In this case:
- reset output on
compare register 1
match - reset output on
external event 4
signal - set output on timer period event
The nice thing about this is that there is no way to specify the wrong event source by accident. Also there is no need to figure out what enum and variant to to use, just pass in the actual object(or reference to it).
However this requires the event source to exist before the listener which might make things more complicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@usbalbin thank for this great example. Yesterday I've merged some changes but it will update in some next sessions (for every timers also). So I think about correct and wrong source for event, it might be provided by internal mark field (I'm not sure that rust can do it).
However this requires the event source to exist before the listener which might make things more complicated?
Following the RM on G4 (and other) family:
Note: The clock of the slave timer or ADC must be enabled prior to receive events from the master timer, and must not be changed on-the-fly while triggers are received from the master timer.
That means consumer must be created first, I've checked it on my g474 — violation rule makes ADC crazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[..] Yesterday I've merged some changes [..]
Nice :)
It least when it comes to hrtim things might get quite complicated(I have not looked at a lot of other timers yet). For example I believe it is possible to use a:
- DAC to output, for example, a saw tooth shape
- This is routed to the comparators inverting input
- The comparators output is routed to the hrtim EEV1
- EEV1 is used as an rst event for an hrtim timers output.
- That same hrtim timer triggers the update of the DAC's wave form, and around we go
Which object should be created first? :)
Whatever API we pick, it should preferably support doing things like that, while preventing the user from doing something that will violate the RM's rules.
examples/hrtim/adc-trigger.rs
Outdated
.HRTIM_COMMON | ||
.hr_control(&mut rcc) | ||
.enable_adc_trigger1_source(Adc13Trigger::TimACmp3) | ||
.enable_adc_trigger1_source(Adc13Trigger::TimACmp4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this enable method was instead defined on HrPwmControl
then the variables cr3
and cr4
could be passed directly to that method without the need for the enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it better to use one work variant instead of any workless.
6a4b3d0
to
da8337e
Compare
I have now gotten rid of a few enums in favor of traits to force the user to pass a reference to the actual type instead of an enum variant. See For anyone wanting to give it a try, I am using usbalbin/stm32-rs@ab9082b until the next stm32-rs/stm32-rs release |
966c1d9
to
9ad72a4
Compare
* Rcc enable is now handled for the entire HRTIM peripheral by dp.HRTIM_COMMON.hr_control() instead of by every timer * HRTIM calibration is now mandatory, also handled by `dp.HRTIM_COMMON.hr_control()` but also by running on that result `_.wait_for_calibration(). * Add `get_state` for `HrOutput` to check what state an output is in: `Idle`, `Running` or `Fault`
* dma - update for new pac * adc - triggers * hrtim
9ad72a4
to
5ba64f8
Compare
@liamkinne or anyone, I am using this or rather yet another branch at work and I would at some point like to merge this or some future version of it. Since this is starting to turn into quite big set of changes, do you have any preferences for how it should be done? It is all just one single peripheral device so of course there are interdependencies. So splitting things up into multiple PRs, while possible, might be less fun after a certain point. |
Also I am using some of the not yet released features from the pac so depending a bit on when the next pac will be released I would prefer to at least wait for that. |
I'm not familiar enough with the peripheral to know if it would be easy to break down. Happy to help in reviewing when that time comes. |
@usbalbin was just thinking about this for an upcoming project. Do you think it makes sense do this as a separate |
Not sure how big the difference is between HRTIM in g4x4 vs f334 vs h7, but sure why not :) |
According to this (section 1.4): https://www.st.com/resource/en/application_note/an4539-hrtim-cookbook-stmicroelectronics.pdf There are only a few differences. tbh I would still go ahead an when it's ready get this merged into G4-hal and later look at making it generic. |
Thats seems promising :)
I currently depend on yet to be released pac features. However if that takes too long then perhaps I will try to make it work with the current version. Or ripp out the affected changes in my future PR. |
I will close this in favor of #146 |
Work in progress.
Currently requires stm32-rs/stm32-rs#860 , thus all the CI errors
Thus this(or some future version of this) should probably not be merged until after #102
TODO:
PreloadSource
,MasterPreloadSource