Add RGBLEDBoard Class #631

bennuttall · 2018-03-25T21:51:24Z

Adding an RGBLEDBoard class which allows users to create arbitrarily sized boards comprising multiple RGB LEDs. The API is assumed based on a mix of LEDBoard and RGBLED, i.e. it has on/off/toggle/blink/pulse, optional PWM, on/off/toggle should take a numerical argument (to control a single LED or a number of LEDs rather than the whole board), blink should allow on_color, off_color, etc.

from gpiozero import RGBLEDBoard

leds = RGBLEDBoard((2, 3, 4), (5, 6, 7), (7, 8, 9))

leds.value = ((1, 0, 0), (0, 1, 0), (0, 0, 1))

This paves the way for the API for classes for smarter RGB LEDs like APA102 (#551) and neopixels (#583) when we support those underlying protocols.

bennuttall · 2018-03-25T21:53:27Z

gpiozero/boards.py

@@ -59,15 +59,15 @@ def on(self):
        Turn all the output devices on.
        """
        for device in self:
-            if isinstance(device, (OutputDevice, CompositeOutputDevice)):
+            if isinstance(device, (OutputDevice, CompositeOutputDevice, RGBLED)):


I don't like this. @waveform80 @lurch do you think it's possible for RGBLED to extend OutputDevice or CompositeOutputDevice rather than just Device?

Actually I got around this by implementing on/off/toggle directly in RGBLEDBoard rather than using the parent class. Just working on getting blink to work too.

IIRC @waveform80 implemented RGBLED as a child of Device rather than as a child of OutputDevice or CompositeOutputDevice because the value of an RGBLED is a 3-tuple and not a boolean or 0 -> 1 float.
And yup, RGBLED also implements it's own on / off / toggle / blink / pulse methods.

codecov-io · 2018-03-25T21:56:53Z

Codecov Report

Merging #631 into master will increase coverage by 0.09%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
+ Coverage   85.58%   85.68%   +0.09%     
==========================================
  Files          36       36              
  Lines        7258     7336      +78     
==========================================
+ Hits         6212     6286      +74     
- Misses       1046     1050       +4

Impacted Files	Coverage Δ
gpiozero/__init__.py	`100% <ø> (ø)`	⬆️
tests/test_boards.py	`100% <100%> (ø)`	⬆️
gpiozero/boards.py	`92.01% <82.6%> (-0.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5032124...a63c181. Read the comment docs.

lurch · 2018-03-26T10:09:47Z

gpiozero/boards.py

+    board or collection of LEDs.
+
+    The following example turns on all the LEDs on a board containing 5 LEDs
+    attached to GPIO pins 2 through 6::


Did you mean "2 through 9" ? ;)
EDIT: 10

copy/paste error!

lurch · 2018-03-26T10:10:39Z

gpiozero/boards.py

+
+        from gpiozero import RGBLEDBoard
+
+        leds = RGBLEDBoard((2, 3, 4), (5, 6, 7), (7, 8, 9))


Hmm, probably shouldn't be using GPIO7 twice?

lurch · 2018-03-26T10:20:16Z

gpiozero/boards.py

+                pins_or_board
+                if isinstance(pins_or_board, RGBLEDBoard) else
+                RGBLED(
+                    *pins_or_board, pwm=pwm, active_high=active_high, initial_value=(initial_value, ) * 3,


Why have you got initial_value=(initial_value, ) * 3, ? IMHO RGBLEDBoard should have the same init-params as https://github.com/RPi-Distro/python-gpiozero/blob/master/gpiozero/output_devices.py#L581

I grabbed some implementation from LEDCollection which uses True / False for initial_value. Why do LEDCollection and LEDBoard have boolean initial_value when .value itself is an n-tuple?

I'm not sure - perhaps @waveform80 remembers more?
You could argue that the initial_value for an LEDCollection should be a tuple-of-booleans of the same size as the .value, or you could argue that the initial_value is only a default value, so you'd want to initialise every LED in an LEDCollection with the same value? https://gpiozero.readthedocs.io/en/stable/api_boards.html#ledboard

I guess another option would be to for the __init__ to support both an initial_value param (a boolean) and an initial_values param (a tuple of values), with an exception being raised if both are supplied, or an exception being raised if the number of tuples in initial_values is incorrect?

*shrug*

Oh, and is LEDCollection the class that allows nesting, i.e. each child-element can be an LED or another LEDCollection object? I guess that would also make an initial_values param slightly more complex? (because you'd need to check that nested LEDCollections have the same number of tuples / sub-nested LEDCollections etc.)

I think we need consistency foremost. So I'll go for initial_value being a single value for now. Do we have any classes where initial_value is a tuple the length (and shape) of the board itself? I do like the idea of an optional single value or tuple. The tricky thing (I think) is working out the length and shape at the point of looking it up in kwargs.

Actually, on second thoughts, it makes more sense that it follows RGBLED in expecting a ringle 3-tuple (in this case all RGBLEDs would be set to the same colour). This is also consistent with LEDBoard in that LEDBoard takes the same initial_value as LED or PWMLED.

Anyone wanting to set a more specific range of colours can do so manually after init.

Actually, on second thoughts, it makes more sense that it follows RGBLED in expecting a ringle 3-tuple (in this case all RGBLEDs would be set to the same colour).

Haha, exactly as I suggested above ;-)

Got there in the end!

bennuttall · 2018-04-12T19:59:36Z

Getting there! All tests passing except the blink ones, and I need to add some more blink and pulse tests, as well as look at nested RGBLEDBoards.

I'm not entirely happy with the implementation of the blink tests:

board.blink()
assert board.value == ((1, 1, 1), (1, 1, 1))
board.off()
board[0].blink()
assert board.value == ((1, 1, 1), (0, 0, 0))

Ping @martinohanlon

martinohanlon · 2018-04-13T10:58:50Z

tests/test_boards.py

+    with pytest.raises(ValueError):
+        RGBLEDBoard(2, 3, 4)
+
+def test_rgbledboard_bad_init_pwm():


I am confused by these tests.

RGBLEDBoard((2, 3, 4)) is expected to raise PinPWMUnsupported because the default for pwn is True (https://github.com/RPi-Distro/python-gpiozero/pull/631/files#diff-8e73cd872390a78e8e626359ec7c5837R1074)

But why are tests such as RGBLEDBoard((2, 3, 4), (5, 6, 7)) expected to pass?

Because those tests are in the list of tests to use MockPWMPin: https://github.com/RPi-Distro/python-gpiozero/blob/70fc6af18b2a2dc07dced4a85c2ba62830f7517b/tests/test_boards.py#L20

got ya.

I like the comment above it # dirty, but it does the job

…nto rgbledboard

bennuttall · 2018-10-04T11:56:06Z

@lurch I'm struggling to comprehend the herculean blink method as implemented in RGBLED. Would you be able to take a look and see if you can implement it for RGBLEDBoard or perhaps we could pair on it some time?

bennuttall added 2 commits March 25, 2018 22:49

Implement RGBLEDBoard

9ae76e0

Add RGBLEDBoard tests

a63c181

bennuttall changed the title ~~Provide wrappers for LED groups on add-on boards, close #81~~ WIP: Add RGBLEDBoard Class Mar 25, 2018

bennuttall commented Mar 25, 2018

View reviewed changes

lurch reviewed Mar 26, 2018

View reviewed changes

Extend RGBLEDBoard tests and reimplement

70fc6af

martinohanlon reviewed Apr 13, 2018

View reviewed changes

bennuttall changed the title ~~WIP: Add RGBLEDBoard Class~~ [WIP] Add RGBLEDBoard Class Sep 20, 2018

bennuttall force-pushed the master branch from f9b5b2e to 7a1776f Compare September 22, 2018 10:08

bennuttall added 5 commits September 22, 2018 11:14

Implement RGBLEDBoard

ee789c9

Add RGBLEDBoard tests

592c376

Extend RGBLEDBoard tests and reimplement

a8306c3

Merge branch 'rgbledboard' of github.com:rpi-distro/python-gpiozero i…

6eee1ea

…nto rgbledboard

Fix error in RGBLED example

11aa011

bennuttall force-pushed the rgbledboard branch from 8db7745 to 11aa011 Compare September 22, 2018 10:26

Refactor RGBLEDBoard tests and update docstring

781ef0a

bennuttall added this to v1.5 (if possible) in Roadmap Oct 4, 2018

bennuttall self-assigned this Oct 4, 2018

bennuttall added new device help wanted labels Oct 4, 2018

bennuttall moved this from v1.5 (if possible) to v1.6 (definite) in Roadmap Jan 15, 2019

Merge branch 'master' into rgbledboard

42adacf

bennuttall added this to Triage in Triage Feb 8, 2019

bennuttall moved this from Triage to In progress in Triage Feb 9, 2019

bennuttall marked this pull request as draft February 22, 2021 18:29

bennuttall changed the title ~~[WIP] Add RGBLEDBoard Class~~ Add RGBLEDBoard Class Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RGBLEDBoard Class #631

Add RGBLEDBoard Class #631

bennuttall commented Mar 25, 2018 •

edited

bennuttall Mar 25, 2018

bennuttall Mar 25, 2018

lurch Mar 26, 2018

codecov-io commented Mar 25, 2018 •

edited

lurch Mar 26, 2018 •

edited

bennuttall Mar 26, 2018

lurch Mar 26, 2018

bennuttall Mar 26, 2018

lurch Mar 26, 2018

bennuttall Mar 26, 2018

lurch Mar 26, 2018

bennuttall Mar 26, 2018

bennuttall Mar 26, 2018

lurch Mar 27, 2018

bennuttall Mar 27, 2018

bennuttall commented Apr 12, 2018

martinohanlon Apr 13, 2018

bennuttall Apr 13, 2018

martinohanlon Apr 13, 2018

bennuttall Apr 13, 2018

bennuttall commented Oct 4, 2018


		from gpiozero import RGBLEDBoard

		leds = RGBLEDBoard((2, 3, 4), (5, 6, 7), (7, 8, 9))

Add RGBLEDBoard Class #631

Are you sure you want to change the base?

Add RGBLEDBoard Class #631

Conversation

bennuttall commented Mar 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 25, 2018 • edited

Codecov Report

lurch Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennuttall commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennuttall commented Oct 4, 2018

bennuttall commented Mar 25, 2018 •

edited

codecov-io commented Mar 25, 2018 •

edited

lurch Mar 26, 2018 •

edited