Support 16bit color per channel

evq commented 5 years ago

Some LED strips (like APA102 with global brightness bits) can achieve greater than 8-bit per color channel resolution under certain conditions. Similarly LEDs driven using 12-bit or 16-bit pwm (either via peripherals or an external PWM chip) would also be nice to support.

For context I'm working on a crate that implements the interpolation, gamma correction and temporal dithering techniques from fadecandy. It supports writing into the internal state using the SmartLedsWrite trait and itself writes to the underlying LEDs via the same. I have a few custom LED spotlights that use 16-bit pwm and would like to re-use the interpolation and gamma correction to take advantage of the 16-bit pwm during fades while still only having to provide 8-bit per channel input.

It seems like either a new 16-bit specific trait could be defined or now that this is using the rgb crate a generic trait over RGB could be defined. e.g.

pub use rgb::RGB;

pub type Color = rgb::RGB8;
pub type Color16 = rgb::RGB16;

pub trait SmartLedsWrite<RGB> {
    type Error;
    fn write<T>(&mut self, iterator: T) -> Result<(), Self::Error>
    where
        T: Iterator<Item = RGB>;
}

sajattack commented 5 years ago

It looks like our current brightness and gamma correction functions in smart-leds assume u8, but maybe we should change that. What do you think @david-sawatzke?

david-sawatzke commented 5 years ago

@sajattack I think having a dedicated function at least for gamma is important, as we're pretty constrained timing wise & thus need tables.

So we currently have at least three things that aren't nicely captured by the existing traits:

RGBA (doesn't match 100%, but the brightness regulation leds like the apa102) While there are sometimes valid reasons not to use it (like the slow pwm frequency on apa102) in general it might be quite useful.
RGBW Dedicated white leds aren't that rare and we have to do our own type (or figure out how to do it based on RGBA without them being interchangable)
16/12/whatever bit leds

I think implementing a generic trait makes sense, we'd have to break our api, but as there don't seem to be that many implementations apart from this org & @evq, that's probably alright. I'd also like to remove our own Color type then and just reexport RGB8 & stuff from the rgb crate, as there's no reason to have two different names for the same thing.

I don't think smart-leds should necessarily provide implementations for anything other than RGB8, as that seems pretty led specific for a lot of things (especially for the RGBA & RGBW types) & also not that common.

The one remaining question for me is if we just want the type or anything that can be converted into it, e.g.:

pub trait SmartLedsWrite<RGB> {
    type Error;
    fn write<T, I>(&mut self, iterator: T) -> Result<(), Self::Error>
    where
        T: Iterator<Item = I>,
        I: Into<RGB>;
}

It would make it a bit easier to mix & match, but it's also potentially a bit confusing ("16 bit leds kind of jump with gamma correction").

evq commented 5 years ago

It may be ideal to avoid using RGBA for describing the APA102 style global brightness since RGBA is commonly used to refer to Red, Green, Blue and Amber LEDs.

I've prototyped changes to the SmartLedsWrite trait and additional types and traits that work for my use case.

Basically the crate I described above is intended create a "virtual LED strip" that performs interpolation, gamma correction and dithering (when the output type is RGB<u8> or RGBW<u8>) in between frames of data that are sent at a much lower rate. This crate needs to be generic over the output type of the underlying "physical LED strip", both with respect to pwm resolution (8 or 16 bit) and color channels (RGB, RGBW, RGBA, etc) - but it should only accept 8 bit per color pixels in where the number of color channels should match the output.

Trait changes: https://github.com/smart-leds-rs/smart-leds-trait/compare/master...evq:wip P9813 impl w/ trait changes: https://github.com/evq/p9813-spi-rs/blob/wip/src/lib.rs "virtual LED strip" crate: https://github.com/evq/ditherable-leds/blob/master/ditherable-leds/src/lib.rs

Not super happy with the trait bounds in the ditherable-leds crate, open to any suggestions. Also, happy to move the GenericPixel struct and Pixel trait into my crate (they do add a dependency on generic-array) - but maybe they would be useful for others?

david-sawatzke commented 5 years ago

I'd move GenericPixel to smart-leds, it does seem useful. I'm not sure how useful Pixel for most applications.

Didn't even think of amber leds, but if we can avoid confusion over the RGBA name, it's for the better. Directly repurposing it for RGBW sounds dangerous, maybe we could do something like this:

struct White<C>(C);
type RGBW<ComponentType, WhiteComponentType = ComponentType> =
    RGBA<ComponentType, White<WhiteComponentType>>;

That would retain most of the benefits of using an already existing type, while not being able to interchange them (and same thing for brightness, since it's not exactly an alpha channel). For easier interchangeable use we could add From & Into traits to White.

Is there a reason why Pixel in SmartLedsWrite is an associative type? I can imagine times when having the option to implement different ones is useful, e.g. to increase the pwm frequency when using pwm with u8 instead of u16.

smart-leds-rs / smart-leds-trait

Support 16bit color per channel #3