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/nucleo: share board.h and board.c #8065

Merged
merged 6 commits into from
Jan 24, 2018
Merged

boards/nucleo: share board.h and board.c #8065

merged 6 commits into from
Jan 24, 2018

Conversation

haukepetersen
Copy link
Contributor

@haukepetersen haukepetersen commented Nov 16, 2017 •
edited
Loading

Based on #8058 and cuts out part from #8044

As #8044 is getting out of hand, I think it makes sense to split its aspects into separate PRs. This PR does two things:

  • make a distinction between common/nucleo and common/nucleo64
  • make all nucleo boards use shared board.h (by type, one for 32, 64, and 144) and a single shared board.c file

Especially the diff of the last commit (175 additions and 2,293 deletions) seems to be a good start :-)

@haukepetersen haukepetersen added Area: boards Area: Board ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 16, 2017
This was referenced Nov 17, 2017
Closed
Closed
@haukepetersen haukepetersen removed the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 30, 2017
@haukepetersen
Copy link
Contributor Author

rebased, as #8058 was merged.

@vincent-d
Copy link
Member

Looks good. I have no time to test it today.

Let's give Murdock a try.

@vincent-d vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2017
@smlng
Copy link
Member

smlng commented Dec 4, 2017

please also consider #8189 to keep the doc consistent

@haukepetersen
Copy link
Contributor Author

yes, will adapt the doc

@smlng
Copy link
Member

smlng commented Dec 4, 2017

yes, will adapt the doc

great! Merging #8189 (before this one), would be nice, too - but it needs approval, first 😉

@haukepetersen
Copy link
Contributor Author

will take a look later today

@aabadie
Copy link
Contributor

aabadie commented Dec 18, 2017

#8189 is merged, can you rebase @haukepetersen ?

@haukepetersen
Copy link
Contributor Author

sorry for the delay: adapted doxygen groups to the changes in #8189 and rebased.

@aabadie
Copy link
Contributor

aabadie commented Jan 5, 2018

Looks good for me. I'll test on a subset of boards next week.

aabadie
aabadie requested changes Jan 15, 2018
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.

Tested examples/default on a set of nucleo boards: works for nucleo32-l031, nucleo-l152 (when disabling LSE), nucleo-l476, nucleo32-l432, nucleo-f072, nucleo144-f207, nucleo32-f303, nucleo-f446 and nucleo144-f746. I don't have a nucleo-f103 (I lent it) but except this one I tested all nucleo cpu families.

Otherwise, I think I found an issue with doxygen grouping and there's the problem with the AUTO_INIT_LED0 that doesn't apply to nucleo144 boards.

boards/common/nucleo/board.c Outdated
*/

/**
* @ingroup boards_nucleo
Copy link
Contributor

Choose a reason for hiding this comment

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

If I get it right, it should be boards_common_nucleo

boards/common/nucleo/board.c

/* initialization of on-board LEDs
* NOTE: LED0 must be explicitly enabled as it is also used for SPI_DEV(0) */
#ifdef AUTO_INIT_LED0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only the case for nucleo32 and nucleo64 boards. nucleo144 boards are using different pins for SPI and on-board leds.

@aabadie
Copy link
Contributor

aabadie commented Jan 17, 2018

Please rebase @haukepetersen

@haukepetersen
Copy link
Contributor Author

rebased and added a fix to always initialize LED0 for Nucleo144 boards.

kaspar030
kaspar030 requested changes Jan 22, 2018
View reviewed changes
boards/common/nucleo/Makefile.include

# select OpenOCD configuration depending on CPU type
ifeq (,$(OPENOCD_CONFIG))
ifeq ($(CPU),stm32f0)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about
export OPENOCD_CONFIG := $(RIOTBOARD)/common/nucleo/dist/$(patsubst stm32%,openocd-%.cfg,$(CPU)).

And please add an error case, e.g., err out on if $(,$(filter stm32%,$(CPU))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. Ok if I do it in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done #8391: 9bbd78d

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@haukepetersen haukepetersen mentioned this pull request Jan 22, 2018
Closed
@haukepetersen
Copy link
Contributor Author

grr, have to rebase again -> because of #8405 this time. Can we please get this merged soon, so this will not happen every time? Also things like #8405 are so much easier to do with this PR merged...

@haukepetersen
Copy link
Contributor Author

adapted to #8405

@haukepetersen
Copy link
Contributor Author

@kaspar030 @aabadie: you think we could this in soonish? I get tired of maintaining this PR and all of your previous comments should be addressed :-)

aabadie
aabadie approved these changes Jan 24, 2018
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-retested ACK. The change of auto init led0 should be ok with nucleo-144.

Thanks for rebasing again btw

@kaspar030
Copy link
Contributor

Tested on nucleo-f401. ACK.

@kaspar030 kaspar030 merged commit 2b73449 into RIOT-OS:master Jan 24, 2018
@haukepetersen
Copy link
Contributor Author

finally, thanks a lot for reviewing and testing!

@haukepetersen haukepetersen deleted the opt_boards_nucleo-board.hc branch January 24, 2018 14:58
This was referenced Jan 25, 2018
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@aabadie aabadie aabadie approved these changes

Assignees

@aabadie aabadie

Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@haukepetersen @vincent-d @smlng @aabadie @kaspar030