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

Update machine state on unhandled exceptions #1560

Merged

Conversation

sol
Copy link
Contributor

@sol sol commented Sep 1, 2022

Summary of changes

When the machine thread dies due to an unhadled exception then indicate this to the engine (and by extension to the user) by setting the machine state to "disconnected".

This is a first step. Possible follow-ups:

  1. We may choose to handle serial.serialutil.SerialException more explicitly as a "disconnect" (say, not even print a stack trace to stderr).
  2. What I really want from a users perspective is that the machine re-connects automatically (I think it's feasible to do so, stay tuned for a follow-up PR).
  3. For SerialStenotypeBase specifically, we also want to close the serial port on unhandled exceptions.

Even if we do (1) and/or (2) later, I think we still want this PR as a fallback for arbitrary exceptions.

I will have a separate PR for (3), but again, I think this PR provides merit on its own. So let’s focus on this first.

Closes #1559.

Pull Request Checklist

  • Changes have tests
  • News fragment added in news.d. See documentation for details

@sol sol force-pushed the update-machine-state-on-disconnect branch from 17bbfcf to 2c44a27 Compare September 1, 2022 14:43
@sol sol marked this pull request as ready for review September 1, 2022 14:43
@user202729
Copy link
Member

Somewhat related to #1054, although I can't immediately tell if they're identical.

@sol
Copy link
Contributor Author

sol commented Sep 5, 2022

@user202729 thanks for taking the time to look at this.

Somewhat related to #1054, although I can't immediately tell if they're identical.

They are addressing the same underlying issue. One difference is that #1054 is technically a breaking change as it changes the base class of ThreadedStenotypeBase (see #1559).

@sammdot sammdot merged commit 8a8d360 into openstenoproject:main Sep 27, 2023
@sol sol deleted the update-machine-state-on-disconnect branch September 27, 2023 05:50
@sol
Copy link
Contributor Author

sol commented Sep 27, 2023

@sammdot thanks for merging this. I had more improvements in the pipeline, but the inertia of this project let me eventually give up on it, and on steno as a whole.

The thing is that if things are fundamentally broken, there have been bug reports for years and several PRs that address these, but they don't get merged, then from a user perspective that's not very attractive. I certainly was not up for maintaining my own fork indefinitely + upstream did not work for me.

@sol
Copy link
Contributor Author

sol commented Sep 27, 2023

Also, don't understand me wrong, I have patience + I completely understand if nobody has time to look at some thing. I myself suffer from RSI and I have not been able to keep up with PRs to my projects in the past. I even chose to ignore PRs that I considered not crucial, or that had severe issues, like no tests or other code issues.

I tried very hard to make it as easy as possible for an interested maintainer to merge this by keeping it as focused as possible.

It was only after I realized that people are actually looking at this, and still choose not to merge it, after I realize that other PRs get merged, but not this one, when I decide to cut my looses and move on.

@sammdot
Copy link
Member

sammdot commented Sep 27, 2023

I understand the frustration. 😞 It's not exactly easy not having enough bandwidth (especially after the two lead devs stepped down) to take care of this on top of all my existing commitments, and as much as I would have liked to contribute more, even the process to set up a dev environment was painful for me, and I only just got more permissions to review and merge stuff a few days ago.

I can't promise that I'll be especially active moving forward, but I'm doing my best in my current situation.

@sammdot
Copy link
Member

sammdot commented Sep 27, 2023

(For what it's worth, having had this project dropped on me officially picked this project up just recently, I didn't exactly have a proper system for triaging PRs; the main perceived difficulty with this one was deciding between this and #1054, and being unable to test either locally I had to rely on CI-built artifacts. This then broke the build which I had to patch in #1634, etc. It was a whole thing 🙃)

@sol
Copy link
Contributor Author

sol commented Sep 27, 2023

@sammdot to be clear, that criticism was not directed towards you.

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

Successfully merging this pull request may close these issues.

Unplugging a serial-over-USB steno machine leaves Plover in an undefined state
3 participants