AddressableLED.setHSV doesn't match Color.fromHSV

[Starlight220]

This fails:

AddressableLEDBuffer buffer = new AddressableLEDBuffer(1);
buffer.setHSV(0, h, s, v);
assertEquals(Color.fromHSV(h, s, v), buffer.getLED(0));

The issue is likely that AddressableLEDBuffer.setHSV() wasn't updated to use the new Color.fromHSV() impl from #4439.

Full code for the above test:

```diff diff --git a/wpilibj/src/test/java/edu/wpi/first/wpilibj/AddressableLEDBufferTest.java b/wpilibj/src/test/java/edu/wpi/first/wpilibj/AddressableLEDBufferTest.java index 6f00ba83a..5b662120e 100644 --- a/wpilibj/src/test/java/edu/wpi/first/wpilibj/AddressableLEDBufferTest.java +++ b/wpilibj/src/test/java/edu/wpi/first/wpilibj/AddressableLEDBufferTest.java @@ -10,11 +10,14 @@ import static org.junit.jupiter.params.provider.Arguments.arguments; import edu.wpi.first.wpilibj.util.Color; import edu.wpi.first.wpilibj.util.Color8Bit; + +import java.util.Random; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; class AddressableLEDBufferTest { @ParameterizedTest @@ -70,4 +73,26 @@ class AddressableLEDBufferTest { assertEquals(firstRedColor8Bit, buffer.getLED8Bit(2)); assertEquals(firstBlueColor8Bit, buffer.getLED8Bit(3)); } + + static Stream randomHsv() { + var random = new Random(); + return Stream.of( + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)), + arguments(random.nextInt(180), random.nextInt(255), random.nextInt(255)) + ); + } + + @MethodSource("randomHsv") + @ParameterizedTest + void algorithmsMatch(int h, int s, int v) { + AddressableLEDBuffer buffer = new AddressableLEDBuffer(1); + buffer.setHSV(0, h, s, v); + assertEquals(Color.fromHSV(h, s, v), buffer.getLED(0)); + } } diff --git a/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/addressableled/Robot.java b/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/addressableled/Robot.java index 5995cd044..2a4d6e2a9 100644 --- a/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/addressableled/Robot.java +++ b/wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/addressableled/Robot.java @@ -7,6 +7,7 @@ package edu.wpi.first.wpilibj.examples.addressableled; import edu.wpi.first.wpilibj.AddressableLED; import edu.wpi.first.wpilibj.AddressableLEDBuffer; import edu.wpi.first.wpilibj.TimedRobot; +import edu.wpi.first.wpilibj.util.Color; public class Robot extends TimedRobot { private AddressableLED m_led; @@ -46,7 +47,7 @@ public class Robot extends TimedRobot { // shape is a circle so only one value needs to precess final var hue = (m_rainbowFirstPixelHue + (i * 180 / m_ledBuffer.getLength())) % 180; // Set the value - m_ledBuffer.setHSV(i, hue, 255, 128); + m_ledBuffer.setLED(i, Color.fromHSV(hue, 255, 128)); } // Increase by to make the rainbow "move" m_rainbowFirstPixelHue += 3; diff --git a/wpilibjExamples/src/test/java/edu/wpi/first/wpilibj/examples/addressableled/RainbowTest.java b/wpilibjExamples/src/test/java/edu/wpi/first/wpilibj/examples/addressableled/RainbowTest.java new file mode 100644 index 000000000..b1297ee08 --- /dev/null +++ b/wpilibjExamples/src/test/java/edu/wpi/first/wpilibj/examples/addressableled/RainbowTest.java @@ -0,0 +1,54 @@ +// Copyright (c) FIRST and other WPILib contributors. +// Open Source Software; you can modify and/or share it under the terms of +// the WPILib BSD license file in the root directory of this project. + +package edu.wpi.first.wpilibj.examples.addressableled; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import edu.wpi.first.hal.HAL; +import edu.wpi.first.wpilibj.simulation.AddressableLEDSim; +import edu.wpi.first.wpilibj.util.Color; +import edu.wpi.first.wpilibj.util.Color8Bit; +import org.junit.jupiter.api.Test; + +class RainbowTest { + @Test + void rainbowPatternTest() { + HAL.initialize(500, 0); + try (var robot = new Robot()) { + robot.robotInit(); + AddressableLEDSim ledSim = AddressableLEDSim.createForChannel(9); + assertTrue(ledSim.getRunning()); + assertEquals(60, ledSim.getLength()); + + var rainbowFirstPixelHue = 0; + for (int iteration = 0; iteration < 100; iteration++) { + robot.robotPeriodic(); + var data = ledSim.getData(); + for (int i = 0; i < 60; i++) { + final var hue = (rainbowFirstPixelHue + (i * 180 / 60)) % 180; + assertIndexColor(data, i, hue, 255, 128); + } + rainbowFirstPixelHue += 3; + rainbowFirstPixelHue %= 180; + } + } + } + + private void assertIndexColor(byte[] data, int index, int hue, int saturation, int value) { + var color = new Color8Bit(Color.fromHSV(hue, saturation, value)); + byte b = data[index * 4]; + byte g = data[(index * 4) + 1]; + byte r = data[(index * 4) + 2]; + byte z = data[(index * 4) + 3]; + + assertAll( + () -> assertEquals(0, z), + () -> assertEquals(color.red, r & 0xFF), + () -> assertEquals(color.green, g & 0xFF), + () -> assertEquals(color.blue, b & 0xFF)); + } +} ```

[PeterJohnson]

Also, we need to make sure the new Color.fromHSV algorithm works as intended too.

[democat3457]

I can independently verify the algorithm itself works - on top is previous algorithm, on bottom is current algorithm; however, I did have to tweak the formula a little (changes which are reflected in #4724) because the existing algorithm relies on clamping to force values into [0,255].