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
Add NeoPixel Illumination plugin #1073
Conversation
@cp2004 would you mind reviewing this plugin for me? figured you'd have the best experience with it... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the 'sudo' password should be stored by the plugin, and definitely not exposed in the settings API either. It's a basic security issue, as it allows anyone with access to the OctoPrint UI access to also login to the server.
There's a few assumptions used that while they may work well for your installation, it might explode on other configurations - I've attempted to point out as many of them used as I have spotted, but do go through and have a look - hardcoded paths will almost never work.
- Ideally don't store password https://github.com/brettvitaz/OctoPrint-Neopixel_Illumination/blob/b4b9950c8f439870d5ab4dc54be4ebfabefe33c1/octoprint_neopixel_illumination/__init__.py#L89, if really not possible then it should not be exposed on the settings API, use
get_settings_restricted_paths
- If
on_settings_save
has been called on your plugin, then one of it's settings has changed - OctoPrint will only call this for your plugin if your plugin's settings have changed. No need to do the hard work twice. https://github.com/brettvitaz/OctoPrint-Neopixel_Illumination/blob/b4b9950c8f439870d5ab4dc54be4ebfabefe33c1/octoprint_neopixel_illumination/__init__.py#L156 - Users can change the username, and may also not have OctoPrint installed in this exact location https://github.com/brettvitaz/OctoPrint-Neopixel_Illumination/blob/b4b9950c8f439870d5ab4dc54be4ebfabefe33c1/octoprint_neopixel_illumination/__init__.py#L204 - use
sys.executable
instead. - It would be good if
time.sleep
was not called here - long running stuff inon_after_startup
will significantly slow down the startup time of the server. https://github.com/brettvitaz/OctoPrint-Neopixel_Illumination/blob/b4b9950c8f439870d5ab4dc54be4ebfabefe33c1/octoprint_neopixel_illumination/__init__.py#L208 - The assumption of the log path here will fall apart if people have OctoPrint installed anywhere that has a different username or specifies a different basedir. They may even have specified a different basedir. https://github.com/brettvitaz/OctoPrint-Neopixel_Illumination/blob/b4158d8bc206aacf67dd12c18a55750dc29fe208/octoprint_neopixel_illumination/sock_api.py#L9-L12 - I pass the path through from OctoPrint so it is always set correctly like this using
self._settings.get_plugin_logfile_path
. That returns the path like<basedir>/logs/plugin_<plugin id>.log
. If the path doesn't exist, log files should not end with a path of./
- that could be anywhere on the system, depending on how OctoPrint is being run.
It may seem like we're picky, but it is mostly correcting the assumptions so that people don't come complaining when things go wrong!
It is possible to run WS281x type LEDs without root permissions, using the SPI pins. That's how my plugin WS281x LED Status does things. I've spent a long time working with these kinds of LEDs and OctoPrint, so if you have any questions do let me know and we can get stuff sorted.
And finally, a preview of what it will look like https://cp2004-plugins-octoprint-org-git-pr-1073-cp2004.vercel.app/plugins/neopixel_illumination/ |
Thank you for that thorough code review. I will start looking into these items, and will review the documentation you have created for your own plugin. Regarding the SPI pins, one of my Raspberry Pi's SPI pins are non-functional and I did not want to limit users when other options were open. I'm not sure what do to about that, and it is one of the reasons for all of the extra work to create the remote service for controlling the neopixels. Maybe I'll just have to come back to that when a better solution presents itself. |
- remove unsupported OS - add borders to table
Stale - Closing |
Use NeoPixel Illumination to control NeoPixels from the OctoPrint interface using the Raspberry Pi's GPIO. Parses GCODE M150 commands for automated control while printing.