Refactor Modbus switch to provide a base for other entities by vzahradnik · Pull Request #33551 · home-assistant/core

vzahradnik · 2020-04-02T20:01:39Z

Proposed change

This PR refactors the Modbus Switch entity, so that we can later build Light and Fan entities on top of the Switch. Also, initialization was simplified to match with other refactored and new components.

Existing functionality is not affected by this 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)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: N/A

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-04-02T20:01:46Z

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!

bdraco · 2020-04-02T22:53:55Z

@vzahradnik

The CI issues should be fixed so it should run cleanly if you rebase

https://developers.home-assistant.io/docs/development_catching_up/

janiversen

I have no problem letting fan/light be defined as mere name changes to switch, but the documentation needs to be updated to reflect these new entities.

I would also like to see test cases, however with #33447 not merged, it is something that can come later

vzahradnik · 2020-04-03T21:12:13Z

@janiversen thanks for the comments. To sum up, the main purpose of this change is to define bare implementations for Modbus lights and fans. Albeit they are technically just switches at the moment, they will appear differently in the UI. Also, we can easily extend them later.

The documentation was already written and approved (PR home-assistant/home-assistant.io#12640).

I plan to write unit tests, but first I wanted to finally upstream my local changes. Now I maintain my own fork of Modbus platform, and it takes time to adapt my code. When it will be on-par with upstream, I can start focusing on tests and/or other features.

janiversen · 2020-04-03T21:23:38Z

I understand what you are doing, and I find it to be the right way, push the few changes and I will approve. Sorry for having overlooked the documentation.

vzahradnik · 2020-04-03T21:28:05Z

@janiversen no problem. If the implementation will be better thanks to our discussions, I'm all in for it :)

janiversen · 2020-04-04T11:57:49Z

Approved ready to go. I am trying to see if I can get a bit of help with getting PR #33447 merged, since that will allow you to make test cases.

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

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:31:07Z

Still working on it.

vzahradnik · 2020-06-04T15:08:12Z

@janiversen please take a look. I reworked Modbus switch setup to async. Light and Fan entities inherit from Switch class, so I decided to do the changes in this PR. Functionality verified manually at the moment.

janiversen · 2020-06-19T08:37:21Z

What is the status of this PR ?

vzahradnik · 2020-06-19T11:55:47Z

@janiversen from my point of view it's ready. I already use this code in my setup. Modbus code is still synchronous but components (switch, fan and light) are now initialized asynchronously.

It would be great to start the reviewing process and to get this code merged. Any tips, who should I tag or where should I write to move this thing forward?

Thanks!

janiversen · 2020-06-19T12:13:59Z

First if all you need to resolve the 2 failing CI parts, then I will do a review.

vzahradnik · 2020-06-19T12:14:59Z

@janiversen OK, I will take a look.

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

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:55Z

Still working on it.

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

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-10-05T19:19:48Z

@makuser it looks like I will be refactoring the Fan component anyway. If you can provide your code, perhaps I could add support also for controlling speed.

If your changes are public, please leave a link to your repository. And if not, maybe you could provide sample of your Fan configuration with allowed values. Then we can push all required changes in a single PR.

…arately

vzahradnik · 2020-10-12T10:07:24Z

I removed the Light and Fan code. As discussed with @MartinHjelmare, this PR will be only for the necessary Switch refactor
Once this change gets merged, I will open separate PRs for Light and Fan
Description and title of this PR was updated. @MartinHjelmare please remove the new-platform flag
Tests are passing, code was tested locally

MartinHjelmare

Looks good!

MartinHjelmare

Sorry, I missed one thing.

MartinHjelmare · 2020-10-12T11:28:42Z

        return self._available

+
+class ModbusCoilSwitch(ModbusBaseSwitch):


We should inherit from SwitchEntity somewhere in the switch entities.

I guess the original code was written before SwitchEntity even existed. Anyway, the SwitchEntity inherits from the ToggleEntity. I will modify the base class to inherit directly from the SwitchEntity. This should solve our problem.

I will let you know.

If the base class inherits from SwitchEntity we cannot inherit from the base class in other platforms than switch.

Good point. Probably I should add the inheritance into the ModbusCoilSwitch and ModbusRegisterSwitch. This would solve our problem, and yet Fan and Light can still inherit from it.

that would be my preference.

Just working on a change now. I will test it real quick, then we'll run automation tests, and we should be good to go.

Code still works, and tests pass.

janiversen

Looks good to me.

MartinHjelmare

Thanks!

MartinHjelmare · 2020-10-12T12:16:56Z

Ready to merge?

vzahradnik · 2020-10-12T12:30:38Z

Ready to merge?

Yes, if @janiversen has no objections.

janiversen · 2020-10-12T12:32:03Z

No objections look good to me, please merge.

vzahradnik · 2020-10-12T12:40:52Z

Great! In the upcoming days, I will create PRs for Fan and Light, and I will reference this PR, where we discussed a lot already.

I'll start with the Light, because this should be ready to merge rather quickly
The Fan entity will require more work. Also, I see that people like @makuser would appreciate speed control. We'll do that in this PR, too.

@makuser you mentioned that you have your custom Fan implementation already. It would really help, if you could provide sample of your configuration. Especially I'm interested in the allowed values for the speed.

My Fan doesn't have speed control, so I can only code the feature. If you can help with that, you won't need to create your own PR later :)

makuser · 2020-10-30T20:26:57Z

@makuser you mentioned that you have your custom Fan implementation already. It would really help, if you could provide sample of your configuration. Especially I'm interested in the allowed values for the speed.

