drivers/lcd: slightly rework params to expose offset values to ili9341 driver #17925
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aabadie
requested review from
smlng,
leandrolanzieri,
MichelRottleuthner and
fjmolinas
as code owners
github-actions
bot
added
Area: boards
aabadie
force-pushed
the
pr/drivers/lcd_params_rework
branch
2 times, most recently
from
d8f14f3 to
25aa562
Compare
aabadie
added
the
CI: ready for build
aabadie
force-pushed
the
pr/drivers/lcd_params_rework
branch
2 times, most recently
from
c0d532d to
cef72ba
Compare
aabadie
force-pushed
the
pr/drivers/lcd_params_rework
branch
from
cef72ba to
d060f47
Compare
aabadie
added
CI: ready for build
benpicco
added
CI: ready for build
aabadie
added
CI: ready for build
fjmolinas
reviewed
Apr 19, 2022
| @@ -107,7 +107,8 @@ extern "C" { | |||
| #define ILI9341_PARAM_NUM_LINES (240U) /**< Number of screen lines */ | |||
| #define ILI9341_PARAM_RGB (1) /**< RGB configuration */ | |||
| #define ILI9341_PARAM_INVERTED (1) /**< Inversion configuration */ | |||
| #define ILI9341_PARAM_ROTATION (LCD_ROTATION_HORZ_FLIP) /**< Rotation mode */ | |||
| #define ILI9341_PARAM_ROTATION (LCD_ROTATION_VERT) /**< Rotation mode */ | |||
| #define ILI9341_PARAM_OFFSET_X (80) /**< Vertical rotation requires a 80 pixel offset */ | |||
There was a problem hiding this comment.
This seems a bit weird, are you able to correctly draw in the whole screen? the 80 offset makes me think it has something to do with the display being 240X320 and with the flip you want to treat it as 320x240, but its strange that this is not handled by the driver.
If you are able to correctly draw to the whole display I'm ok as it is, although it still seems a bit weird to me.
There was a problem hiding this comment.
On the adafruit clue, the display is 240x240 and yes it draws correctly with the 80 pixels offset. I also tested this on the pinetime (also 240x240) and to rotate by 180°, I also had to add an offset (on Y this time).
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This is a follow-up of #16176 that reworks the ili9341 parameters to also expose X/Y offset values. The idea behind this PR is about the adafruit-clue display that was not oriented correctly.
Setting the orientation parameter to the right value (
LCD_ROTATION_VERT) raised an offset issue with the ili9341.It seems that the offset values can be handled in a similar way as for the st7735 driver. So this PR unifies all this.
What is not done by this PR is to add the possibility to play with the ST7735 orientation parameters (they are actually still hard coded) but I couldn't find a combination that works for all possible values.
Testing procedure
tests/driver_ili9341display is correctly oriented on adafruit-clue (tested and it works)Issues/PRs references
Follow-up of #16176