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

Initial tests around rgbw #57

Closed
wants to merge 1 commit into from

Conversation

directionless
Copy link

Some initial testing. This code doesn’t seem to work on my Arduino board. Still debugging.

Due to tinygo-org/tinygo#349 the interface feels cumbersome.

Until we understand how to work with white LEDs, there are a couple of interfaces. It’s all an experiment around #51

Some initial testing. This code doesn’t seem to work on my Arduino board. Still debugging.

Due to tinygo-org/tinygo#349 the interface feels cumbersome.

Until we understand how to work with white LEDs, there are a couple of interfaces. It’s all an experiment
@aykevl
Copy link
Member

aykevl commented May 14, 2019

I see that this PR heavily relies on function pointers. While that results in some nice high-level code, it is also rather bad for optimizations. In general, compilers cannot reason about them and usually can't optimize them. Therefore, I have avoided them everywhere so far.
(Also, I am not entirely convinced that functional options really improve APIs. To me they look like a stop-gap for the lack of optional parameters. But that's my opinion).

Instead, when I wanted to configure something, I have passed a Config struct with sane zero values. For example:

type Config struct {
    Order ColorOrdering
}

You can see the pattern in all the peripherals (I2C, SPI, etc.) in the machine package.
Also, I see you have made color ordering be a string. It is much more efficient to make it an integer. And with that, I would put GRB first because that's what you want in the vast majority of cases, making it possible to just use the zero value of Config: ws2812.New(machine.NEOPIXEL, ws2812.Config{}).

type ColorOrdering uint8

const (
    GRB ColorOrdering = iota
    RGB
    RGBW
)

Also, wouldn't it be much simpler to just say "RGBW reinterprets the A channel as W"? That's a bit dirty, but keeps almost the same API:

func (d Device) WriteColors(buf []color.RGBA) (n int, err error) {
	for _, c := range buf {
		switch d.order {
		case GRB:
			d.WriteByte(c.G)
			d.WriteByte(c.R)
			d.WriteByte(c.B)
		case GRBW:
			// reinterpret the alpha channel as the white channel
			d.WriteByte(c.G)
			d.WriteByte(c.R)
			d.WriteByte(c.B)
			d.WriteByte(c.A)
		}
	}
}

If that is too dirty, I would propose the addition of a new color slice type RGBW instead of reusing RGBA.

My goal here is 1) to make the API less complex, and 2) to make the API less heavy for embedded systems.

@directionless
Copy link
Author

I personally prefer functional arguments to config structs, but I think consistency within the project is important. I will convert to that.

I forget why I used string and not iota there. It's mostly a spike around what interfaces might look like, no big deal to change it.

RGBW reinterprets the A channel as W

This feels wrong to me. It precludes using the alpha channel as intended. One thing I'd like to support, is being able to send RGBA values to a RGBW led, and to send RGBW values to an RGB one. Though that might be too ambitious in practice.

This PR shows two potential interfaces. One defines a new type, ColorsRGBW and creates an interface to consume it. The other adds an option to WriteColors to accept an array of white values.

I'll rework this with some of the above feedback, and we can see how those feel.

@aykevl
Copy link
Member

aykevl commented May 17, 2019

Are there any LEDs with both an alpha channel and a white channel?
Not saying that we shouldn't support that, but limiting to one of them keeps each RGB value to 4 bytes which may be convenient in some cases.

Also some background: when I initially wrote this, the color slice was []uint32. With some shifting and masking you can get the color out, and in fact this is a bit more efficient than something like []struct{byte, byte, byte, byte} because you only need one load instead of four. But it isn't very clean and somewhat error prone so we switched to color.RGBA (especially with the APA102 support that was added around the same time).

@deadprogram
Copy link
Member

Closing due to having changed/refined our testing strategy a bit. Thank you for working on ideas @directionless

@deadprogram deadprogram closed this Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants