Fix Hue motion sensor state if sensor is disabled #63000
Conversation
probot-home-assistant
bot
added
bugfix
integration: hue
small-pr
|
Hey there @balloob, mind taking a look at this pull request as it has been labeled with an integration ( |
| """Return true if the binary sensor is on.""" | ||
| return self.resource.motion.motion | ||
| return self.resource.motion.motion is True |
There was a problem hiding this comment.
So this is a bit of a weird one to review right now.
In the current stable release (2021.12), it should be a boolean that is returned.
However, on the current dev (2022.2), binary sensors actually can return a boolean or None value. In case it is unknown, it should be None.
So I think this is a correct fix for the current stable version, but incorrect behavior for the current dev branch.
Considering that, I would say the existing code is causing an unexpected behavior that is actually expected behavior in the next Home Assistant. My advice would be: Don't fix it; or else it would be a "bug" next month anyways.
There was a problem hiding this comment.
haha! In that case it was already working correctly.
In fact it is unknown because the sensor is disabled in Hue. Even Hue reports it as unknown due to the None value.
There was a problem hiding this comment.
In fact it is unknown because the sensor is disabled in Hue. Even Hue reports it as unknown due to the None value.
Hmm... so shouldn't that be marked as unavailable on the Home Assistant end in that case?
There was a problem hiding this comment.
Good suggestion but unavailable is used for connection state...
There was a problem hiding this comment.
So unavailable on entity/device level, when connecting to hubs, is generally a combined state.
If the Hub is unavailable/cannot be connected to, of course, the device and its entities won't be available either. However, if the device is not available on the hub, the device and entities can be marked unavailable as well, while other devices of the hub remain available.
There was a problem hiding this comment.
Not exactly as the other attributes and entities of the device still work. Unavailable is when the device itself is not reachable. At least that's how we implemented it in Hue ;-)
I've adjusted the code to specific check that enabled state, might be enough for now ?
There was a problem hiding this comment.
I don't agree with that as per above. Sorry.
I think it is really clear the device is disabled in Hue, thus not available for use in Home Assistant.
There was a problem hiding this comment.
In that case I think we should leave it as-is. Attaching the available state to the enabled property also doesn't feel right as users might go looking for connectivity issues, not remembering they disabled the sensor
There was a problem hiding this comment.
And as of Home Assistant 2022.2, Unknown when we don't know.
except that we do know, because their associated switch is turned-off, so it must be Clear (can never be 'Detected')
as said, that feels regression.... A frontend with all unknowns, even though a sensors parent switch is in a known state. And toggling it will fix it.
This has been working fine for years, and now an academic truth (unknown because no positive other state can be determined), while in practice every end user will know if their switch is turned on or off.
There was a problem hiding this comment.
I really don't agree with that perspective actually. There is no difference here compared to e.g., unplugging a bulb. Communication with the hub is possible, true, but communication with the end device has been cut off by the hub because it has been disabled there. The device is therefore not available.
| if not self.resource.enabled: | ||
| return False |
There was a problem hiding this comment.
This change induces the same issue. We don't know if it is false or not. For example, with a motion sensor, there could be motion? We don't know... as the device is disabled.
There was a problem hiding this comment.
On the contrary: the device is disabled, so there can never be any detected motion ;-) That is a 100% certainty, not an 'unknown'.
It must be 'off', because, well, it is switched off. And a binary that is off translates to 'Clear' for a device_class: motion.
There was a problem hiding this comment.
It is very specific for Hue where you can temporary disable the motion sensor (from controlling your lights) and its state is always False in that case. But agreed, strictly taken there can be motion while the sensor is disabled.
I'm tempted to just close this PR as its not a very big deal and Unknown is not that bad ;-)
There was a problem hiding this comment.
On the contrary: the device is disabled, so there can never be any detected motion ;-) That is a 100% certainty, not an 'unknown'.
That is the wrong viewing angle. The point is, the device looks available from HA and reports 'no motion'. That isn't correct in any way.
It must be 'off', because, well, it is switched off.
I think you are confused what binary sensor mean. Off in case of motion sensors mean: No motion. This has been documented.
There was a problem hiding this comment.
I'm tempted to just close this PR as its not a very big deal and Unknown is not that bad ;-)
I'm tempted to actually want to get this fixed, as it is broken in the current state and not correct IMHO. Closing this PR won't fix that either.
There was a problem hiding this comment.
I agree with @frenck here that the current working is not correct and should be fixed anyway.
I see 2 possibilities here:
- Force the return of None if the sensor is disabled (displayed as Unknown in the frontend)
- Mark the entity as unavailable if its disabled.
I have no preference, both are fine to me.
There was a problem hiding this comment.
Agreed. 1: would only be valid in current dev, 2: is preferred I think, but you raised a concern about third-party devices. So that would make 1 the middle ground.
There was a problem hiding this comment.
Adjusted the logic to force return None (unknown) when the sensor is set disabled.
Proposed change
If the Hue motion sensor is turned off, its value is None instead of a boolean.
This change explicitly checks for a boolean True for the motion sensor state.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: