Handle explicit Modbus NaN values #90800

joanwa · 2023-04-04T20:18:14Z

Proposed change

Add optional modbus config for explicit NaN values specified by the device vendor. Without this option, template sensore are required to remove the NaN values. See related issue #69656

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
[ x] 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

This PR fixes or closes issue:
fixes specify Modbus NaN values per sensor #69656
This PR is related to issue: specify Modbus NaN values per sensor #69656
Link to documentation pull request: Modbus nan values home-assistant.io#26865

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
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
[x ] Tests have been added to verify that the new code works.

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

[x ] Documentation added/updated for www.home-assistant.io

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:

I have reviewed two other open pull requests in this repository.

home-assistant · 2023-04-04T20:18:19Z

Hi @joanwa

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant · 2023-04-04T20:18:23Z

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

Code owner commands

Code owners of modbus can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign modbus Removes the current integration label and assignees on the pull request, add the integration domain after the command.

home-assistant · 2023-04-04T20:23:08Z

Hi @joanwa

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

joanwa · 2023-04-04T20:36:51Z

So what I'm unsure about is what the return value should be in case of NaN. Should it be 0 or 'unavailable' or none? In the PR I opted for 0, just because it is of the same return type.
For my current workaround with template sensors I use 'unavailable'. Might be the more logic choice.

There's also some discussions here:
https://community.home-assistant.io/t/difference-between-unknown-unavailable-none-and-null/430966/3

homeassistant/components/modbus/base_platform.py

home-assistant · 2023-04-04T21:12:44Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

janiversen · 2023-04-07T14:35:42Z

You have a CI problem, please solve that.

janiversen · 2023-04-07T14:36:18Z

Furthermore your PR is marked as draft, please mark it as ready for review when you want a formal review.

joanwa · 2023-04-07T18:36:21Z

You have a CI problem, please solve that.

yes will do

janiversen · 2023-04-08T11:52:20Z

Let’s see if your CI problem is solved. This PR is still marked as draft, so we cannot review/approve it.

joanwa · 2023-04-27T13:27:58Z

polite question, is something missing from my side for a revised review?

janiversen · 2023-04-27T14:35:58Z

A polite answer you have not answered my last review comments…and if I remember right your doc. PR also have issues (I might remember wrong ), it is marked as draft, so not ready for review.

joanwa · 2023-04-27T15:44:09Z

A polite answer you have not answered my last review comments…and if I remember right your doc. PR also have issues (I might remember wrong ), it is marked as draft, so not ready for review.

actually answered to all of them, incorporated some changes in the code. After a few days I clicked on 'resolve comment' since I thought this was part of the review process, but I think this just made your comments and my answers vanish :/

janiversen · 2023-04-27T16:03:36Z

The current status is the PR is "changes requested" so it is hard to know that they are solved.

janiversen · 2023-04-27T16:05:56Z

Just controlled there are 6 review comments without response. You might have solved all 6 in the commits you made, but how do you expect me to know that

joanwa · 2023-04-28T23:05:45Z

Just controlled there are 6 review comments without response. You might have solved all 6 in the commits you made, but how do you expect me to know that

I really don't know what to do now. I only see 2 requests for changes from you above, and I changed the code and responded to the latter. I definitely shouldn't have squashed the commits, therefore the original lines of code on which you commented is now gone. I'm eager to do whatever else needs to be done for this PR. It's my first one for HA in general.

janiversen · 2023-04-29T06:04:35Z

Plase look more carefully:

6 open review comments.

frenck · 2023-04-29T08:55:30Z

@janiversen Those comments in your review screenshots, have the pending badge:

Meaning you still need to submit them as a review. You are the only one that sees those.

../Frenck

joanwa · 2023-04-29T09:04:15Z

@janiversen Those comments in your review screenshots, have the pending badge:

Meaning you still need to submit them as a review. You are the only one that sees those.

../Frenck

thanks @frenck for spotting, @janiversen I just wanted to say I don't see any of them and was waiting for something to happen.

janiversen

sorry my fault.

janiversen · 2023-04-23T11:31:21Z

homeassistant/components/modbus/base_platform.py

@@ -105,6 +107,7 @@ def get_optional_numeric_config(config_name: str) -> int | float | None:

        self._min_value = get_optional_numeric_config(CONF_MIN_VALUE)
        self._max_value = get_optional_numeric_config(CONF_MAX_VALUE)
+        self._nan_value = entry.get(CONF_NAN_VALUE)


What happens if nan value is not present in the config (this line fails)

should work with a default None value from the input Schema validation in __init__.py. Forgot the default value there and will add with a commit. I could also do a proper hex string enforcement using voluptuous in the init. What do you think?

janiversen · 2023-04-23T11:33:20Z

homeassistant/components/modbus/base_platform.py

@@ -161,6 +164,10 @@ def __init__(self, hub: ModbusHub, config: dict) -> None:
        self._scale = config[CONF_SCALE]
        self._offset = config[CONF_OFFSET]
        self._count = config[CONF_COUNT]
