Add Modbus light integration #42120

vzahradnik · 2020-10-20T12:27:15Z

Proposed change

This PR adds the Modbus Light entity, which is based on Modbus Switch. At the moment, it supports only the turn_on() and turn_off() methods, but it can be extended.

We've discussed this feature in PR #33551. Feel free to take a look for reference.

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 entry
modbus:
  - name: hub1
    type: tcp
    host: IP_ADDRESS
    port: 502

    lights:
      - name: Light1
        slave: 1
        coil: 13
      - name: Light2
        slave: 2
        coil: 14
        scan_interval: 1
      - name: Light3
        slave: 1
        register: 11

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add documentation for Modbus light home-assistant.io#15329
This PR is related to PR: Refactor Modbus switch to provide a base for other entities #33551

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

To help with the load of incoming pull requests:

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

vzahradnik · 2020-10-20T12:32:23Z

@janiversen I tried to adapt your Modbus Switch tests for Modbus Light. Internally, the tested code is the same, except for component initialization. However, the tests fail on AttributeError in the test method run_base_read_test(...) on this part:

    # Check state
    state = hass.states.get(entity_id).state

If you have a huch what's going on, please let me know. In this PR, I didn't include the tests to get this merged as soon as possible. The Modbus Fan code depends on changes made here.

janiversen · 2020-10-20T18:52:05Z

Seems light do not have a state, it must be called something else, It must be in your read code.I will take a look at your PR tomorrow

vzahradnik · 2020-10-20T19:20:34Z

It is possible. I will definitely take a look at it. However, I can add tests in another PR. At the moment, I don't want to create PR for Fan because it requires the same changes in the init code as Light. So ideally I'd like to get this merged, and then I'd create another PR.

Thanks, we'll talk tomorrow.

janiversen

All ok.

Seems light do have a state (you use state.state) so the change to the tests should be simple

vzahradnik · 2020-10-30T14:46:52Z

I think the problem is in the way how we initialize the components. While the Switch is initialized by the switch component, the Light is initialized inside Modbus.

INFO:homeassistant.loader:Loaded light from homeassistant.components.light
INFO:homeassistant.loader:Loaded modbus from homeassistant.components.modbus

From the log, I can see the light is initialized before Modbus. But it should be initialized inside Modbus.

@janiversen in some PR you mentioned a way how to debug the async code. I'm not sure where it was. Can you tell once again please?

Thanks!

janiversen · 2020-10-30T15:10:13Z

It does sound wrong that light is initialized before modbus (or there are one more thing I do not understand). It does sound like a configuration issue.

I debug a lot, using Visual Code, it works like a charm setting a breakpoint and wait. It´s been a while however since I debugged runtime code. In order to keep things small I try to debug using the test module.

janiversen · 2020-10-30T15:11:34Z

Just thinking loud, since light is defined inside modbus, that way I locate the device might be wrong (look in conftest.py).

janiversen · 2020-10-30T15:12:31Z

Please make the test code available, and I will try to run it and see if I detect anything.

vzahradnik · 2020-10-30T15:14:06Z

I also think that we need to adapt the test code for loading the platform. OK, I will push the code. It's almost identical to the switch.

vzahradnik · 2020-10-30T15:20:56Z

The test code is available now. I took the latest test code for switch from PR #42604, and changed the switch component to light.

janiversen · 2020-10-30T16:05:52Z

The problem is in light.py line 35 “ if discovery_info is None:”. Discovery_info is None, and I do not have a lot of clues as to why, except if the platform is not loaded.

janiversen · 2020-10-30T16:54:04Z

Looking a bit closer and comparing with switch.py, it seems switch.py does:
for coil in config[CONF_COILS]: Etc.
And do not use discovery_info at all.

Maybe that is the change you need to make ?

vzahradnik · 2020-10-30T17:40:37Z

Exactly there's the problem. Loading a config through discovery_info is the recommended way now. It will never be None during entity initialization. But I have no clue how to load the entity from tests and pass discovery_info.

After I start working on whole config refactor we talked about, tests will fail for the Switch too.

Once I figure this out, tests should start working.

First thing, I should find a way how to debug the code. I'll try VS Code as you suggested.

janiversen · 2020-10-30T17:44:02Z

That might be the problem, but I have been testing a while using:
If discovery_info is none:
Discovery_info = config

And I can see that you build the config wrong, you should use CONF_LIGHTS.

I do not know how discovery_info is structured, can you print it to the log and let me see it, then I can tell you what to make different.

As far as I know test do not (yet) support discovery_info, but let me do a little research.

janiversen · 2020-10-30T17:52:47Z

I think I have found out the differences, there are a lot of “test_config_flow*” that sets discovery_info and does a couple of other things.

May I suggest a couple of things:

add a print(discovery_info) in async_platform_setup and let me see the output
remove the test (again) from this PR

Then I will:

add some generic test methods for testing the old way and the new way (with the same test setup).
secure that switch can be configured using the old way or the new way (or more correctly secure, that if discovery_info is none, config can be used in async_setup_platform.

Sounds like a plan to you ?

Btw I have nearly completed tests for writing (switch).

janiversen · 2020-10-30T17:55:00Z

Just ran the switch tests with this PR, and they works unchanged.

vzahradnik · 2020-10-30T17:56:38Z

discovery_info is a dictionary. Basically, it's the same structure as config, but config is reserved for loading via platform, and discovery_info is used for dynamic loading. In this case, I call the methods for dynamic loading during Modbus platform setup.

For testing, you can consider the dict of config and discovery_info to be the same. It is a little bit different, because the yaml config is different, but the differences are small.

I will check the CONF_LIGHTS. Thanks for pointing that out.

May I suggest a couple of things:

It is a plan. I'll prepare the changes tomorrow.

vzahradnik · 2020-10-31T15:26:03Z

@janiversen here's the output of discovery_info:

OrderedDict([('name', 'hub1'), ('type', 'tcp'), ('host', '127.0.0.1'), ('port', 5020), ('lights', [OrderedDict([('name', 'Switch1'), ('slave', 1), ('coil', 1), ('scan_interval', 15.0), ('command_on', 1), ('verify_state', True), ('command_off', 0)])]), ('timeout', 3.0), ('delay', 0)])

For a reference, it is constructed out of this yaml:

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

    lights:
      - name: Switch1
        slave: 1
        coil: 1

Note: You may find out that I am passing the whole yaml. It's because I need to get the hub name to which the light is associated. When I'll start working on the global config refactor, I will fix that.

I also removed the test. Now it's your turn :)

homeassistant/components/modbus/light.py

vzahradnik · 2021-05-03T10:01:27Z

Light code decoupled from Switch. The code is almost exact replica of Switch, including unit tests.

janiversen

I just submitted #50089 because I saw that async_track_time_interval was called with self.hass, which is undefined.

I added the comments here, so you can add it.

homeassistant/components/modbus/light.py

MartinHjelmare

Looks good! Please add tests for the turn_on/turn_off service calls.

homeassistant/components/modbus/light.py

janiversen · 2021-05-05T11:43:21Z

@vzahradnik test_init.py line 270:
async def test_pb_service_write_coil(hass, caplog, mock_modbus):
have examples of the tests you are requested to add.

vzahradnik · 2021-05-05T12:03:46Z

I will first debug the issue #50010. It has higher priority for me... Then I will look at your comments here. Thanks!

janiversen · 2021-05-18T10:09:50Z

@vzahradnik when you update, you will see that class modbusHub have changed, you now need to call async_pymodbus_call(.... CALL_TYPE*) see e.g. cover.py
The other update is that all platforms are now async.
If you do the rebase to resolve the merge conflict, and resolve the requests open, I can give you a hand, to get this finalised

vzahradnik · 2021-05-18T10:15:34Z

Thanks!

janiversen · 2021-05-19T13:02:59Z

Having talked with @vzahradnik today on discord, we have agreed that I bring this PR up to date, and solve the requests from Martin. I am still not sure if I can push to the branch (given that maintainers have permission to edit) or I have to make a new PR.

MartinHjelmare · 2021-05-19T14:43:17Z

Members can push to the branch as long as the PR is open and the author hasn't unchecked that permission.

I normally use the vscode pull request extension to check out PRs. Makes it a breeze.

janiversen · 2021-05-19T16:26:37Z

Thanks, I am on my way to get that extension installed.

janiversen · 2021-05-20T20:16:25Z

@MartinHjelmare I have now update this PR as I promised, I have solved all your request. This version is basically 98% identical to switch, but very different on the UI. Later @vzahradnik will add more functionality.

In theory I have approved this PR, but since I have worked on it that does not count (in my mind at least).

homeassistant/components/modbus/light.py

tests/components/modbus/test_modbus_light.py

homeassistant/components/modbus/__init__.py

homeassistant/components/modbus/light.py

MartinHjelmare

Looks good!

janiversen · 2021-05-21T06:57:55Z

Congratulations. I think we need to have a look at the doc. because I have reworked the document structure.

* Add Modbus Light and add unit tests * Update to original PR. * Review comments. * Review 2. Co-authored-by: jan Iversen <[email protected]>

probot-home-assistant bot added the Hacktoberfest label Oct 20, 2020

homeassistant added the cla-signed label Oct 20, 2020

probot-home-assistant bot added integration: modbus new-integration new-platform by-code-owner labels Oct 20, 2020

vzahradnik mentioned this pull request Oct 20, 2020

Add documentation for Modbus light home-assistant/home-assistant.io#15329

Closed

8 tasks

springstan changed the title ~~New integration: Modbus light entity~~ Add Modbus light platform Oct 25, 2020

janiversen approved these changes Oct 29, 2020

View reviewed changes

MartinHjelmare added the hacktoberfest-accepted label Oct 31, 2020

vzahradnik changed the title ~~Add Modbus light platform~~ Add Modbus light integration Oct 31, 2020

MartinHjelmare requested changes May 3, 2021

View reviewed changes

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

janiversen reviewed May 4, 2021

View reviewed changes

homeassistant/components/modbus/light.py Show resolved Hide resolved

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

MartinHjelmare requested changes May 5, 2021

View reviewed changes

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

MartinHjelmare reviewed May 5, 2021

View reviewed changes

homeassistant/components/modbus/light.py Show resolved Hide resolved

janiversen self-assigned this May 20, 2021

vzahradnik and others added 2 commits May 20, 2021 21:26

Add Modbus Light and add unit tests

8d0bfe7

Update to original PR.

6d66e3f

MartinHjelmare reviewed May 20, 2021

View reviewed changes

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

tests/components/modbus/test_modbus_light.py Outdated Show resolved Hide resolved

homeassistant/components/modbus/__init__.py Outdated Show resolved Hide resolved

Review comments.

79a7b2d

MartinHjelmare reviewed May 21, 2021

View reviewed changes

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

homeassistant/components/modbus/light.py Outdated Show resolved Hide resolved

Review 2.

a1004c5

MartinHjelmare approved these changes May 21, 2021

View reviewed changes

janiversen merged commit 80d1721 into home-assistant:dev May 21, 2021

janiversen mentioned this pull request May 21, 2021

Rework modbus documentation. home-assistant/home-assistant.io#17874

Merged

8 tasks

vzahradnik deleted the feature/modbus-light branch May 22, 2021 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Modbus light integration #42120

Add Modbus light integration #42120

vzahradnik commented Oct 20, 2020 •

edited

Loading

vzahradnik commented Oct 20, 2020

janiversen commented Oct 20, 2020 •

edited

Loading

vzahradnik commented Oct 20, 2020

janiversen left a comment •

edited

Loading

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

vzahradnik commented Oct 31, 2020 •

edited

Loading

vzahradnik commented May 3, 2021

janiversen left a comment

MartinHjelmare left a comment

janiversen commented May 5, 2021

vzahradnik commented May 5, 2021

janiversen commented May 18, 2021

vzahradnik commented May 18, 2021

janiversen commented May 19, 2021

MartinHjelmare commented May 19, 2021

janiversen commented May 19, 2021

janiversen commented May 20, 2021

MartinHjelmare left a comment

janiversen commented May 21, 2021

Add Modbus light integration #42120

Add Modbus light integration #42120

Conversation

vzahradnik commented Oct 20, 2020 • edited Loading

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

vzahradnik commented Oct 20, 2020

janiversen commented Oct 20, 2020 • edited Loading

vzahradnik commented Oct 20, 2020

janiversen left a comment • edited Loading

Choose a reason for hiding this comment

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

janiversen commented Oct 30, 2020

vzahradnik commented Oct 30, 2020

vzahradnik commented Oct 31, 2020 • edited Loading

vzahradnik commented May 3, 2021

janiversen left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

janiversen commented May 5, 2021

vzahradnik commented May 5, 2021

janiversen commented May 18, 2021

vzahradnik commented May 18, 2021

janiversen commented May 19, 2021

MartinHjelmare commented May 19, 2021

janiversen commented May 19, 2021

janiversen commented May 20, 2021

MartinHjelmare left a comment

Choose a reason for hiding this comment

janiversen commented May 21, 2021

vzahradnik commented Oct 20, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

janiversen commented Oct 20, 2020 •

edited

Loading

janiversen left a comment •

edited

Loading

vzahradnik commented Oct 31, 2020 •

edited

Loading