Skip to content

Reconnect before mochad switch send command #11296

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
MartinHjelmare merged 10 commits into from
Jan 8, 2018
Merged

Reconnect before mochad switch send command #11296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9fbade8
Connection to mochad occasionally stalls on RPi and CM19A. Reconnect …
aosadchyy Dec 24, 2017
58be914
Formatting and exception hanling fixes
aosadchyy Dec 27, 2017
b1f5344
Moved import inside the method. Logging outside the try-catch.
aosadchyy Dec 28, 2017
aebb820
Tailing whitespaces.
aosadchyy Dec 28, 2017
811b45d
MockDependency on pymochad in unit tests to resolve exceptions
aosadchyy Dec 29, 2017
973e365
patch pymochad MochadException in unit tests to resolve exceptions
aosadchyy Jan 4, 2018
d935eb5
patch pymochad MochadException in unit tests to resolve exceptions
aosadchyy Jan 4, 2018
eb3da23
cleaned unused import
aosadchyy Jan 4, 2018
de1e7ea
lint issue fixed
aosadchyy Jan 4, 2018
d1650d1
pylint issue fixed
aosadchyy Jan 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions homeassistant/components/switch/mochad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,12 @@ def __init__(self, hass, ctrl, dev):
self._comm_type = dev.get(mochad.CONF_COMM_TYPE, 'pl')
self.device = device.Device(ctrl, self._address,
comm_type=self._comm_type)
self._state = self._get_device_status()
# Init with false to avoid locking HA for long on CM19A (goes from rf
# to pl via TM751, but not other way around)
if self._comm_type == 'pl':
self._state = self._get_device_status()
else:
self._state = False

@property
def name(self):
Expand All @@ -59,17 +64,37 @@ def name(self):

def turn_on(self, **kwargs):
"""Turn the switch on."""
self._state = True
from pymochad.exceptions import MochadException
_LOGGER.debug("Reconnect %s:%s", self._controller.server,
self._controller.port)
with mochad.REQ_LOCK:
self.device.send_cmd('on')
self._controller.read_data()
try:
# Recycle socket on new command to recover mochad connection
self._controller.reconnect()
self.device.send_cmd('on')
# No read data on CM19A which is rf only
if self._comm_type == 'pl':
self._controller.read_data()
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@aosadchyy aosadchyy Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@aosadchyy aosadchyy Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self._state = True
except (MochadException, OSError) as exc:
_LOGGER.error("Error with mochad communication: %s", exc)

def turn_off(self, **kwargs):
"""Turn the switch off."""
self._state = False
from pymochad.exceptions import MochadException
_LOGGER.debug("Reconnect %s:%s", self._controller.server,
self._controller.port)
with mochad.REQ_LOCK:
self.device.send_cmd('off')
self._controller.read_data()
try:
# Recycle socket on new command to recover mochad connection
self._controller.reconnect()
self.device.send_cmd('off')
# No read data on CM19A which is rf only
if self._comm_type == 'pl':
self._controller.read_data()
self._state = False
except (MochadException, OSError) as exc:
_LOGGER.error("Error with mochad communication: %s", exc)

def _get_device_status(self):
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@aosadchyy aosadchyy Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jan 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does ask the mochad device for status.

Copy link
Copy Markdown
Contributor Author

@aosadchyy aosadchyy Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Get the status of the switch from mochad."""
Expand Down
1 change: 1 addition & 0 deletions tests/components/switch/test_mochad.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def pymochad_mock():
"""Mock pymochad."""
with mock.patch.dict('sys.modules', {
'pymochad': mock.MagicMock(),
'pymochad.exceptions': mock.MagicMock(),
}):
yield

Expand Down