Add struct for coordinates in disp_dev API#14051
Add struct for coordinates in disp_dev API#14051Citrullin wants to merge 3 commits intoRIOT-OS:masterfrom
Conversation
Citrullin
requested review from
MichelRottleuthner,
aabadie,
fjmolinas,
leandrolanzieri and
smlng
as code owners
|
I don't think that changing the ili9341 driver is also required (even if convenient). And you forgot to update the call to disp_dev in the lvgl package. |
True, not necessary. Maybe, we even want to keep the old API for backwards compatibility. Not sure, if there anyone is using it. |
|
There is ongoing discussion on how to represent coördinates and colors in RIOT in #13787. I think we should first come to a conclusion there before further modifying the API here. |
Ohh, haven't seen this issue. Yes, I had the same issue, so I added it to the API. Consider it as proposal then. Will create a PR with reference to it. |
miri64
added
Area: drivers
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
|
"State: don't stale" Still an open topic |
stale
bot
removed
the
State: stale
aabadie
left a comment
aabadie
left a comment
There was a problem hiding this comment.
The changes in the ili9341 driver implementation are not needed. Only the adaption layer to disp_dev needs to map the x1,y1,x2,y2 coordinates to the coordinate struct of disp_dev.
Also, please rebase!
| /** | ||
| * @brief Type to store the update coordinates in | ||
| */ | ||
| typedef struct { | ||
| uint16_t x1; /**< Left coordinate */ | ||
| uint16_t x2; /**< Right coordinate */ | ||
| uint16_t y1; /**< Top coordinate */ | ||
| uint16_t y2; /**< Bottom coordinate */ | ||
| } disp_dev_coordinates_t; | ||
|
|
There was a problem hiding this comment.
There's no need to change the driver API. Just the disp_dev API needs the be changed and the ili9341 adapter functions can handle the conversion.
This will keep this PR minimal and avoid an API change (for something that is not marked as experimental, the ili9341 driver).
There was a problem hiding this comment.
Okay, fair enough. But why shouldn't I touch this as well? So I can reduce the amount of arguments there as well? To get to the ideal of 3/4 arguments for a function.
There was a problem hiding this comment.
why shouldn't I touch this as well? So I can reduce the amount of arguments there as well?
The PR title says that it touches the disp_dev API, the underlying display driver doesn't have to be changed. This keeps the PR focused on the disp_dev changes and this eases and speeds up the reviews.
There was a problem hiding this comment.
Okay, fair enough. But why shouldn't I touch this as well? So I can reduce the amount of arguments there as well? To get to the ideal of 3/4 arguments for a function.
@Citrullin could you split the ili9341 changes into another PR? It can be based on this one, that way everyone is happy (I think :) )
| uint16_t x1, uint16_t x2, uint16_t y1, uint16_t y2, | ||
| const uint16_t *color) | ||
| void disp_dev_map(disp_dev_t *dev, disp_dev_coordinates_t *coordinates, | ||
| const uint16_t *color) |
There was a problem hiding this comment.
| const uint16_t *color) | |
| const uint16_t *color) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
6 participants
Contribution description
In order to simplify the API and reduce the amount of parameters, I changed the
disp_devAPI to use a struct to store the coordinates. I also changed theili9341driver to make us of it.Testing procedure
Haven't tried the tests yet. Just checked for compile errors and warnings.
Changed tests are available in
tests/disp_devanddriver_ili9341