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

Define common display interface #31

Merged
merged 3 commits into from Apr 24, 2019

Conversation

Projects
None yet
3 participants
@conejoninja
Copy link
Contributor

commented Mar 12, 2019

definition of Display interface

issue #27
definition of Display interface
@aykevl

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

To me this looks good, but perhaps it's better to wait with merging this until an actual implementation has been written.

@conejoninja

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

I was waiting for this to make the implementation 😅 I'll send the driver first.

In the current implementation I'm working (hub75) it's not possible to implement DisplayRect() (or I'm not sure how to, need to look in more detail). But I consider it useful as some other devices may support it and left it in the interface.

@aykevl

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Yeah maybe DisplayRect should be left out of the base interface. My idea was that it updates only the relevant part of the screen if possible, but a driver implementation can fall back to doing a full Display call if it does not support partial updates. For now, perhaps it's easier/simpler to leave it out. It can always be added in the future if needed. A driver user could also do a typecheck to see whether the driver implements this function.
(You may notice I have a strong tendency towards interfaces that are as simple as they could possibly be 🙂)

@conejoninja

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Affected by tinygo-org/tinygo#270 , but let's keep the discussion in #27

display.go Outdated

import "image/color"

type Display interface {

This comment has been minimized.

Copy link
@deadprogram

deadprogram Apr 23, 2019

Member

I suggest this be named Displayer

@deadprogram deadprogram changed the title issue #27 Define common display interface Apr 23, 2019

display.go Outdated

// DisplayRect sends this part of the buffer to the screen for incremental updates
// (e.g. button press animations, blinking cursor, etc.).
DisplayRect(x1, y1, x2, y2 int16)

This comment has been minimized.

Copy link
@deadprogram

deadprogram Apr 23, 2019

Member

Was @aykevl suggesting to remove this?

This comment has been minimized.

Copy link
@conejoninja

conejoninja Apr 23, 2019

Author Contributor

Maybe I misunderstood his comment #31 (comment)

In some devices it might not be possible to update just a part of the display, I'm thinking about the SSD1306 or the PCD8544, both use a byte to represent a 8 pixel column and it's the minimum you can send to the display, you will end up updating a bigger area (you need to calculate that new area). In other devices, such a function might not exists. I think it might be useful, it's easy to fallback and update the whole screen (or a bigger area) but the results might not be what you expected.

I left it there for the moment (this is not yet in hub75)

@conejoninja conejoninja force-pushed the conejoninja:display_interface branch from bf92a32 to 1dbb3f6 Apr 23, 2019

Display() error

// DisplayPixel sends a single pixel to the screen.
DisplayPixel(x, y int16, c color.RGBA)

This comment has been minimized.

Copy link
@deadprogram

deadprogram Apr 24, 2019

Member

This should also return error.


// DisplayRect sends this part of the buffer to the screen for incremental updates
// (e.g. button press animations, blinking cursor, etc.).
DisplayRect(x1, y1, x2, y2 int16)

This comment has been minimized.

Copy link
@deadprogram

deadprogram Apr 24, 2019

Member

This should also return error.

Show resolved Hide resolved displayer.go
@deadprogram

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

OK made another comment, and I think that is my last one, about methods returning error. Seems like a more idiomatic and future-proof signature.

@aykevl
Copy link
Member

left a comment

I'm not so sure about DisplayPixel and I wasn't already so sure about DisplayRect. Perhaps we can just drop the two? The only 3 functions that are absolutely necessary are Size, SetPixel and Display. They implement a bare-bones buffered display. You can use type asserts to check for more methods.

Show resolved Hide resolved displayer.go
@deadprogram

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Hard to argue against the benefits of less. Let's do that.

@aykevl

aykevl approved these changes Apr 24, 2019

Copy link
Member

left a comment

The interface looks good from my perspective.

@deadprogram

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

OK then. Time to merge.

@deadprogram deadprogram merged commit 772a104 into tinygo-org:master Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.