-
-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Only accept valid hvac actions sent via mqtt #59919
Merged
emontnemery
merged 5 commits into
home-assistant:dev
from
Grennith:fix_climate_mqtt_receive_action
Nov 25, 2021
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
cb4ce57
Only accept valid hvac actions sent via mqtt
Grennith 7cf82e6
Only accept valid hvac actions sent via mqtt
Grennith 40665f9
Fix existing action test and remove old one
Grennith c354321
Remote None as valid hvac action being accepted
Grennith 0e68582
Change loglevel of wrong hvac action received
Grennith File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, it's not clear from the documentation that the action should be set to
None
.Is that allowed?
We probably also need a special payload which will be interpreted as
None
when templates are not used, if it's allowed to set the action toNone
. This is used in MQTT fan to reset the percentage toNone
: https://www.home-assistant.io/integrations/fan.mqtt/#payload_reset_percentageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically just added None there based on @bdraco's comment. Setting None via async_fire_mqtt_message doesn't work apparently(?).
Anyway, based on the doc, would it make sense to have the unit test run something along the lines of
assert all(elem in actions for elem in CURRENT_HVAC_ACTIONS)
in order to catch future edits of CURRENT_HVAC_ACTIONS?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible to receive
None
directly over MQTT, an empty MQTT payload will be interpreted as the empty string.If it's necessary to allow resetting the action to
None
, we should enable that functionality in the same way as for MQTT fan.If it's not necessary, drop the None-check.
Your idea for a test seems reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I am not sure None is a valid action that can be set given the description of HVAC action. I suppose None really just makes sense as the initial value to be interpreted as "unknown" - which no further update should result in? Unless a device is paired/reset again maybe.
I removed the None check for now given your remarks and added the assert all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity
The explicit
None
is a valid valuehvac_action
The string
"None"
is not a valid value forhvac_action
. (which is what was seen in the HomeKit PR)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, if we allow
None
as a HVAC action (or perhaps rather the absence of a HVAC action) , we should:"None"
.There's a somewhat related discussion about Fan, about no longer supporting
None
: #59688There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, https://www.home-assistant.io/integrations/climate.mqtt#action_topic currently only lists the 6 valid values checked in this PR.
Based on @bdraco's comment above, we could default to
None
(not"None"
) upon receiving an invalid value since there is some unknown state not handled by HA in place until further notice (similar to what the state within HA is upon startup, thus being a valid case?).However, any side-effects of doing so are sorta unknown I guess? A warning should be logged in that case imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
None
isn't actually supported now there is no need to add it. It would also be perfectly reasonable to reject invalid values instead of setting them toNone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how should we proceed? Take this PR as the bugfix that it is intended to be and not accept values not documented or extend it to allow some variation of
None
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think None is only valid as an initial state. There is no reason to reset it later e.g. if the device becomes unavailable. This is different from the MQTT fan where speed percentage and preset_mode state can become invalid, e.g. when a fan has one mode
auto
, and setting a fixed speed percentage will reset this mode.Further: The value template will return
None
if a value not could be parsed, we should not use this for other means. I think this PR is fine, but I would log a warning, instead of an error.