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

Modified to reset and close the Stream Deck on panic or termination signal #57

Merged
merged 3 commits into from
Jan 21, 2022

Conversation

henryso
Copy link
Contributor

@henryso henryso commented Jan 18, 2022

  1. I deferred a reset/close of the Stream Deck on return from main
  2. I changed the os.Exit calls in the fatal and fatalf functions to panic so that the defer from above is called
  3. I added signal handling to eventLoop to catch Control-C and kill -15 so that the defer from above is called

main.go Outdated
@@ -154,19 +171,22 @@ func initDevice() (*streamdeck.Device, error) {
}
ver, err := dev.FirmwareVersion()
if err != nil {
closeDevice(&dev)
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we shouldn't just try to reset the device once after opening it? After closeDevice we're terminating deckmaster anyway, as we return an error below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resetting the device puts it back (at least visually) to it's power-on state, with the Elgato logo. If we don't reset before closing it, The screen is left showing what it had before the close, which might have stuff like a frozen clock or CPU/Memory widget.

Copy link
Owner

Choose a reason for hiding this comment

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

That's why I suggest resetting it once after opening and removing the closeDevice calls except the deferred one in line 204. This should have the same effect then: if we encounter any error during initialization (lines 174, 181, 189) we would have already reset the device and can just return the error.

Copy link
Contributor Author

@henryso henryso Jan 18, 2022

Choose a reason for hiding this comment

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

The problem here is that what needs to be done is a close of the device (dev.Close()) more than a reset of the device. If dev.Close() is not called, then the Stream Deck will need to be unplugged and replugged. In order for this to work, I initDevice() will need to return dev (rather than nil) along with the error so that main can close the device.

The code would then look like

dev, err := initDevice()
if dev != nil {
   defer closeDevice(dev)
}
if err != nil {
    fatal(err)
}

Sound OK?

Copy link
Owner

Choose a reason for hiding this comment

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

Understood, but instead of closing the device and then shutting down again, maybe we could just try to close the device and re-open it inside Deckmaster, if we encounter an error during initialization? That would prevent the user from having to re-plug the device or restart Deckmaster. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might not be what you're getting at, but in my case, when the device is in a bad state (pretty much when dev.Close() is not called), it stays in that bad state until I re-plug the device.

In using deckmaster so far (which is only for a few days), those initialization operations (get firmware version, reset, set brightness) have never failed, but that doesn't mean they won't ever fail. What I'm trying to say is that I probably don't care too much how this is handled here, but at least we should dev.Close() so that there's less chance of needed a re-plug at this point.

As for a retry of initialization, I suppose that's possible, but I personally don't think it's necessary. At initialization time, it would be pretty obvious if things are not working, and the user could start troubleshooting. If it's hardware failure, we probably don't want to keep retrying "known good" things that are suddenly failing.

I've pushed the change modeled on what I said above, but I'll change it to whatever you feel is best here since I don't feel too strongly about how things fail at initialization time.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you, starting to understand the situation a little better now!

@henryso
Copy link
Contributor Author

henryso commented Jan 18, 2022

Let me know if that's not what you want.

@henryso henryso requested a review from muesli January 18, 2022 16:13
@henryso
Copy link
Contributor Author

henryso commented Jan 19, 2022

@muesli Is there something else you would like me to do for this PR?

@muesli
Copy link
Owner

muesli commented Jan 19, 2022

@muesli Is there something else you would like me to do for this PR?

All good, I'm just trying to improve the situation a bit, introducing a generic shutdown chan so we can make use of defers in main without having to rely on panic. Will probably push a few changes to your branch soon!

@muesli muesli merged commit 9bf033d into muesli:master Jan 21, 2022
@muesli
Copy link
Owner

muesli commented Jan 21, 2022

Ok, added on top of your changes and cleaned up the error handling a bit. Thank you for your contributions!

@henryso henryso deleted the split branch January 21, 2022 01:50
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.

2 participants