OpenVoiceOS / ovos-gui-plugin-shell-companion

Apache License 2.0
3 stars 4 forks source link

vcgencmd binary not in previous location #12

Closed mikejgray closed 10 months ago

mikejgray commented 1 year ago

2023-07-02 17:41:02.754 - gui - ovos_gui_plugin_shell_companion.brightness:discover:112 - ERROR - [Errno 2] No such file or directory: '/opt/vc/bin/vcgencmd': '/opt/vc/bin/vcgencmd'

However, it is still available on the Mark 2 Neon image without any additional installation:

$ which vcgencmd
/usr/bin/vcgencmd

StackOverflow indicates it's been this way since at least Ubuntu 20.04.

Should probably make the location configurable, or at least based on what enclosure type it finds, as this will not work for non-Linux systems. It looks like this effort was started but the path is still hardcoded.

FWIW, ddcutil wasn't installed in the Debian image for Neon, so I had to install it. Not sure if it's on any of the OVOS images but probably needs to be for buildroot at least.

JarbasAl commented 1 year ago

the hardcoded paths should be updated to also use

vcgencmd = find_executable("vcgencmd") or isfile("/opt/vc/bin/vcgencmd")
ddcutil = find_executable("ddcutil") or isfile("/usr/bin/ddcutil")

other platforms might need different utils, i think we should use dbus for desktop installs for example

 bus = dbus.SessionBus()
 remote_object = bus.get_object("org.freedesktop.PowerManagement","/org/kde/Solid/PowerManagement/Actions/BrightnessControl") 
 remote_object.setBrightness(25, dbus_interface = "org.kde.Solid.PowerManagement.Actions.BrightnessControl")

PRs welcome, dbus-next is the library we use for dbus across ovos

denics commented 1 year ago

what if, as an easy fix, we add the possibility to configure the binary location in mycroft.conf ? we are using it for other commands too, it would be consistent and it would allow the use of dbus overriding its result if defined.

JarbasAl commented 1 year ago

what if, as an easy fix, we add the possibility to configure the binary location in mycroft.conf ? we are using it for other commands too, it would be consistent and it would allow the use of dbus overriding its result if defined.

vcgencmd = find_executable("vcgencmd") <- this is the easy fix and should find the executable

mycroft.conf should only be used if the whole command itself can be changed such as in the play commands

long term this should be made to use dbus as a more cross platform solution with less dependencies, the whole brightness implementation has always been very hacky and somewhat specific to the specific hardware in use, dont remember all details since aix is the one who worked on this code but i think this checked 2 different binaries which depended on what screen was in use

denics commented 1 year ago

this PR works on my NeonAI v23.8.15 on DevKit