7
\$\begingroup\$

One of the projects that have been on my to-do list since years involves controlling a number of Inter-Integrated Circuit (\$I^2C\$) peripherals. Lately I've been turning everything into it's own library and I'm wondering if I'm doing it right.

While the libraries are usually called by a server-like script, they all have a demo function as well.

My goal is to write a production-quality library for multiple controllers. To make sure I didn't make any silly mistakes, I put one up for review.

The following library will control a LED-shield connected by I2C to a Raspberry Pi (B+). Registers 2 till 8 are responsible for the lighting and take values from 0 to 255. Overflows (values above 255) are not a problem, since only the last 8 bits will be saved in the hardware. However, this is potentially wasteful on the bus.

There's one method of setting the white LED and two methods of setting the red, green and blue LED. The latter can be set by using RGB or hue, saturation, brightness (HSB) values.

The library makes use of the System Management Bus (SMBus), which is a simplified version of \$I^2C\$. Since nothing time-critical is going on, this does suffice.

A library should have docstrings just about everywhere for introspection and plenty of other reasons (see PEP 257). Those get tedious quickly, and can probably be improved. The docstrings are explicitly up for review.

Led.py

import smbus


class LED:
    """
    I2C LED controller.

    Set registers 2 till 8.
    Registers 2 till 5 are directly responsible for the power of one LED each:
    White, red, green and blue.
    Those registers will be named by their number.
    Registers 6 till 8 (HSB) are an alternative method of setting
    red, green and blue.
    RGB and HSB will override each other.
    """
    def __init__(self, bus=1, address=0x71):
        """
        Set bus and address.
        """
        self.bus = smbus.SMBus(bus)
        self.address = address

    def writeWhiteRegister(self, value=0):
        """
        Set white register.
        """
        self.bus.write_byte_data(self.address, 2, value)

    def writeRedRegister(self, value=0):
        """
        Set red register.
        """
        self.bus.write_byte_data(self.address, 3, value)

    def writeGreenRegister(self, value=0):
        """
        Set green register.
        """
        self.bus.write_byte_data(self.address, 4, value)

    def writeBlueRegister(self, value=0):
        """
        Set blue register.
        """
        self.bus.write_byte_data(self.address, 5, value)

    def writeRgbwRegisters(self, red=0, green=0, blue=0, white=0):
        """
        Set red, green, blue and white registers.

        Default for all is 0.
        """
        self.writeRedRegister(red)
        self.writeGreenRegister(green)
        self.writeBlueRegister(blue)
        self.writeWhiteRegister(white)

    def writeHueRegister(self, value=0):
        """
        Set hue.
        """
        self.bus.write_byte_data(self.address, 6, value)

    def writeSaturationRegister(self, value=0):
        """
        Set saturation.
        """
        self.bus.write_byte_data(self.address, 7, value)

    def writeBrightnessRegister(self, value=0):
        """
        Set brightness.
        """
        self.bus.write_byte_data(self.address, 8, value)

    def writeHsbRegisters(self, hue=0, saturation=0, brightness=0):
        """
        Set hue, saturation and brightness.

        Warning:

        Setting any of saturation or brightness on 0 will result in LEDs off.
        """
        self.writeHueRegister(hue)
        self.writeSaturationRegister(saturation)
        self.writeBrightnessRegister(brightness)

ledDemo.py

import Led
from argparse import ArgumentParser


def main():
    parser = ArgumentParser(description='LED Demo')
    group = parser.add_mutually_exclusive_group(required=True)
    group.add_argument(
        '--rgbw',
        type=int,
        nargs=4
        )
    group.add_argument(
        '--hsb',
        type=int,
        nargs=3
        )
    args = parser.parse_args()

    led = Led.LED()
    if args.rgbw:
        led.writeRgbwRegisters(*args.rgbw)
    else:
        led.writeHsbRegisters(*args.hsb)

if __name__ == '__main__':
    main()

Example usage:

python ledDemo.py --rgbw 90 100 110 120
python ledDemo.py --hsb 120 130 140

I'm not certain whether adding shebangs is a standardized practice in libraries and their example usage. I suspect it's not for the former but is for the latter. Any input on this during a review would be great.

\$\endgroup\$
5
\$\begingroup\$

PEP8

Most of it is respected aside from naming conventions:

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability.

this means you should use led.py instead of Led.py and, even though it does not seem to be used publicly, led_demo.py instead of ledDemo.py.

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

This applies to each and every one of your methods.

Naming

