Set node value to -1 when ISY reports it as unknown (blank string)

[OverloadUT]

There is a very distinct difference between blank values and 0 values as reported by the ISY.

Blank values are used when the ISY does not know the status of a node, such as in cases where a battery-powered sensor has not reported a state since the ISY last booted up. The current implementation would cause leak sensors, for example, to report as WET (the dry node would be 0).

This change makes the value be -1 to hopefully reduce the magnitude of breaking change that this could be, while still allowing consuming applications to treat UNKNOWN values as different than OFF. (I didn't want to go so far as to make the Value sometimes be a non-integer.) Still, I would recommend a minor version number update to indicate the potential breaking change.

[rmkraus]

I understand the incentive for this change. I do think, though, if we are going to change the behavior, it should be changed to something that is the most correct. How will Home Assistant behave if a value of None is returned? I think returning None rather than -1 would be preferable. I can check this with Home Assistant now.

[rmkraus]

This line would cause problems if the value was ' 5'. I'm not sure if that every happens, but it feels plausible. I really don't like any of the original logic on these lines. What if we did something like this:

val = val.strip()
if val == "":
    val = -1
val = int(val)

[OverloadUT]

Ah ha, that didn't occur to me. I wasn't exactly sure what that original logic was trying to catch in the first place I couldn't get that condition to occur from my actual ISY responses. Stripping is the much better way to go; I'll change it to that.

[OverloadUT]

I can certainly make sure Home Assistant handles a None value. I agree that it would be more correct!

I've already got the ISY994 component's guts all splayed out to implement the changes that depend on these library updates, so I can roll any necessary changes in there.

[rmkraus]

Upon further investigation, VarEvents doesn't like that value changing types.

I do have concerns about nodes that will return integers as their value. There could possible be a node that returns positive and negative values which will make this logic equally ambiguous.

You could set the value, when unknown, to:

-1 * float("inf")

The other interesting thing about changing this logic is that Home Assistant will recognize unknown values as "ON" rather than "OFF". This completely changes the behavior in Home Assistant. I'm not sure how I feel about that. Do you plan on updating the code in Home Assistant to support this change?

[rmkraus]

Better question, what do you plan on having HASS do when a value is unkown? I could be wrong, but I don't think it currently has any mechanism for handling that case.

[OverloadUT]

Home Assistant has a STATE_UNKNOWN value that is used for unknown states, which is what I'm doing in my sensor overhaul changes :)

I will definitely update the other pieces to support however we decide to represent unknown here. That way the only behavior change will be some entities appearing as STATE_UNKNOWN, which should only be more desirable than an OFF state in those cases.

[rmkraus]

Sounds good to me. Let's go with -inf as that should be unusual enough to be safe. I'll then merge an publish the change with PyISY. Once I have done that, you can update the PyISY version dependency in Home Assistant.

[OverloadUT]

Excellent; I expect to implement all of the changes tonight!

[rmkraus]

Awesome. I’ll try not to wait 5 years to review your changes again. No promises though.

On Tue, Nov 28, 2017 at 20:57 Greg Laabs ***@***.***> wrote: Excellent; I expect to implement all of the changes tonight! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEuhGpOag9mdDK4pVCUnI-L_z1iIv_PIks5s7LnvgaJpZM4QqrT8> .

-- Thanks, Ryan Kraus

[JudgeDreddKLC]

Just wanted to thank you both for all the work! Kent L. Calero Sent from the field through my iPhone. On Nov 28, 2017, at 9:02 PM, Ryan Kraus <notifications@github.com<mailto:notifications@github.com>> wrote: Awesome. I’ll try not to wait 5 years to review your changes again. No promises though.

On Tue, Nov 28, 2017 at 20:57 Greg Laabs ***@***.******@***.***>> wrote: Excellent; I expect to implement all of the changes tonight! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEuhGpOag9mdDK4pVCUnI-L_z1iIv_PIks5s7LnvgaJpZM4QqrT8> .

-- Thanks, Ryan Kraus — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#38 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFqEWoG_dHbS_qshzGhTCfmj4p_hDHEvks5s7LsagaJpZM4QqrT8>.

[OverloadUT]

Change made! It now uses -inf as the unknown state. I'll make the changes so this is supposed in all entity types as part of home-assistant/core#10664 (which will be the PR that bumps the PyISY version requirement.)

[OverloadUT]

@rmkraus: With my latest commits to home-assistant/core#10664, Home Assistant is ready for this change. All device types will handle -inf and show an UNKNOWN state!

(Just need to update the REQUIREMENTS line to whatever the new version number will be)

[rmkraus]

👍