Skip to content

Refactor rfxtrx#9117

Merged
pvizeli merged 8 commits into
devfrom
refactor_rfxtrx
Aug 29, 2017
Merged

Refactor rfxtrx#9117
pvizeli merged 8 commits into
devfrom
refactor_rfxtrx

Conversation

@Danielhiversen
Copy link
Copy Markdown
Member

@Danielhiversen Danielhiversen commented Aug 24, 2017

Description:

@pvizeli : Could you have look at this?
For reference: #6844 (comment)

@mention-bot
Copy link
Copy Markdown

mention-bot commented Aug 24, 2017

@Danielhiversen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @badele and @turbokongen to be potential reviewers.

pvizeli
pvizeli requested changes Aug 24, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

We have 2 Options:

  1. Add the device callback subscription to async_added_to_hass
  2. Run all device communication (update) on EVENT_HOMEASSISTANT_START

Comment thread homeassistant/components/rfxtrx.py Outdated
@asyncio.coroutine
def async_added_to_hass(self):
"""Register callbacks."""
self._added_to_hass = True
Copy link
Copy Markdown
Member

@pvizeli pvizeli Aug 24, 2017

Choose a reason for hiding this comment

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

Add your device call back subscription on that pleace like sonos

Copy link
Copy Markdown
Member Author

@Danielhiversen Danielhiversen Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The device call back subscription is done only once for all the switches (and the same for lights and sensors), and should not be done for each switch.
So do not know how to solve that?

Copy link
Copy Markdown
Member

@pvizeli pvizeli Aug 25, 2017

Choose a reason for hiding this comment

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

You can also subscribe that on EVENT_HOMEASSISTANT_START

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 25, 2017

You need work like update(). That will be first call after EVENT_HOMEASSISTANT_START. But you can follow this guide:
-> First, the setup is only for initialize the device and mabye set a init state. We should not process data on that level.
-> If you have a global subscribe/start function do that on EVENT_HOMEASSISTANT_START
-> If you have a per device subscribe/start function you can also use async_added_to_hass

houndci-bot
houndci-bot reviewed Aug 25, 2017
View reviewed changes
Comment thread homeassistant/components/rfxtrx.py Outdated
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/rfxtrx/
"""
import asyncio
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

'asyncio' imported but unused

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 25, 2017 •
edited
Loading

Last point is to remove global and use hass.data and everthing is perfect and all errors gone away :)

@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 25, 2017

By the way #9058 was my first component and I do the same at the moment :)

houndci-bot
houndci-bot reviewed Aug 25, 2017
View reviewed changes
Comment thread homeassistant/components/rfxtrx.py Outdated
elif command == "stop_roll":
for _ in range(self.signal_repetitions):
self._event.device.send_stop(RFXOBJECT.transport)
self._event.device.send_stop(self.hass.data[RFXOBJECT].transport)
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

Comment thread homeassistant/components/rfxtrx.py Outdated
elif command == "roll_down":
for _ in range(self.signal_repetitions):
self._event.device.send_close(RFXOBJECT.transport)
self._event.device.send_close(self.hass.data[RFXOBJECT].transport)
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

Comment thread homeassistant/components/rfxtrx.py Outdated
elif command == "roll_up":
for _ in range(self.signal_repetitions):
self._event.device.send_open(RFXOBJECT.transport)
self._event.device.send_open(self.hass.data[RFXOBJECT].transport)
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

Comment thread homeassistant/components/rfxtrx.py Outdated
elif command == 'turn_off':
for _ in range(self.signal_repetitions):
self._event.device.send_off(RFXOBJECT.transport)
self._event.device.send_off(self.hass.data[RFXOBJECT].transport)
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

Comment thread homeassistant/components/rfxtrx.py Outdated
elif command == "dim":
for _ in range(self.signal_repetitions):
self._event.device.send_dim(RFXOBJECT.transport,
self._event.device.send_dim(self.hass.data[RFXOBJECT].transport,
Copy link
Copy Markdown

@houndci-bot houndci-bot Aug 25, 2017

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

@Danielhiversen Danielhiversen changed the title WIP: Refactor rfxtrx Refactor rfxtrx Aug 25, 2017
@Danielhiversen
Copy link
Copy Markdown
Member Author

Danielhiversen commented Aug 27, 2017

@pvizeli Is this fine now?

@pvizeli pvizeli merged commit ee28b43 into dev Aug 29, 2017
@pvizeli pvizeli deleted the refactor_rfxtrx branch August 29, 2017 14:22
@pvizeli
Copy link
Copy Markdown
Member

pvizeli commented Aug 29, 2017 •
edited
Loading

Looks good 👍

Small improvment for future. Get the device object from hass.data first on top and work with that pointer instead of access every time over the dict to this pointer.

matemaciek added a commit to matemaciek/home-assistant that referenced this pull request Aug 30, 2017
@matemaciek
Merge remote-tracking branch 'upstream/dev' into dev
e8eee97 
* upstream/dev: (113 commits)
  Fix fitbit error when trying to access token after upgrade. (home-assistant#9183)
  Upgrade sendgrid to 5.0.1 (home-assistant#9215)
  Upgrade pyasn1 to 0.3.3 and pyasn1-modules to 0.1.1 (home-assistant#9216)
  directv: extended discovery via REST api, bug fix  (home-assistant#8800)
  Bayesian Binary Sensor (home-assistant#8810)
  Add cloud auth support (home-assistant#9208)
  Abode push events and lock, cover, and switch components (home-assistant#9095)
  Lint Sonarr tests
  Upgrade pymysensors to 0.11.1 (home-assistant#9212)
  Refactor rfxtrx (home-assistant#9117)
  Issue home-assistant#6893 in rfxtrx (home-assistant#9130)
  Support for season sensor (home-assistant#8958)
  Add counter component (home-assistant#9146)
  Fix and optimize digitalloggers platform (home-assistant#9203)
  Prevent error when no forecast data was available (home-assistant#9176)
  Add "status" to Sonarr sensor (home-assistant#9204)
  fix worldtidesinfo home-assistant#9184 (home-assistant#9201)
  Update pushbullet.py (home-assistant#9200)
  Fix dht22 when no data was read initially home-assistant#8976 (home-assistant#9198)
  Prevent iCloud exceptions in logfile (home-assistant#9179)
  ...
@balloob balloob mentioned this pull request Sep 7, 2017
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pvizeli pvizeli pvizeli approved these changes

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed component integration: rfxtrx platform Ready for review requirement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Danielhiversen @mention-bot @pvizeli @houndci-bot @balloobbot @homeassistant