Reconnect before mochad switch send command by aosadchyy · Pull Request #11296 · home-assistant/core

aosadchyy · 2017-12-24T21:34:23Z

Description:

Connection to mochad occasionally stalls on RPi and CM19A. Re-connect the underlying socket in controller.PyMochad when X10 swtich is set to on/off.
Current design of mochad in homeassistant is to keep 24/7 connection to mochad server open. There is no watchdog nor automatic reconnect routine.

There is a known issue for mochad to get locked up occasionally. e.g. as seen often on RPi.
https://sourceforge.net/p/mochad/discussion/1320003/thread/1c193f01/

When this happens, mochad can be recovered by restarting it. However, mochad.py components in home-assistant once connected on HA startup, won't ever reconnect.

This improvement allows to reconnect underlying socket in controller. PyMochad when X10 commands are sent to mochad server. It's tested to work fine on my HA.

Related issue (if applicable): fixes #
https://sourceforge.net/p/mochad/discussion/1320003/thread/1c193f01/

…one switch send command.

houndci-bot · 2017-12-24T21:34:31Z

+                self._state = False
+                # Recycle socket on new command to recover mochad connection
+                _LOGGER.debug("Reconnect %s:%s", self._controller.server,
+                                self._controller.port)


continuation line over-indented for visual indent

Ok. Fixed in commit 9fbade8

houndci-bot · 2017-12-24T21:34:31Z

+                self._state = True
+                # Recycle socket on new command to recover mochad connection
+                _LOGGER.debug("Reconnect %s:%s", self._controller.server,
+                                self._controller.port)


continuation line over-indented for visual indent

Ok. Fixed in commit 9fbade8

MartinHjelmare

Fix the indentation issues outlined by hound and see below.

MartinHjelmare · 2017-12-25T12:43:52Z

-            self.device.send_cmd('on')
-            self._controller.read_data()
+            try:
+                self._state = True


Put this at the end so that the state is not changed if there's an error in the communication with the device.

Ok. Fixed in commit 9fbade8

MartinHjelmare · 2017-12-25T12:44:43Z

