luisiam / homebridge-cmdswitch2

CMD Plugin for HomeBridge (API 2.0): https://github.com/nfarina/homebridge
Apache License 2.0
176 stars 29 forks source link

Status error #5

Closed jdtsmith closed 7 years ago

jdtsmith commented 7 years ago

I notice that the plugin is currently checking for something on stdout to signify non-error or "on" state. Far more useful IMO would be to use the error state itself, since it derives from the return code for the command. Then, for example, one can check for a particular service running with systemctl status service. And so on. Very simple fix:

    var state = error? false: true; // stdout ? true : false;

Thanks for your work on this. Great for stopping and starting my shairport-sync service.

luisiam commented 7 years ago

I developed the state checking mechanism based on the use of grep. Not everything will return an error if the device is off.

jdtsmith commented 7 years ago

Thanks for getting back and for this plugin. Luckily grep and other tools all helpfully use their exit status to indicate a successful match; try:

(echo "device is on" | grep -q "device is off"); echo $?

So my suggested change would work fine for grep-based state checking, and also work for tools that don't produce anything to stdout, or produce output independent of the return result.

luisiam commented 7 years ago

Good idea. I will definitely take a look into that.

luisiam commented 7 years ago

I believe checking the return code will be more useful since the error state is not reliable based on my testing

var state = parseInt(stdout, 10) === 0 ? true : false;

jdtsmith commented 7 years ago

stdout isn't typically an integer though. Did you find cases where the error parameter didn't match the return code of the command?

luisiam commented 7 years ago

In my testing, the error parameter is always null

jdtsmith commented 7 years ago

Strange, I've made my suggested change and it's working great for a systemctl status call that reports info to stdout whether running or not. Are you certain the command you are trying doesn't always succeed?

luisiam commented 7 years ago

I am mainly using it on my PS4 and I updated the command to the following: ps4-waker search | grep -i '200Ok' >/dev/null; echo $? When it's on, stdout is 0 and error is null. When it's off, stdout is 1 and error is still null.

luisiam commented 7 years ago

BTW, the exit code will always be an integer. http://www.tldp.org/LDP/abs/html/exitcodes.html

ryanho87 commented 7 years ago

I think there may be a typo. I had to change ps4-waker search | grep -i '200Ok' >/dev/null; echo $? to ps4-waker search | grep -i '200 Ok' >/dev/null; echo $? for it to behave as expected.

jdtsmith commented 7 years ago

If you can send the error code to stdout, node should pick it up without doing so. Maybe you're not using error correctly?

On success, error will be null. On error, error will be an instance of Error. The error.code property will be the exit code of the child process while error.signal will be set to the signal that terminated the process. Any exit code other than 0 is considered to be an error.

So you can look for any non-null error. I haven't found a case where this doesn't work as advertised.

luisiam commented 7 years ago

For the grep function, it won't return an error status even if it doesn't find anything. So I cannot only check the error and determine the state.

jdtsmith commented 7 years ago

Strange. All grep's i interact with always do:

% echo "200 BAD" | grep "200 OK" ; echo $?
1
% echo "200 OK" | grep "200 OK" ; echo $?
200 OK
0

Tested on OS X and FC Linux.

luisiam commented 7 years ago

Based on my testing, grep will have exit code 1 if nothing is found and 0 if it is found. Your result actually matches with my observation.

jdtsmith commented 7 years ago

OK good. You see that 0 means success. That means that the Error object will be non-null only when nothing is found when you grep for the positive condition (like 200 OK). Which means you can test for the error and skip echo $?.

luisiam commented 7 years ago

That 0 or 1 comes from echo $? and it's in stdout, error is null in both cases

jdtsmith commented 7 years ago

Of course error is null if that last command you do is echo. In that case, you're looking at the return result of the echo command. Remove it, and you'll see that error returns to expected behavior.

luisiam commented 7 years ago

Pull request #3 mentioned that error is not reliable for some user. That's why we don't rely on error parameter

jdtsmith commented 7 years ago

If that's true — that error doesn't always respect the command's exit code as advertised — then that's an upstream bug in node's child_process.exec and should be fixed there. But my suspicion is people just got confused about the meaning of the error parameter, and that it does map perfectly well to $? as it has in my testing.