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

disp_dev: color depth and coordinate system #13787

Open
silkeh opened this issue Apr 1, 2020 · 8 comments
Open

disp_dev: color depth and coordinate system #13787

silkeh opened this issue Apr 1, 2020 · 8 comments
Assignees
Labels
Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: new feature The issue requests / The PR implemements a new feature for RIOT Type: question The issue poses a question regarding usage of RIOT

Comments

@silkeh
Copy link
Contributor

silkeh commented Apr 1, 2020

I just added an implementation of disp_dev (see #13262) to #12509. I have some (admittedly minor) feedback about the current API:

  • The color parameter for disp_dev_map is an uint16_t pointer. This implies that pixels should at least be 16 bits. The e-Paper display in black/white e-Paper/e-Ink display driver #12509 uses a single byte per 8 pixels, so it would be more natural to use uint8_t there. Especially when dealing with something like a 8x3 pixel image.

  • In the implementation for the ili9341 x2 and y2 describe the coordinate of the inner pixel of the corner, not the outside corner. This is also noted in the documentation of that driver. It is not, however, noted in disp_dev_map. I also think that (even when explicitly noted) this is not an intuitive use of coordinates: a rectangle with two opposing corners having the coordinates (5,5) and (5,5) has an area of 0, not 1.

  • disp_dev_set_invert will probably not apply to all displays. It is currently not possible to communicate this to the user.

Proposals:

  • Make color an uint8_t.

  • Use width (w) and height (h) instead of x2 and y2 to avoid any confusion. This is also what SDL, Gegl and Cairo do.

  • Add a return type to disp_dev_set_invert indicating success/failure. This should probably also be the case for disp_dev_map.

@bergzand bergzand added Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: question The issue poses a question regarding usage of RIOT labels Apr 1, 2020
@bergzand
Copy link
Member

bergzand commented Apr 1, 2020

Add a return type to disp_dev_set_invert indicating success/failure. This should probably also be the case for disp_dev_map.

A different solution for this could be to add a number of capability flags to a driver to indicate different driver capabilities.

@Citrullin
Copy link
Contributor

Citrullin commented May 11, 2020

I have proposal for the color depth issue in the PR #14054 .
While I did some simplifications of the API for coordinates in #14051 .

I also think that (even when explicitly noted) this is not an intuitive use of coordinates: a rectangle with two opposing corners having the coordinates (5,5) and (5,5) has an area of 0, not 1.

But the Coordinator simplification doesn't address this issue. Just a simplification of the API itself.

Use width (w) and height (h) instead of x2 and y2 to avoid any confusion. This is also what SDL, Gegl and Cairo do.

I agree with that one. Should address the issue.

@Citrullin
Copy link
Contributor

Is it somehow possible to push this topic, so it is not going to die? :)

@miri64
Copy link
Member

miri64 commented Jul 6, 2020

Ping @aabadie @bergzand?

@miri64 miri64 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Jul 6, 2020
@silkeh
Copy link
Contributor Author

silkeh commented Oct 22, 2020

Ping @aabadie @bergzand?

@stale
Copy link

stale bot commented Jun 3, 2021

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 stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021
@aabadie aabadie removed the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

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 stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@Citrullin
Copy link
Contributor

Still an issue.

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: new feature The issue requests / The PR implemements a new feature for RIOT Type: question The issue poses a question regarding usage of RIOT
Projects
None yet
Development

No branches or pull requests

6 participants