Add Modbus cover

[vzahradnik]

Proposed change

This pull request adds a new Modbus entity - cover. At the moment, this implementation supports open, closed, opening, and closing states.

User can control the cover using a coil or a holding register. If a coil is used, we can't detect intermediary states like opening or closing. Therefore, an optional status_register attribute was defined, so that the cover state can be read from the register.

If user decides to use holding register to control the cover, this register is also used for reading the states back. However, user can specify status_register which will take priority also in this case.

To map register values to actual states, four optional attributes were defined (with reasonable default values). User can override any or all of the values, e.g., state_open=1, or state_closing=3. If the cover is controlled by the holding register, the state_open and state_closed attributes are also used for writing into the register, and controlling the state of the cover.

Configuration validation was adjusted, so that either coil or register is specified. User needs to specify one of these attributes. If both or none of them are specified, a config error is logged.

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: IP_ADDRESS
      port: 502

      covers:
        - name: Door1
          hub: hub1
          slave: 1
          coil: 1
          scan_interval: 10
          device_class: door
        - name: Door2
          hub: hub1
          slave: 2
          coil: 2
          device_class: door

Note: for more examples, please refer to the documentation PR. I provided several examples to make the usage clear to the user.

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request: Add documentation for Modbus cover home-assistant.io#12687

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]

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!

[janiversen]

We should think of moving the read functions into modbushub in init.py as it seems there are most (if not all) entities, use the exact same pattern (inclduding the testing for HOLDING or INPUT). However this can be done in another PR.

The code looks good, good work!

[janiversen]

Your 3 PR are all OK from my side, now we just need to wait for someone with write access to look at them.

Regarding the score, once I am over the current set of errors (open issues), I will deal with the logging.

[stale]

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]

@janiversen what's the status with the async pymodbus code? I'm thinking of making this code synchronous again just for the sake of better maintainability. It shouldn't be much work, and at least we could use my components.

[janiversen]

Sorry for the delay in responding, I was occupied with the change in “state of alert” here in Spain, which are slowly moving back to a somewhat normal life.

The async code is waiting for a new version from pymodbus, I am working on an alternative that bypasses the communication part of pymodbus, but it is only moving slowly forward.

I prefer if it is not much work, that you convert to sync and we get it merged. When in future we make an async version I will make the needed changes for all components.

[vzahradnik]

@janiversen I was busy with other stuff too. But I will start working on it again. Converting my code shouldn't be much work. Also, I'll keep async versions. Later, either you could modify it or just ask me and I'll provide the files.

[janiversen]

Perfect, let me know when you want a review, so we can get this code merged, it is long overdue.

[janiversen]

Ready to be merged.

[vzahradnik]

@janiversen I still work on this. I didn't verify my latest changes manually. Also, I would love to include at least some unit tests, but I didn't grasp how existing tests are written. Tests for Modbus sensor are clear, but cover is more complicated. My main issue is that the tests in general are not well documented, and sometimes I have no clue what they're doing.

However, this PR can be changed from draft again, and you guys can start reviewing my code. I can always include tests in a separate PR.

Thanks!

[bradkeifer]

Sorry for the delay in responding, I was occupied with the change in “state of alert” here in Spain, which are slowly moving back to a somewhat normal life.

The async code is waiting for a new version from pymodbus, I am working on an alternative that bypasses the communication part of pymodbus, but it is only moving slowly forward.

I prefer if it is not much work, that you convert to sync and we get it merged. When in future we make an async version I will make the needed changes for all components.

@janiversen - If I can help with testing of the async solution, please let me know.

[vzahradnik]

@janiversen I fixed a couple of issues, and tested the code manually. Now it's ready for review. Please take a look at it now. Thanks!

[janiversen]

Looks real good, ready to be merged from my side.

Lets see if @MartinHjelmare is happy too.

[MartinHjelmare]

Looks good! Some comments.

[vzahradnik]

@MartinHjelmare I implemented changes according to your comments. Please check now. Thanks!

[MartinHjelmare]

Nice!

[vzahradnik]

@MartinHjelmare, @janiversen please take a look at the latest code.

• Reworked to read cover config from the Modbus section
• Fixed a bug with non-existing DEFAULT_HUB

CONFIG CHANGE

