HSV to RGB conversion is wrong

[jounathaen]

Hi,

I think the HSV to RGB conversion is wrong. It also panics for hues above 190.

Testcode:

fn main() {
    for i in (0..=360).step_by(10) {
        let hsv_col = smart_leds::hsv::Hsv{hue: i, sat: 255, val: 255};
        println!("Hue: {} | {:?}", hsv_col.hue, smart_leds::hsv::hsv2rgb(hsv_col));
    }
}

Output:

Hue: 0 | RGB { r: 251, g: 0, b: 0 }
Hue: 10 | RGB { r: 211, g: 39, b: 0 }
Hue: 20 | RGB { r: 171, g: 79, b: 0 }
Hue: 30 | RGB { r: 131, g: 119, b: 0 }
Hue: 40 | RGB { r: 91, g: 159, b: 0 }
Hue: 50 | RGB { r: 51, g: 199, b: 0 }
Hue: 60 | RGB { r: 11, g: 239, b: 0 }
Hue: 70 | RGB { r: 23, g: 0, b: 227 }
Hue: 80 | RGB { r: 63, g: 0, b: 187 }
Hue: 90 | RGB { r: 103, g: 0, b: 147 }
Hue: 100 | RGB { r: 143, g: 0, b: 107 }
Hue: 110 | RGB { r: 183, g: 0, b: 67 }
Hue: 120 | RGB { r: 223, g: 0, b: 27 }
Hue: 130 | RGB { r: 0, g: 243, b: 7 }
Hue: 140 | RGB { r: 0, g: 203, b: 47 }
Hue: 150 | RGB { r: 0, g: 163, b: 87 }
Hue: 160 | RGB { r: 0, g: 123, b: 127 }
Hue: 170 | RGB { r: 0, g: 83, b: 167 }
Hue: 180 | RGB { r: 0, g: 43, b: 207 }
Hue: 190 | RGB { r: 0, g: 3, b: 247 }
thread 'main' panicked at 'internal error: entered unreachable code', /home/jonathan/.cargo/registry/src/github.com-1ecc6299db9ec823/smart-leds-0.2.0/src/hsv.rs:56:14
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

Depending on how you interpret the hue type (degree as u16 or 360°/255) the values should be: Degree:

Hue: 0   | RGB { r: 255, g: 0  , b:   0}
Hue: 30  | RGB { r: 255, g: 127, b:   0}
Hue: 60  | RGB { r: 255, g: 255, b:   0}
Hue: 90  | RGB { r: 127, g: 255, b:   0}
Hue: 120 | RGB { r:   0, g: 255, b:   0}
Hue: 150 | RGB { r:   0, g: 255, b: 127}
Hue: 180 | RGB { r:   0, g: 255, b: 255}
Hue: 210 | RGB { r:   0, g: 127, b: 255}
Hue: 240 | RGB { r:   0, g: 0  , b: 255}
Hue: 270 | RGB { r: 127, g: 0  , b: 255}
Hue: 300 | RGB { r: 255, g: 0  , b: 255}
Hue: 330 | RGB { r: 255, g: 0  , b: 127}

Fraction:

Hue:  0  | RGB { r: 255, g: 0  , b:   0}
Hue:  21 | RGB { r: 255, g: 128, b:   0}
Hue:  42 | RGB { r: 255, g: 255, b:   0}
Hue:  63 | RGB { r: 128, g: 255, b:   0}
Hue:  85 | RGB { r:   0, g: 255, b:   0}
Hue: 106 | RGB { r:   0, g: 255, b: 128}
Hue: 127 | RGB { r:   0, g: 255, b: 255}
Hue: 148 | RGB { r:   0, g: 128, b: 255}
Hue: 170 | RGB { r:   0, g: 0  , b: 255}
Hue: 191 | RGB { r: 128, g: 0  , b: 255}
Hue: 212 | RGB { r: 256, g: 0  , b: 255}
Hue: 233 | RGB { r: 256, g: 0  , b: 128}

[sajattack]

cc @thezoq2

[TheZoq2]

This was discussed briefly in #6. The reason its wrong is that the current code emulates what FastLED does, which wraps around for values over 192.

I believe this is because the hue value has to be separated into 3 different branches consisting of 120 degrees each. However, division by 3 is slow, so they have opted to use 0..192 because they can then use division by 4.

Keep in mind, this is just speculation as their docs seem to suggest that the hue values is between 0 and 255, but running the code suggests otherwise.

Perhaps the performance penalty from division isn't that bad and we can use that instead?

[jounathaen]

I'd personally rather have a correct and simple to use general purpose library (and that's what smart-leds seems to be) and optimize for speed only if necessary 😛. ("Premature Optimization is the root of all evil" - D. Knuth).

I mean, If for some project or hardware the algorithm is indeed not fast enough, it can easily be exchanged by a custom and optimized one.

[TheZoq2]

Yep, I think I agree, and the only reason I didn't extend it to 255 in that PR is that I couldn't be bothered to figure out the required changes to the algorithm for that to work. In this case, I just used the same algorithm that FastLED does

[david-sawatzke]

I've overlooked that fastled scales the hue value from 0-255 to 0-191 before using it (https://github.com/FastLED/FastLED/blob/dae69768c643f69a8856dfc1b769d62ae051b624/hsv2rgb.cpp#L259). Their implementation is pretty simple (https://github.com/FastLED/FastLED/blob/6af86cea6cbe110b0ec9432c52db9e74d1704717/lib8tion/scale8.h#L20), so hue = (hue * 192) / 256 should work and be equivalent.