Naming is hard, but I feel that using Register at the end of each method name does not add value. It is an implementation detail and users of the module will only be interested in setting a color, knowing that a value must be put in a register for that is not required.

The same kind of argument could be made for write. No need to know that the provided value has to be written somewhere. Users should only want to "assign" a certain value to a certain LED without having to deal with the implementation details. Following common terminology, I could suggest using set instead. But in this particular case, I might choose illuminate or power instead.

So I'd rename your methods:

def power_white(self, value=0):
    ...

def power_red(self, value=0):
    ...

def power_green(self, value=0):
    ...

def power_blue(self, value=0):
    ...

def power_RGBW(self, red=0, green=0, blue=0, white=0):
    ...

def set_hue(self, value=0):
    ...

def set_saturation(self, value=0):
    ...

def set_brightness(self, value=0):
    ...

def set_HSB(self, hue=0, saturation=0, brightness=0):
    ...

Getting current state

Since smbus provide ways to read values from registers, it could be a good idea to format these data to your users by providing means of reading the current state of the LEDs.

This means you will have both a setter and a getter for each "color" (HSB included): time to use the more pythonic property decorator:

@property
def white(self):
    ...
@white.setter
def white(self, value):
    ...

@property
def red(self):
    ...
@red.setter
def red(self, value):
    ...

@property
def green(self):
    ...
@green.setter
def green(self, value):
    ...

@property
def blue(self):
    ...
@blue.setter
def blue(self, value):
    ...

@property
def RGBW(self):
    ...
@RGBW.setter
def RGBW(self, values):
    r, g, b, w = values
    self.power_RGBW(r, g, b, w)  # or self.power_RGBW(*values)
def power_RGBW(self, red=0, green=0, blue=0, white=0):
    ...

@property
def hue(self):
    ...
@hue.setter
def hue(self, value):
    ...

@property
def saturation(self):
    ...
@saturation.setter
def saturation(self, value):
    ...

@property
def brightness(self):
    ...
@bightness.setter
def brightness(self, value):
    ...

@property
def HSB(self):
    ...
@HSB.setter
def HSB(self, values):
    h, s, b = values
    self.set_HSB(h, s, b)  # or self.set_HSB(*values)
def set_HSB(self, hue=0, saturation=0, brightness=0):
    ...

Usage includes:

led = LED()
led.red  # read current red value
led.blue = 125  # set new blue value
led.HSB = (236, 85, 26)  # set new HSB triplet
led.power_RGBW(white=54, green=256)  # set new RGBW quadruplet

Using both @RGBW.setter def RGBW(...) and def power_RGBW(...) (same for HSB) is required due to the way property handles arguments. It only provide 1 parameter to the setter, so it must be a tuple to handle multiple values. Having a regular method next to it let you use default values (and thus variable number) for parameters.

Managing multiple values at once

I never used smbus and I don't know how it works, but if you can use it in such a way that you can feed it multiple values and they are written all at once in various registers, you should use that instead for power_RGBW and set_HSB instead of performing multiple operations.

Example code

At some point I was tempted to suggest to move the content of ledDemo.py into your main file, under an if __name__ == '__main__' clause. So it could serve as documentation. But it doesn't seems to me that it is a good idea anymore, as it is too close to your needs and not necessarily a general purpose demo, but you might want to consider it.

There was also a discussion in the comment about the use of the mutually exclusive groups. In your current implementation, the user is required to provide all values for a kind of assignment (4 for RGBW and 3 for HSB). But you could allow for variable number of arguments:

from argparse import ArgumentParser
import led


def type_RGBW(values):
    # Do not try to catch ValueError since argparse will do and provide an error message
    values = map(int, values.split(','))
    values += [0] * (4 - len(values))
    # consider values[0] as white and thus put it at the end
    return values[1:] + [values[0]]

def type_HSB(values):
    values = map(int, values.split(',')
    return values + [0] * (3 - len(values))

def main():
    parser = ArgumentParser(description='LED Demo')
    group = parser.add_mutually_exclusive_group(required=True)
    group.add_argument(
        '--rgbw', type=type_RGBW, nargs=1,
        metavar='[white[,red[,green[,blue]]]]'
    )
    group.add_argument(
        '--hsb', type=type_HSB, nargs=1,
        metavar='[hue[,saturation[,brightness]]]'
    )
    args = parser.parse_args()

    demo_led = led.LED()
    if args.rgbw:
        demo_led.RGBW = args.rgbw
    else:
        demo_led.HSB = args.hsb
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.