-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add struct for coordinates in disp_dev API #14051
Conversation
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. |
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. |
"State: don't stale" Still an open topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Contribution description
In order to simplify the API and reduce the amount of parameters, I changed the
disp_dev
API to use a struct to store the coordinates. I also changed theili9341
driver 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_dev
anddriver_ili9341