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.

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

boards: efm32: adapt pr feedback for other boards #8794

Merged
merged 5 commits into from
Mar 17, 2018

Conversation

basilfx
Copy link
Member

@basilfx basilfx commented Mar 16, 2018

Contribution description

During the review of #8630 some comments were raised that have slipped through. This PR adapts these comments for the:

  • ikea-tradfri
  • sltb001a
  • stk3600
  • stk3700

Issues/PRs references

#7929
#8047
#8585
#8520

@basilfx basilfx added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Mar 16, 2018
@basilfx basilfx requested review from bergzand and kYc0o March 16, 2018 19:08
@basilfx basilfx force-pushed the feature/efm32_fix_style branch 2 times, most recently from 9d8b826 to 2a0ba35 Compare March 16, 2018 19:13
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.

Thanks for fixing this. I went through your changes and they look good.

Untested-ACK

@basilfx
Copy link
Member Author

basilfx commented Mar 16, 2018

For what its worth, this is the proof that it still runs after 4ff507a 🤓

SLTB001a:
schermafbeelding 2018-03-16 om 20 20 20

SLSTK3401a:
schermafbeelding 2018-03-16 om 20 23 44

@aabadie
Copy link
Contributor

aabadie commented Mar 16, 2018

Minor: there's a typo in 4555570 commit message (tradfir instead of tradfri).

@basilfx basilfx force-pushed the feature/efm32_fix_style branch from 2a0ba35 to f75d426 Compare March 16, 2018 19:39
@basilfx
Copy link
Member Author

basilfx commented Mar 16, 2018

Fixed that one.

#include "periph/gpio.h"
#include "pic.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this include could be protected with a #ifdef MODULE_SILABS_PIC ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add that, but it compiles fine without (if I remove silabs_pic from the USEMODULES).

Let me know what you prefer.

Copy link
Member Author

@basilfx basilfx Mar 16, 2018

Choose a reason for hiding this comment

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

On second thought: I'll add it. That's more similar to how we also include optional peripherals.

Will do the same for AEM.

@basilfx
Copy link
Member Author

basilfx commented Mar 16, 2018

Added the conditional includes, and tested that it still works.

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.

ACK again, and go

@aabadie aabadie merged commit 2a594b6 into RIOT-OS:master Mar 17, 2018
@basilfx basilfx deleted the feature/efm32_fix_style branch January 14, 2020 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants