search

onlaj / Piano-LED-Visualizer

Piano LED Visualizer: Connect an LED strip to your Raspberry Pi and create an immersive visual experience for your piano playing
MIT License
534 stars 112 forks source link

connectall code duplication avoidance #322

Closed SirLefti closed 2 years ago

SirLefti commented 2 years ago

The connectall.py script has been ported to python recently and has been copied into midiports.py. To avoid this code copying I changed the script file to a single function that is called when running the file as a script (top-level code environment check). Now it is possible to import it in midiports and call the function as a replacement.

I intentionally just rewired the existing connectall in midiports because @jwessel added a reconnect call just before in his pull request #321. I hope this can be merged automatically.

jwessel commented 2 years ago

In general that is a good idea. The only problem is that the call to aconnect should be "sudo aconnect" in the updated version of the connectall.py. The visualizer can be run as a different user, and it will break otherwise. It will be backward compatible because root can run sudo directly.

SirLefti commented 2 years ago

I also noticed that, but I had no issues running it with default pi user, obviously with sudo, because it is required to run it without any issues (referring to README.md). I assumed that the subprocess.call inherits the permissions from the call, meaning it has sudo implicitly. Am I wrong?

jwessel commented 2 years ago

Further testing has shown that you do not need the sudo so long as you are in the right group which can write to /dev/snd/seq. That means you can leave your patch as is. :-)

I am not running the visualizer entirely as root. In my version, right before the first menu.show() in the visualizer.py I have the following:

os.system("id") print("Dropping root privileges") os.setuid(1000) os.system("id")

My intent is to get rid of as much of the sudo bit as possible as well as not run this as root and run it on a read-only file system.

Currently it impossible to start the visualizer as a non-root user because the user space gpio doesn't support PWM which is in use for the light bar, so this means it must be initialized before dropping the root privileges.

SirLefti commented 2 years ago

Alright. That's probably something for a future change :) Thank you for clarification

onlaj commented 2 years ago

I hope this can be merged automatically.

@SirLefti There was a conflict with https://github.com/onlaj/Piano-LED-Visualizer/pull/321 , can you take a look if I resolved it correctly?

SirLefti commented 2 years ago

I re-added the reconnect_all() call that probably caused the conflict. Should be fine now.