Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

boards/nucleo144-f413 add inital support #6565

Merged
merged 2 commits into from
Feb 24, 2017
Merged

boards/nucleo144-f413 add inital support #6565

merged 2 commits into from
Feb 24, 2017

Conversation

vincent-d
Copy link
Member

This adds support for nucleo144-f413 (mainly imported from #6553) and support for stm32f413.

It needs a patched openOCD because the f413 is not correctly detected. I'll try to submit my patch upstream ASAP.

@vincent-d vincent-d added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 7, 2017
@vincent-d vincent-d mentioned this pull request Feb 7, 2017
Merged
@vincent-d
Copy link
Member Author

OpenOCD patch is here http://openocd.zylin.com/#/c/3969/

aabadie
aabadie requested changes Feb 8, 2017
View reviewed changes
Copy link
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.

A few comments. I can't test on hardware. @kYc0o do you have this one in your office for testing ?

boards/nucleo144-f413/board.c
@@ -0,0 +1,33 @@
/*
* Copyright (C) 2016 Inria
Copy link
Contributor

Choose a reason for hiding this comment

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

2017, OTAKeys with Inria ?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I didn't change the code from you PR, I didn't think about the copyright. Addressed

boards/nucleo144-f413/board.c
* @file
* @brief Board specific implementations for the nucleo144-f413 board
*
* @author Alexandre Abadie <alexandre.abadie@inria.fr>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add yourself in the list .

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed

boards/nucleo144-f413/include/board.h
@@ -0,0 +1,79 @@
/*
* Copyright (C) 2016 Inria
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment and probably in other places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addessed

boards/nucleo144-f413/include/periph_conf.h
@@ -0,0 +1,268 @@
/*
* Copyright (C) 2016 Inria
* Copyright (C) 2017 OTA keys S.A.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok :)

cpu/stm32f4/vectors.c Outdated
@@ -108,6 +108,10 @@ WEAK_DEFAULT void isr_otg_hs_ep1_in(void);
WEAK_DEFAULT void isr_otg_hs_wkup(void);
WEAK_DEFAULT void isr_otg_hs(void);
WEAK_DEFAULT void isr_dcmi(void);
WEAK_DEFAULT void isr_can3_tx(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe protect with the same if defined as below or the build will fail for other boards (not sure).

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed, and add a comment with the reference manual

@vincent-d vincent-d added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 8, 2017
aabadie
aabadie requested changes Feb 13, 2017
View reviewed changes
Copy link
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.

Jenkins also reported trailing whitespaces in the cmsis file.

cpu/stm32f4/vectors.c
@@ -103,11 +105,20 @@ WEAK_DEFAULT void isr_dma2_stream7(void);
WEAK_DEFAULT void isr_usart6(void);
WEAK_DEFAULT void isr_i2c3_ev(void);
WEAK_DEFAULT void isr_i2c3_er(void);
#if defined(CPU_MODEL_STM32F413ZH)
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, I think the specific defined is not required. I tested it for another CPU (F0) and it still builds if the function is not used.
I don't know what's the best practice here. Maybe @haukepetersen can help ?

@aabadie aabadie mentioned this pull request Feb 20, 2017
Open
@vincent-d
Copy link
Member Author

Cleaned up vendor header (remove trailing spaces)

@aabadie
Copy link
Contributor

aabadie commented Feb 20, 2017

Looks good now. Can you squash the commits ?

@vincent-d
Copy link
Member Author

Squashed

@vincent-d vincent-d removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 21, 2017
aabadie
aabadie approved these changes Feb 21, 2017
View reviewed changes
Copy link
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.

Un-tested ACK (surprisingly, I don't have this one ;) )

@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 Feb 21, 2017
@vincent-d
Copy link
Member Author

rebased

@aabadie
Copy link
Contributor

aabadie commented Feb 24, 2017

Even if it's all green, I'm not sure if I should press the green button now : I prefer waiting for your OpenOCD patch to be merged first

@vincent-d
Copy link
Member Author

We will need the cpu support soon for our hardware, so I would like to have this merged if possible.

@aabadie
Copy link
Contributor

aabadie commented Feb 24, 2017 •
edited
Loading

I see. Can you verify that the documentation in the wiki is ok ? In particular, the "flashing" section.
If you are ok with it, I'll merge.

@vincent-d
Copy link
Member Author

wiki looks OK

@aabadie
Copy link
Contributor

aabadie commented Feb 24, 2017

wiki looks OK

then go !

@aabadie aabadie merged commit 3f57790 into RIOT-OS:master Feb 24, 2017
@vincent-d vincent-d deleted the pr/nucleo144-f413 branch March 2, 2017 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabadie aabadie aabadie approved these changes

Assignees

@aabadie aabadie

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: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vincent-d @aabadie