+            try:
+                self._state = True
+                # Recycle socket on new command to recover mochad connection
+                _LOGGER.debug("Reconnect %s:%s", self._controller.server,


Move this up outside the try... except.

Ok. Fixed in commit 9fbade8

MartinHjelmare · 2017-12-25T12:51:49Z

+                # No read data on CM19A which is rf only
+                if (self._comm_type == 'pl'):
+                    self._controller.read_data()
+            except Exception as e:


Don't except all exceptions. Import MochadException from pymochad.exceptions and except that exception and OSError to catch socket errors.

Also change variable name to eg exc.

Ok. Fixed in commit 9fbade8

houndci-bot · 2017-12-27T06:34:21Z

 from homeassistant.const import (CONF_NAME, CONF_DEVICES,
                                 CONF_PLATFORM, CONF_ADDRESS)
 from homeassistant.helpers import config_validation as cv
+from pymochad.exceptions import MochadException 


trailing whitespace

Move this import to inside each method where the exception is used. Imports from libraries that are specified in the requirements list need to be done inside each method since the requirements will be installed and made available during setup, ie not before starting home assistant.

Moved it as requested. See commit b1f5344

MartinHjelmare · 2017-12-27T12:12:59Z

 from homeassistant.const import (CONF_NAME, CONF_DEVICES,
                                 CONF_PLATFORM, CONF_ADDRESS)
 from homeassistant.helpers import config_validation as cv
+from pymochad.exceptions import MochadException 


Move this import to inside each method where the exception is used. Imports from libraries that are specified in the requirements list need to be done inside each method since the requirements will be installed and made available during setup, ie not before starting home assistant.

MartinHjelmare · 2017-12-27T12:15:47Z

-            self._controller.read_data()
+            try:
+                # Recycle socket on new command to recover mochad connection
+                _LOGGER.debug("Reconnect %s:%s", self._controller.server,


This should still be move up outside try... except. Put as little code as possible within try... except, ie only the code that's relevant to catch.

Moved it as requested. See commit b1f5344

houndci-bot · 2017-12-28T04:13:44Z

    def turn_off(self, **kwargs):
        """Turn the switch off."""
-        self._state = False
+        from pymochad.exceptions import MochadException 


trailing whitespace

houndci-bot · 2017-12-28T04:13:45Z

    def turn_on(self, **kwargs):
        """Turn the switch on."""
-        self._state = True
+        from pymochad.exceptions import MochadException 


trailing whitespace

MartinHjelmare · 2017-12-28T12:06:07Z

There are some linting issues and you need to mock the dependency pymochad.exceptions in the tests for the switch.

aosadchyy · 2017-12-29T05:11:32Z

Regarding mocking the exceptions for switches. do you have any example? I looked in home-assistant\tests\components\switch\ and didn't see any.

MartinHjelmare · 2017-12-29T07:05:13Z

I think you can extend the existing pymochad mock fixture and patch pymochad.exceptions in sys.modules.

houndci-bot · 2018-01-04T06:13:46Z

 from homeassistant.components.switch import mochad

-from tests.common import get_test_home_assistant
+from tests.common import (


'tests.common.MockDependency' imported but unused

houndci-bot · 2018-01-04T06:13:46Z

 from homeassistant.components.switch import mochad

-from tests.common import get_test_home_assistant
+from tests.common import (


'tests.common.MockDependency' imported but unused

MartinHjelmare

Please clarify if it's possible to poll the mochad device for state, and why that isn't done currently for each service call. It seems some devices don't support this, but some do. But I think the current implementation isn't polling for state when it should do that.

MartinHjelmare · 2018-01-05T08:00:57Z

+                self.device.send_cmd('on')
+                # No read data on CM19A which is rf only
+                if self._comm_type == 'pl':
+                    self._controller.read_data()


Why is this done without handling the return value? Shouldn't the return value be used to update state?

Original code has read_data() after each send_cmd(). This works for pl, but not for rf.
pl = powerline devices (like appliance switches). rf = wireless devices (mostly sensors). When sending data to pl devices = they respond back with the state. rf devices do not do that. CM19A unlike CM15A, handles all commands as rf. It relies on a separate translator device - TM751 for rf->pl translation. And it's only one way. There is no read back. The original code gets in stuck on read_data() for me until any data shows up from sensors in the buffer. Making it conditional fixes the problem without affecting CM15A behaviours.

I understand that RF devices don't support this call, but I don't understand why the call is done at all, when the return value is not handled.

This call is done for pl devices in order to clear the receive buffer. When pl device is set to On/Off state, it responds back with it's new value. This read command reads it and ignores it. The state is already known on hass side.

Ok, the empty read buffer makes sense, but since the device responds with the new state, that should be used in home assistant. Home assistant doesn't know the new state for sure. Sending a command to the device over some kind of network transport doesn't guarantee that the device receives the command or changes state. We can only be sure after the device has replied to home assistant with its new state. So whenever possible, a device should feedback its state to home assistant when that changes.

MartinHjelmare · 2018-01-05T08:01:29Z

+            except (MochadException, OSError) as exc:
+                _LOGGER.error("Error with mochad communication: %s", exc)

    def _get_device_status(self):


It seems _get_device_status can be used to update state, but why isn't that called in an update method? Then the new state would automatically be polled after each service call.

get_device_status is shortcut to the parent SwitchDevice.get_status(). It doesn't make a trip to Mochad and the device. Rather it's a status of the device in hass. No need to change anything here. Not my code. But I did a little research. Mochad supports status_request for pl commands, but not for rf. https://bfocht.github.io/mochad/mochad_reference.html I think the reasoning behind this original code is as I described in my above comment.

It does ask the mochad device for status.

Yes, you're correct. I might invest more time into all this x10 stuff at some later point. For now, whatever changes I made here, work for my home with ca 20 x10 devices of different kinds. I can also do bug fix if anyone reports an issue.

MartinHjelmare

This PR is ok and for RF devices that can't feedback state my comments don't matter. But it would be good if someone knowledgeable about this platform could take a look. I don't think the current implementation is doings things correct.

Connection to mochad occasionally stalls on RPi and CM19A. Reconnect …

9fbade8

…one switch send command.

homeassistant added platform: switch.mochad cla-signed labels Dec 24, 2017

houndci-bot reviewed Dec 24, 2017

View reviewed changes

MartinHjelmare changed the title ~~Connection to mochad occasionally stalls on RPi and CM19A. Reconnect on...~~ Reconnect before mochad switch send command Dec 25, 2017

MartinHjelmare requested changes Dec 25, 2017

View reviewed changes

Formatting and exception hanling fixes

58be914

homeassistant added the cla-signed label Dec 27, 2017

houndci-bot reviewed Dec 27, 2017

View reviewed changes

MartinHjelmare requested changes Dec 27, 2017

View reviewed changes

Moved import inside the method. Logging outside the try-catch.

b1f5344

houndci-bot reviewed Dec 28, 2017

View reviewed changes

Tailing whitespaces.

aebb820

MockDependency on pymochad in unit tests to resolve exceptions

811b45d

patch pymochad MochadException in unit tests to resolve exceptions

973e365

houndci-bot reviewed Jan 4, 2018

View reviewed changes

aosadchyy added 4 commits January 3, 2018 22:30

patch pymochad MochadException in unit tests to resolve exceptions

d935eb5

cleaned unused import

eb3da23

lint issue fixed

de1e7ea

pylint issue fixed

d1650d1

MartinHjelmare reviewed Jan 5, 2018

View reviewed changes

MartinHjelmare approved these changes Jan 8, 2018

View reviewed changes

MartinHjelmare merged commit 2ba9d82 into home-assistant:dev Jan 8, 2018

balloob mentioned this pull request Jan 11, 2018

0.61 #11589

Merged

home-assistant locked and limited conversation to collaborators May 29, 2018

ghost removed the platform: switch.mochad label Mar 21, 2019

ghost added the integration: mochad label Mar 21, 2019

Uh oh!

Conversation

aosadchyy commented Dec 24, 2017 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Dec 28, 2017

Uh oh!

aosadchyy commented Dec 29, 2017

Uh oh!

MartinHjelmare commented Dec 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aosadchyy Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

aosadchyy commented Dec 24, 2017 •

edited by MartinHjelmare

Loading

aosadchyy Jan 9, 2018 •

edited

Loading