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/pinetime: add defines for controlling the backlight pin #13657

Merged
merged 3 commits into from
Mar 24, 2020

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Mar 19, 2020

Contribution description

This PR adds some defines (BACKLIGHT_ON, BACKLIGHT_OFF) for controlling the Pinetime backlight pin.
These defines are meant to be generic, similar to on-board LED macros, and should be defined in boards that need it (the adafruit-clue needs it for example).

Testing procedure

  • Check the tests/driver_ili9341, tests/disp_dev and tests/pkg_lvgl are displaying something on the pinetime
  • Check that with any other applications (examples/hello-world) the screen remains off.

Issues/PRs references

Will help with #13276

@aabadie aabadie 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 19, 2020
@aabadie aabadie requested review from kaspar030 and bergzand March 19, 2020 09:31
@kaspar030
Copy link
Contributor

Why defines? (Why are defines creeping into the codebase lately?)
What's wrong with a proper function?

@aabadie
Copy link
Contributor Author

aabadie commented Mar 19, 2020

What's wrong with a proper function?

I'm adding one in disp_dev. But we need a common define at board level.

@bergzand
Copy link
Member

Why defines? (Why are defines creeping into the codebase lately?)

We need a way to 1. define a common pin and 2. define whether it is active low or active high. The current macros allow for that, but a set of common defines to select a gpio_t and indicate active high or low would also work. A new param struct for this seems a bit overkill I think.

@aabadie aabadie force-pushed the pr/boards/backlight_common_defines branch 2 times, most recently from f09d4ec to 2df63d2 Compare March 19, 2020 10:14
@bergzand
Copy link
Member

@kaspar030 Are you okay with the current changes here or do you have a different proposal?

@bergzand bergzand self-assigned this Mar 24, 2020
@bergzand
Copy link
Member

@kaspar030 Are you okay with the current changes here or do you have a different proposal?

@kaspar030 mentioned offline that he is fine with the current changes as it is an iprovement of the current state, but that we're going to run into issues with multiple displays and when hooking up a display to an existing board.

@bergzand
Copy link
Member

@aabadie Please squash!

Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Ack!

@aabadie aabadie force-pushed the pr/boards/backlight_common_defines branch from e74bfc0 to 6202e2d Compare March 24, 2020 11:03
@aabadie
Copy link
Contributor Author

aabadie commented Mar 24, 2020

Squashed!

@aabadie aabadie merged commit 6faae9a into RIOT-OS:master Mar 24, 2020
@aabadie aabadie deleted the pr/boards/backlight_common_defines branch March 24, 2020 12:53
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 26, 2020
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.

None yet

4 participants