Enable pingsToChange for pingCommands #243
Conversation
pingsToChange will now affect both pingCommands or normal pinging.
jackklink
changed the title
|
Could you describe the use case a bit? As you've probably noted in the code, I've worked from the assumption that we should trust ping commands. The plugin can't tell if the command is expensive to run or even if it's idempotent, so leaving it up to the user (basically a for loop or other means of requiring multiple values before changing), ensures that we don't get in the way of some uses of the feature. Edit: Perhaps I should've just read the issue first where I had already replied. Since the config is per device already it shouldn't be an issue either way. Sorry 😄 |
There was a problem hiding this comment.
Looks great, thank you for your time in gettings this work in! I just found a couple of minor things.
This is a breaking change, should we default to pingsToChange: 1 if we're using a ping command rather than regular ping? Or should the default be 5 either way if we believe that it would benefit most users? If so we'll bump the major version instead.
Another way is to simply add a pingCommandsToChange or something along those lines. That way it's a new feature that we can have defaulting to 1 so that it won't affect current users.
| @@ -216,6 +216,12 @@ | |||
| "functionBody": "return (model.pingCommand && model.pingCommand.length > 0)" | |||
| } | |||
| }, | |||
| { | |||
| "key": "pingsToChange", | |||
There was a problem hiding this comment.
Have you tried this with the UI? Since it's the same option as in the pinging section, will the value just change in both fields of the UI? Also, will changing either write to the same config?
There was a problem hiding this comment.
Also, near the top of this file, we'll need to update the description of pingCommand.
The number of pings necessary to trigger a state change, only used if the IP address is set.
| return; | ||
| this.history.push(isOnline); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Put the else and braces on the same line; } else {.
|
Thanks so much for following up on this! It has been so long since I did this, I can't remember exactly how I left things. I think pingCommandsToChange is a good idea, but also I do think this feature would benefit most users so up to you if you think making it work with pingsToChange is acceptable. I feel like it makes the plugin much more reliable and most would benefit. The UI is the one thing I wasn't sure how to update. I was going to leave that and other tweaks up to someone with a little more experience with Homebridge! Ha. Let me know if there's anything I can help with or if you want me to test a build. |
|
Wait as I look now, I did get it working in the UI with pingsToChange being set to appear in the UI for both normal pings and pingCommands |
pingsToChange will now affect both pingCommands or normal pinging.