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
Replace Float 'nan' with None for modbus floats #93832
Conversation
Hey there @adamchengtkc, @janiversen, @vzahradnik, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
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.
Test cases are missing, to verify your changes work.
v_result.append(f"{float(v_temp):.{self._precision}f}") | ||
# NaN float detection replace with None | ||
# Issue: https://github.com/home-assistant/core/issues/93297 | ||
if v_temp != v_temp: |
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.
This looks wrong. this can never be true.
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.
floats that equal NaN are never equal to itself. The array temp variable, "v_temp" and further down the "val_result" are floats. I will provide testing evidence showing the case when NaN is received.
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.
you test if the variable if different from the same variable.
If you look at the failing tests, pylint also mark this as an error.
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.
Correct, that's one way of detecting NaN. See if it is equal to its self.
https://stackoverflow.com/questions/10034149/why-is-nan-not-equal-to-nan
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I had a read of https://developers.home-assistant.io/docs/development_testing .. and I'm out of my depth.. I don't know where to start with creating sufficient test records. Below is a test that shows that New code when register is NaN... New code returning val_result when float32 register is a normal number.. Additionally I see the pylint and mypy didn't pass above in the checks. The comparison to it's self is one way to detect NaN, but the the following line I don't know how to solve as the function is passing back a string list, where one of those float values COULD be a NaN... e.g. If unpack() returns a tuple greater than 1, and one of those registers is (0x7FC0)
If you can't send |
look in the test directory and e.g. add it to the existing sensor tests. The modbus integration is proud of having 100% test coverage (all code lines are passed at least once in a test run) and we want to keep it that way. |
Detect NaN and replace with 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.
LGTM (with the extras)
Proposed change
Detect NaN from modbus registers. Float 16/32/64 can return NaN 0x7FC0 which currently gets converted to a string, resulting in the core throwing an error as it's expecting a number. The PR detects 'nan' in unpack_structure_result, replacing with None
Type of change
Additional information
fixes Modbus - Exception since 2023.5.0: state class None: numeric value, however, it has the non-numeric value. #93297
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
.To help with the load of incoming pull requests: