[RFC] Improve stm32f0 interrupts vector implementation

[aabadie]

This PR modifies the cpu/stm32f0/vectors.c file to better fit the interruptions defined for each type of CPU in this family (following the cmsis header file): f030r8, f042k6, f051r8, f070rb, f072rb and f091.

The change here is based on define but makes the code hard to read/follow. I know it's not a good solution, as it put a lot of mess in the file. It will also become more and more harder with more complex CPU (f3, f4, etc). I see other ways to better handle this:

• protect the whole vector within define for each CPU in one vectors.c file:

#if defined(CPU_XXX)
(void*) isr_systick,            /* SysTick interrupt, not used in RIOT */
/* STM specific peripheral handlers */
(void*) isr_wwdg,               /* windowed watchdog */
[...]
(void*) isr_cec,                /* consumer electronics control */
(void*) isr_usb                 /* USB */
#elif defined(CPU_YYY)
(void*) isr_systick,            /* SysTick interrupt, not used in RIOT */
/* STM specific peripheral handlers */
(void*) isr_wwdg,               /* windowed watchdog */
[...]
(void*) isr_cec,                /* consumer electronics control */
(void*) isr_usb                 /* USB */
#end

• define and implement the vectors in separate *.c file for each cpu : stm32fxxx_vectors.c, stm32fyyy_vectors.c, etc and build the right one at compile time. This would require changes in the Makefile but I don't know how to do that (as I don't fully understand them).
• Leave it as it is already in master and close this PR.

What do you think ?

[kYc0o]

Looks OK for me. Can you maybe rebase to include Murdock2?

[kYc0o]

Oh, it was added automatically. No need for rebase.

[aabadie]

Well this PR is not meant to be merged as is. I opened it in order to find to better solution (or leave things as they are now) to the actual way of implementing vectors.c files with stm32.

[aabadie]

Ah, I just noticed there is a Requests for Comments label...

[haukepetersen]

Actually, I don't like this, I think those defines make it completely unreadable and therefore next to impossible to actually spot errors. Just think what happens if we add 15 more stm32f0s to the list (or even merge all stm cpus at one point...).

IMHO a nicer solution would be to pre-define complete tables for the single cpu types and just load the fitting one into the final table at compile time.

[aabadie]

Actually, I don't like this, I think those defines make it completely unreadable and therefore next to impossible to actually spot errors. Just think what happens if we add 15 more stm32f0s to the list (or even merge all stm cpus at one point...).

That was actually one of the reasons why I opened this PR initially.

a nicer solution would be to pre-define complete tables for the single cpu types and just load the fitting one into the final table at compile time.

Looks interesting and doable with the prepropessor. What do you think of the second point I mentionned in the PR: use a dedicated stm32fxxx_vectors.c for each supported cpu ?

[aabadie]

@haukepetersen, ea9edca implements your suggestion about injecting complete tables at compile time. The file/directory names could be improved. Better suggestions are welcome.

[jnohlgard]

@aabadie how about just inserting the file directly into the vector specification?

ISR_VECTORS void * const interrupt_vector[] = {
[CORTEXM_VECTORS]
#if defined(CPU_STM32something)
#include "vectors/stm32something.c"
#elif defined(CPU_STM32sthelse)
#include "vectors/stm32sthelse.c"
#endif
};

[aabadie]

how about just inserting the file directly into the vector specification?

I thought including .c file in another .c file was not a good practice but maybe in this particular case it could be done.

[jnohlgard]

@aabadie yes, I agree it's bad practice, but this file would become incredibly long and impossible to navigate if we don't split it up. Having a macro like the current proposal avoids that bad practice, but instead we need to escape the newlines which may also be seen as a bit of a hassle. I'm not really sure what would be the better solution of the two, actually.

[kaspar030]

Why not a .c file per vector table?

[kYc0o]

Why not a .c file per vector table?

👍

[aabadie]

Why not a .c file per vector table?

Yes that was one of the initial alternatives I proposed. The problem comes from my poor knowledge of the build system : can you give some advice on how to build the right .c depending on the type of MCU ?

@astralien3000 also suggested me IRL to skip the macro and directly include a .h file.

[aabadie]

I pushed a couple of commits with what @astralien3000 had in mind. I think it's much better now : no more macro and the file in included once.

[astralien3000]

You could also add some code to prevent to include several .h, like :

#ifdef MCU_SPECIFIC_VECTOR
#error "Two MCU-specific interrupt vector enabled"
#endif
#define MCU_SPECIFIC_VECTOR

But it is maybe overkill.

[kaspar030]

Hm, I don't like it. Now we have a bunch of headers with essentially broken syntax. Could you try the one-file-per-vectortable?

I suggest creating a module "stm32_vectors" (e.g., as subfolder of cpu/stm32_common or /f0).

In there you create files "vectors_<cpu>.c", containing the full vector table:

ISR_VECTOR const void *mcu_interrupt_vector[] = {
   /* device specific vector */
};

In cpu/<whatever>/stm32_vectors/Makefile you select the correct source file with something like:
```SRCS := vectors_$(CPU_MODEL as lowercase).c``

In cpu/<whatever>/vectors.c you define the Cortex-M base vectors:

ISR_VECTOR const void *cortexm_interrupt_vectors[] = {
   /* cortex-m vectors */
};

This should suffice, as the linker will sort mcu* after cortex*. If not, a sorting statement might need to be added to the linker script section.

Advantages over including:

• in order to add a new MCU vector table, only the correct vectors_<cpu>.c needs to be created, and the build system will pick it up. No need to add another ifdef case anywhere,

Actually this would be a nice improvement for all Cortex-M, not just stm32.

edit fixed < > being invisible

[aabadie]

@kaspar030 thanks, your suggestion indeed is a nice idea. I'll try it.

[aabadie]

ping @kaspar030

[kaspar030]

@aabadie While this is is definitely an improvement, I have the feeling that a lot of lines get duplicated this way.

• can we move the weak declarations into a shared header?
• with make: fix linking #5757 merged, one dummy default should be enough?

[aabadie]

can we move the weak declarations into a shared header?
with #5757 merged, one dummy default should be enough?

Yes to both without #5757. I moved everything to a single header file.

[kaspar030]

@aabadie You're the one juggling with nucleo boards. Do you think this is an improvement? To me it looks like a lot of duplication. The nucleos are so similar...

[aabadie]

You're the one juggling with nucleo boards.

We have plenty of them here :)

Do you think this is an improvement? To me it looks like a lot of duplication. The nucleos are so similar...

Not sure I understand. Did you mean the stm32 cpus instead ?
There are indeed subtle differences between them. At the beginning, I just wanted to use #ifdef macros in vectors.c which was simple but not so nice.
Now, the common pieces are extracted and moved to single places (vectors.c and vectors_stm32f0.h) and kept the specific part in dedicated files. I don't think we can do better than that, but maybe I'm wrong.

[haukepetersen]

It seems to me (I might be wrong so), that the current state is having two problems:

• do we always build all .c files in the vectors folder? We should definitely only build the fitting one...
• do I see it correctly, that the resulting vector table depends on the order, in which the specific vector and the shared vector are linked and thereby put into the vector section? I would prefer a order independent approach...

[aabadie]

Closing this one in favor of #7535