Handle explicit Modbus NaN values

[joanwa]

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 #69656
• This PR is related to issue: #69656
• Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/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[bot]]

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[bot]]

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[bot]]

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]

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

[home-assistant[bot]]

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

Learn more about our pull request process.

[janiversen]

You have a CI problem, please solve that.

[janiversen]

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

[joanwa]

You have a CI problem, please solve that.

yes will do

[janiversen]

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

[janiversen]

please run the tests locally, so you don’t call a review before the code works.

[joanwa]

please run the tests locally, so you don’t call a review before the code works.

I did and they pass. The failure is at Set up Python 3.10: Error: API rate limit exceeded for installation ID 689304.

[janiversen]

Still fails, probably because you need to rebase on the newest dev.

[home-assistant[bot]]

Hi @tristangrichard

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[bot]]

Hi @tristangrichard

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]

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

[janiversen]

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]

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]

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

[janiversen]

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]

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]

Plase look more carefully:

6 open review comments.

[frenck]

@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]

@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]

@frenck good catch, thanks.

[janiversen]

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]

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]

Ok will change it in a followup PR.