Add support for Tinker Board

[p1r473]

@linusg Hi, yes, that helps very much.
I've been working on this for 2 hours so far and seem to hit a roadblock.
I will keep this pull request open this time and we can work in this one, as I closed the last one

First, I'd like to check in with you and make sure I am following your plan so far.

Second, I can't seem to get this to work

if((self._board_type == BoardType.TINKER_BOARD)

Can you see if I am doing something bad with enum? I cant seem to get it to work unless I do

if((self._board_type.name == "TINKER_BOARD")

BTW, I couldnt use auto for enum, I think it is not supported on my Tinkerboard... Was not able to import it.

[linusg]

First, I'd like to check in with you and make sure I am following your plan so far.

It looks mostly good so far, thanks for woking on it! (some of the code violates PEP 8, but we can fix that once everything else is working. black is your friend.)

I guess I did not think backlight_sysfs_path through properly, let's do it a bit differently:

_BACKLIGHT_SYSFS_PATHS = {
    BoardType.RASPBERRY_PI: "/sys/class/backlight/rpi_backlight/",
    BoardType.TINKER_BOARD: "/sys/devices/platform/ff150000.i2c/i2c-3/3-0045/",
}

and then change the parameter's default value to None:

backlight_sysfs_path: Optional[Union[str, "PathLike[str]"]] = None,

and then check if we weren't given a sysfs path, and assign the right one if so:

if not isinstance(board_type, BoardType):
    raise TypeError("board_type must be a a member of the BoardType enum, got {0}".format(type(board_type)))

if not backlight_sysfs_path:
    backlight_sysfs_path = _BACKLIGHT_SYSFS_PATHS[board_type]
elif backlight_sysfs_path == _EMULATOR_MAGIC_STRING:
    ... existing code ...

Let's also keep _get_value and _set_value free of any board type logic please, and instead check the board type before calling these functions - like you did in fade_duration 👍

Second, I can't seem to get this to work

That is likely because you define the enum twice, so the value you pass to the contructor and the one you compare against are actually not considered equal. That's expected behaviour, I think.

Just define the enum once and then import it in cli.py 🙂

BTW, I couldnt use auto for enum, I think it is not supported on my Tinkerboard... Was not able to import it.

Yes, sorry about that - it was added in Python 3.6, so your solution using ints is fine for now!

---

If you get stuck or have any questions, I'm happy to answer them!

[p1r473]

Hi @linusg
Made some good progress today. Think Im more than half way done. Would like you to make sure they are up to your specifications.

Please note, Tinkerboard only has a brightness value, and no power or max brightness values.

One thing of note: I was going to implement a smooth option, -s, for when you want to power on smoothly. But, I have commented this out as Im not sure youd like this, as brightness function does the same.

If you are okay with the changes, tonight I plan on testing everything (have not begun intensive testing yet), and trying to learn how to black format.

[linusg]

Made some good progress today. Think Im more than half way done. Would like you to make sure they are up to your specifications.

Just saw it, looking good! Just want to mention it, don't take everything I say/request for granted - it might be a good idea, if if you can think of somethink better implement that instead 😃

One thing of note: I was going to implement a smooth option, -s, for when you want to power on smoothly. But, I have commented this out as Im not sure youd like this, as brightness function does the same.

Let's discuss this in a separate issue/PR - please remove these changes for now as they're not relevant to this PR (support for Tinker Board). Having a clean history is somewhat important to me, and the changes will be easier to review if there's less "noise" in the diff.

[p1r473]

Hi @linusg
I have tested and black formatted and made some changes
Ready for your final approval I think

[linusg]

Ready for your final approval I think

There are still CI checks failing 🙂

it's a few mypy errors:

rpi_backlight/__init__.py:140: error: Missing return statement
rpi_backlight/__init__.py:188: error: Missing return statement
rpi_backlight/__init__.py:206: error: Incompatible return value type (got "int", expected "bool")
rpi_backlight/__init__.py:208: error: Incompatible return value type (got "int", expected "bool")
Found 4 errors in 1 file (checked 5 source files)

The first two could probably be solved by changing

if self._board_type == BoardType.RASPBERRY_PI:
    ...
elif self._board_type == BoardType.TINKER_BOARD:
    ...

to

if self._board_type == BoardType.RASPBERRY_PI:
    ...
elif self._board_type == BoardType.TINKER_BOARD:
    ...
else:
    raise RuntimeError("Invalid board type")

This is handling the unlikely case that self._board_type is being changed after class instantiation.

The last two are proper bugs, the power property getter is supposed to return a bool, you're returning ints. You probably got confused by the comment # 0 is on, 1 is off - that is Raspberry Pi specific and means, if there's a 0 in the file the display is on, if it's a 1 it's off. Still returns a boolean.

[linusg]

Got some feedback below. I'd also suggest updating the readme and docs and ideally test as well if you feel comfortable doing that!

[p1r473]

I am still getting the CI errors...
I am trying to figure out this Travis thing to see how to fix them

[p1r473]

Hi @linusg
Good news and bad news

Good news first: I am finally finished testing, everything seems to work, I am passing on Travis now, bugs seem to be out. Please do a review after reading the bad news.

Bad news: Tonight I have also upgraded my Tinkerboard to latest 2.0.11 version image. In this version, they have implemented /sys/class/backlight/rpi_backlight/
I believe this path was not in previous versions, but I could swear these files didnt exist before, which is what drove me to create v1 before (but now I an doubting myself)

Some things to point out

1. both /sys/class/backlight/rpi_backlight/ and /sys/devices/platform/ff150000.i2c/i2c-3/3-0045/ exist simultaneously in latest Tinkerboard. I can set brightness through either "brightness" in the former and "tinker_mcu_bl" in the latter. Changing one file changes the other dynamically.
2. Several of the files of the Pi now exist (max_brightness, ml_power, actual_brightness, brightness)
3. Changing the brightness file does indeed work. Meaning, people can use YOUR latest version to change brightness on Tinkerboard
4. Changing bl_power does nothing. Though the file exists, changing it from 0/1 does NOT work.
5. Permission denied when trying to edit max_brightness and actual_brightness
6. To maintain functionality with people on older Tinkerboards, we should add in my new changes
7. Regardless of whether brightness works, setting the power through bl_power does NOT work, even though the file exists. So still, my new code is required for power toggling

However, I am mentioning these points to you in case you want to completely remove /sys/devices/platform/ff150000.i2c/i2c-3/3-0045/ and do everything instead from /sys/class/backlight/rpi_backlight/ which does exist now
-This MAY alienate users on previous builds on Tinkerboard

Thoughts?

[linusg]

Made one minor comment but apart from that this looks great - thanks! Will do some testing this evening.

[linusg]

Bad news: Tonight I have also upgraded my Tinkerboard to latest 2.0.11 version image. In this version, they have implemented /sys/class/backlight/rpi_backlight/
I believe this path was not in previous versions, but I could swear these files didnt exist before, which is what drove me to create v1 before (but now I an doubting myself)

Some things to point out

...

That's very much possible; I don't have a Tinker Board to verify - but nonetheless it still seems to be necessary to add these changes, as you say. And as long as they don't remove the old path, let's just stick with it 👍

[linusg]

Hi @p1r473,

I tested the PR, works great! I have to request a few more changes, not your fault:

• We need to add BoardType to __all__ so it's being picked up by Sphinx when generating the docs
• We have to add a docstring to it to replace the default one ("An enumeration.")
• We have to add #: comments so it includes the enum members

This is what the docs will look like:

• Swapping board_type and backlight_sysfs_path was actually bad advice, sorry. It would be a breaking change as we do not enforce keyword arguments - e.g. here:

https://github.com/linusg/rpi-backlight-emulator/blob/6b5b89732fbb9cf28edd65306ec2a36e03eafb14/rpi_backlight_emulator/__init__.py#L127

• Let's also change the board type identifiers for the CLI to use hyphen instead of underscore. Just feels less weird?

For all changes I made see the diff below:

diff --git a/rpi_backlight/__init__.py b/rpi_backlight/__init__.py
index d775501..6d3e1ba 100644
--- a/rpi_backlight/__init__.py
+++ b/rpi_backlight/__init__.py
@@ -10,11 +10,15 @@ if TYPE_CHECKING:
 
 __author__ = "Linus Groh"
 __version__ = "2.0.1"
-__all__ = ["Backlight"]
+__all__ = ["Backlight", "BoardType"]
 
 
 class BoardType(Enum):
+    """Enum to specify a board type in the :class:`~rpi_backlight.Backlight` constructor."""
+
+    #: Raspberry Pi
     RASPBERRY_PI = 1
+    #: Tinker Board
     TINKER_BOARD = 2
 
 
@@ -38,8 +42,8 @@ class Backlight:
 
     def __init__(
         self,
-        board_type: BoardType = BoardType.RASPBERRY_PI,
         backlight_sysfs_path: Optional[Union[str, "PathLike[str]"]] = None,
+        board_type: BoardType = BoardType.RASPBERRY_PI,
     ):
         """Set ``backlight_sysfs_path`` to ``":emulator:"`` to use with rpi-backlight-emulator."""
         if not isinstance(board_type, BoardType):
diff --git a/rpi_backlight/cli.py b/rpi_backlight/cli.py
index 19f5084..377f5cb 100644
--- a/rpi_backlight/cli.py
+++ b/rpi_backlight/cli.py
@@ -2,9 +2,9 @@ from argparse import ArgumentParser
 
 from . import Backlight, __version__, BoardType
 
-board_types = {
-    "raspberry_pi": BoardType.RASPBERRY_PI,
-    "tinker_board": BoardType.TINKER_BOARD,
+BOARD_TYPES = {
+    "raspberry-pi": BoardType.RASPBERRY_PI,
+    "tinker-board": BoardType.TINKER_BOARD,
 }
 
 
@@ -56,8 +56,8 @@ def _create_argument_parser():
     parser.add_argument(
         "-B",
         "--board-type",
-        default="raspberry_pi",
-        choices=board_types.keys(),
+        default="raspberry-pi",
+        choices=BOARD_TYPES.keys(),
         help="board type",
     )
     return parser
@@ -69,7 +69,7 @@ def main():
     args = parser.parse_args()
 
     backlight = Backlight(
-        board_type=board_types[args.board_type], backlight_sysfs_path=args.sysfs_path
+        board_type=BOARD_TYPES[args.board_type], backlight_sysfs_path=args.sysfs_path
     )
 
     if args.get_brightness:

Thank you!

[p1r473]

Changes made, thanks

[linusg]

looks like you accidentally added new files instead of updating the ones in the rpi_backlight/ directory :)

[p1r473]

Oopsies

[p1r473]

Let me check why Travis is failing now...

[p1r473]

@linusg have you had any luck installing black on your Pi?
Cant install it on Tinkerboard.. no package

[linusg]

Merged, thanks! I'll update the docs to reflect this change and then release a new version on PyPI.

have you had any luck installing black on your Pi?

Yes, but only with --pre (as they haven't released a "stable" version yet):

$ pip3 install --user --pre black
Looking in indexes: https://pypi.org/simple, https://www.piwheels.org/simple
Collecting black
  Downloading https://files.pythonhosted.org/packages/fd/bb/ad34bbc93d1bea3de086d7c59e528d4a503ac8fe318bd1fa48605584c3d2/black-19.10b0-py36-none-any.whl (97kB)
    100% |████████████████████████████████| 102kB 333kB/s 
Collecting pathspec<1,>=0.6 (from black)
  Downloading https://files.pythonhosted.org/packages/34/fa/c5cc4f796eb954b56fd1f6c7c315647b18b027e0736c9ae87b73bbb1f933/pathspec-0.7.0-py2.py3-none-any.whl
Requirement already satisfied: toml>=0.9.4 in ./.local/lib/python3.7/site-packages (from black) (0.10.0)
Requirement already satisfied: typed-ast>=1.4.0 in ./.local/lib/python3.7/site-packages (from black) (1.4.1)
Requirement already satisfied: appdirs in ./.local/lib/python3.7/site-packages (from black) (1.4.3)
Requirement already satisfied: attrs>=18.1.0 in ./.local/lib/python3.7/site-packages (from black) (19.1.0)
Collecting regex (from black)
  Downloading https://www.piwheels.org/simple/regex/regex-2020.2.20-cp37-cp37m-linux_armv7l.whl (644kB)
    100% |████████████████████████████████| 645kB 409kB/s 
Requirement already satisfied: click>=6.5 in ./.local/lib/python3.7/site-packages (from black) (7.0)
Installing collected packages: pathspec, regex, black
Successfully installed black-19.10b0 pathspec-0.7.0 regex-2020.2.20

[p1r473]

Hold up, seems to be a problem setting the default board type.
On my end, changed the code to:

def __init__(
        self,
        backlight_sysfs_path: Optional[Union[str, "PathLike[str]"]] = None,
        board_type: BoardType = BoardType.TINKER_BOARD,
    ):

However, even with the default being TINKER_BOARD, board_type is still being detected as RASPBERRY_PI, when not using -B 2

[p1r473]

Looks like we setting the default in the init and ALSO in the parser.
Should we remove one of the defaults?

parser.add_argument(
        "-B",
        "--board-type",
        default="raspberry-pi",

def __init__(
        self,
        backlight_sysfs_path: Optional[Union[str, "PathLike[str]"]] = None,
        board_type: BoardType = BoardType.TINKER_BOARD,
    ):

[linusg]

Yes, that's expected - you're not supposed to change the library source code 😄

The default for the CLI is set here:

rpi-backlight/rpi_backlight/cli.py

Line 59 in bdda78b

default="raspberry-pi",

[linusg]

Should we remove one of the defaults?

I think it's fine, as long as they match. Most users are probably using the display with a Raspberry Pi anyway, they don't have to do anything as the defaults will just work. Tinker Board users will have to set board_type=BoardType.TINKER_BOARD and --board-type tinker-board.