8000 Asyncify rfxtrx startup and event handling by elupus · Pull Request #38155 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Asyncify rfxtrx startup and event handling #38155

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

Jump to bottom
Merged
merged 4 commits into from
Jul 24, 2020
Merged

Asyncify rfxtrx startup and event handling #38155

merged 4 commits into from
Jul 24, 2020

Conversation

elupus
Copy link
Contributor
@elupus elupus commented Jul 24, 2020 •
edited
Loading

Proposed change

  • Change setup of platform to be mostly async, only go to executor when needed.
  • Fix call of async function from executor context

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

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

@probot-home-assistant
Copy link

Hey there @Danielhiversen, mind taking a look at this pull request as its been labeled with an integration (rfxtrx) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@MartinHjelmare MartinHjelmare changed the title Asyncify startup and event handling Asyncify rfxtrx startup and event handling Jul 24, 2020
MartinHjelmare
MartinHjelmare requested changes Jul 24, 2020
View reviewed changes
homeassistant/components/rfxtrx/__init__.py

def _shutdown_rfxtrx(event):
"""Close connection with RFXtrx."""
rfx_object.close_connection()

listener = hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, _shutdown_rfxtrx)
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_START, _start_rfxtrx)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to wait until home assistant start to set the callback on rfx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it can start providing events, and those trigger calls to different things. For example the event handlers on platforms will call async_write_ha_state which i don't think we want before start?

8000
Copy link
Member

Choose a reason for hiding this comment

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

It should be ok to use the entity state update callbacks as soon as the entity is added to home assistant. That doesn't need to wait until home assistant start.

It's good to make sure the integration is ready before the home assistant start event, to allow automations to use the integration on that event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is useable at that stage, just won't be getting any event updates until after the event. (you can trigger sends of commands). But i'll look at changing it. It wasn't as easy as just directly setting it. Since there are some hass.data variables that looks like they are needed to be set before we can accept events.

I'll look at it separately.

@MartinHjelmare
Copy link
Member
MartinHjelmare commented Jul 24, 2020 •
edited
Loading

Side note: I was browsing the code and found an extra, double, state update:

core/homeassistant/components/rfxtrx/switch.py

Lines 131 to 132 in 686e6b8

self._send_command("turn_on")
self.schedule_update_ha_state()

core/homeassistant/components/rfxtrx/__init__.py

Lines 487 to 521 in 686e6b8

def _send_command(self, command, brightness=0):
rfx_object = self.hass.data[DOMAIN][DATA_RFXOBJECT]
if command == "turn_on":
for _ in range(self.signal_repetitions):
self._device.send_on(rfx_object.transport)
self._state = True
elif command == "dim":
for _ in range(self.signal_repetitions):
self._device.send_dim(rfx_object.transport, brightness)
self._state = True
elif command == "turn_off":
for _ in range(self.signal_repetitions):
self._device.send_off(rfx_object.transport)
self._state = False
elif command == "roll_up":
for _ in range(self.signal_repetitions):
self._device.send_open(rfx_object.transport)
self._state = True
elif command == "roll_down":
for _ in range(self.signal_repetitions):
self._device.send_close(rfx_object.transport)
self._state = False
elif command == "stop_roll":
for _ in range(self.signal_repetitions):
self._device.send_stop(rfx_object.transport)
self._state = True
if self.hass:
self.schedule_update_ha_state()

@elupus
Copy link
Contributor Author
elupus commented Jul 24, 2020

yea i need to refactor the sending stuff. it's quite ugly now i think.

@elupus elupus merged commit 5d28e10 into home-assistant:dev Jul 24, 2020
@elupus elupus deleted the rftrx_async branch July 24, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

@Danielhiversen Danielhiversen

Labels
bugfix by-code-owner cla-signed code-quality integration: rfxtrx
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elupus @MartinHjelmare @Danielhiversen @homeassistant
0