Skip to content

ESPHome trigger reconnect immediately when mDNS record received#48129

Merged
bdraco merged 7 commits intohome-assistant:devfrom
OttoWinter:esphome-mdns-reconnect
Mar 21, 2021
Merged

ESPHome trigger reconnect immediately when mDNS record received#48129
bdraco merged 7 commits intohome-assistant:devfrom
OttoWinter:esphome-mdns-reconnect

Conversation

@OttoWinter
Copy link
Copy Markdown
Member

@OttoWinter OttoWinter commented Mar 19, 2021

Proposed change

Problem: ESPHome's native API has been difficult to use with the deep sleep feature, because home assistant only checks every 60s if the device is online again after sleep, so the device has to stay awake longer until HA connects and wasting power.

Idea: I had an idea yesterday: actually HA already knows when a device comes up again, because on startup the device broadcasts mDNS records on the network.

This PR: So this change implements an mDNS/zeroconf listener that triggers the reconnection logic immediately when such a DNS record is received.

In the process, I also refactored the logic into a new class ReconnectLogic because the code would have gotten messy otherwise. Note: not happy with the tries/connected/reconnect_event variables, but could not find more suitable synchronization primitives

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: none
  • This PR is related to issue: none
  • 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 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.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver (I think?)
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 20, 2021

Can we reload the config entry instead when it gets rediscovered via zeroconf?

#47683 now passes mdns updates and triggers zeroconf discovery

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 20, 2021

Actually it looks like that will already happen now since #34753 updates the ip address and #47683 will allow the updates to come through.

@OttoWinter
Copy link
Copy Markdown
Member Author

Hi,

Unfortunately, that does not work already and would not work.

The problem is that HA internally uses the zeroconf ServiceBrowser interface for catching these events. However, this interface only calls the handler when a new record is received and the matching old one is already expired (source), but the timescale we're working with here is often less than the TTL (with short deep sleep phases) but we would still like to receive the update. Hence also why I need to use RecordUpdateListener instead of ServiceBrowser, because I need direct access to the records coming in. I know it's not perfect, but I believe this is the best way for the logic to be informed of matching PTR records.

Also, it doesn't work already because the #34753 PR only updates the config entry data to set the host, but otherwise does not inform the reconnect logic.

If you're worried about resource usage with ZC, this is not an issue because the RecordUpdateListener interface really just forwards events from the Zeroconf object and does not start a new thread in constrast to ServiceBrowser.

Comment thread homeassistant/components/esphome/__init__.py Outdated
Comment thread homeassistant/components/esphome/__init__.py Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 20, 2021

Hi,

Unfortunately, that does not work already and would not work.

The problem is that HA internally uses the zeroconf ServiceBrowser interface for catching these events. However, this interface only calls the handler when a new record is received and the matching old one is already expired (source), but the timescale we're working with here is often less than the TTL (with short deep sleep phases) but we would still like to receive the update. Hence also why I need to use RecordUpdateListener instead of ServiceBrowser, because I need direct access to the records coming in. I know it's not perfect, but I believe this is the best way for the logic to be informed of matching PTR records.

Also, it doesn't work already because the #34753 PR only updates the config entry data to set the host, but otherwise does not inform the reconnect logic.

If you're worried about resource usage with ZC, this is not an issue because the RecordUpdateListener interface really just forwards events from the Zeroconf object and does not start a new thread in constrast to ServiceBrowser.

Makes sense. Thanks for the through explanation.

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM. Two small suggestions above.

@OttoWinter
Copy link
Copy Markdown
Member Author

Ok, updated per suggestions. 👍
Also added a new wait_task variable to track the waiting task so that we don't delay HA shutdown.

Comment thread homeassistant/components/esphome/__init__.py Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 20, 2021

Looks good. Will do some manual testing later today

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 21, 2021

2021-03-21 00:46:53 DEBUG (Thread-7) [homeassistant.components.dhcp] Processing updated address data for 192.168.106.243: mac=D8F15BD6030E hostname=maui_es015_tb_c

Lots of plugging and unplugging and it reconnects just about right after it gets an ip address 👍

This looks good

@bdraco bdraco merged commit 0193f16 into home-assistant:dev Mar 21, 2021
if self._wait_task is not None:
return

self._wait_task = self._hass.loop.create_task(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can use asyncio.create_task instead.

while True:
try:
await self._reconnect_once()
except asyncio.CancelledError: # pylint: disable=try-except-raise
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From Python 3.8 CancelledError inherits BaseException instead of Exception. So we don't need this case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not to catch CancelledExceptions, but rather so that if the reconnect task is cancelled (meaning the reconnect loop should stop) we will actually cancel the loop, not just log the error and continue forever.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, except Exception: won't catch asyncio.CancelledError.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah sorry, yes now I understand.

Thanks, didn't know that! Will fix this later.

"""Start the reconnecting logic background task."""
# Create reconnection loop outside of HA's tracked tasks in order
# not to delay startup.
self._loop_task = self._hass.loop.create_task(self._reconnect_loop())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use asyncio.create_task.

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants