[riot-notifications] [RIOT-OS/RIOT] boards/stm32_common: Clean up IMR (et al.) handling (#12163)
notifications at github.com
Tue Sep 3 16:52:24 CEST 2019
During implementation and review of #12144 @benpicco and I came across an issue where the peripheral access layer headers of STM32 boards have been altered in order to accommodate the rest of the RIOT code. While this works, it has lead us to think that this is rather not the ideal implementation. Here's the actual conversation we had about it:
> 16:04 mrus > benpicco: that's why I asked for a review before, because I was unsure. However, changing the code would
> actually mean a crapload if #ifdefs across the stm32_common. Besides: Take a look at e.g. stm32l432xx.h.
> It was altered as well and is not in its original form. In fact it was altered in the exact same places
> to accomodate the code that utilizes it.
> 16:05 benpicco > 😭
> 16:06 mrus > so, if we do it in a clean manner (-> adjusting RIOT's stm32_common code) we should do it for *all*
> 16:06 benpicco > usually those ifdef parts can be capsuled nicely into inline functions
> 16:06 mrus > otherwise the mixup might be more confusing in the end, because you could not tell which code was
> altered and which one was worked around.
> 16:06 benpicco > I wasn't aware that the vendor files had been tampered with before
> 16:08 mrus > wait, I might be wrong on the example I gave, that file looks untampered
> 16:08 mrus > but I think I saw an example, one second
> 16:09 mrus > no, sorry, I was right
> 16:09 mrus > https://raw.githubusercontent.com/ARMmbed/mbed-os/master/targets/TARGET_STM/TARGET_STM32L4/TARGET_STM32L4
> 16:10 mrus > search for IMR1
> 16:10 mrus > you'll find it
> 16:10 benpicco > but this is madness
> 16:10 mrus > in the RIOT version:
> 16:10 mrus > __IO uint32_t IMR;
> 16:10 mrus > it was altered from IMR1 to IMR, just the way I did.
> 16:10 benpicco > it's pretending IMR2 would not exist
> 16:12 mrus > I don't think RIOT's code uses IMR2 anywhere
> 16:13 benpicco > It's stuff like this that makes adding new MCUs to RIOT so bothersome
> 16:13 benpicco > if it had been done properly, IMR1 would be handled in the driver and all new MCUs that use that instead
> of IMR would just work
> 16:13 benpicco > instead now we have to tamper with each new vendor file
> 16:14 mrus > what's your suggestion?
> 16:15 benpicco > restore the original vendor file and handle IMR1 properly
> 16:15 benpicco > I mean you already had the code to handle it
> 16:16 mrus > but the code was really just a #define. Shouldn't there be another mechanism that handles IMR1 and IMR2?
> 16:16 benpicco > I would suspect they still work the same as IMR, just now that there are two of them
> 16:17 mrus > but the second set won't be something that's just there for the thrill of it, is it? I mean.. they do
> serve a function.. but RIOT is simply not using it at the moment. No?
> 16:19 benpicco > I didn't read the data sheet for this. I would suspect some pins are connected to IMR1 and some to IMR2
> 16:19 benpicco > https://github.com/LonelyWolf/stm32/blob/master/stm32l4-template/periph/exti.c
> 16:20 benpicco > > // Initialize the EXTI lines(s) in range from 0 to 31 according to the specified parameters
> 16:20 benpicco > -> EXTI1
> 16:20 benpicco > // Initialize the EXTI lines(s) in range from 32 to 39 according to the specified parameters
> 16:20 benpicco > -> EXIT2
> 16:21 benpicco > eerm I mean IMR1 & IMR2
> 16:22 mrus > okay, here's a suggestion:
> 16:26 mrus > Let's stick to the tampered source file for this commit - otherwise this is going to become madness -
> and open an issue about cleaning up this spill. Because as I've shown, it needs to be cleaned up in
> different places, not only for this board. However, this has to be a team effort, because I only have a
> handful of boards here (L412KB, L031K6, L432KC & L476RG). Maybe in that issue we can agree on code that
> 16:26 mrus > we use throughout the project for this purpose (remember, there are things like
> e0R79 as well) - and then I can offer to begin cleanup for the boards that I have here.
> 16:26 mrus > wdyt?
> 16:32 benpicco > well the child is already in the well for this one, so I guess if you don't volunteer for the cleanup,
> you can of course continue in the olden ways
> 16:32 benpicco > btw: instead of #if defined(CPU_MODEL_STM32L412KB)
> 16:33 benpicco > in common code, try using something like #if defined(TIM_CCR4_CCR4) when you want to use CCR4
> 16:33 benpicco > so when adding a new MCU that also uses those registers, it will just work instead of throwing errors
> (or even worse, silently fail because no path is selected)
> 16:34 benpicco > I know this is done elsewhere in RIOT too and I already tried to fix it for sam0
> 16:35 mrus > I do volunteer for the cleanup, as I said above. I just don't think it makes sense to clean up only one
> newly implemented board but rather make it a team effort PR where we clean up all boards that require
> cleanup at once.
It probably makes sense to discuss this further in this issue and agree on how to proceed on this topic. The ideal outcome would be to be able to use the original headers provided by STM (without modifications), just like e.g. Zephyr uses them. I can imagine that it would also be nice it IMR2 would actually be functional. 🤷🏻♂️
I've put together a list of headers that *I believe* were altered. I didn't double-check it yet:
Wdyt? Feedback on this is appreciated!
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the notifications