+        if self._nan_value is not None:
+            self._nan_value = struct.unpack(
+                self._structure, bytes.fromhex(self._nan_value[2:])


According to the doc it is a hex string (without specific definition) but it seems the code assumes something special for the first 2 chars.

also you cannot use _structure, that will give wrong results or break when configuring a custom format.

why no simply do something like:
‘’’
x = int("deadbeef", 16)
‘’’
that works with 0x as well

1st comment
yes, the special 0x. Could do prettier things like a split after 0x, which is less performant. But if format is enforced with the preceding 0x, will this be ok?

2nd comment
this doesn't work, as x should have different values when using e.g. uint16 or int16, which again is set with the struct from _structure. int("deadbeef", 16) will always convert it to the same int value in base 16.

janiversen · 2023-04-23T11:34:30Z

homeassistant/components/modbus/base_platform.py

@@ -161,6 +164,10 @@ def __init__(self, hub: ModbusHub, config: dict) -> None:
        self._scale = config[CONF_SCALE]
        self._offset = config[CONF_OFFSET]
        self._count = config[CONF_COUNT]
+        if self._nan_value is not None:
+            self._nan_value = struct.unpack(


What happens if the user configures "jan was here" which are allowed since it is defined as a string.

See above comment, could do hex string enforcement using voluptuous in the __init__.py or also just preceding here with a regex check with proper error message is hex format is incorrect?

janiversen · 2023-04-23T11:37:20Z

homeassistant/components/modbus/base_platform.py

@@ -161,6 +164,10 @@ def __init__(self, hub: ModbusHub, config: dict) -> None:
        self._scale = config[CONF_SCALE]
        self._offset = config[CONF_OFFSET]
        self._count = config[CONF_COUNT]
+        if self._nan_value is not None:
+            self._nan_value = struct.unpack(
+                self._structure, bytes.fromhex(self._nan_value[2:])


_structure is optional, why use that to unpack your hex string?

yes, otherwise pythons int("hex_string") yields the integer representation of "hex_string"regardless of the modbus datatype, which can be uint16, int16, uint32, int32, float32,... and all as either little endian or big endian.
The "hex_string" NaN value has to be decoded in the same way as the actual incoming modbus sensor value is decoded.

see above this breaks with custom formats.

Couldn't find a good solution yet

janiversen · 2023-04-23T11:38:23Z

homeassistant/components/modbus/base_platform.py

+    def __process_raw_value(self, entry: float | int | str) -> float | int | str:
+        """Process value from sensor with NaN handling, scaling, offset, min/max etc."""
+        if self._nan_value and entry == self._nan_value:
+            return STATE_UNAVAILABLE


this should return a value not a state, this might have a number of side effects.

my initial version had return value 0, and was wondering if "unavailable" was better. For this, you suggested it should be "unavailable".
Or is your issue STATE_UNAVAILABLE vs. "unavailable"?

do you have any guidance here?

janiversen · 2023-04-23T11:41:45Z

tests/components/modbus/test_sensor.py

+        (
+            {
+                CONF_DATA_TYPE: DataType.INT32,
+                CONF_NAN_VALUE: "0x80000000",


You miss tests:

nan_value configured as "jan was here"

nan_value configured as a hex string (no 0x) allowed according to the doc

nan_value, configured but no match

nan_value match (check that raw_value is not broken).

In general you need to look at the coverage output and secure the lines you have changed are covered by tests.

janiversen · 2023-04-29T10:30:48Z

@frenck good catch, thanks.

janiversen · 2023-05-14T07:14:30Z

Any progress on this, there are a number of open review comments and it is out of date with the dev branch.

In case of no progress it will be closed at a later date.

joanwa · 2023-05-15T17:07:51Z

Any progress on this, there are a number of open review comments and it is out of date with the dev branch.

In case of no progress it will be closed at a later date.

will commit updated unittests, and the things where I know what to do. Still some open questions, see above

janiversen

Added extra to make it work.

janiversen

LGTM (with the extras).

joanwa · 2023-08-07T02:37:45Z

homeassistant/components/modbus/validators.py

thanks for picking this up, but I think this has issues when the modbus configuration variable swap is used with swapped order of bytes/words. This was my motivation to use the above struct.unpack() with the same self._structure in homeassistant/components/modbus/base_platform.py used to encode the actual incoming modbus values. Since you mentioned this is not a good idea, I didn't how to proceed.

MartinHjelmare · 2023-08-21T20:54:54Z

homeassistant/components/modbus/base_platform.py

@@ -225,6 +230,10 @@ def unpack_structure_result(self, registers: list[int]) -> str | None:
        # the conversion only when it's absolutely necessary.
        if isinstance(val_result, int) and self._precision == 0:
            return str(val_result)
+        if isinstance(val_result, str):
+            if val_result == "nan":
+                val_result = STATE_UNAVAILABLE  # pragma: no cover


STATE_UNAVAILABLE should never be set directly as state. We use the entity property available set to False to set an entity state as unavailable.

@MartinHjelmare
Something is not right, when doing it I get:

File "/Users/jan/repos/core/homeassistant/components/modbus/base_platform.py", line 193, in __process_raw_value self.available = False ^^^^^^^^^^^^^^ AttributeError: property 'available' of 'ModbusRegisterSensor' object has no setter

It's a property, so it can't be set as an attribute. It needs to return a value. You can use self._attr_available if you want to set the value directly.

https://developers.home-assistant.io/docs/core/entity#property-implementation

janiversen · 2023-08-21T21:43:03Z

Ok will change it in a followup PR.

joanwa requested a review from janiversen as a code owner April 4, 2023 20:18

home-assistant bot added cla-needed has-tests integration: modbus new-feature small-pr PRs with less than 30 lines. labels Apr 4, 2023

home-assistant bot assigned janiversen Apr 4, 2023

home-assistant bot added the Quality Scale: gold label Apr 4, 2023

joanwa force-pushed the modbus-nan-values branch from 9c3d48e to d7efd54 Compare April 4, 2023 20:23

joanwa mentioned this pull request Apr 4, 2023

Modbus nan values home-assistant/home-assistant.io#26865

Merged

6 tasks

home-assistant bot added cla-recheck cla-signed and removed cla-recheck cla-needed labels Apr 4, 2023

janiversen requested changes Apr 4, 2023

View reviewed changes

homeassistant/components/modbus/base_platform.py Outdated Show resolved Hide resolved

home-assistant bot marked this pull request as draft April 4, 2023 21:12

MartinHjelmare changed the title ~~handle explicit Modbus NaN values~~ Handle explicit Modbus NaN values Apr 4, 2023

joanwa force-pushed the modbus-nan-values branch from a739f18 to 62ab9cd Compare April 8, 2023 20:08

joanwa force-pushed the modbus-nan-values branch 3 times, most recently from dbd35e3 to efce9bd Compare April 17, 2023 06:58

janiversen requested changes Apr 29, 2023

View reviewed changes

home-assistant bot marked this pull request as draft April 29, 2023 10:29

janiversen approved these changes Aug 6, 2023

View reviewed changes

janiversen force-pushed the modbus-nan-values branch from f89b2a3 to 8a2ede2 Compare August 6, 2023 11:33

joanwa and others added 2 commits August 6, 2023 13:34

Implement explicit modbus NaN value handling

6f1176a

Extras

b71212d

janiversen force-pushed the modbus-nan-values branch from 8a2ede2 to b71212d Compare August 6, 2023 11:34

janiversen marked this pull request as ready for review August 6, 2023 11:35

home-assistant bot requested a review from janiversen August 6, 2023 11:35

janiversen approved these changes Aug 6, 2023

View reviewed changes

coverage.

c2c5685

janiversen merged commit c4a5373 into home-assistant:dev Aug 6, 2023
21 checks passed

joanwa commented Aug 7, 2023

View reviewed changes

github-actions bot locked and limited conversation to collaborators Aug 8, 2023

MartinHjelmare reviewed Aug 21, 2023

View reviewed changes

home-assistant unlocked this conversation Aug 21, 2023

github-actions bot locked and limited conversation to collaborators Aug 22, 2023

Handle explicit Modbus NaN values #90800

Handle explicit Modbus NaN values #90800

Conversation

joanwa commented Apr 4, 2023 • edited by janiversen

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 4, 2023

home-assistant bot commented Apr 4, 2023

home-assistant bot commented Apr 4, 2023

joanwa commented Apr 4, 2023

home-assistant bot commented Apr 4, 2023

janiversen commented Apr 7, 2023

janiversen commented Apr 7, 2023

joanwa commented Apr 7, 2023

janiversen commented Apr 8, 2023

joanwa commented Apr 27, 2023

janiversen commented Apr 27, 2023 • edited

joanwa commented Apr 27, 2023

janiversen commented Apr 27, 2023

janiversen commented Apr 27, 2023

joanwa commented Apr 28, 2023

janiversen commented Apr 29, 2023

frenck commented Apr 29, 2023

joanwa commented Apr 29, 2023 • edited

janiversen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanwa May 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanwa May 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanwa May 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanwa Apr 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanwa Apr 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen commented Apr 29, 2023

janiversen commented May 14, 2023

joanwa commented May 15, 2023

janiversen left a comment

Choose a reason for hiding this comment

janiversen left a comment

Choose a reason for hiding this comment

joanwa Aug 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janiversen commented Aug 21, 2023

joanwa commented Apr 4, 2023 •

edited by janiversen

janiversen commented Apr 27, 2023 •

edited

joanwa commented Apr 29, 2023 •

edited

joanwa May 1, 2023 •

edited

joanwa May 1, 2023 •

edited

joanwa May 1, 2023 •

edited

joanwa Apr 29, 2023 •

edited

joanwa Apr 29, 2023 •

edited

joanwa Aug 7, 2023 •

edited