Update Author for UlendoCaaS

[SamThomp]

Ulendo calibration as a service is a plugin designed to help users quickly calibrate their printers, and select the input shaping parameters that will work the best for their machine.

[jneilliii]

I haven't reviewed the actual plugin yet, but the first comments here will need to be addressed. Also, don't duplicate images in multiple folders, they should all only reside in a folder that is the same name as the id/plugin_identifier of your plugin (case matters).

[jneilliii]

@SamThomp please see comments, basically you still need to update the install url, and delete the extra UlendoCaas folder.

[jneilliii]

Oh, and in addition. If there is a pricing model it would be a good idea to include that. I can't seem to find anything related to how your service actually works outside of this plugin registration.

[jneilliii]

Software Update hook on your plugin is wrong.

https://github.com/S2AUlendo/UlendoCaaS/blob/80dbd33cb8f4db88ace0a11fde16dd85d07d6656/octoprint_ulendocaas/__init__.py#L839-L857

should be

    def get_update_information(self):
        return {
            "ulendocaas": {
                "displayName": "Ulendo Calibration Plugin",
                "displayVersion": self._plugin_version,

                # version check: github repository
                "type": "github_release",
                "user": "S2AUlendo",
                "repo": "UlendoCaaS",
                "current": self._plugin_version,

                # update method: pip
                "pip": "https://github.com/S2AUlendo/UlendoCaaS/archive/{target_version}.zip",
            }
        }

[jneilliii]

@foosel as this is a paid service I'd like to get your final review. I don't see anything left from my comments that need to be addressed.

[foosel]

On first look I'm seeing some major issues in the plugin code:

1. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/ulendo_autocal/lib/autocal_serviceabstraction.py#L110C1-L114C59 and https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/ulendo_autocal/lib/autocal_serviceabstraction.py#L158C1-L161C24 - you are dumping (temporary?) buffer files into a random folder in your plugin's source tree. This will most likely NOT work in installations in the field, and even if it does is not at all a clean solution to whatever you are trying to do here. OctoPrint provides a data folder for plugins to store data in, but you are also expected to keep that clean and now just dump debug information in there. From what I can see every single response from your service will be cached on disk and slowly fill up the installation folder of the plugin package with .txt files. You need to change that. Check whether you actually need to create a file on disk here in the first place (I see that you are reading these files in the main plugin class, however, I'm pretty sure this could be circumvented with some changes to actually use a (async?) function or callback that simply returns the results instead of pushing them onto the file system and then expecting some other thread to fetch them from there), check if a true temporary file is maybe the answer, and if not use the aforementioned official data folder and add a clean up job for it.
2. At the same location - error handling. File reads and writes can fail. As things are now, if for some reason (e.g. permissions) your txt files can't be written, you'll leave an open file descriptor dangling. Those are limited by the OS, so at some point a reboot will be needed to make things work again if the errors add up. That's obviously not good. I strongle recommend the use of the open context manager here instead.
3. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/static/js/ulendocaas.js#L572C1-L585C10 - Your wizard forces the user to agree with your privacy policy to get the wizard to close again. You are thus effectively blocking the user from accessing OctoPrint after installing your plugin, unless they fill in your credentials and agree to your privacy policy. That's not ok. Either get rid of the wizard altogether (it's not really needed here, the user can be expected to open the settings dialog and configure your plugin without getting forced to do so), or allow the user to exit it without entering anything, but this kind of workflow is a dark pattern that's not ok.
4. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L503-L506 & https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L1029-L1032 & https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L140C13-L140C85 - command line calls can fail, this needs error handling
5. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L121 - this has the same effect as just setting the initial FSM state to idle, but needs two runs through the while loop and a state transfer for that. I'd strongly recommend to save those CPU cycles.
6. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L140C13-L140C85 - why require a full blown shell just to read a file? with open("/proc/cpuinfo", "r"): ... (of course with error handling!)
7. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L157C1-L158C30 - should be self._printer, see injected properties.
8. https://github.com/S2AUlendo/UlendoCaaS/blob/841693ecc84cfed941c8675955c604203f73202d/octoprint_ulendocaas/__init__.py#L432-L441 - this looks like you might be happier with octoprint.util.RepeatedTimer instead, combined with its runtime conditions.

All in all, the code in general looks very fragile with regards to how it calls commands and reads files. There needs to be way more error resilience everywhere. Things should not go into an unhandled broken state that only a restart of OctoPrint and/or uninstall of the plugin can recover from if one of the many commands doesn't work out.

And communication between threads through txt files in a subfolder in the plugin installation folder is definitely the wrong way to do this.

