Add custom data type support into Modbus climate by vzahradnik · Pull Request #32439 · home-assistant/core

vzahradnik · 2020-03-03T13:24:44Z

Breaking change

Climate configuration was moved from the platform directly into the Modbus integration. This change was required to meet the
latest architecture requirements for Home Assistant.

Before

climate:
  - platform: modbus
    name: Watlow F4T
    hub: hub1
    slave: 1
    data_type: uint
    data_count: 1
    scale: 0.1
    offset: 0
    precision: 1
    max_temp: 30
    min_temp: 15
    temp_step: 1
    target_temp_register: 2782
    current_temp_register: 27586

After

# Example configuration.yaml
modbus:
  - name: hub1
    type: tcp
    host: 127.0.0.1
    port: 5020

    climates:
      - name: Watlow F4T
        slave: 1
        data_type: uint
        data_count: 1
        scale: 0.1
        offset: 0
        precision: 1
        max_temp: 30
        min_temp: 15
        temp_step: 1
        target_temp_register: 2782
        current_temp_register: 27586

Proposed change

This commit adds support for custom data type into Modbus climate,
and improves error handling.

Currently if a user sets data_count into a value other than 1, 2 or 4 bytes,
code throws an exception because it didn't support the parsing.

New behavior is to notify user that parsing was not successful,
and to inform him to specify a custom data structure to use.

This behavior will be the same as used in Modbus sensor.

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
modbus:
  - name: hub1
    type: tcp
    host: 127.0.0.1
    port: 5020

    climates:
      name: Watlow F4T
      slave: 1
      data_type: custom
      structure: ">f"
      data_count: 1
      target_temp_register: 2782
      current_temp_register: 27586

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add custom data type support for Modbus climate home-assistant.io#12250

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

probot-home-assistant · 2020-03-07T10:20:42Z

Hey there @adamchengtkc, mind taking a look at this pull request as its been labeled with a integration (modbus) you are listed as a codeowner for? Thanks!

vzahradnik · 2020-03-28T11:35:02Z

@springstan thanks for the comments. I will postpone my work until PR #32557 gets merged. It refactored most of the Modbus integration, and my code will need to be adapted for that.

janiversen · 2020-03-28T11:44:19Z

I think that is a good idea. Be aware I am working on changing the tests, so that there a common part (basically run_test) and each entity only contains the actual test

vzahradnik · 2020-03-28T16:32:38Z

@janiversen yeah, I saw your comments in that other PR. Thanks for the heads up.

janiversen

This PR needs to be updated to our new standard. Once updated I will make a more in depth review, but in general it looks good.

janiversen · 2020-04-06T07:36:44Z

The reworked test harness is now merged, so no excuses for not writing test cases :-)

vzahradnik · 2020-04-12T09:12:41Z

@janiversen, I rebased the code against latest dev. Also, I moved the DEFAULT_STRUCT_FORMAT into the const.py file, so that it can be shared between climate and sensor. Could you take a look? Thanks!

janiversen

I had hoped you would have added just a couple of tests. We need that to get a higher (than no-score) on our integration.

My question about >f is just a question, not a request that you do it differently.

janiversen · 2020-04-12T16:58:42Z

Regarding the pytest error, pull newest dev and rebase then it is gone.

vzahradnik · 2020-04-12T19:12:12Z

@janiversen just today I checked the integration quality scale requirements. If they're still up to date (and they should be), we meet almost all requirements to get the silver rating. Unit tests are not a requirement there.

What we don't meet is following:

Handles internet unavailable. Log a warning once when unavailable, log once when reconnected.
Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected.

If we manage to fix the logging, this integration could be certified. At the moment, pymodbus warning is logged on every failed attempt, e.g., a connection is lost.

And regarding the tests, I didn't have time this week. I'm glad that I found some time to go through all my pull requests.

janiversen

Reviewed this version of the code, and it is ready (from my side) to be merged.

stale · 2020-05-16T16:51:03Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

vzahradnik · 2020-05-20T09:30:44Z

Still working on it.

vzahradnik · 2020-06-04T15:38:14Z

@janiversen please check this PR. Rebased against latest dev and re-tested.

stale · 2020-08-01T11:34:49Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

