2
\$\begingroup\$

System76 sells laptops that come preloaded with their Ubuntu spinoff Pop_OS. The laptop keyboards have backlights that can change color and brightness, but so far I haven't found any way to keep the setting when I restart the machine; it goes back to bright white.

Here's someone else struggling with the same problem somewhat recently. The comments link to a useful python script, but it doesn't seem to be finished to the point where it would do what's wanted. (It's primary goal seems to be a GUI, with persistence on the wishlist.)

I finally got sick of it and implemented the behavior I wanted using systemd, which I'm using to call a Python script.

Since other people have asked the same questions as me in the past, I figured I'd share my solution, but the first step is to ask for feedback.

Is this a good solution to the problem? Are there mistakes or other reasons people shouldn't use it?

#!/usr/bin/env python3

import argparse as ag
import os.path as path
import re

print("Attempting to set system76 keyboard backlight color and brightness.")

"""
Partially cribbed from
https://github.com/ahoneybun/keyboard-color-chooser/blob/master/keyboard-color-switcher.py

Called by 
/etc/systemd/system/system76-kb-backlight.service
as decribed here:
https://www.howtogeek.com/687970/how-to-run-a-linux-program-at-startup-with-systemd/

Example file /etc/systemd/system/system76-kb-backlight.service
``
[Unit]
Description=Set color and brightness of system-76 laptop keyboard backlight

[Service]
Type=simple
ExecStart=systemd-cat /path_to_this_executable/set_system76_kb_backlight -g 55 -r FF -B 150

[Install]
WantedBy=multi-user.target
``
(Don't forget to enable the service.)
"""

def color_fragment(string):
    if re.fullmatch(r'^[0-9A-F]{2}$', string):
        return string
    else:
        raise ag.ArgumentTypeError(f'"{string}" is not a two-digit hex value.')

def brightness_fragment(string):
    if re.fullmatch(r'^[0-9]{1,3}$', string) and 0 <= int(string) and 255 >= int(string):
        return string
    else:
        raise ag.ArgumentTypeError(f'"{string}" is not an integer 0-255.')

arg_parse = ag.ArgumentParser(description="Set the color and brightness of the system76 keyboard backlight.")
arg_parse.add_argument('-r', help="The red RGB value (00 to FF).", default="00", type=color_fragment)
arg_parse.add_argument('-g', help="The green RGB value (00 to FF).", default="00", type=color_fragment)
arg_parse.add_argument('-b', help="The blue RGB value (00 to FF).", default="00", type=color_fragment)
arg_parse.add_argument('-B', help="The brightness (0 to 255).", default="48", type=brightness_fragment)

args = arg_parse.parse_args()
red = args.r
green = args.g
blue = args.b
brightness = args.B
color = f'{red}{green}{blue}'

ledPath = "/" + path.join('sys', 'class', 'leds', 'system76_acpi::kbd_backlight') + '/'
if not path.exists(ledPath):
    ledPath = "/" + path.join('sys', 'class', 'leds', 'system76::kbd_backlight') + '/'

regions = ['left', 'center', 'right', 'extra']
region_paths = [ledPath + f'color_{r}' for r in regions]
brightness_path = ledPath + 'brightness'
settings = {brightness_path: brightness,
            **{rp: color for rp in region_paths}}

for (p,s) in settings.items():
    with open(p, 'w') as f:
        f.write(s)

print("Successfully set system76 keyboard backlight brightness.")
arg_parse.exit(0)
\$\endgroup\$
  • 1
    \$\begingroup\$ Not enough for an answer, but another option could be a -c flag, where the user just enters colors like 00FF00, instead of a separate flags for each color. \$\endgroup\$ – Linny Dec 30 '20 at 3:07
2
\$\begingroup\$

Namespaces...

... are not for creating taxonomies. They are there to avoid name collisions. Also the chosen module alias for argparse seems strange. It's name is derived from argument parser so, if any, the obvious abbreviation should be ap. However, it might be better to just import the needed names, instead of creating taxonomies, since there are no name collisions in your code base.

from argparse import ArgumentTypeError, ArgumentParser
from os.path import exists, join
from re import fullmatch

Don't run business logic on module load

Currently, your script parses the command line arguments and writes to files, as soon as the module is laoded. This can be avoided by using an if __name__ == '__main__': guard and / or a main() function.

