Skip to content

Fix handling None or empty value for numeric MQTT sensor#87004

Merged
jbouwh merged 4 commits into
home-assistant:devfrom
jbouwh:mqtt-sensor-ignore-empty-numeric-state
Feb 7, 2023
Merged

Fix handling None or empty value for numeric MQTT sensor#87004
jbouwh merged 4 commits into
home-assistant:devfrom
jbouwh:mqtt-sensor-ignore-empty-numeric-state

Conversation

@jbouwh
Copy link
Copy Markdown
Contributor

@jbouwh jbouwh commented Jan 31, 2023

Breaking change

The behavior for receiving value on sensors that expect numeric* values has changed.

  • From now on a 'None' value or a value rendered to 'None' will set such a sensor sensor to an unknown state.
  • Empty values on such sensor ('') are ignored and will not effect the state of the sensor.
    Integrations need to be corrected to send the correct values if an update is published and no valid update value is available for the sensor.
  • Other sensors that do not expect a numeric value will still accept an empty string as a value.

* Sensors expect numeric values if at least one of the following applies:

Proposed change

MQTT sensors can accept string or numeric values. When device_class is set a numeric value is expected. Many configurations use value_templates when getting a sensor value from a published JSON.
When a sensor has no value, and the device_class is set, this would give warnings in the log indicating an invalid non-numeric value was received.

This PR will change the behavior for MQTT sensors that have a device_class set and receive a value in two ways:

  • ignore when empty string value '' is returned from the value_template for a numeric sensor.
  • set a sensor to an unknown state when None as value us returned from the value template.

This is needed to allow maintainers get rid of warnings about invalid non-numeric in values in the logs asking people to open an issue.

example log for value rendering to an empty state:

Sensor sensor.power_dc has device class power, state class measurement and unit W thus indicating it has a numeric value; however, it has the non-numeric value: (<class 'str'>); Please update your configuration if your entity is manually configured, otherwise create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue+label%3A%22integration%3A+mqtt%22

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of mqtt can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign mqtt Removes the current integration label and assignees on the issue, add the integration domain after the command.

@jbouwh jbouwh changed the title Allow None for numeric MQTT sensor, ignore empty values Fix handling None or empty value for numeric MQTT sensor Jan 31, 2023
Comment thread homeassistant/components/mqtt/sensor.py Outdated
@epenet
Copy link
Copy Markdown
Contributor

epenet commented Jan 31, 2023

Note: it is a breaking change, but should it be considered for 2023.2 or are we too close to release?

@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented Jan 31, 2023

I think we should add this to 2023.2 since the alternative is a lot of warnings asking users to open issues.

@jbouwh jbouwh modified the milestone: 2023.2.0 Jan 31, 2023
@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Jan 31, 2023

@epenet We will not add this to 2023.2, that allows us up update the sensor base class first as you suggested. Will set this PR to draft for now.

@jbouwh jbouwh marked this pull request as draft January 31, 2023 09:32
@jbouwh jbouwh force-pushed the mqtt-sensor-ignore-empty-numeric-state branch from d112266 to e304566 Compare February 1, 2023 18:42
@jbouwh jbouwh marked this pull request as ready for review February 1, 2023 18:43
@jbouwh jbouwh requested a review from epenet February 1, 2023 18:43
@jbouwh jbouwh linked an issue Feb 2, 2023 that may be closed by this pull request
@jbouwh jbouwh force-pushed the mqtt-sensor-ignore-empty-numeric-state branch from 81f9af9 to 7220ec7 Compare February 6, 2023 10:16
Copy link
Copy Markdown
Member

@bieniu bieniu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, very useful change 👍

@jbouwh
Copy link
Copy Markdown
Contributor Author

jbouwh commented Feb 6, 2023

Would like to merge #87129 before this PR, and then update the docs PR to say that a numeric value is also expected if suggested_display_precision is set.

@jbouwh jbouwh force-pushed the mqtt-sensor-ignore-empty-numeric-state branch from 7220ec7 to 45908ec Compare February 7, 2023 08:00
Copy link
Copy Markdown
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @jbouwh 👍

@jbouwh jbouwh merged commit c78cae4 into home-assistant:dev Feb 7, 2023
@jbouwh jbouwh deleted the mqtt-sensor-ignore-empty-numeric-state branch February 7, 2023 10:23
Olen added a commit to Olen/homeassistant-plant that referenced this pull request Feb 7, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warning in HA 2023.2.0: has the non-numeric value: None

5 participants