Skip to content

kinetis: Generic ISR vector table for all Kinetis CPUs#7878

Merged
smlng merged 4 commits intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-one-isr-vector
Oct 31, 2017
Merged

kinetis: Generic ISR vector table for all Kinetis CPUs#7878
smlng merged 4 commits intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-one-isr-vector

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Oct 26, 2017 •
edited
Loading

Replaces #7761.

Use one generic ISR vector table with contents depending on what is available in the selected vendor header. The numbering is handled by the vendor header definition of enum IRQn, so it will always match each CPU.

This table was generated using an AWK script and then manually edited where the script generated the wrong preprocessor conditions.

@jnohlgard jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 26, 2017
@jnohlgard jnohlgard force-pushed the pr/kinetis-one-isr-vector branch from dcc088f to 941dc5c Compare October 26, 2017 23:36
@jnohlgard jnohlgard mentioned this pull request Oct 26, 2017
Merged
@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 27, 2017

may replace #7761 completely if squashed.

I agree, they go hand-in-hand and its fine to have this done here completely.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 27, 2017

I will close #7761 to focus on this instead.

smlng
smlng approved these changes Oct 27, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

Tested with PhyNode (pba-d-01-kw2x) the only kinetis board I've available: works! Build size decreases by 8B, wasn't expecting any change though ... but hey 8B is 8B 😉

Side question why is MK22F10.h removed, not needed anymore?

@jnohlgard jnohlgard mentioned this pull request Oct 27, 2017
Closed
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 27, 2017

@smlng yes, a553497

k22f: Remove unused MK22F10.h header …
gebart committed on Aug 4
According to NXP material, there are no 100 MHz K22F parts in 144 pin
packages, which is the reference manual that this header is supposed to
correspond to.
The header was originally included from Keil uVision Kinetis support
packages. It is possible that this header was only used with
engineering samples of the K22F.

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 27, 2017

ah thanks for the info, haven't read the (self explanatory) long commit message 😞

@smlng smlng added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 27, 2017
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 27, 2017

regarding #7761 (comment)
@haukepetersen

On first sight, I am not too happy with this change. Having the vector names following the coding rules makes the code just so much nicer to look at, and I more pressingly feel that if we start to make an exception to the rules here, people will start to make these exceptions in many other places (be it for good reasons or not), leading to a more incoherent code base... But maybe I am to picky here?!

It will be easier to verify the implementation when the vector table in the binary can be compared with the IRQ list in the vendor header.

I see the point, but I imagine that this test is mostly done automatically using some kind of script, right? In that case, I think it is easy to convert a string from upper to lower case. Also when using some kind of regex to do these checks, it is mostly also simple to ignore the case.

But in the end I stated my doubts, but I will not block this PR if it is decided to go with it, I just won't be the one merging this :-)

@smlng @haukepetersen are you fine with this if I keep the ISR names as proposed but make them all lowercase?

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 27, 2017 •
edited
Loading

I agree, lowercase would be consistent with RIOTs coding style and other ISR vectors, for instance the once ones for the STM families.

@jnohlgard jnohlgard force-pushed the pr/kinetis-one-isr-vector branch from 941dc5c to 21abdbe Compare October 27, 2017 12:10
@smlng smlng removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Oct 27, 2017
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Oct 27, 2017

Yes, that would be great!

@jnohlgard jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 27, 2017
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 27, 2017

@haukepetersen @smlng I changed the names to lowercase now.
I'll let Murdock do a compile run to see if I have missed anything.

@jnohlgard jnohlgard force-pushed the pr/kinetis-one-isr-vector branch from 081ab99 to 09b9af9 Compare October 28, 2017 14:58
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 28, 2017
@jnohlgard jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 31, 2017
Joakim Nohlgård added 4 commits October 31, 2017 05:45
k22f: Remove unused MK22F10.h header
f98e286 
According to NXP material, there are no 100 MHz K22F parts in 144 pin
packages, which is the reference manual that this header is supposed to
correspond to.
The header was originally included from Keil uVision Kinetis support
packages. It is possible that this header was only used with
engineering samples of the K22F.
k22f: Update to latest vendor headers
2945532
kinetis_common: Use separate array for padding ISR vector table
222842c
kinetis: Merge all ISR vector tables into one generic mutant
dea1676 
The ISR names have been changed to match the name of the IRQ number they
are servicing.
@jnohlgard jnohlgard force-pushed the pr/kinetis-one-isr-vector branch from 09b9af9 to dea1676 Compare October 31, 2017 04:45
@jnohlgard jnohlgard added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 31, 2017
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Oct 31, 2017

rebased, squashed
@smlng does your ACK still hold?

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 31, 2017

retested with PhyNode pba-d-01-kw2x, still works. Again: nice work!

@smlng
Copy link
Copy Markdown
Member

smlng commented Oct 31, 2017

ACK & GO!

@smlng smlng merged commit 6501e71 into RIOT-OS:master Oct 31, 2017
@jnohlgard jnohlgard deleted the pr/kinetis-one-isr-vector branch October 31, 2017 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@smlng smlng smlng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@haukepetersen haukepetersen

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jnohlgard @smlng @haukepetersen