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

automatically set machine state to error upon disconnect of writer #1054

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

@tschulte
Copy link

@tschulte tschulte commented Mar 6, 2019

Most of the implementations of the run method of the ThreadedStenotypeBase
do not catch exceptions. This causes the thread to silently die (the
exception is only logged to the console).

Now an exception in the run method is reported as an error to the system
and additionally the SerialStenotypeBase class does close the com port on
error.

Otherwise the com port resource might still be taken by plover upon
reconnect of the writer, and a different com port might be taken by the OS
and plover might not be able to reconnect to that machine.

This is a first part to solving issue #596. In addition there must be some
mechanism to retry connecting when the state is error.

@tschulte
Copy link
Author

@tschulte tschulte commented Mar 13, 2019

I now created a (plugin https://github.com/tschulte/plover_auto_reconnect_machine) that does the reconnect. But that plugin relies on this pull request to be applied.

@benoit-pierre benoit-pierre added this to the post 4.0.0 milestone Apr 24, 2019
self.machine.run()
except Exception:
if not self.machine.finished.isSet():
log.error(self.machine.name + ' got disconnected.')
Copy link
Member

@morinted morinted Apr 5, 2020

Choose a reason for hiding this comment

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

Can we simplify the message to just "GeminiPr disconnected"?

Suggested change
log.error(self.machine.name + ' got disconnected.')
log.error('%s disconnected', self.machine.name)

Too bad there's no easy way to get the plugin name from what I can see.

Subclasses should override run.
"""
def __init__(self):
threading.Thread.__init__(self)
self.name += '-machine'
self.name = self.__class__.__name__ + '-machine'
Copy link
Member

@morinted morinted Apr 5, 2020

Choose a reason for hiding this comment

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

Suggested change
self.name = self.__class__.__name__ + '-machine'
self.name = self.__class__.__name__

Most of the implementations of the run method of the ThreadedStenotypeBase
do not catch exceptions. This causes the thread to silently die (the
exception is only logged to the console).

Now an exception in the run method is reported as an error to the system
and additionally the SerialStenotypeBase class does close the com port on
error.

Otherwise the com port resource might still be taken by plover upon
reconnect of the writer, and a different com port might be taken by the OS
and plover might not be able to reconnect to that machine.

This is a first part to solving issue openstenoproject#596. In addition there must be some
mechanism to retry connecting when the state is error.
@pianohacker
Copy link

@pianohacker pianohacker commented Dec 16, 2020

@tschulte Are you still interested in getting this in? Would really like this functionality.

Have pushed a branch with the suggested edits: https://github.com/pianohacker/plover/tree/issue-596-auto-reconnect-writer

@tschulte tschulte force-pushed the issue-596-auto-reconnect-writer branch from 2661006 to c4b8956 Jan 4, 2021
@tschulte
Copy link
Author

@tschulte tschulte commented Jan 4, 2021

@pianohacker yes, in principle I am interested in getting this in, although I currently don't have this issue any more, since my setup is different at the moment. Sorry that this pull request dropped of my mind.

As I recall, a problem might be, that these classes seem to be part of the API, or at least there are plugins that extend these classes. Therefore changing these might cause them to break. At least for example the treal machine is implemented as a plugin, and it does also implement the reconnect logic. But I think that would have only been an issue with my first attempt where I copied that reconnect logic from treal to ThreadedStenotypeBase. In the current sollution that should not be a problem. Even the treal machine automatically handling the reconnect itself should not be a problem.

I reset --hard my branch to your rebased branch and pushed it. Therefore this pull request now contains your changes.

@user202729
Copy link
Contributor

@user202729 user202729 commented Apr 15, 2021

Edit: it appears to be a kernel module/pyserial error. Open a new issue #1273.


Remark: with a recent operating system update, this has a weird behavior that I haven't seen before (tl;dr: it stopped working after sleep/wake up):

If I do this

  • Connect a machine
  • Sleep the computer
  • Wake it up

=> The machine stopped responding; however the "Machine disconnected" message is not shown until the USB is unplugged.

If I additionally do this

  • Unplug the USB
  • Plug it again before the "machine is disconnected" message is shown

Then the (linked) plugin will repeatedly try (and fail) to connect to the old port; while the machine is moved to a new port.

If I unplug the USB in the process, this message will be printed

Exception in thread Thread-4:                                                                                  
Traceback (most recent call last):
  File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.9/threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "/home/user202729/plover_auto_reconnect_machine/plover_auto_reconnect_machine.py", line 43, in run
    if isinstance(self._engine._machine, SerialStenotypeBase) and not self._port_exists(self._engine._machine.se:
  File "/home/user202729/plover_auto_reconnect_machine/plover_auto_reconnect_machine.py", line 65, in _port_exiss
    for port in list_ports.comports():
  File "/usr/lib/python3.9/site-packages/serial/tools/list_ports_linux.py", line 102, in comports
    for info in [SysFS(d) for d in devices]
  File "/usr/lib/python3.9/site-packages/serial/tools/list_ports_linux.py", line 102, in <listcomp>
    for info in [SysFS(d) for d in devices]
  File "/usr/lib/python3.9/site-packages/serial/tools/list_ports_linux.py", line 52, in __init__
    self.vid = int(self.read_line(self.usb_device_path, 'idVendor'), 16)
TypeError: int() can't convert non-string with explicit base

and the plugin stopped working completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants