Skip to content

boards/nucleo144-f722: add initial support#6991

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:nucleo144_f722
Aug 18, 2017
Merged

boards/nucleo144-f722: add initial support#6991
aabadie merged 2 commits intoRIOT-OS:masterfrom
aabadie:nucleo144_f722

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented May 2, 2017

Similar to #6987 this PR is based on #6907 and has the same issues.

@aabadie aabadie 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 May 2, 2017
@aabadie aabadie added this to the Release 2017.07 milestone May 2, 2017
@aabadie aabadie requested a review from haukepetersen May 2, 2017 08:19
@aabadie aabadie mentioned this pull request May 2, 2017
Open
@aabadie
Copy link
Contributor Author

aabadie commented May 19, 2017

Had problem flashing the board because the MCU is not supported by openocd. I was about to submit a patch when I discovered someone proposed one yesterday.

To conclude: if one wants to flash this board, he will need this openocd patch

haukepetersen
haukepetersen requested changes Aug 15, 2017
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Sorry for the late review - just one minor thing and one open question...

boards/nucleo144-f722/include/periph_conf.h Outdated
* @name DAC configuration
* @{
*/
#define DAC_NUMOF (0)
Copy link
Contributor

@haukepetersen haukepetersen Aug 15, 2017

Choose a reason for hiding this comment

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

should be removed, as the remodeled DAC driver does not like this :-)

cpu/stm32f7/include/cpu_conf.h
#elif defined(CPU_MODEL_STM32F769NI) || defined(CPU_MODEL_STM32F767ZI)
#define CPU_IRQ_NUMOF (110U)
#elif defined(CPU_MODEL_STM32F722ZE)
#define CPU_IRQ_NUMOF (104U)
Copy link
Contributor

@haukepetersen haukepetersen Aug 15, 2017

Choose a reason for hiding this comment

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

just to make sure: does the stm32f722 need any adaption to the isr vector definitions, also?

Copy link
Contributor Author

@aabadie aabadie Aug 16, 2017

Choose a reason for hiding this comment

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

I have to check but yes probably...

Copy link
Contributor Author

@aabadie aabadie Aug 16, 2017

Choose a reason for hiding this comment

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

I adapted the vector definitions but now it's a bit a mess of defines...

@aabadie
Copy link
Contributor Author

aabadie commented Aug 16, 2017

OpenOCD now supports stm32f722 (see here ) so no need for the patch mentioned above anymore.

@aabadie aabadie 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 Aug 16, 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 Aug 17, 2017
@haukepetersen
Copy link
Contributor

haukepetersen commented Aug 17, 2017

True, those vector defines are not very nice to look at and I can't tell if it is correct or not - very hard to maintain... Even though it means a some code duplication: how about we define the complete set of vectors for each CPU model?!

So I think of something like:

// all the vectors that are the same for all models
    ...
    (void*) isr_otg_hs,
#ifdef CPU_MODEL_STM32F722ZE
    // the rest of the 722ZE specific vectors
    ...
#elif defined(CPU_MODEL_STM32F7xx
    // the rest of xx specific vectors
    ....
#endif
};

I think this could be way easier to read an verify?!

@aabadie
Copy link
Contributor Author

aabadie commented Aug 17, 2017

There was a PR opened regarding this problem: #6617

@haukepetersen
Copy link
Contributor

haukepetersen commented Aug 17, 2017

I see, havn't seen that. But either way, I would suggest simplifying the definition for the F7 in a simple way for now, and then we can figure out how we could proceed for all the STM CPUs and adapt the F7 later.

@aabadie aabadie force-pushed the nucleo144_f722 branch 2 times, most recently from ce87803 to ceba00d Compare August 18, 2017 07:05
@aabadie
Copy link
Contributor Author

aabadie commented Aug 18, 2017

@haukepetersen I updated the interrupt vectors file as you suggested and polished it a bit. I hope it's fine now.

haukepetersen
haukepetersen approved these changes Aug 18, 2017
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Yes, much nicer to read now -> ACK

@haukepetersen
Copy link
Contributor

haukepetersen commented Aug 18, 2017

oh, and please squash...

@haukepetersen haukepetersen added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 18, 2017
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 18, 2017
@aabadie
Copy link
Contributor Author

aabadie commented Aug 18, 2017

squashed

@aabadie
Copy link
Contributor Author

aabadie commented Aug 18, 2017

Since I got an ACK and Murdock is green, let's go

@aabadie aabadie merged commit 7b79f4d into RIOT-OS:master Aug 18, 2017
This was referenced Aug 21, 2017
Merged
Merged
@aabadie aabadie deleted the nucleo144_f722 branch February 26, 2018 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees

@haukepetersen haukepetersen

@astralien3000 astralien3000

@kYc0o kYc0o

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

Release 2017.10

Development

Successfully merging this pull request may close these issues.

4 participants

@aabadie @haukepetersen @astralien3000 @kYc0o