search

home-assistant / supervisor

:house_with_garden: Home Assistant Supervisor
https://home-assistant.io/hassio/
Apache License 2.0
1.73k stars 635 forks source link

LEDs in `/sys/class/leds` not usable due to Supervisor's read-only mount; related to `GPIO: true` #4560

Open diablodale opened 1 year ago

diablodale commented 1 year ago

Describe the issue you are experiencing

My addon to control RPi power and activity LEDS needs write access to /sys/class/leds. https://docs.kernel.org/leds/leds-class.html. HassOS + Supervisor mounts it read-only. No perm setting, e.g. full_access: true can enable this. This issue is the mount as read-only. Therefore, it is not possible to control these LEDs from an addon. 😞

This issue is more about permission policies/feature. Refer to the sister issue of GPIO support gpio: true in https://github.com/home-assistant/supervisor/issues/201#issuecomment-333289235.

On RPi+kernels I've tested...individual LEDs in /sys/class/leds are symlinks to ../../devices/platform/leds aka /sys/devices/platform/leds. The technical solution is a rw mount of /sys/class/leds and /sys/devices/platform/leds like was done for GPIO in the sister issue and implemented here

https://github.com/home-assistant/supervisor/blob/2605f856687cac5a4918d2e545a563a0a8de1b23/supervisor/docker/addon.py#L409

A fixes can be trivial. It is a philosophy choice...

What type of installation are you running?

Home Assistant OS

Which operating system are you running on?

Home Assistant Operating System

Steps to reproduce the issue

Repro

  1. Haos install on RaspberryPi 3 or 4B. (will likely reproduce on all RPi)
  2. Create a basic addon.
  3. Add every possible permission to config.yaml. Any combination will repro the issue. For example 
    usb: true
