andig / pimatic-fritz

Pimatic plugin for Fritz!Box SmartHome and FritzDect!200 Node
GNU General Public License v2.0
8 stars 6 forks source link

Switch turns to new state without error if DECT 200 is unplugged #17

Closed mwittig closed 8 years ago

mwittig commented 8 years ago

If DECT 200 is unplugged and it is switch in pimatic no error message is given.

@andig said on issue #15 : Wondering if it should be an error or just be updated back to its previous state.

@sweetpi may correct me if I am wrong: I think it should be rejected with an error message string. This way, the error message string gets displayed in an error dialog box. This is behaviour implemented by pimatic-sispmctl for example.

andig commented 8 years ago

Could you check if this works for you:

# Retuns a promise that is fulfilled when done.
changeStateTo: (desiredState) ->
  @plugin.fritzCall((if desiredState then "setSwitchOn" else "setSwitchOff"), @config.ain)
    .then (actualState) =>
      @_setState(if actualState then on else off)
      return Promise.reject("Could not switch outlet") if actualState != desiredState
      Promise.resolve()
andig commented 8 years ago

Or rather, this would still be wrong. The two states could incidentally match. But checking device presence with each call also feels kinda overkill.

I'm leaning towards wontfix, maybe with a workaround of exposing the switches presence state?

mwittig commented 8 years ago

Well, this is new to me. I don't think we have such concept for switches right now. Shouln't "setSwitchOn" else "setSwitchOff" fail if it can't be switched?

I'm leaning towards wontfix,

Well, I am not sure about the previous behaviour. Need to test with an earlier version.

andig commented 8 years ago

Shouln't "setSwitchOn" else "setSwitchOff" fail if it can't be switched?

Mhm. Then I'd probably rather move this to the actual smartfritz library- same behaviour for all set... functions?

mwittig commented 8 years ago

Well, may be this can be done easier. Take the setswitchon method for example, see https://github.com/andig/smartfritz/blob/master/smartfritz.js#L230-L235 The inner block already tests if the result body contains 1. Couldn't this be used to reject the promise if false? I'll also do some testing to find out what will be returned in case. What I have in mind si something like the following (pseudo code):

return executeCommand(sid, 'setswitchon', ain, options).then(function(body) {
        return /^1/.test(body)?Promise.resolve(true):Promise.reject("failed to turn the switch on")
});

EDIT: Nice idea, but this won't work. As setswitchon will return 1 even if the switch is unplugged. Apparently, the same behaviour has been implemented by AVM in the Web UI. Even worse, the Web UI switch is blocked for a couple of seconds. Pretty dull, inmho.

andig commented 8 years ago

Could you check what getState returns after you setswitchon/off to a state that cannot be set due to missing device?

mwittig commented 8 years ago

Hi, I have added the following code as part of the @plugin.fritzCall? promise in the changeStateTo method. As a result, "newState" is always "false" if the plug is unplugged.

I have create a pull request for testing.

       .finally =>
          @plugin.fritzCall("getSwitchState", @config.ain)
            .then (newState) =>
              env.logger.debug "changeStateTo state", newState
            .catch (error) =>
              env.logger.debug "changeStateTo error", error
andig commented 8 years ago

@mwittig I have now added code that checks if new state equals desired state. PR no longer necessary?

mwittig commented 8 years ago

@andig The PR was for testing, only. You can reject it.

I'll test your master code asap.