Skip to content
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

Added plugin portretry #1325

Merged
merged 3 commits into from
Sep 15, 2024
Merged

Added plugin portretry #1325

merged 3 commits into from
Sep 15, 2024

Conversation

vehystrix
Copy link
Contributor

Added the portretry plugin

See https://github.com/vehystrix/OctoPrint-PortRetry

Copy link
Contributor

@jneilliii jneilliii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version of OctoPrint is in the future?

The user field listed here should match your GitHub username, case sensitive so should be vehystrix.

I'd also point out that there is a RepeatedTimer bundled in OctoPrint that you can utilize. You can see the docs here. An example of its usage can be seen here.

Looking at your code it would probably benefit from switching to that one, or you need to add the daemon property to your timer set to True or it can cause issues related to restart or shutdown of OctoPrint.

Other than the above comments, great implementation. I wonder if it might benefit from using the EventHandlerPlugin mixin to start/cancel the timer as well for PrinterStateChanged event?


Should also work to solve the issue with Prusa printers that don't remove the serial port from the system when powering down

Caveats: only tested on OctoPrint 1.10.4, python 3 and on linux
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OctoPrint 1.10.2 I assume as that is latest version?

@vehystrix
Copy link
Contributor Author

I changed the plugin to use the bundled RepeatedTimer. I also now run the timer only when the printer is disconnected, using the EventHandlerPlugin.
I also changed the case of the github username to all lowercase.

I did indeed mean version 1.10.2 not 1.10.4

@jneilliii
Copy link
Contributor

I'm not sure your event handler approach is the best. For one, match is only available in python 3.10 I think, this should be changed to if/elif logic and you don't need to call the octoprint.plugin.EventHandlerPlugin.on_event(self, event, payload) function . Something like this should be sufficient to accomplish your needs and be compatible with older python versions, which there are a lot of: https://data.octoprint.org/#python.

	def on_event(self, event: str, payload: dict):
		if event == 'Connected':
			self._logger.info('Printer connected, stopping timer')
			self.__stop_timer()
		elif event == 'Disconnected':
			self._logger.info('Printer disconnected, starting timer')
			self.__start_timer()

@vehystrix
Copy link
Contributor Author

I changed the on_event function to use an if/elif construct, and also removed the superclass on_event call. Anything else I missed?

@jneilliii
Copy link
Contributor

jneilliii commented Sep 14, 2024

So I should have noticed this before, and sorry I didn't. The serial.Serial call should probably include a baud rate.

I was also curious, instead of using your own internal setting you could reference the one saved in OctoPrint's connection panel using self._settings.global_get(["serial", "port"]) and self._settings.global_get_int(["serial", "baudrate"]). You would have to document that the setting needs to be saved in the connection panel, and account for AUTO which is an unset value.

@vehystrix
Copy link
Contributor Author

Accounting for AUTO baudrate would probably just mean I use the default value of 9600 like I do now. For my printer this works even tough it auto connects with a baudrate of 115200 (I just need to detect if the port is open, baudrate doesn't seem to matter for my printer for this detection)
Accounting for an AUTO port is probably not going to work. The point of this is to keep polling the same port every time, just in case the printer opens it up again, with AUTO set I don't know what port I would need to poll.

If you know how I would have to handle the AUTO port issue, I'm happy to add it, for now I'll just add the baudrate in there

@jneilliii
Copy link
Contributor

So if the connection panel is configured for AUTO the global_get call will return either None or AUTO, so something like this would default the value.

port = "/dev/ttyUSB0" if self._settings.global_get(["serial", "port"]) in [None, "AUTO"] else self._settings.global_get(["serial", "port"])

But I think the better approach would be to add a condition and on_finish callback to the RepeatedTimer function.

	def __init__(self):
		super().__init__()
		self._timer = None
	def __timer_condition(self):
		if self._settings.global_get(["serial", "port"]) in [None, "AUTO"] or self._printer.is_closed_or_error() is False:
			return False
		return True
	def __timer_cancelled(self):
		self._timer = None
	def __create_timer(self):
		if self._timer is None:
			self._timer = RepeatedTimer(self.__get_interval(), self.do_auto_connect, condition=self.__timer_condition, on_finish=self.__timer_cancelled)
	def __start_timer(self):
		self.__create_timer()
		self._timer.start()
	def __stop_timer(self):
		if self._timer:
			self._timer.cancel()

	def on_event(self, event: str, payload: dict):
		if event == "Connected":
			self.__stop_timer()
		elif event == "Disconnected":
			self.__start_timer()

	def on_after_startup(self):
		self.__start_timer()

	def on_shutdown(self):
		self.__stop_timer()

basically when the repeated timer runs if the return value of the __timer_condition function is False the do_auto_connect function will not be run. then the __timer_cancelled function will reset self._timer to None.

@vehystrix
Copy link
Contributor Author

Updated the code along the lines of what you posted
Also added to the documentation that the port must be specified in the global settings for the plugin to work. I tested it and when set to AUTO the printer won't reconnect, but when set to the correct port in the global settings, it works. First time after changing from AUTO to a specific port requires a manual connect to actually trigger the timer update

@jneilliii
Copy link
Contributor

First time after changing from AUTO to a specific port requires a manual connect to actually trigger the timer update

Yeah, this initial connection is required for the "save settings" checkbox to actually save the setting.

@jneilliii jneilliii merged commit cede3fc into OctoPrint:gh-pages Sep 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants