Skip to content

cpu/samd21: Added CPU_MODEL_SAMD21G18A interrupt config#7125

Merged
aabadie merged 1 commit intoRIOT-OS:masterfrom
kbumsik:mkr-fix
Jun 22, 2017
Merged

cpu/samd21: Added CPU_MODEL_SAMD21G18A interrupt config#7125
aabadie merged 1 commit intoRIOT-OS:masterfrom
kbumsik:mkr-fix

Conversation

@kbumsik
Copy link
Copy Markdown
Contributor

@kbumsik kbumsik commented Jun 1, 2017

I noticed the exti_config table was uninitialized while porting WINC1500 inside of Arduino MKR1000 (It took me a few days to figure out 😂 ). So I added a proper table for SAMD21G18A.

I also added a guard to make sure there is a matched CPU_MODEL in the source.

Related: #7092 #6881 #6666


This change is Reviewable

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jun 1, 2017
@aabadie aabadie added this to the Release 2017.07 milestone Jun 1, 2017
@aabadie aabadie changed the title mkr-common: Added CPU_MODEL_SAMD21G18A interrupt config cpu/samd21: Added CPU_MODEL_SAMD21G18A interrupt config Jun 1, 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 Jun 1, 2017
@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 2, 2017

@kbumsik Could you confirm that you can register and use several EXTI at the same time on your board please ? I suspect a bug in gpio driver for SAML21 and SAMD21 (see #6904 and #6932 )

@kbumsik
Copy link
Copy Markdown
Contributor Author

kbumsik commented Jun 2, 2017

@dylad That looks pretty important. I will try and post it on #6932 .

@dylad
Copy link
Copy Markdown
Member

dylad commented Jun 7, 2017

@kbumsik Thanks you ! I'm looking forward to your feedback :)

@photonthunder
Copy link
Copy Markdown

Thanks for this PR, I also would have spent days trying to figure out why the EXTI was not working. Just posted on #6932 that I did not see a problem with two EXTI on my samd21g18a.

@TomKeddie
Copy link
Copy Markdown

@kbumsik can I suggest we add a #else clause with all entries -1 in it? or a #error clause in it? This will give more meaningful results where the CPU_MODEL is not catered for by this block.

@kbumsik
Copy link
Copy Markdown
Contributor Author

kbumsik commented Jun 16, 2017

@TomKeddie That's what I was thinking at the beginning. But I ended up with this kind of bug is hard to chatch unless the compiler doesn't explicitly warn about this. Since I am a mere user of RIOT I don't know if a scenario that a matched CPU_MODEL is not listed in that block is normal or can be intentional. If this case cannot be intentional I believe we better refuse compiling so that a future developer would notice this when he/she is porting a new SAMD CPU.
I would like to ask about this to the maintainers.

Or, as the middle point, we might do like this using #warning instead:

#else 
    #warning CPU_MODEL is not matched here. This may lead to malfunctioning EXTI.
     {-1,  -1, -1, -1, ... 
     },
     {-1,  -1, -1, -1, ... 
     },
#endif

By the way, -Werror is enabled by default in RIOT. What do you think?

@TomKeddie
Copy link
Copy Markdown

@kbumsik I think that's a great idea, lets push it to the reviewers and see what they think.

Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Changes are correct here (according to the datasheet). ACK

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 22, 2017

and go!

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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants