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.

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

When a device reports no targetValue, should targetValue be set to currentValue? #895

Closed
AlCalzone opened this issue Jul 2, 2020 · 14 comments
Assignees
Labels
bug Something isn't working investigate 🔎 Not sure what's going on here - need to investigate

Comments

@AlCalzone
Copy link
Member

AlCalzone commented Jul 2, 2020

Technical background (Relevant for all Switch type CCs):
Newer devices have the ability to report a targetValue that is different from the currentValue. E.g. for a multi-second dimming operation (or shutters). This way you can see where the value is going, where it currently is and how long it will take
You don't have that option when blindly merging current and target value.


We could think about how to make this smarter but its not trivial:

a) Old CC version, not distinguishing between target and current

  • Could merge them into value

b) New CC version, reporting current but not target version

  • Merge? How can we be sure that the device does not support different target/current?
  • Overwrite target with current? Might lose information about the target value this way

c) New CC version, reporting both

  • Keep both

The current behavior (setting targetValue to undefined when a node does not report it) however causes this:
#1090 (comment)

@AlCalzone

This comment has been minimized.

@AlCalzone AlCalzone changed the title When the interview reports no targetValue, should it be set to currentValue? When a device reports no targetValue, should targetValue be set to currentValue? Nov 22, 2020
@AlCalzone AlCalzone reopened this Nov 22, 2020
@robertsLando
Copy link
Member

IMO the correct behavior is to set the target to current when it doesn't automatically report it

@darkbasic
Copy link
Contributor

But how do we know if it doesn't automatically report it?

@AlCalzone
Copy link
Member Author

Depending on the CC version the device supports

@darkbasic
Copy link
Contributor

I though that my Qubino had the new CC version but yet it still didn't report the target.

@AlCalzone
Copy link
Member Author

I didn't take a look at your logs yet but it is entirely possible that it only reports the current value although it supports reporting both.

@robertsLando
Copy link
Member

@AlCalzone How does ozw handles this? Anyway I'm +1 for the solution you proposed here

@AlCalzone
Copy link
Member Author

They update the target value if it is included in the report:
https://github.com/OpenZWave/open-zwave/blob/master/cpp/src/command_classes/SwitchBinary.cpp#L116-L124

I have a feeling that exposing targetValue as undefined isn't even what I intended to happen. I'll have to check it again.

@robertsLando
Copy link
Member

I have a feeling that exposing targetValue as undefined isn't even what I intended to happen. I'll have to check it again.

I think exposing it as undefined is never a good option, when it's undefined it should be set to the current always

@AlCalzone AlCalzone added bug Something isn't working and removed investigate 🔎 Not sure what's going on here - need to investigate labels Nov 23, 2020
@AlCalzone
Copy link
Member Author

AlCalzone commented Nov 23, 2020

The current behavior is not working as intended (was broken by #1103, will be fixed by #1139).

This leaves the synchronization of current and target on old CC versions (a). I don't think (b) is feasibly because this is possible:

--> set targetValue (*)
    node starts a transition
<-- node reports intermediate currentValue, no targetValue
<-- node reports another intermediate currentValue, no targetValue
<-- node reports final value (currentValue equals targetValue from *)

If we used currentValue as the targetValue in any of the reports, it would be wrong, because it is not the targetValue.

@AlCalzone AlCalzone added the investigate 🔎 Not sure what's going on here - need to investigate label Nov 23, 2020
@darkbasic
Copy link
Contributor

@AlCalzone did #1139 end up in 5.5.0?

@AlCalzone
Copy link
Member Author

It did

@kpine
Copy link
Contributor

kpine commented Jan 12, 2021

I did not follow the entire conversion, but the current state of this is confusing, could you help clarify?

I have a Multilevel V2 Switch, which has no target value support. Here's what I've observed:

  • A targetValue with read/write access is exposed to the application. The targetValue is used for a Set, but has no relevance for reporting values
  • A currentValue with read access is exposed to the application. This is the only value I can currently use for the dimmer state.

How should an application (namely HA), which should support all versions of switches, deal with these values? Currently HA uses the targetValue to report brightness. My switch currently shows a brightness of 100% because it's converting 255 to max brightness. In reality, the 255 was the toggle command from the Set.

It cannot simply check for the existence of targetValue, because it seems to always be defined, but invalid for V2 switches.

Should it check the version of the CC to determine if it should look at targetValue? Applications should be insulated from knowing these details.

Can the targetValue be set to write-only for < V4 switches? An application could check if it's readable, if not use the currentValue.

Should it, for now, always use currentValue? Ideally, the application would want to set targetValue if it's available to avoid incorrect state.

Some of these questions would be answered based on how this bug is ultimately fixed. But for now, what would be the right guidance?

@AlCalzone
Copy link
Member Author

AlCalzone commented Jan 12, 2021

If you want to report a state, use currentValue. targetValue is used to control the device.
If a device has support for it, targetValue can be used to advertise which target a device is transitioning to, e.g. after physically switching a roller shutter.

Can the targetValue be set to write-only for < V4 switches?

That would be ideal, but currently it is not possible due to how value IDs are defined in zwave-js. It is on the agenda though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigate 🔎 Not sure what's going on here - need to investigate
Projects
None yet
Development

No branches or pull requests

4 participants