vzahradnik · 2020-08-03T07:04:36Z

Still working on it.

stale · 2020-09-02T11:02:51Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

vzahradnik · 2020-09-03T18:26:14Z

This PR is still pending review from maintainers. I can, however, rebase the change to find out it still works with latest Home Assistant code.

janiversen · 2020-09-03T19:02:25Z

??? as one of the code owners and maintainer I approved the code 13 of April. You have a number of good Prs which should have been merged for a long time, I hope we can soon get it done.

Let me know if I can do anything?

vzahradnik · 2020-09-03T19:39:56Z

@MartinHjelmare sorry, I missed that comment. Yes, I also want my PRs to get merged. Just I don't have a time to work on them at this moment. I plan to review all of them when I finally get some free time.

Thanks, I will let you know if there's anything.

janiversen · 2020-09-18T10:12:15Z

Any idea when you will continue working on this PR. We have a issue open on climate, but I do not want to make your work more difficult by adding a new PR for the same code.

vzahradnik · 2020-09-23T14:14:11Z

Any idea when you will continue working on this PR. We have a issue open on climate, but I do not want to make your work more difficult by adding a new PR for the same code.

@janiversen it looks like I'll need to rework the way how the Climate component is configured. If you want to work on the issue, go ahead. I will deal with your changes, and adapt my code accordingly.

janiversen · 2020-09-23T17:46:59Z

OK, I will continue first by adding a couple of simple tests (I am in the process of adding tests for all devices), and then solve the bug, which I think is easy.

I will comment on the other PR regarding the config changes.

vzahradnik · 2020-09-23T17:51:21Z

@janiversen OK, thanks! Today I also reworked how config for the cover is handled. If @MartinHjelmare approves the way how it's done, I will implement the same way also here to meet the HA requirements:

https://github.com/home-assistant/architecture/blob/master/adr/0007-integration-config-yaml-structure.md

janiversen

I am happy with the code, it looks good. But I would really like to see the sensor test cases expanded.

vzahradnik · 2020-09-28T06:50:49Z

I am happy with the code, it looks good. But I would really like to see the sensor test cases expanded.

I will take a look at it.

MartinHjelmare · 2020-09-30T23:32:32Z

Please add a before and after example view of the configuration yaml section to the breaking changes paragraph.

vzahradnik · 2020-10-01T07:27:37Z

before and after example view

Done.

MartinHjelmare

Nice!

janiversen · 2020-10-06T19:54:36Z

The failing test is not your problem. Simply push again (e.g. with an updated commit text)

This commit adds support for custom data type and improves error handling. Currently, if a user set data_count into a value other than 1, 2 or 4 bytes, code threw an exception because it didn't support the parsing. New behavior is to notify user that parsing was not successful, and to inform him to specify custom data structure to use. This behavior will be the same as used in Modbus sensor. Fix linter warnings

Home Assistant requires this method; however, because our Climate component doesn't support other modes than AUTO, we'll keep the method body empty. The only purpose is to avoid getting NotImplementedError due to this.

vzahradnik · 2020-10-07T10:35:21Z

The checks were failing on the linter error - unnecessary pass statement. Everything is still working, and all the checks are green now. I think this PR is finally ready to be merged. Please let me know if there's anything left to do.

Thanks!

janiversen · 2020-10-07T18:08:37Z

Interesting it worked fine in my environment (test), but super that you found it, and I see that @MartinHjelmare have approved the PR, so lets hope it gets merged soon.

vzahradnik · 2020-10-07T18:11:19Z

Interesting it worked fine in my environment (test)

It was a linter issue - Unnecessary pass keyword. Apparently, as long as you have some comment in a method, the pass keyword is not necessary. I didn't know that.

MartinHjelmare

Great!

homeassistant added the cla-signed label Mar 3, 2020

vzahradnik mentioned this pull request Mar 3, 2020

Add custom data type support for Modbus climate home-assistant/home-assistant.io#12250

Merged

7 tasks

springstan reviewed Mar 6, 2020

View reviewed changes

Comment thread homeassistant/components/modbus/climate.py

Comment thread homeassistant/components/modbus/climate.py Outdated