Yes, indeed. Here are a couple of important registers, we set the speed to CONF_SETPOINT_REGISTER, which is the only register to write to, the rest are just attributes that need to be added to the entity for monitoring purposes.

        vol.Optional(CONF_SETPOINT_REGISTER, default=0): cv.positive_int,
        vol.Optional(CONF_ACTUAL_POSITION_REGISTER, default=2): cv.positive_int,
        vol.Optional(CONF_ACTUAL_FLOW_REGISTER, default=3): cv.positive_int,
        vol.Optional(CONF_ACTUAL_FLOW_VOLUME_REGISTER, default=4): cv.positive_int,
        vol.Optional(CONF_ACTUAL_PRESSURE_REGISTER, default=5): cv.positive_int,
        vol.Optional(CONF_NOMINAL_FLOW_REGISTER, default=384): cv.positive_int,

CONF_ACTUAL_PRESSURE_REGISTER needs a scaling factor of 10x, the others 100x.

The speed settings I would offer right now are:

VALUE_TO_SPEED = {
    0: SPEED_OFF,
    2: SPEED_SLEEP,
    5: SPEED_LOW,
    10: SPEED_MEDIUM,
    15: SPEED_HIGH,
    24: SPEED_VERY_HIGH,
}

Do you know of any way in home assistant to set an array as config entry? That way both the amount of speed entries and their values could be configured.
Otherwise, these speed entries seem to be sensible defaults for common 125mm air pipes@530m³ nominal flow.

janiversen · 2020-10-30T20:37:51Z

Please do not continue discussing on a PR that is merged. Please create an issue so we can trace the progress

vzahradnik · 2020-10-30T20:39:11Z

First, I will need to check out how other Fan implementations work. If the speed is set as a number, e.g., via some slider in the UI, I will implement my code the same way. Thanks for your input, I will think about how can I incorporate it into my code.

At the moment, I created a PR for Modbus light. It has some changes, which need to be merged before I can work on Fan. I will let you know when I'll have something ready to test.

@janiversen good point. I will create an issue tomorrow.

makuser · 2020-10-31T10:06:06Z

First, I will need to check out how other Fan implementations work. If the speed is set as a number, e.g., via some slider in the UI, I will implement my code the same way. Thanks for your input, I will think about how can I incorporate it into my code.

A slider would also be the best solution for me by far, but I have not seen this for fans yet.

homeassistant added the cla-signed label Apr 2, 2020

probot-home-assistant Bot added integration: modbus new-platform labels Apr 2, 2020

vzahradnik mentioned this pull request Apr 2, 2020

Add documentation for Modbus light and fan home-assistant/home-assistant.io#12640

Closed

8 tasks

janiversen reviewed Apr 3, 2020

View reviewed changes

janiversen requested changes Apr 3, 2020

View reviewed changes

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

janiversen approved these changes Apr 4, 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:02

stale Bot added the stale label Aug 1, 2020

stale Bot removed the stale label Aug 3, 2020

vzahradnik changed the title ~~Add Modbus light and fan entities~~ Refactor Modbus switch to provide a base for other entities Oct 12, 2020

vzahradnik added 7 commits October 12, 2020 11:23

Add Modbus light and fan entities

76f36df

Rework Fan and Light components to new config structure

99ff45c

Fix Linter issue

950eae7

Rework Modbus switch initialization

533c758

Properly update state on actuator methods

07454dd

Remove Fan and Light entity from this change. They will be merged sep…

868f795

…arately

Fix loading of the Switch platform

2c71a27

MartinHjelmare removed the new-platform label Oct 12, 2020

MartinHjelmare approved these changes Oct 12, 2020

View reviewed changes

MartinHjelmare requested changes Oct 12, 2020

View reviewed changes

janiversen approved these changes Oct 12, 2020

View reviewed changes

Modbus switch - inherit from the SwitchEntity

dd71f19

MartinHjelmare approved these changes Oct 12, 2020

View reviewed changes

MartinHjelmare merged commit 111afbb into home-assistant:dev Oct 12, 2020

vzahradnik mentioned this pull request Oct 20, 2020

Add Modbus light integration #42120

Merged

21 tasks

vzahradnik mentioned this pull request Oct 31, 2020

Add Modbus Fan integration #42678

Closed

21 tasks

vzahradnik mentioned this pull request Mar 31, 2021

Add Modbus fan integration #48558

Merged

21 tasks

vzahradnik deleted the feature/modbus-fan-and-light branch April 7, 2021 12:49

		return self._available


		class ModbusCoilSwitch(ModbusBaseSwitch):

Uh oh!

Conversation

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

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Apr 2, 2020

Uh oh!

bdraco commented Apr 2, 2020

Uh oh!

janiversen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vzahradnik commented Apr 3, 2020

Uh oh!

janiversen commented Apr 3, 2020

Uh oh!

vzahradnik commented Apr 3, 2020

Uh oh!

janiversen commented Apr 4, 2020

Uh oh!

stale Bot commented May 16, 2020

Uh oh!

vzahradnik commented May 20, 2020

Uh oh!

vzahradnik commented Jun 4, 2020

Uh oh!

janiversen commented Jun 19, 2020

Uh oh!

vzahradnik commented Jun 19, 2020

Uh oh!

janiversen commented Jun 19, 2020

Uh oh!

vzahradnik commented Jun 19, 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 Oct 5, 2020

Uh oh!

vzahradnik commented Oct 12, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

vzahradnik Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

vzahradnik Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

janiversen Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

vzahradnik Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

vzahradnik commented Apr 2, 2020 •

edited

Loading

MartinHjelmare left a comment •

edited

Loading

vzahradnik commented Oct 12, 2020 •

edited

Loading