Skip to content

boards/nucleo144-f413 add inital support#6565

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
OTAkeys:pr/nucleo144-f413
Feb 24, 2017
Merged

boards/nucleo144-f413 add inital support#6565
aabadie merged 2 commits intoRIOT-OS:masterfrom
OTAkeys:pr/nucleo144-f413

Conversation

@vincent-d
Copy link
Copy Markdown
Member

@vincent-d vincent-d commented Feb 7, 2017

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
Copy Markdown
Member Author

vincent-d commented Feb 7, 2017

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

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

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
Copy Markdown
Contributor

@aabadie aabadie Feb 8, 2017

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
Copy Markdown
Member Author

@vincent-d vincent-d Feb 8, 2017

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
Copy Markdown
Contributor

@aabadie aabadie Feb 8, 2017

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
Copy Markdown
Member Author

@vincent-d vincent-d Feb 8, 2017

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
Copy Markdown
Contributor

@aabadie aabadie Feb 8, 2017

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
Copy Markdown
Member Author

@vincent-d vincent-d Feb 8, 2017

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
Copy Markdown
Contributor

@aabadie aabadie Feb 8, 2017

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
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
Copy Markdown
Contributor

@aabadie aabadie Feb 8, 2017

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
Copy Markdown
Member Author

@vincent-d vincent-d Feb 8, 2017

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
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.

Jenkins also reported trailing whitespaces in the cmsis file.

cpu/stm32f4/vectors.c
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
Copy Markdown
Contributor

@aabadie aabadie Feb 13, 2017

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
Copy Markdown
Member Author

vincent-d commented Feb 20, 2017

Cleaned up vendor header (remove trailing spaces)

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Feb 20, 2017

Looks good now. Can you squash the commits ?

@vincent-d
Copy link
Copy Markdown
Member Author

vincent-d commented Feb 21, 2017

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
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.

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
Copy Markdown
Member Author

vincent-d commented Feb 23, 2017

rebased

@aabadie
Copy link
Copy Markdown
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
Copy Markdown
Member Author

vincent-d commented Feb 24, 2017

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

@aabadie
Copy link
Copy Markdown
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
Copy Markdown
Member Author

vincent-d commented Feb 24, 2017

wiki looks OK

@aabadie
Copy link
Copy Markdown
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