springstan added the integration: modbus label Mar 7, 2020

vzahradnik requested a review from springstan March 22, 2020 09:03

springstan reviewed Mar 27, 2020

View reviewed changes

Comment thread homeassistant/components/modbus/climate.py Outdated

Comment thread homeassistant/components/modbus/climate.py Outdated

Comment thread homeassistant/components/modbus/climate.py Outdated

janiversen requested changes Apr 4, 2020

View reviewed changes

janiversen reviewed Apr 12, 2020

View reviewed changes

janiversen approved these changes Apr 13, 2020

View reviewed changes

vzahradnik mentioned this pull request Apr 16, 2020

Rollback modbus to version 0.107.7 keep new functionality #34287

Merged

11 tasks

MartinHjelmare marked this pull request as draft April 16, 2020 16:37

stale Bot added the stale label May 16, 2020

stale Bot removed the stale label May 20, 2020

vzahradnik marked this pull request as ready for review June 4, 2020 15:37

stale Bot added the stale label Aug 1, 2020

stale Bot removed the stale label Aug 3, 2020

stale Bot removed the stale label Sep 3, 2020

MartinHjelmare requested changes Sep 3, 2020

View reviewed changes

Comment thread homeassistant/components/modbus/climate.py Outdated

janiversen requested changes Sep 28, 2020

View reviewed changes

Comment thread homeassistant/components/modbus/climate.py Outdated

MartinHjelmare reviewed Sep 30, 2020

View reviewed changes

Comment thread homeassistant/components/modbus/climate.py Outdated

Comment thread homeassistant/components/modbus/climate.py Outdated

MartinHjelmare added the breaking-change label Sep 30, 2020

MartinHjelmare approved these changes Oct 6, 2020

View reviewed changes

vzahradnik added 6 commits October 7, 2020 11:57

Migrate Modbus climate to new schema

0f9502d

Fix exception due to not implemented method

4aa0b72

Rename climates array to entities

23c062d

Rework set_hvac_mode()

a13fcd0

Home Assistant requires this method; however, because our Climate component doesn't support other modes than AUTO, we'll keep the method body empty. The only purpose is to avoid getting NotImplementedError due to this.

Fix linter error

3d67b77

MartinHjelmare approved these changes Oct 7, 2020

View reviewed changes

MartinHjelmare merged commit ba84d0b into home-assistant:dev Oct 7, 2020

vzahradnik deleted the feature/modbus-climate-custom-data-type branch October 7, 2020 21:04

wills106 mentioned this pull request Oct 22, 2020

ModBus Read multiple registers (count:) only displays first register when using data_type: custom #41944

Closed

Uh oh!

Conversation

vzahradnik commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Before

After

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

Uh oh!

Uh oh!

probot-home-assistant Bot commented Mar 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vzahradnik commented Mar 28, 2020

Uh oh!

janiversen commented Mar 28, 2020

Uh oh!

vzahradnik commented Mar 28, 2020

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

janiversen commented Apr 6, 2020

Uh oh!

vzahradnik commented Apr 12, 2020

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

janiversen commented Apr 12, 2020

Uh oh!

vzahradnik commented Apr 12, 2020

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

stale Bot commented May 16, 2020

Uh oh!

vzahradnik commented May 20, 2020

Uh oh!

vzahradnik commented Jun 4, 2020

Uh oh!

stale Bot commented Aug 1, 2020

Uh oh!

vzahradnik commented Aug 3, 2020

Uh oh!

stale Bot commented Sep 2, 2020

Uh oh!

vzahradnik commented Sep 3, 2020

Uh oh!

janiversen commented Sep 3, 2020

Uh oh!

Uh oh!

vzahradnik commented Sep 3, 2020

Uh oh!

janiversen commented Sep 18, 2020

Uh oh!

vzahradnik commented Sep 23, 2020

Uh oh!

janiversen commented Sep 23, 2020

Uh oh!

vzahradnik commented Sep 23, 2020

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vzahradnik commented Sep 28, 2020

Uh oh!

Uh oh!

Uh oh!

MartinHjelmare commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

vzahradnik commented Mar 3, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

MartinHjelmare commented Sep 30, 2020 •

edited

Loading