Obviously, the dark patterned wizard issue is also a big problem.

[SamThomp]

Many thanks @foosel for your detailed response. We are working to address the issues that you mentioned using your recommendations. We will implement and test them. Thanks again for your patience with us on our first submission.

[ianku-ulendo]

Hello @foosel, we have made changes to our plugin repo based on your requests and recommendations. Please let us know if you have any more concerns or feedback. Thank you.

[SamThomp]

Hi @foosel, thanks again for your feedback. We have incorporated all of the major changes that you requested, including changes where the files are stored. Please let us know if this works, or what other changes might be required.. Can you take another look at the plugin?

[foosel]

Please understand that as the project leader and sole developer of OctoPrint I have a lot of responsibilities, so things like this can take a bit more than just a few days to get back around to. Repeatedly pinging me won't make it happen faster.

[foosel]

• You are still running a dark pattern on the wizard. "By completing this wizard, you are providing your consent that the data related to your machine and your credentials will be collected." Once your plugin is installed, the user can't do anything BUT completing the wizard requested by it. That's why wizards should only be used for data without the plugin cannot actually proceed. Instead of this approach, don't show a mandatory wizard and don't collect anything prior to the user explicitly configuring the plugin and agreeing to data processing.
• You are still leaking file descriptors in several places. Every call to open also needs to close the opened handle under any and all circumstances or you have a leak. The with context manager does this for you. I suggest you use it everywhere you access a file. Here's the locations I spotted, there might be more:
  • https://github.com/S2AUlendo/UlendoCaaS/blob/f4ac2f06f96282cc1e75dccbd33a71f48e3d3994/octoprint_ulendocaas/__init__.py#L202
  • https://github.com/S2AUlendo/UlendoCaaS/blob/f4ac2f06f96282cc1e75dccbd33a71f48e3d3994/octoprint_ulendocaas/__init__.py#L523
  • https://github.com/S2AUlendo/UlendoCaaS/blob/f4ac2f06f96282cc1e75dccbd33a71f48e3d3994/octoprint_ulendocaas/ulendo_autocal/lib/autocal_adxl345.py#L117-L119 (here you even leak three file descriptors pointing at the same file! Yes, there are calls to close further down, but no adequate error handling in between - use with open, it's a best practice for a reason)
• There's still no error handling in several places where external requests or hardware access happens. try: ... except: ... is your friend. E.g. (there might be more):
  • octoprint_ulendocaas/ulendo_autocal/lib/autocal_adxl345.py
  • octoprint_ulendocaas/ulendo_autocal/lib/autocal_serviceabstraction.py
• The sample collection and file writing in https://github.com/S2AUlendo/UlendoCaaS/blob/f4ac2f06f96282cc1e75dccbd33a71f48e3d3994/octoprint_ulendocaas/ulendo_autocal/lib/autocal_adxl345.py#L94 is extremely hack-ish. Create one file descriptor. Write to that. Opening a file three times and then appending to it from three consecutive code lines, each time using a different handle, that's exceptionally ugly. Especially if you then can't close them when an error arises and thus leak not one but even three descriptors.
• https://github.com/S2AUlendo/UlendoCaaS/blob/f4ac2f06f96282cc1e75dccbd33a71f48e3d3994/octoprint_ulendocaas/__init__.py#L522: This will hang for all eternity, in a busy loop (100% CPU use!) if something fails within your called script. It should have a timeout, and at the very least it should also have a time.sleep within the loop. Actually, on second look, given that your script is run in a blocking context, this whole loop strikes me as unnecessary - either the file exists when reaching this line, or it doesn't, no need to loop. As things are implemented now, this has a high risk of locking up the server. On a side note, you could solve all of this using real interprocess communication to retrieve the data from your sub process, instead of using this temporary file workaround. Check how OctoPrint's gcode interpreter is implemented for an example. In order to not block the full server loop while your script runs, this should also be running in its own thread.
• Is there a particular reason you insist on writing your tmp* files to a data folder within your plugin's data folder? That seems a bit redundant given that these tmp files are the only things you use your plugin's data folder for. You could save yourself the trouble of having to first create that data subfolder.
• Remove octoprint_ulendocaas/ulendo_autocal/data, it's no longer needed

Sadly, the code still is very fragile. You are still assuming that none of your external requests will ever fail, that all GPIO access will always run without hiccups, and that sub processes will never fail to run. Meanwhile, you are still heavily leaking file descriptors. Please look into error resilience, and make sure your plugin doesn't cause issues for the system running it. Additionally, get rid of the wizard and only do anything once the user has filled in their credentials and by that implicitly agreed to using your service.