Rollback modbus to version 0.107.7 keep new functionality by janiversen · Pull Request #34287 · home-assistant/core

janiversen · 2020-04-16T13:38:49Z

Breaking change

Proposed change

This PR does not solve the problems, but enables users to use modbus again, by reverting the change to async. It is basically an intelligent revert to version 0.107.7, meaning the new functionality is kept but we do not use pymodbus async.

Some of the testers have thankfully pinpointed (and reported) errors in the pymodbus asyncio implementation. It is not known when the reported bugs will be fixed, traditionally asyncio in pymodbus is not moving fast. In order to make the users happy, and revert is needed (this PR)

However the async version are to be updated to use the serial/tcp already
available instead of the flaky pymodbus version. pymodbus is still
needed to encode/decode the messages. In order to be able to share the new (not production) version with e.g. alfa testers a subdirectory have been added.

Having a subdirectory with new not production code, is not the standard way, but considering that this might take quite a time to solve, it seems to be the only way of not loosing the code and be able to work on it.

Of course test cases are also updated.

In order not to change documentation and/or configs already in place, the parameter “delay” is accepted with a warning that is currently is unused.

Before merging this change, it is important to get feedback from a couple of beta testers.

Once merged, please mark this PR to be included in the next patch release.

Type of change

Dependency upgrade
[ x] 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

Additional information

This PR fixes or closes issue: fixes #
fixes Modbus crash at startup 0.108.5 #34269, fixes Modbus #33872 does not work with Serial Devices #34046, fixes Device tracker - all devices become "Away" #34233, fixes Modbus rtuovertcp sends Modbus TCP packets instead on HA 0.108.2 #33947
This PR is related to issue:
Link to documentation pull request:

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:

[x ] The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
[ x] 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-16T13:38:59Z

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!

janiversen · 2020-04-16T13:49:42Z

@vzahradnik could you take a look ? I am aware that many details could have been done better, but I have tried to strike a balance between reverting 100% to 0.107.7 and keep the new functionality.

I like the bot, it added “new integration”, I did not change THAT much :-)

vzahradnik · 2020-04-16T14:14:22Z

@janiversen it looks good. As for my open PRs, I think it would be best not to merge them at this point. I will keep working with the async code, and maybe finally writing some unit tests.

Please let me know if there's any progress with the pymodbus. Because I'm not yet in the manifest, I'm not notified about all the changes.

Thanks!

Update manifest to not use async. Rollback entities to sync version. Keep newer modifications apart from async. Rollback __init__ to sync version but keep the new functionality. add async sub directory Adding the current (not working) version in a sub directory, to allow easy sharing with a few alfa testers. The async version are to be updated to use the serial/tcp already available instead of the flaky pymodbus version. pymodbus is still needed to encode/decode the messagess. Update test cases to reflect sync implementation, but keep the new functionality like e.g. conftest.py.

janiversen · 2020-04-16T14:25:28Z

@vzahradnik thanks for looking. I agree it is better to postpone merging of your PRs. I am not sure what is best to close them or put a big note in them.

Once this PR is merged I will get started on getting the async version back to life, but with the use of non-io parts (basically framer) from pymodbus and io parts from hass.

vzahradnik · 2020-04-16T14:28:41Z

@janiversen I think it would be best to put a note in them and to keep them open. They are open for a while, and it does no harm in keeping it so.

I will update them and put a reference to this PR. If maintainers decide to close them, I will create new ones when the code is ready again.

janiversen · 2020-04-16T14:40:29Z

Sounds like a good plan. Apart from the “async” and “await” both interfaces (test and runtime) are the same..the changes are internal.

MartinHjelmare · 2020-04-16T14:41:45Z

You can publish a branch on your github fork with the async changes if you need beta testers. I don't think we should add it here if that package is not used.

The async version will be made available in a forked repo, until it is ready to replace the production code.

janiversen · 2020-04-16T14:47:39Z

Removed async in for test and ha.

MartinHjelmare

Looks good!

MartinHjelmare · 2020-04-16T16:22:48Z

Should we convert to draft until user tests are done, to protect from merging?

janiversen · 2020-04-16T16:28:59Z

That is a very good idea, thanks

vzahradnik · 2020-04-16T16:35:13Z

Should we convert to draft until user tests are done, to protect from merging?

Great idea! Probably this would be useful for my pull requests too. They are all based on async code.

For a reference: PR #33642, #32439, and #33551.

MartinHjelmare · 2020-04-16T16:37:59Z

Converted all three.

janiversen · 2020-04-16T16:40:06Z

@MartinHjelmare thanks. Any idea when the next patch will be closed? (to get a timeframe for testing)

MartinHjelmare · 2020-04-16T17:04:35Z

Not sure. If there's enough PRs on a milestone we try to release asap. Last days before beta cut we often don't do patch releases since the beta is coming up anyway.

dolenec · 2020-04-16T17:25:23Z

How to test it? I have same problem but do not know how to test it.. when can I get changed file?

janiversen · 2020-04-16T17:28:41Z

I am not sure what you mean, the files are in this PR. You test it by modifying your system with these files.

lopez3 · 2020-04-16T19:49:51Z

