Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement automatic board detection #32

Merged
merged 23 commits into from Jun 14, 2021
Merged

Implement automatic board detection #32

merged 23 commits into from Jun 14, 2021

Conversation

p1r473
Copy link
Contributor

@p1r473 p1r473 commented Jun 10, 2021

Hi Linus,
I have taken a stab at implementation.
It works!

A few points for you to review
-I am not sure if I did the enumeration to your liking.
-I removed the part in the readme, now that it works out of the box.
-After I blacked the files, it rearranged the whitespace on your argument parser
-I didnt know if I should put the _get_board in the Backlight class. I'm not very good with OOP, so I was not able to access it with self._get_board(). My guess is because it hasn't been init'd yet.

Thanks,

Fixes #30

@p1r473
Copy link
Contributor Author

p1r473 commented Jun 10, 2021

Looks like the build is failing with

FileNotFoundError: [Errno 2] No such file or directory: '/proc/device-tree/model'

Perhaps the error checker does not have this file.
However, my TinkerBoard 1 + 2 and the Pi have this

I've reimplemented the try:except so the build works.

According to https://gitlab.freedesktop.org/Miouyouyou/RockMyy/-/issues/11#note_135858
The Tinkerboard shows "Rockchip RK3288 Tinker Board"  when you `cat /proc/device-tree/model`
So I improved the logic.
I put the try/except back in to see if the build passes now.
Catch invalid board types with try:except so build passes
Copy link
Owner

@linusg linusg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice start, thank you! Some feedback below.

rpi_backlight/__init__.py Outdated Show resolved Hide resolved
rpi_backlight/__init__.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/utils.py Outdated Show resolved Hide resolved
rpi_backlight/utils.py Outdated Show resolved Hide resolved
rpi_backlight/cli.py Outdated Show resolved Hide resolved
p1r473 and others added 2 commits June 11, 2021 15:26
Co-authored-by: Linus Groh <mail@linusgroh.de>
@p1r473
Copy link
Contributor Author

p1r473 commented Jun 11, 2021

I am ready for a re-review
Thanks,

rpi_backlight/cli.py Outdated Show resolved Hide resolved
rpi_backlight/utils.py Outdated Show resolved Hide resolved
@p1r473
Copy link
Contributor Author

p1r473 commented Jun 11, 2021

Seems to be failing with

rpi_backlight/utils.py:1: error: Module "__future__" has no attribute "annotations"
Found 1 error in 1 file (checked 9 source files)
Error: Process completed with exit code 1.

Without
from __future__ import annotations
I get

root@Harbormaster:/home/linaro/rpi-backlight# python3 setup.py install;
Traceback (most recent call last):
  File "setup.py", line 2, in <module>
    from rpi_backlight import __version__
  File "/home/linaro/rpi-backlight/rpi_backlight/__init__.py", line 10, in <module>
    from . import utils
  File "/home/linaro/rpi-backlight/rpi_backlight/utils.py", line 11, in <module>
    def detect_board_type() -> Optional[BoardType]:
NameError: name 'BoardType' is not defined

According to https://stackoverflow.com/questions/46641078/how-to-avoid-circular-dependency-caused-by-type-hinting-of-pointer-attributes-in this import is required to allow for names to not be evaluated at compilation

Looks like it may be Python 3.7+ only

@linusg
Copy link
Owner

linusg commented Jun 11, 2021

oh, hmm. Let's remove the annotations import then and make the return type Optional["BoardType"], as a string. I'm not used to this old Python anymore :D

@p1r473
Copy link
Contributor Author

p1r473 commented Jun 11, 2021

Thanks Linus for your guidance
As you can tell I am not as good at Python as you, so I am thankful for your teachings. I learned a lot this round!
Thanks,

rpi_backlight/cli.py Outdated Show resolved Hide resolved
@p1r473
Copy link
Contributor Author

p1r473 commented Jun 14, 2021

Hi,
I've removed the unused import and ready for the final review
Thanks,

@linusg
Copy link
Owner

linusg commented Jun 14, 2021

#32 (comment)

Please add the comma and restore the original formatting :)

@p1r473
Copy link
Contributor Author

p1r473 commented Jun 14, 2021

Fixed the version parser argument formatting, thanks!

@linusg linusg merged commit 1770ea2 into linusg:main Jun 14, 2021
@linusg
Copy link
Owner

linusg commented Jun 14, 2021

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic board detection for initiation
2 participants