Add support for TIM6 #101
Add support for TIM6 #101
Conversation
Tested the interrupt on an STM32L071KBTx, seems to work fine. The `CountDown` impl was not tested, but I don't see a reason why it shouldn't work.
|
It should be hidden behind feature gate only for models that do have this timer WDYT? |
|
Thank you @dbrgn and @almusil!
I agree. Please restrict TIM6 to the models that have it. If you don't feel up to checking the documentation of all STM32L0's, feel free to restrict it to the models you know to have it and add a comment to that effect. Anyone who needs it for more models later, can submit a pull request. (Better to cause an obvious compiler error than have silent breakage that is harder to track down.) |
I personally don't think that this is a good approach. It will lead to people being very confused why certain peripherals are available while others are not. If possible at all, I would try to find a way to determine this information based on machine readable information by ST. In the case of alternate functions, this was possible by using https://github.com/dbrgn/cube-parse. For the timers, I have not yet figured out how to extract this information from the database (but I'm fairly sure it's there). It might map to the product categories. |
I agree, that's a real disadvantage. However, I think making peripherals available where they shouldn't be, is worse. None of those solutions are good, but if I have a choice between a loud error and silent breakage, I'm going to choose the loud error any day.
I agree that this would be the optimal solution. It would be less optimal but still great if someone read all the documentation and made sure TIM6 is available where it should be. If you're willing to do either of those, that's great. But as a general rule, I can't expect that kind of effort from someone who just wants to use TIM6 on their chip. That just leads to incomplete PRs that are never going to get finished. And I'd rather have limited support for TIM6 (to be improved upon later, as necessary), than not have TIM6 support at all. |
|
I see your point and I'm willing to take a look again at the database for this PR, maybe we can find a good solution there. However, I also think that it's a bad user experience for Rust Embedded in general if users of a HAL have to create a few pull requests in order to whitelist the MCU they're using for all peripherals they're using. There are currently 158 MCUs in the STM32L0 family, so we would need a lot of whitelisting to support all of them in a manual way (and then would need to update this every time new MCUs are released). |
|
By the way, I just found this: https://www.st.com/resource/en/application_note/dm00042534-stm32-crossseries-timer-overview-stmicroelectronics.pdf
It doesn't even mention TIM6 at all, indicating that "it's complicated" 🙂 Also, the "Not available on all products in the line. Check the datasheet for details" footnote indicates that this might really be device-specific. I hoped for some kind of general pattern. However, doing My suggestion would be to emrge this as-is for now, and to open a new issue with the goal of properly feature gating all timers. Feel free to assign me to that issue. |
Totally, but as I said above, I think accidental silent breakage is an even worse user experience. Ideally it should "just work", but someone need to put in the work to make that happen. That could be you or me right now, or many small contributors over a longer period of time.
Okay, fine. I'd still prefer restricting TIM6 availability to a known-good subset, but I don't feel that strongly about it. |
Tested the interrupt on an STM32L071KBTx, seems to work fine. The
CountDownimpl was not tested, but I don't see a reason why it shouldn't work.I would have added TIM7 as well, but the PAC does not yet expose it.
Note that not all models have TIM6 / TIM7.