Clean up PWM code, fix race conditions, improve STM32L0x2 support #21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Please note that I found some incorrect uses of `unsafe` while making this change. I've clearly marked them with comments in this commit, but they were there before.
The `channels!` macro is not flexible enough to support all the channels on STM32L0x2. This means I need to refactor or replace it, and moving stuff out of it is a good step in preparation for that.
As noted in a previous commit, the current structure of the `pwm` is not flexible enough to support the capabilities of STM32L0x2 fully. Specifically the approach of defining the channels in sets that all use the same alternate function number does not match the hardware. I believe a better approach is to define each channel separately, as that provides the most flexibility. I also believe it will be simpler to make each channel accessible as a separate struct, which requires having a main struct that has them as fields. This main struct is the new `pwm::Timer` struct which is added in this commit. In addition, I've taken the chance to clean up the PWM initialization a bit: - I've removed the `PwmExt` struct, replacing it with a constructor. This is simple and straight-forward. - I've replace the old macro with a simpler, more lightweight one. This means more code lives outside of the macro, which has a number of advantages.
Move the duty cycle changes into the loop (no more missing the beginning and having to restart the program), and reduce the delay between them (faster to verify that the program works as intended).
It doesn't have any channels defined yet, so it can't really be used yet.
I'm a bit torn about this change, because it seems a bit less readable when looking only at the struct. Unfortunately I can't name them `Instance` and `Channel`, as that would conflict with the trait names. However, this makes the struct consistent with most of the rest of the module, which should improve readability overall. Plus, the all-uppercase names look weird, as they are usually used for constants and statics, not types.
With this new structure, the whole configuration is much more flexible now, as each channel can be defined and assigned separately.
|
Rebased on latest master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a big clean-up effort of the
pwmmodule. It fixes the following problems:unsafeaccess to timer registers was unsound, as it didn't take race conditions into account.Overall, I believe that this new approach, with each channel being its own struct that can be configured separately, is a step in the right direction. In fact, I think we should consider using the same approach for the
timermodule, maybe even merge both modules. (I'm not sure if the hardware supports this, but if both modules were merged it could be possible to use one channel for PWM, another for something else, e.g. as a hardware trigger for ADC.)cc @lthiery