gpio: true
apparmor: false
privileged:
 - SYS_ADMIN
 - SYS_RAWIO
  4. In your addon Dockerfile, make it run a bash script that has echo "none" > /sys/class/leds/PWR/trigger
  5. install your addon, disable protection if you want (doesn't matter), and start your addon

Result

Addon fails with error in logs

/run.sh: line xx: /sys/class/leds/PWR/trigger: Read-only file system

If you add in your run.sh a line with mount, then the log will also show all mounts within that container. And you will see the cause of the error is due to the RO mount of /sys.

sysfs on /sys type sysfs (ro,nosuid,nodev,noexec,relatime)

Expected

No error.

Anything in the Supervisor logs that might be useful for us?

n/a

System Health information

System Information

version core-2023.9.1
installation_type Home Assistant OS
dev false
hassio true
docker true
user root
virtualenv false
python_version 3.11.5
os_name Linux
os_version 6.1.21-v8
arch aarch64
timezone Europe/Berlin
config_dir /config
Home Assistant Cloud logged_in | false -- | -- can_reach_cert_server | failed to load: timeout can_reach_cloud_auth | failed to load: timeout can_reach_cloud | ok
Home Assistant Supervisor host_os | Home Assistant OS 10.5 -- | -- update_channel | stable supervisor_version | supervisor-2023.08.3 agent_version | 1.5.1 docker_version | 23.0.6 disk_total | 57.8 GB disk_used | 5.0 GB healthy | true supported | true board | rpi3-64 supervisor_api | ok version_api | failed to load: timeout installed_addons | AdGuard Home (4.8.14), Studio Code Server (5.10.1), Z-Wave JS (0.1.90), Cloudflared (4.2.6), Terminal & SSH (9.7.1), Raspberry Pi LED control (0.1.37)
Dashboards dashboards | 2 -- | -- resources | 0 views | 1 mode | storage
Recorder oldest_recorder_run | September 12, 2023 at 3:00 PM -- | -- current_recorder_run | September 12, 2023 at 5:00 PM estimated_db_size | 47.34 MiB database_engine | sqlite database_version | 3.41.2

Supervisor diagnostics

config_entry-hassio-2cb0d81ab06d8ffebfd5deffc58e5aaa.json.txt

Additional information

No response

mdegat01 commented 1 year ago

I do not prefer this solution due to its complexity and that it requires a Supervisor install.

I'm confused, you are asking for a way to change the LEDs from an addon. Addons only exist in a supervisor install. So a supervisor install is required regardless of how the solution is implemented here...

Regardless our preferred approach here is to have OS Agent publish a Dbus interface for supported boards with options for that board. Rather then having addon developers modify the host OS directly. This doesn't add any extra dependency though because like I said, addons do not exist in an HA install if there is no supervisor.

Also the main goal here is to add options to HA frontend to allow users to set features of their board (like LEDs). The frontend can only talk to HA and Supervisor APIs so we must expose these features that way. I wasn't aware of a use case where addon developers wanted to modify LEDs until now but if so the APIs are available for them too. Be aware though that currently changes to the LEDs require a reboot to take effect.

diablodale commented 1 year ago

The goal is for "something" in HA to render an config/options UI that when used adjusts the LEDs on a RPi. This "something" would also expose a set of services that allow any other something in HA to call them to programatically adjust same LEDs.

My experience to date is HAos. Addons are the "something" that I have so far learned that allow me to write code, that touches hardware, that can render a UI, and expose services.

@mdegat01 the dtparams approach is one way some LEDs on RPi can be controlled and it requires a reboot. My addon uses the realtime and standard Linux approach with /sys/classes/leds. I also use USB control transfers to control the LEDs on the ethernet controller; that part of my addon works with usb: true.

I've briefly reviewed the "green" and "yellow" board code that works with dbus. That is the 3rd verbose approach I listed. I didn't notice that it needed a reboot. No one likes rebooting. Isn't that one of Linux' glorious benefits? 😉

mdegat01 commented 1 year ago

HAOS has supervisor. The only two types of installs with addons are an HAOS install and a Supervised install. Both of which include supervisor.

diablodale commented 1 year ago

Great. Then option 1 works. To add the two LED class folders to the gpio perm group.

agners commented 1 year ago

What is your intention? Do you plan to control the LED through automations?

The preferred way for boards is through OS Agent. Yes it adds some boiler plate code in OS Agent and Supervisor, but it makes a clear abstraction. That way, since the OS Agent is part of the OS, if the sysfs paths change (e.g. due to kernel/device tree changes from the Raspberry Pi), we can simply adjust the LED paths in the OS Agent.

diablodale commented 1 year ago

I view this issue (perhaps I should re-title it) as a GPIO permission inquiry. There are some GPIOs that the existing Supervisor codebase does not include in their RW mount for gpio: true. My request is to add the missing GPIOs. These can then be used by me for RPi LEDs or anyone else in the community for those or different LEDs.

I learned that the RPi LEDs are all controlled by the primary GPIOs or the VPU GPIO extender. https://github.com/raspberrypi/firmware/blob/1e1773b025bf3d1d7e9b5b48c2ebc2ed14f02c90/extra/dt-blob.dts#L1507 https://github.com/raspberrypi/linux/issues/1332#issuecomment-194364647

Given that, I consider gpio: true to be the correct grouping/perms for these missing GPIOs. The fix is trivial...it is only adding two path strings on line 409 linked in OP.

You write if the sysfs paths change (e.g. due to kernel/device tree changes from the Raspberry Pi), we can simply adjust the LED paths in the OS Agent. That is still true with the one-line fix. You simply update that one line.

I don't see any value in the OS agent approach.

Reminder on my addon's intention. But this discussion is really about missing GPIOs...

The goal is for "something" in HA to render an config/options UI that when used adjusts the LEDs on a RPi. This "something" would also expose a set of services that allow any other something in HA to call them to programatically adjust same LEDs.

mdegat01 commented 1 year ago

Just to be clear, the LED features in OS agent and supervisor had nothing to do with addons. Addons controlling LEDs wasn't even a considered use case tbh. That's why I said earlier, this was the first I heard of it.

The purpose of the feature was to get this screen working in the HA frontend: Screenshot 2023-09-14 at 9 29 52 AM

The frontend runs on the client. It cannot mount things into a container from the host to modify them. The only way the frontend can enact change is via an API. So supervisor having APIs to set board features is required.

As you noted, addons don't have this limitation, they can mount things into their container and change them. And already have some access to gpio via gpio: true. I'm fine with expanding what that config option means if @agners is as he is the expert in HAOS and sees far more of the complications that occur there then I do. However...

Current yellow/green approach requires reboot. We are a people in a community that want our LED lights to switch on/off without rebooting.

I was half wrong on this. Apparently for green we do not require a reboot. Yellow still does but that will be corrected soon. The consequence however is that these values are not persisted on the host. Supervisor will save the latest values it has seen via its API and resubmit them every time it restarts. This approach likely means modifying the host directly will not work very well as supervisor will undo your changes each restart.

So if we want to support this approach, supervisor also needs a way to know what happened on host without its involvement so it can update its cached settings. Or we need to make these changes persistent on the host again so supervisor does not have to cache settings.

Again though to be clear, addon support is not the primary use case. The primary use case is the UI I screenshoted above. If we have to choose between making it easy and simple for addons to set LEDs and making it easy and simple to set values of LEDs in the UI, we are choosing the latter.

diablodale commented 1 year ago

Again this is about use of GPIOs from AddOns that are not yet included with gpio: true. I use as an example...my addon that will controls the RPi LEDs https://github.com/diablodale/dp_rpi_led_ctrl. Ether LED works today. RPIs power+activity LEDs also work if you run the command-line tool in it. I'm blocked in further dev+test due to this gpio permission issue.

My references to yellow and green are only to caution that their approach does not appear sustainable. This option#3 having the amount of (rpiBoards 400 changes) over (rpiBroads 16) files is extreme. Perhaps you misunderstand...I have no ask for anything with Agent, Yellow, or Green.

Option#1 is only an add of 47 characters to line 409 in OP. That is multiple magnitudes less code and inter-component/file complexity -- resulting in easier to understand, test, and maintain. It keeps in alignment the feature of enabling an AddOn to access gpios gpio: true by recognizing that LEDs are gpios.

AddOns controlling hardware are very common. I reference one in the OP. Another is Deconz that needs gpio, usb, and rawio https://github.com/home-assistant/addons/blob/cd11be29f99ad544b76679b09a33cdf86291bbe2/deconz/config.yaml. The nature of a new feature/progress...is that someone before had not heard or thought of it. Nothing unusual about that.

My addon that needs this GPIO aka LED access will not do anything on yellow or green boards. It will not read or write to them or conflict. I explicitly list only valid machines (in the config.yaml) and further check for valid machines and paths within the addon code itself. The addon will only be enabled -and- actually run on a subset of RPis (with more RPi to come later with testing). Home assistant AddOns persist their own state automatically. I use standard AddOn features...there is no need for Supervisor to be involved in these settings. Therefore, the concern of your last two paragraphs is not relevant.

I can imagine an Option#4: It would be a one-line method to add needed RW mounts for GPIOs for a given board to the Supervisor codebase. Look at the following code...it defines in a central place needed mounts...

https://github.com/home-assistant/supervisor/blob/e1232bc9e7dc4b69fd4659470479b57ed6e2edd1/supervisor/docker/const.py#L73-L91

That constants file is a central place where mounts are described. Those constants are then used in the code from the OP.

https://github.com/home-assistant/supervisor/blob/e1232bc9e7dc4b69fd4659470479b57ed6e2edd1/supervisor/docker/addon.py#L451-L453

As I review this code as I type, I believe the GPIO code in that same file has a hack in it. The conversations I write above led to commit https://github.com/home-assistant/supervisor/commit/9eee8eade6520359631f01e1a77f7b17ce852d29 hardcoding the path here instead of using the constant file. If this issue causes the code to be touched, I recommend fixing this -- move the hardcoded GPIO paths on line 409 to the central const.py constants file.

https://github.com/home-assistant/supervisor/blob/e1232bc9e7dc4b69fd4659470479b57ed6e2edd1/supervisor/docker/addon.py#L407-L419

Next, where do the GPIO paths for boards go? that .../soc/ path (I think) is only for RPi-like machines. It could be coded directly in the above const.py file. A slight alternative could be that the const.py file or slightly refactored code in addon.py file references a board-specific const file that contains a json object that lists all needed paths for a GPIO. No board const file, then its only the default /sys/class/gpio. But when the code finds a board const file, then it appends that board's specific GPIO paths into the json object that is used in addon.py to mount. This means the /sys/devices/platform/soc path would move to an RPi specific const file.

I encourage yellow and green to be interactive. That's good news to read 🎺 though doesn't help RPi customers. https://analytics.home-assistant.io/ shows that RPi 4 is the # 1 hardware platform (not vm) and HaOS the # 1 install method. Adding the needed GPIO aka LED permissions for the number 1 on number 1 can enable them to easily create solutions (like my LED addon).

pvizeli commented 1 year ago

"/sys/class/gpio", "/sys/devices/platform/soc" is an ugly hack that get removed soon as the Linux kernel has a new standard interface to communicate with GPIO without sysfs. Docker has /sys set to ro by design, and we just hacked it around for old software that does not support the "new" (it's not new, now it's a long time out now) way to use GPIO.

We do not use RPI as a multi-propose board nor support all ways you can use an RPI. It is just like green and yellow, and we offer our way of running an RPI.

diablodale commented 1 year ago

@pvizeli thanks for perspective. I agree using standards is a good approach when balanced with needs. For clarity... are you referencing the "new standard" for GPIO as /dev/gpiochipXXX as in https://sergioprado.blog/new-linux-kernel-gpio-user-space-interface/ ?

Are there non-NabuCasa APIs available that can easily expose LED control when Haos is used? Methods for non-NabuCasa platforms/boards (Rpi, orange, odroid, xyzabc)? I see yellow+green have a complex method. And that method seems to be "our way" NabuCasa only. Most of these platforms have up to 3 LEDs (power, activity, network)...a lowest common denominator on/off.

The standard up to kernel 6.6rc2 for controlling LEDs continues to be https://docs.kernel.org/leds/leds-class.html. I see no writing in the 6.6 docs suggesting deprecation or that new LED APIs are coming. There is a read-the-brightness /dev/uleds device from https://www.kernel.org/doc/html/latest/leds/uleds.html that blocks 😝 until something else changes the LED brightness and returns a single brightness byte. No control of the LED -- not useful. To date, I have found only /sys/class/leds that Linux kernel exposes for control of LEDs.

I probed gpiochipXXX using gpioinfo and I can see the raw GPIOs underlying the two RPi LEDs (ACT and PWR). But this API does not interface+expose hardware LED controll for triggers and states that is managed by the LED subsystem at /sys/class/leds.

gpiochip0 - 58 lines:
    ...
    line  42: "STATUS_LED_G_CLK" "ACT" output active-high [used]
    ...
gpiochip1 - 8 lines:
    ...
    line   2: "PWR_LED_OFF" "PWR" output active-low [used]
    ...
github-actions[bot] commented 11 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

diablodale commented 11 months ago

keepalive

I'm waiting to hear back from Casa/core team on my two questions above https://github.com/home-assistant/supervisor/issues/4560#issuecomment-1726254006

agners commented 7 months ago

First off, a bit background on the OS Agent implementation: We've added a board specific D-Bus API mainly to expose firmware/bootloader functionality. One of the primary idea was to abstract board configuration such as overlays to be loaded on Raspberry Pi (typically controlled via config.txt). This functionality is highly board specific, firmware version dependent, and usually needs write permission to location we'd rather not want to expose to the rest of the stack (boot partition). Now from that point of view, it is not too far to put the boards specific LEDs into the same basket, and let it be controlled through that same interface.

But I fully agree with your observations: The implementation ended up quite clunky, especially on Yellow. The reboot requirement was mostly because we used the overlay capability to store the LED state. Using overlays make sense for other hardware configuration, but not so much for LED. The board implementation of Green already departed from that approach, writing to sysfs directly. The state is stored in Supervisor. At that point the question why we need to go through D-Bus to "just" write a file is legitimate.

Are there non-NabuCasa APIs available that can easily expose LED control when Haos is used? Methods for non-NabuCasa platforms/boards (Rpi, orange, odroid, xyzabc)?

While it looks like we would limit this to Nabu Casa hardware, that is not the intention. We are an open source project, any similar implementation for other boards are welcome (after all, the initial design had very much Raspberry Pi in mind).

There is always value in abstraction. But also cost. It is a tradeoff.

We've decided to continue the approach to give add-ons direct access to hardware where we have a good abstraction from the kernel. And the LED subsystem qualifies for that.

Again this is about use of GPIOs from AddOns that are not yet included with gpio: true.

You seem to use GPIO and LED somewhat interchangeable in your comments. In https://github.com/home-assistant/supervisor/issues/4560#issuecomment-1721449902 you say your issue is about missing GPIOs, whereas https://github.com/home-assistant/supervisor/issues/4560#issuecomment-1726254006 emphasize on missing /sys/class/leds. LEDs are probably quite often just GPIO drive, but they might also behind a dedicated LED driver connected through a I2C controller, or driven using PWM.

In any case, I agree with your findings that /sys/class/leds/ is the common way of how LEDs are exposed on Linux, and there seem to be no change on the horizon on how they are exposed. So this is the userspace interface we are given by the kernel.

However, we definitely should not conflate this user space API with the gpio add-on setting. A new setting is appropriate here, and I guess led or leds make sense here.

diablodale commented 7 months ago

Got it. I'm following your train of thought.

While I would like to support random LED manipulation apis so to support those that don't use sys/class/leds...I can also understand that allowing broad rootish access to various paths isn't a good direction to go. So having a declarative permission which allows access to only sys/class/leds I can support. It'll push work back to driver authors to mirror their existing apis to that Linux standard vpath.

Traveling this week, can better engage next week.