Skip to content

cpu/stm32: cleanup interrupt vectors#7485

Merged
haukepetersen merged 8 commits intoRIOT-OS:masterfrom
aabadie:stm32_cleanup_vectors
Aug 28, 2017
Merged

cpu/stm32: cleanup interrupt vectors#7485
haukepetersen merged 8 commits intoRIOT-OS:masterfrom
aabadie:stm32_cleanup_vectors

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Aug 21, 2017

After doing this for stm32f7 family in #6991, I though it would be nice to do it for the others family.

The strategy is the same:

  • I split isr definitions for each cpu type, trying to keep the vectors.c as readable as possible.
#if defined(CPU_MODEL_STM32FXXX)
...
(void*) isr_xxx,  /* [i] interrupt comment*/
...
#eilf defined(CPU_MODEL_STM32FYYY)
...
(void*) isr_jjj,  /* [j] interrupt comment*/
...
  • Added comments for each line with the interrupt number: this simplify readability, I think and hence maintainability

While working on this, some issues were fixed:

  • wrong number of IRQ for some cpus
  • isr not defined, or defined whereas not available (I don't have the full list but there quite a few in this case)

Boards/CPU to test (feel free to add any missing ones):

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Aug 21, 2017
@aabadie aabadie added this to the Release 2017.10 milestone Aug 21, 2017
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 21, 2017
@aabadie aabadie force-pushed the stm32_cleanup_vectors branch 3 times, most recently from ef6321e to 2250d22 Compare August 21, 2017 08:48
@haukepetersen
Copy link
Contributor

I am not sure if we can't do better, I think this is definitively a big improvement in terms of readability (and just see the number of bugs found with this refactoring...). So for now I like the PR.

@vincent-d what do you say about it?

@aabadie
Copy link
Contributor Author

aabadie commented Aug 21, 2017

I am not sure if we can't do better

Agreed, and I didn't want this PR to be the ultimate solution. I already tried different things in #6617 but none were accepted (for many reasons).
At least, as you said, here we have something readable, where all types of MCU are correctly configured (I hope). It will make things simpler if one day we find a better solution

@vincent-d
Copy link
Member

I like it! It's much easier to read than before (thanks to the [i] comment I guess).

(void*) isr_spi4, /* [84] SPI4 global Interrupt */
#endif
#endif
}; No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, directly squashed

(void*) isr_tim1_trg_com_tim17, /* [26] TIM1 Trigger and Commutation Interrupt */
(void*) isr_tim1_cc, /* [27] TIM1 Capture Compare Interrupt */
(void*) isr_tim2, /* [28] TIM2 global Interrupt */
#if defined(STM32L432KC)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong: should be CPU_MODEL_STM32L432KC

@aabadie aabadie force-pushed the stm32_cleanup_vectors branch from 2250d22 to 3498ddc Compare August 21, 2017 15:35
@haukepetersen
Copy link
Contributor

As said, I also like it. But before giving my ACK, I would like to do some more intensive testing, as this PR has means to break quite a bit of boards :-) Should we somehow synchronize testing efforts?

@aabadie
Copy link
Contributor Author

aabadie commented Aug 21, 2017

Should we somehow synchronize testing efforts?

That would be good indeed :)
I edited the PR comment with a first list of boards but maybe I missed some.

@aabadie
Copy link
Contributor Author

aabadie commented Aug 21, 2017

Another question: what kind of tests ? (all configured periph tests I guess ?)

@haukepetersen
Copy link
Contributor

we can't test everything, but testing the default example (incl. SAUL mappings to the button) and maybe a timer test for selected boards together with a thorough code review should do the trick.

@aabadie aabadie force-pushed the stm32_cleanup_vectors branch from 3498ddc to ab292a9 Compare August 23, 2017 16:25
@aabadie aabadie force-pushed the stm32_cleanup_vectors branch from ab292a9 to a02ba69 Compare August 23, 2017 20:20
@haukepetersen
Copy link
Contributor

Confirmed working on the discovery boards.

I think we have covered enough boards to verify this PR, so we can safely merge this.

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK after giving this another thorough review. Still amazing how many miss-configurations we had in the code before...

@aabadie
Copy link
Contributor Author

aabadie commented Aug 28, 2017

I think we have covered enough boards to verify this PR, so we can safely merge this.

+1

@haukepetersen
Copy link
Contributor

and go.

@haukepetersen haukepetersen merged commit ac95d42 into RIOT-OS:master Aug 28, 2017
@aabadie aabadie deleted the stm32_cleanup_vectors branch February 26, 2018 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants