Add modbus sensor string data_type #35269

bradkeifer · 2020-05-05T20:27:13Z

Proposed change

This change enables a string data_type to provide support for reading holding registers that contain ascii text. The length of text to be read is based on the count of holding registers read.

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)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml
# Shows the current date and time. The date and time is updated continuously.
# The clock does not begin counting until a setting different from default has been made in parameter 0-70 Date and Time.
# This is a 16 character string.
    - name: D1 Date Time Readout
      hub: hub1
      slave: 1 
      register: 889
      count: 8
      data_type: string

Additional information

This PR fixes or closes issue: fixes Modbus integration: TypeError when trying to read ascii string with custom data_type #34943
This PR is related to issue:
Link to documentation pull request: Update sensor.modbus.markdown home-assistant.io#13302

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:

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.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

Adds support for a string data_type. This allows the reading of holding registers that contain ascii string data

modbus sensor: string data_type

homeassistant · 2020-05-05T20:27:15Z

Hi @bradkeifer,

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!

MartinHjelmare

The changed files are incorrectly placed.

The modbus sensor module is here:
https://github.com/home-assistant/core/blob/dev/homeassistant/components/modbus/sensor.py

bradkeifer · 2020-05-06T00:30:09Z

Thanks Martin. I am new to both GitHub and Home Assistant. How do I move the changed files to the correct location in the repository?

MartinHjelmare · 2020-05-06T00:37:44Z

Copy the changed files on top of the existing files and commit the changes. Then remove the files here and commit again. Then push.

Fingers crossed that I am putting these file in the correct location this time

bradkeifer · 2020-05-06T02:05:12Z

Thanks Martin. I have done that and I think all is good for review now. Sorry for my mistake.

MartinHjelmare · 2020-05-06T02:35:15Z

@janiversen do you have time to review?

probot-home-assistant · 2020-05-06T02:46:42Z

Hey there @adamchengtkc, @janiversen, mind taking a look at this pull request as its been labeled with a integration (modbus) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

janiversen

I miss tests in core/tests/components/modbus.

janiversen · 2020-05-06T06:25:26Z

homeassistant/components/modbus/sensor.py

-            self._value = str(val)
-            if self._precision > 0:
-                self._value += "." + "0" * self._precision
+        if self._data_type != DATA_TYPE_STRING:


This seems complicated especially since “int” is the most common form.

Now that we have data_type it should be used instead of isinstance() please.

Sorry for the comment edits, I managed to confuse myself.

Hi @janiversen - Thanks for your prompt review. Is there anything you would like me to action at the moment?

We can do the change of isinstance() but it would be real nice to have test cases for the string type. See core/tests/component/modbus

Sounds great to me. I am new to python, HA and GitHub, so I might make some mistakes along the way. I'll read up about test cases and give it a go.

I've added a commit for a test of the new string data_type

janiversen · 2020-05-06T06:54:14Z

@MartinHjelmare review done, are there a reason why #34679 and #34678 which both fixes issues are hanging ?

MartinHjelmare · 2020-05-06T12:04:07Z

No special reason, we just have too many PRs and too little time to always triage effectively.

MartinHjelmare

Looks good!

Codeowner should approve before we merge.

janiversen

Thanks for adding tests.

All looks good to me.

One small detail:
for future PR, please do not merge dev, when catching up, please follow this way:
https://developers.home-assistant.io/docs/development_catching_up/
(this is common for most opwn source projects)

Thanks for the work.

bradkeifer added 2 commits May 5, 2020 11:15

modbus sensor: string data_type

e5acafe

Adds support for a string data_type. This allows the reading of holding registers that contain ascii string data

Merge pull request #1 from bradkeifer/modbus-sensor-string

c20abd9

modbus sensor: string data_type

homeassistant added the cla-needed label May 5, 2020

probot-home-assistant bot added new-feature small-pr PRs with less than 30 lines. labels May 5, 2020

bradkeifer mentioned this pull request May 5, 2020

Update sensor.modbus.markdown home-assistant/home-assistant.io#13302

Merged

8 tasks

homeassistant added cla-signed and removed cla-needed labels May 5, 2020

MartinHjelmare changed the title ~~modbus sensor: string data_type~~ Add modbus sensor string data_type May 5, 2020

MartinHjelmare requested changes May 5, 2020

View reviewed changes

bradkeifer added 3 commits May 6, 2020 11:13

Add modbus sensor string data_type #35269

7cc28de

Fingers crossed that I am putting these file in the correct location this time

Delete const.py

14b969c

Delete sensor.py

cf924f5

MartinHjelmare added the integration: modbus label May 6, 2020

janiversen requested changes May 6, 2020

View reviewed changes

bradkeifer added 5 commits May 7, 2020 15:59

Add files via upload

1f69147

modbus sensor - add string support. Upload from local dev environment

60d5169

Merge branch 'dev' of https://github.com/bradkeifer/core into dev

01a648b

fix executable stauts

be9fa72

fix flake8 failure

4a38b09

bradkeifer requested a review from MartinHjelmare May 7, 2020 20:30

MartinHjelmare approved these changes May 7, 2020

View reviewed changes

janiversen approved these changes May 8, 2020

View reviewed changes

MartinHjelmare merged commit 4663845 into home-assistant:dev May 8, 2020

lock bot locked and limited conversation to collaborators May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modbus sensor string data_type #35269

Add modbus sensor string data_type #35269

bradkeifer commented May 5, 2020 •

edited

homeassistant commented May 5, 2020

MartinHjelmare left a comment

bradkeifer commented May 6, 2020

MartinHjelmare commented May 6, 2020

bradkeifer commented May 6, 2020

MartinHjelmare commented May 6, 2020

probot-home-assistant bot commented May 6, 2020

janiversen left a comment

janiversen May 6, 2020 •

edited

bradkeifer May 6, 2020

janiversen May 6, 2020

bradkeifer May 6, 2020

bradkeifer May 7, 2020

janiversen commented May 6, 2020

MartinHjelmare commented May 6, 2020

MartinHjelmare left a comment

janiversen left a comment

Add modbus sensor string data_type #35269

Add modbus sensor string data_type #35269

Conversation

bradkeifer commented May 5, 2020 • edited

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

homeassistant commented May 5, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

bradkeifer commented May 6, 2020

MartinHjelmare commented May 6, 2020

bradkeifer commented May 6, 2020

MartinHjelmare commented May 6, 2020

probot-home-assistant bot commented May 6, 2020

janiversen left a comment

Choose a reason for hiding this comment

janiversen May 6, 2020 • edited

Choose a reason for hiding this comment

bradkeifer May 6, 2020

Choose a reason for hiding this comment

janiversen May 6, 2020

Choose a reason for hiding this comment

bradkeifer May 6, 2020

Choose a reason for hiding this comment

bradkeifer May 7, 2020

Choose a reason for hiding this comment

janiversen commented May 6, 2020

MartinHjelmare commented May 6, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

janiversen left a comment

Choose a reason for hiding this comment

bradkeifer commented May 5, 2020 •

edited

Example entry for `configuration.yaml`:

janiversen May 6, 2020 •

edited