-
Notifications
You must be signed in to change notification settings - Fork 8
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
Notify on keydown #9
base: master
Are you sure you want to change the base?
Conversation
lutron-config.js
Outdated
node.telnet.end() | ||
.then(() => done()) | ||
.catch(() => this.telnet.destroy().then(() => done())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this run done()
a second time if the end()
succeeds and the first done()
throws an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, done shouldn't throw an exception since it's a callback provided by nodered. The catch is in case the end() throws an exception to ensure we still force the cleanup and call done().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't throw, but you don't control nodered's code. This would be safer and simpler to write as
node.telnet.end() | |
.then(() => done()) | |
.catch(() => this.telnet.destroy().then(() => done())); | |
node.telnet.end() | |
.catch(() => this.telnet.destroy()) | |
.then(() => done()); |
This way an exception from done()
won't cause it to repeat (or to call destroy()
), and you're not repeating the callback either.
Also, question: What happens if destroy()
throws an exception? Neither version of the code will call done()
in that case. Maybe it should? If it should, then my version would use .finally(() => done())
instead (or just .finally(done)
as finally
already passes no arguments).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, forgot finally was available with promises. Thanks, I'll update.
}); | ||
} else if (value === 4) { | ||
var m = ''; | ||
} else if (value === 3 || node.sendObj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely test || (value === 4 && node.sendObj)
. I know the only documented actions for pico remotes are 3 and 4 but that's no reason to make assumptions.
FullOn => a=2, p=3 | ||
up => a=5, p=3 | ||
down => a=6 p=3 | ||
off => a=4, p=3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing issue, but this is missing Button 2 (a=3
), and it's also missing the buttons for the PJ2-4B model (8
/9
/10
/11
), which is rather a problem as that's the model I have 😅
Adding ability to run actions based on keydown/keyup if specified, otherwise it retains the original behavior.