Due to the change to move cover config inside Modbus section, I had to introduce a breaking change. Originally, under Modbus we list all the hubs. Because I needed to add a cover section, the list of hubs was moved under a new config entry called hubs. If the cover section followed the list of hubs directly, such a YAML file was invalid.

For a better demonstration, here's the original config:

modbus:
  - name: plc_01
    type: tcp
    host: 127.0.0.1
    port: 5020

And here's a new config. Note the hubs and covers sections:

modbus:
  hubs:
    - name: plc_01
      type: tcp
      host: 127.0.0.1
      port: 5020

  covers:
    - name: plc_01_door
      scan_interval: 1
      hub: plc_01
      slave: 1
      coil: 0
      device_class: door
      status_register: 4
      status_register_type: input
      state_opening: 1
      state_open: 2
      state_closing: 3
      state_closed: 4

This config is very readable, but it's a breaking change. What's your suggestion on this? Perhaps I could introduce a backward compatibility. However, in such cases users cannot use the new cover. Such YAML file is invalid (checked with a YAML validator).

Warnings

When I run such a config, Home Assistant issues a warning. It's not critical and has no effect on the component. However, please take a look at the log output.

2020-07-03 17:40:51 WARNING (Recorder) [homeassistant.components.recorder] Event is not JSON serializable: <Event platform_discovered[L]: service=load_platform.cover, platform=modbus, discovered=[OrderedDict([('name', 'plc_01_door'), ('scan_interval', datetime.timedelta(seconds=1)), ('hub', 'plc_01'), ('slave', 1), ('coil', 0), ('device_class', 'door'), ('status_register', 4), ('status_register_type', 'input'), ('state_opening', 1), ('state_open', 2), ('state_closing', 3), ('state_closed', 4)])]>

DEFAULT HUB

Update: I created a separate PR to deal with this issue: #37546

Each modbus component has an optional hub parameter. If not specified, it uses a DEFAULT_HUB. However, this hub is never initialized and such code fails. I introduced a small fix, where I assign first hub from the config to be default. However, because nobody reported an error (as far as I know), I assume all users define this parameter in their configs. I think it would be better to make this parameter mandatory to eliminate ambiguity. This would be a breaking change, too.

Please advise on both issues.

[MartinHjelmare]

Looks good!

[vzahradnik]

Finally, I updated this PR with some of the requested changes:

• Covers config was moved under hub configuration. Documentation PR was also updated
• Fixed one issue mentioned in the PR review

Please take a look at it, so we can finally start cleaning this PR.

Thanks!

[janiversen]

LGTM.

[janiversen]

I would really not like to have/make a global change to the config of the whole modbus, for a couple of reasons:

• I am keeping the async version updated in a seperate branch, and it seems pymodbus is moving as we need. So bigger changes are just a lot of work.
• The integrations that uses modbus will probably be affected by a global config change, so we might break more than just modbus. My preference would be to keep the old configuration way alongside the new one for a couple of point releases, making a LARGE warning when the old configuration is used.

[vzahradnik]

I would really not like to have/make a global change to the config of the whole modbus

The code as is implemented now doesn't break anything. It adds Modbus covers as an optional list inside hub definition (see updated description). Existing config should NOT be affected.

Global config change makes sense, but it can be postponed. However, we should do it at some point, because maintenance and coding new features is harder.

[janiversen]

Yes a global change surely makes sense, but it must be timed !!

I saw your config did not break the existing ones, sorry for not mentioning that.

It would be real nice to get PR merged.

[janiversen]

Cover is new, so how can this be a breaking change?

[MartinHjelmare]

That's stale.

[MartinHjelmare]

Looks good!

[vzahradnik]

@MartinHjelmare thanks! What's the next step? Should I squash the commits before we merge it?

[MartinHjelmare]

Sorry, this old comment hadn't been addressed.

[MartinHjelmare]

No, we'll squash when we merge.

[MartinHjelmare]

Thanks!

[makuser]

@vzahradnik may I ask which modbus devices this integration works with on your end? I might be looking for a couple hundred of these.

[vzahradnik]

@makuser I use this integration with Controllino Mega, which is an industry-grade Arduino Mega. I have most of the logic there, and Home Assistant provides a frontend and automation logic to it. I also have some other Modbus devices, but even in this case the data goes through Controllino. It takes Modbus over TCP, and sends it as Modbus RTU over RS485 (serial).

This solution works very well. I am thinking of writing a blog post about it. In the meantime, however, feel free to ask.