Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add availability status to Modbus entities #31073

Merged
merged 1 commit into from Feb 12, 2020

Conversation

@vzahradnik
Copy link
Contributor

vzahradnik commented Jan 22, 2020

Proposed change

This change adds support for reporting availability of the Modbus platform. In the current state, if Modbus communication fails, sensors won't notify the state machine, and the UI is not updated. It is a problem because the user is not aware of an issue unless he checks for Home Assistant logs.

Support was added for all existing Modbus components and manually tested.

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: N/A
  • This PR is related to issue: N/A
  • 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:

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
@project-bot project-bot bot added this to Needs review in Dev Jan 22, 2020
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Jan 22, 2020

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!

@adamchengtkc

This comment has been minimized.

Copy link
Contributor

adamchengtkc commented Jan 22, 2020

I think this is a great addition. Polling of HA periodically crash my modbus TCP gateway. Would love to track the availability and do an automation with a ESP DC relay that trigger a power cycle.

I think the main thing to change is the syntax on catching multiple exception as required by pylint. Here is copied from http://pylint.pycqa.org/en/latest/technical_reference/features.html
"Exception to catch is the result of a binary "%s" operation Used when the exception to catch is of the form "except A or B:". If intending to catch multiple, rewrite as "except (A, B):"

@vzahradnik

This comment has been minimized.

Copy link
Contributor Author

vzahradnik commented Jan 22, 2020

@adamchengtkc technically I think that the first Exception is enough. I kept the rest just because it was in the original code. While I tested this, I noticed that the AttributeException is not always thrown, and the status is not reported to the state machine. For the time being, I will modify the code as you suggested.

However, with Modbus, there are more problems to solve. This one is just the most crucial for me. If the Modbus server goes down, Home Assistant log files grow very quickly. We're talking a few hundred kilobytes per second; if you have more Modbus hubs, they will grow even more. The pymodbus library doesn't use any delayed re-try mechanism.

@adamchengtkc

This comment has been minimized.

Copy link
Contributor

adamchengtkc commented Jan 22, 2020

I agree there is a lot to fix for modbus. For example my TCP modbus gateway can't handle the frequent polling of my 10+ sensors. Every minute or so the gateway will just drop some of the request. Which then fills up my HA log with errors

@vzahradnik vzahradnik force-pushed the vzahradnik:feature/modbus-availability branch from 36e496e to dab7445 Jan 22, 2020
@vzahradnik

This comment has been minimized.

Copy link
Contributor Author

vzahradnik commented Jan 22, 2020

Syntax for catch block adapted as suggested by @adamchengtkc.

@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Jan 23, 2020
@vzahradnik vzahradnik force-pushed the vzahradnik:feature/modbus-availability branch from dab7445 to 3b7bd9d Jan 26, 2020
@vzahradnik

This comment has been minimized.

Copy link
Contributor Author

vzahradnik commented Jan 26, 2020

I adapted the code as advised by @balloob. Also, I added connectivity checks for Modbus writes - otherwise errors are not catched, and are extensively logged.

At the moment, Modbus still generates a lot of errors due to loss of connectivity, but at least it's not in these components, which log the unavailable status only once now.

2020-01-26 12:02:58 ERROR (SyncWorker_4) [pymodbus.client.sync] Connection to (127.0.0.1, 5020) failed: [Errno 111] Connection refused
2020-01-26 12:02:58 ERROR (SyncWorker_7) [pymodbus.client.sync] Connection to (127.0.0.1, 5020) failed: [Errno 111] Connection refused
2020-01-26 12:02:58 ERROR (SyncWorker_6) [pymodbus.client.sync] Connection to (127.0.0.1, 5020) failed: [Errno 111] Connection refused

I will deal with the extensive Modbus logging in a separate pull request.

@vzahradnik vzahradnik force-pushed the vzahradnik:feature/modbus-availability branch 2 times, most recently from a7dda96 to 273aaa7 Feb 11, 2020
else:
result = self._hub.read_discrete_inputs(self._slave, self._address, 1)

def _set_unavailable():

This comment has been minimized.

Copy link
@balloob

balloob Feb 11, 2020

Member

Let's make this an instance method and not define it inside update.

This comment has been minimized.

Copy link
@vzahradnik

vzahradnik Feb 12, 2020

Author Contributor

Done.

result = self._hub.read_discrete_inputs(self._slave, self._address, 1)

def _set_unavailable():
if self._available:

This comment has been minimized.

Copy link
@balloob

balloob Feb 11, 2020

Member

Make this a guard clause. if not self._available: return

This comment has been minimized.

Copy link
@vzahradnik

vzahradnik Feb 12, 2020

Author Contributor

Done.

result = self._hub.read_coils(self._slave, self._address, 1)
else:
result = self._hub.read_discrete_inputs(self._slave, self._address, 1)
if isinstance(result, (ModbusException, ExceptionResponse)):

This comment has been minimized.

Copy link
@balloob

balloob Feb 11, 2020

Member

huh, exceptions are returned as results and not raised? 馃

This comment has been minimized.

Copy link
@vzahradnik

vzahradnik Feb 11, 2020

Author Contributor

This surprised me too. I don't know internal workings of this library. However, it was clear from the error logs that the result object got returned and we were trying to access an attribute. I got errors like ModbusIOException has no attribute register.

This comment has been minimized.

Copy link
@balloob

balloob Feb 11, 2020

Member

Ok, well it's what it is :(

This comment has been minimized.

Copy link
@vzahradnik

vzahradnik Feb 12, 2020

Author Contributor

I moved the isinstance(...) code below the Exception clause. I think it looks better now, and the try...except block now wraps only calls to pymodbus.

@vzahradnik vzahradnik force-pushed the vzahradnik:feature/modbus-availability branch from 273aaa7 to 0326db9 Feb 12, 2020
Dev automation moved this from Review in progress to Reviewer approved Feb 12, 2020
Copy link
Member

balloob left a comment

So clean now, nice! 馃帀

@balloob balloob merged commit 378c432 into home-assistant:dev Feb 12, 2020
10 checks passed
10 checks passed
CI Build #20200212.29 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 0700d38...0326db9
Details
codecov/project 94.66% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 12, 2020
@vzahradnik vzahradnik deleted the vzahradnik:feature/modbus-availability branch Feb 12, 2020
@lock lock bot locked and limited conversation to collaborators Feb 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can鈥檛 perform that action at this time.