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.

Sign up for GitHub

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
+59 −24
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3707a1
Modified to reset and close the Stream Deck on panic or termination s…
henryso Jan 18, 2022
3517556
Modified so there's only one deferred call to close the device.
henryso Jan 18, 2022
10fe905
Introduce shutdown channel to gracefully terminate the application
muesli Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"flag"
"fmt"
"os"
"os/signal"
"path/filepath"
"sync"
"syscall"
"time"

"github.com/bendahl/uinput"
Expand Down Expand Up @@ -34,12 +36,12 @@ const (

func fatal(v ...interface{}) {
fmt.Fprintln(os.Stderr, v...)
os.Exit(1)
panic(fmt.Sprintln(v...))
}

func fatalf(format string, a ...interface{}) {
fmt.Fprintf(os.Stderr, format, a...)
os.Exit(1)
panic(fmt.Sprintf(format, a...))
}

func expandPath(base, path string) (string, error) {
Expand All @@ -60,6 +62,9 @@ func expandPath(base, path string) (string, error) {
}

func eventLoop(dev *streamdeck.Device, tch chan interface{}) {
sigs := make(chan os.Signal, 1)
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)

var keyStates sync.Map
keyTimestamps := make(map[uint8]time.Time)

Expand Down Expand Up @@ -117,10 +122,22 @@ func eventLoop(dev *streamdeck.Device, tch chan interface{}) {
case ActiveWindowChangedEvent:
handleActiveWindowChanged(dev, event)
}

case <-sigs:
return
}
}
}

func closeDevice(dev *streamdeck.Device) {
if err := dev.Reset(); err != nil {
fmt.Fprintln(os.Stderr, "unable to reset Stream Deck")
}
if err := dev.Close(); err != nil {
fmt.Fprintln(os.Stderr, "unable to close Stream Deck")
}
}

func initDevice() (*streamdeck.Device, error) {
d, err := streamdeck.Devices()
if err != nil {
Expand Down Expand Up @@ -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 •
edited
Loading

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!

return nil, err
}
fmt.Printf("Found device with serial %s (%d buttons, firmware %s)\n",
dev.Serial, dev.Keys, ver)

if err := dev.Reset(); err != nil {
closeDevice(&dev)
return nil, err
}

if *brightness > 100 {
*brightness = 100
}
if err = dev.SetBrightness(uint8(*brightness)); err != nil {
closeDevice(&dev)
return nil, err
}

Expand All @@ -181,6 +201,7 @@ func main() {
if err != nil {
fatal(err)
}
defer closeDevice(dev)

// initialize dbus connection
dbusConn, err = dbus.SessionBus()
Expand Down