Allow discovery configuration of modbus platforms

[janiversen]

Breaking change

Users can now use the new style configuration when configuring the modbus integration. The existing configuration style is kept, allowing users to change gradually.

Remark the existing configuration style will be removed in a couple of releases.

Example existing configuration:

modbus:
  - name: hub1
    type: tcp
    host: IP_ADDRESS
    port: 502

sensor:
  platform: modbus
  registers:
    - name: Sensor1
      hub: hub1
      unit_of_measurement: °C
      slave: 1
      register: 100

Same configuration in new style:

modbus:
  - name: hub1
    type: tcp
    host: IP_ADDRESS
    port: 502
    sensors:
      - name: Sensor1
        slave: 1
        address: 100

Proposed change

Added possibility to do:
modbus:
binary_sensors: []
sensors: []
switches: []

REMARK the old configuration style is still supported, but will be removed at a later point release, therefore this is not
a breaking change.

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)
• [ x] Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

home-assistant/home-assistant.io#16678

Checklist

• [x ] The code change is tested and works locally.
• [ x] Local tests pass. Your PR cannot be merged unless tests pass
• [ x] There is no commented out code in this PR.
• [ x] I have followed the development checklist
• [x ] 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:

• 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:

• [ x] No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

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

[probot-home-assistant]

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

[janiversen]

@vzahradnik I closed the other PR, because it had label "breaking change", and I modified the commits to make sure it is not a breaking change. Please have a look at this (look in new_config.txt for explanation of the changes).

Once you approve the PR, I will add a PR to update the documentation.

[vzahradnik]

@janiversen good job! I found one small issue in the new_config.txt. It's an easy fix.

[janiversen]

Documentation PR added (#16678) so now it is ready to be merged.

[yury-sannikov]

@janiversen looks like your Documentation PR link pointing to a wrong repo. This is the correct one.

[janiversen]

It was not wrong, I just only added the # number and not the full link. However I have updated it to the full link.

[janiversen]

Updated to newest dev, no real changes. This PR is ready to be merged.

[janiversen]

Updated PR due to changes in dev. These last changes are needed to pass the checks! The last commit was due to coverage failure, and instead of disabling coverage I added new tests.

@MartinHjelmare you motivated me to change the configuration to the new style, and your review comments are very useful (not only to make the integration better, but also for me to learn more about the internals of HA). So please when you have time, please let me hear what should be changed in order to get this merged, thanks in advance.

We have a bunch of open issues and other PR´s depending on this PR (because we are not allowed to change the old config).

[vzahradnik]

@janiversen I just noticed one thing:
You have defined SCAN_INTERVAL in the BASE_COMPONENT_SCHEMA. However, this attribute is actually in effect only for the COVER and CLIMATE. Other components are now also initialized via discovery_info, in which case a default scan interval for the platform is applied.

Modbus Cover and Climate disable polling by the platform:

    @property
    def should_poll(self):
        """Return True if entity has to be polled for state.

        False if entity pushes its state to HA.
        """
        # Handle polling directly in this entity
        return False

And handle the update by themselves:

    async def async_added_to_hass(self):
        """Handle entity which will be added."""
        async_track_time_interval(
            self.hass, lambda arg: self._update(), self._scan_interval
        )

I stuck to this issue while rebasing the LIGHT against this PR.

[janiversen]

@vzahradnik good catch, I wonder why my production system works without the should_poll property...anyhow I will add this to all platforms.

[vzahradnik]

@janiversen also add the call self.schedule_update_ha_state() to methods like self._update(). See the Cover as a reference.

[vzahradnik]

@janiversen the should_poll() method should be False in our case. It is true by default, which means the platform is polling for Modbus changes. However, when we initialize the platform via discovery, there's no way how to pass the scan_interval value.

This is the reason, I set polling to False and set up a code to handle polling by itself. It was implemented for Cover and Climate.

[vzahradnik]

@janiversen when I look at the current code of Light, after we modify the Switch to handle the polling by itself, there's very little custom code left in the Light component. But it still can be extended further.

I can prepare a PR against your branch with required changes. Please let me know if you agree.

[vzahradnik]

@janiversen I confirmed that the scan_interval doesn't work in this setup. Binary sensor ignores the value and defaults to 30 seconds. For now, I made a fix for the binary sensor, later today I will fix the rest.

My changes are made against this PR, I will create a PR to your branch.
https://github.com/JSC-electronics/home-assistant-core/tree/fix_polling

[janiversen]

@vzahradnik There must be a way we can tell core which scan_interval we want to use, it does not sound logical that all platforms need to implement their own timers. I will take a look at the polling code and look for a function to call.

I looked at the "old" code that allowed scan_interval to be defined e.g. at sensor level (not for each single sensor), but I cannot see how the configured scan_interval is passed on to core.

No need to create a PR, I will copy your branch.

[vzahradnik]

While I implemented this, I looked at other integrations. So far I didn't find a better way. Anyway, I will prepare the change to at least make it work again. If you find a better way, perhaps we could implement this in a separate PR because this is the way how cover and climate work, too.

[vzahradnik]

@janiversen I'm not able to create a PR against your branch. All the changes are implemented here as 3 separate commits:
https://github.com/JSC-electronics/home-assistant-core/tree/fix_polling

Feel free to take a look and you can also pull from there. If you find a better solution, I am open to that. These commits at least fix the current behavior, which is broken with your change.

[janiversen]

Thanks a lot for helping out. I will amend your changes to this PR later this evening. I wish we had seen this problem a bit earlier, but luckily you found it (and corrected it) before the PR was merged.

We might be able to find a better solution, but that is for another PR later....right now I am eager to submit a bunch of PRs to close most of the open issues (but they all depend on this PR).

[vzahradnik]

@janiversen and my plan is to finally get Light and Fan merged. I will prepare two very simple PRs to get them reviewed quickly. I remember others suggested some improvements to my Fan code, but I can work on that on another PRs... All these changes depend on this PR, so let's hope it gets merged soon.

[janiversen]

@MartinHjelmare I should have addressed all your suggestion (accepted or implemented all). Please let me know if you have further comments.

[janiversen]

Rebased to newest dev.

[janiversen]

I believe that build error is not caused by my changes, but a fluke. Force pushed to see if the error occurs again.

[MartinHjelmare]

Nice!

[MartinHjelmare]

Please add a breaking change paragraph to the PR description that we can copy to the release notes. Briefly describe the deprecation and what users can do to cope with the change.

[janiversen]

Updated PR, hope you like the text, otherwise feel free to change it.

[MartinHjelmare]

Can we add an example of the old style vs the new style of configuration YAML? Then it will be super clear.

[janiversen]

Of course, updated PR with example.

[MartinHjelmare]

I don't see the examples in the PR description.

[janiversen]

hrmffff pressing "update comment" absolutely helps !! my error. It is visible now.

[MartinHjelmare]

Great!