def main():
    """Runs the script."""

    arg_parse = ag.ArgumentParser(description="Set the color and brightness of the system76 keyboard backlight.")
    arg_parse.add_argument('-r', help="The red RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-g', help="The green RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-b', help="The blue RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-B', help="The brightness (0 to 255).", default="48", type=brightness_fragment)

    args = arg_parse.parse_args()
    …

if __name__ == '__main__':
    main()

It might be even better to outsource the argument parsing into an own function.

def get_args():
    """Parses the command line arguments."""

    arg_parse = ag.ArgumentParser(description="Set the color and brightness of the system76 keyboard backlight.")
    arg_parse.add_argument('-r', help="The red RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-g', help="The green RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-b', help="The blue RGB value (00 to FF).", default="00", type=color_fragment)
    arg_parse.add_argument('-B', help="The brightness (0 to 255).", default="48", type=brightness_fragment)
    return arg_parse.parse_args()


def main():
    """Runs the script."""

    args = get_args()
    …

Input validation

Your input validation seems complicated. In both cases, for the RGB values and the brightness, an integer within a certain range is expected.

def parse_rgb(text):
    """Parses an RGB value from a string."""
    
    rgb = int(text)
    
    if not 0 <= rgb <= 0xff:
        raise ValueError('RGB value out of bounds.')

    return rgb


def parse_brightness(text):
    """Parses a value for the brightness from a string."""
    
    brightness = int(text)
    
    if not 0 <= brightness <= 255:
        raise ValueError('Brightness out of bounds.')

    return brightness

Note that argparse will automatically display an error message if a parser specified at type= raises a ValueError. So you can also get rid of one import and work with a built-in exception instead.

The downside of the integer parsing is, that you'll have to convert the ints back to strings then:

red = hex(args.r)[2:].upper().zfill(2)
green = hex(args.g)[2:].upper().zfill(2)
blue = hex(args.b)[2:].upper().zfill(2)
brightness = str(args.B)
…

You may also subclass an int for this use case:

class RGB(int):
    def __init__(self, value):
        if not 0 <= value <= 0xff:
            raise ValueError('RGB value out of bounds.')

    def __str__(self):
        return hex(self)[2:].upper().zfill(2)

    …

arg_parse.add_argument('-r', help="The red RGB value (0x00 to 0xff).", default=RGB(0), type=RGB)
…
red = str(args.r)
\$\endgroup\$
  • \$\begingroup\$ This is all good advice, thanks! My current version is frankly bloated more than I'd like; whatever. One detail: using a ValueError instead of a ArgumentTypeError in the type callable causes argeparse to ignore/shadow the provided error message. \$\endgroup\$ – ShapeOfMatter Dec 30 '20 at 21:02
1
\$\begingroup\$

I think this is a strange use of the path library:

"/" + path.join('sys', 'class', 'leds', 'system76_acpi::kbd_backlight') + '/'

It would be clearer as a single string, given that we don't need portability to non-Linux hosts:

'/sys/class/leds/system76_acpi::kbd_backlight/'
\$\endgroup\$
  • \$\begingroup\$ Thanks. I think that, while path.join is maybe overkill here, it's a normal way of working with paths. On the other hand, the mixing of string-concat and path.join is weird. Reading the docs again, I guess path.join('/', 'sys', 'class', 'leds', 'system76_acpi::kbd_backlight', '') would be proper? \$\endgroup\$ – ShapeOfMatter Jan 13 at 16:27
  • \$\begingroup\$ It still seems somewhat odd, in that we're using path.join() which abstracts away the separator char, but then mixing literal / back in. Just an opinion, anyway. \$\endgroup\$ – Toby Speight Jan 13 at 17:08
  • 1
    \$\begingroup\$ Agreed; it's odd that os.path doesn't give a way of building absolute paths. On the other hand, the script at hand wouldn't be relevant outside of the one particular system, so portability is less of a concern :) \$\endgroup\$ – ShapeOfMatter Jan 13 at 21:20
  • \$\begingroup\$ Yes, I meant to mention that we weren't using the portability that path.join() was created for. I've updated now. \$\endgroup\$ – Toby Speight Jan 13 at 21:28

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.