Tested in my system, works fine. Fixed #34269.

dolenec · 2020-04-16T20:00:27Z

Currently all seems fine.. But normaly it will take from 1 to 24 hours to stop working.. But currently is working with changed files..

Deez73 · 2020-04-16T21:07:31Z

I can also confirm that this PR seems to fix the previous problems with modbus serial failing when upgraded from 0.107.7 to any 0.108.x version.

HarrisonPace · 2020-04-17T06:05:26Z

+1 Looks good from my end too.

janiversen · 2020-04-17T06:23:12Z

@MartinHjelmare We have enough positive test results. Can you please remove the “draft” for this PR so it can be merged, thanks in advance.

dolenec · 2020-04-17T07:39:36Z

I can confirm that changes made in PR is solution and is working.

iltomma · 2020-04-18T08:23:13Z

Hi, I upgrade HA to 0.108.6 from 0.108.5 and serial Modbus doesn't work.
Last working HA version is 0.107.7
This is my log:

2020-04-18 10:11:22 ERROR (MainThread) [homeassistant.setup] Error during setup of component modbus
Traceback (most recent call last):

  File "/usr/src/homeassistant/homeassistant/setup.py", line 171, in _async_setup_component
    hass, processed_config

  File "/usr/src/homeassistant/homeassistant/components/modbus/__init__.py", line 139, in async_setup
    await hass.async_add_executor_job(start_modbus)

  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)

  File "/usr/src/homeassistant/homeassistant/components/modbus/__init__.py", line 111, in start_modbus
    client.setup()

  File "/usr/src/homeassistant/homeassistant/components/modbus/__init__.py", line 207, in setup
    loop=self._loop,

  File "/usr/local/lib/python3.7/site-packages/pymodbus/client/asynchronous/serial.py", line 75, in __new__
    yieldable = factory_class(framer=framer, port=port, **kwargs)

  File "/usr/local/lib/python3.7/site-packages/pymodbus/client/asynchronous/factory/serial.py", line 104, in async_io_factory
    client = AsyncioModbusSerialClient(port, proto_cls, framer, loop, **kwargs)

  File "/usr/local/lib/python3.7/site-packages/pymodbus/client/asynchronous/asyncio/__init__.py", line 689, in __init__
    self._connected_event = asyncio.Event()

  File "/usr/local/lib/python3.7/asyncio/locks.py", line 249, in __init__
    self._loop = events.get_event_loop()

  File "/usr/local/lib/python3.7/asyncio/events.py", line 644, in get_event_loop% threading.current_thread().name)

RuntimeError: There is no current event loop in thread 'SyncWorker_14'.

``

Deez73 · 2020-04-18T08:38:40Z

According to change notes, this fix/PR was not part of the 0.108.6 release.
Use this PR’s files until it’s part of a .7 release or stay at 0.107.7

homeassistant added the cla-signed label Apr 16, 2020

probot-home-assistant Bot added has-tests integration: modbus new-integration type: bugfix labels Apr 16, 2020

probot-home-assistant Bot added the by-code-owner label Apr 16, 2020

janiversen mentioned this pull request Apr 16, 2020

Modbus rtuovertcp sends Modbus TCP packets instead on HA 0.108.2 #33947

Closed

do not publish async version

7c5506f

The async version will be made available in a forked repo, until it is ready to replace the production code.

MartinHjelmare approved these changes Apr 16, 2020

View reviewed changes

MartinHjelmare removed the new-integration label Apr 16, 2020

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

lopez3 mentioned this pull request Apr 16, 2020

Modbus crash at startup 0.108.5 #34269

Closed

MartinHjelmare marked this pull request as ready for review April 17, 2020 07:55

MartinHjelmare merged commit 8277ebc into home-assistant:dev Apr 17, 2020

MartinHjelmare added this to the 0.108.7 milestone Apr 17, 2020

janiversen deleted the modbus_issue6 branch April 17, 2020 09:53

lock Bot locked and limited conversation to collaborators Apr 19, 2020

balloob added cherry-picked and removed cherry-picked labels Apr 21, 2020

balloob added the bugfix label May 4, 2020

Uh oh!

Conversation

janiversen commented Apr 16, 2020 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

vzahradnik commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

vzahradnik commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

MartinHjelmare commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Apr 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

vzahradnik commented Apr 16, 2020

Uh oh!

MartinHjelmare commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

MartinHjelmare commented Apr 16, 2020

Uh oh!

dolenec commented Apr 16, 2020

Uh oh!

janiversen commented Apr 16, 2020

Uh oh!

lopez3 commented Apr 16, 2020

Uh oh!

dolenec commented Apr 16, 2020

Uh oh!

Deez73 commented Apr 16, 2020

Uh oh!

HarrisonPace commented Apr 17, 2020

Uh oh!

janiversen commented Apr 17, 2020

Uh oh!

dolenec commented Apr 17, 2020

Uh oh!

iltomma commented Apr 18, 2020

Uh oh!

Deez73 commented Apr 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

janiversen commented Apr 16, 2020 •

edited by MartinHjelmare

Loading

Example entry for `configuration.yaml`:

janiversen commented Apr 16, 2020 •

edited

Loading