Skip to content

Extract PingSource and shared cross-cutting helpers#850

Merged
bdraco merged 2 commits into
mainfrom
split-monitor-ping
May 14, 2026
Merged

Extract PingSource and shared cross-cutting helpers#850
bdraco merged 2 commits into
mainfrom
split-monitor-ping

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 14, 2026

What does this implement/fix?

Third PR in the device-state-monitor split arc — see device_state_monitor_split.md plan (untracked working-tree doc, content mirrored below). Mechanical-only structural extraction per feedback_split_then_clean_strict.

Mirrors the legacy esphome/dashboard/status/ping.py shape: a PingSource class taking the monitor in __init__, owning the ICMP loop and per-device probe.

ping.py — new PingSource class:

  • run — was _ping_loop; bootstrap → presence-gated resolve → sweep → interval wait.
  • _ping_sweep — batched DNS pre-resolve + per-batch asyncio.gather of _ping_device.
  • _select_ping_targets — filters the device list against the shared source-precedence rules + DNS-failure cache.
  • _ping_device — single ICMP probe + RTT capture + apply(ONLINE/OFFLINE, "ping").

Constants _PING_INTERVAL / _PING_BOOTSTRAP_DELAY / _PING_BATCH_SIZE and the icmp_ping / ICMPLibError imports move to ping.py.

shared.py — new module for cross-cutting bits that straddle the mdns and ping concerns (per the plan's resolved question #1):

  • should_ping(monitor, device) — source-precedence rule read by both the ping target selector and the active-resolve candidate filter.
  • apply_resolved_addresses(monitor, name, addresses) — the "non-empty list → claim mDNS-ONLINE + record IPs" funnel, called from both the browser-callback refresh path and the active-resolve batch.
  • resolve_non_api_mdns_targets(monitor) — was _resolve_non_api_mdns_targets; mirrors the legacy async_refresh_hosts poll path.
  • _SOURCE_PRIORITY ledger and _MDNS_HOSTNAME_RESOLVE_TIMEOUT constant.

Free functions taking the monitor — same shape as the firmware-sync helpers. controller.py imports _MDNS_HOSTNAME_RESOLVE_TIMEOUT / _SOURCE_PRIORITY back for its remaining inline uses (refresh_mdns and the apply() family).

controller.py wiring: __init__ now builds self._ping = PingSource(self); start() schedules asyncio.create_task(self._ping.run()). refresh_mdns's single _apply_resolved_addresses call becomes apply_resolved_addresses(self, …).

Test redirects (mechanical):

  • monitor._ping_loop()monitor._ping.run().
  • monitor._ping_sweepmonitor._ping._ping_sweep.
  • monitor._ping_devicemonitor._ping._ping_device.
  • monitor._should_pingshared.should_ping(monitor, …).
  • monitor._resolve_non_api_mdns_targetsshared.resolve_non_api_mdns_targets(monitor).
  • Patches against controller.icmp_ping / controller._PING_BOOTSTRAP_DELAY / controller.asyncio.sleep redirect to the ping module.
  • caplog.at_level(…, logger=state_monitor_module.__name__)ping_module.__name__ for the two log-line assertions whose log now fires from ping.py.
  • __new__-built test fixtures now wire monitor._ping = PingSource(monitor) after constructing MonitorState.

Sizes: controller.py 1453 → 1143 lines. ping.py 243 lines. shared.py 154 lines. Full suite: 3424 passed.

Related issue or feature (if applicable):

  • fixes none — third PR in a planned multi-PR split arc

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed
  • Companion frontend PR: esphome/device-builder-dashboard-frontend#

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.json has not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

Third PR in the device-state-monitor split arc. Mirrors the legacy
``esphome/dashboard/status/ping.py`` shape: a :class:`PingSource`
taking the monitor in ``__init__``, owning the ICMP loop and
per-device probe.

**``ping.py``** — new ``PingSource`` class with methods:

* ``run`` — was ``_ping_loop``; bootstrap → presence-gated
  resolve → sweep → interval wait.
* ``_ping_sweep`` — batched DNS pre-resolve + per-batch
  ``asyncio.gather`` of ``_ping_device``.
* ``_select_ping_targets`` — filters the device list against the
  shared source-precedence rules + DNS-failure cache.
* ``_ping_device`` — single ICMP probe + RTT capture +
  apply(ONLINE/OFFLINE, "ping").

Constants ``_PING_INTERVAL`` / ``_PING_BOOTSTRAP_DELAY`` /
``_PING_BATCH_SIZE`` and the ``icmp_ping`` / ``ICMPLibError``
imports move to ping.py.

**``shared.py``** — new module for cross-cutting bits that
straddle the mdns and ping concerns:

* ``should_ping(monitor, device)`` — source-precedence rule
  read by both the ping target selector and the active-resolve
  candidate filter.
* ``apply_resolved_addresses(monitor, name, addresses)`` — the
  "non-empty list → claim mDNS-ONLINE + record IPs" funnel,
  called from both the browser-callback refresh path and the
  active-resolve batch.
* ``resolve_non_api_mdns_targets(monitor)`` — was
  ``_resolve_non_api_mdns_targets``; mirrors the legacy
  ``async_refresh_hosts`` poll path.
* ``_SOURCE_PRIORITY`` ledger and
  ``_MDNS_HOSTNAME_RESOLVE_TIMEOUT`` constant.

Free functions taking the monitor — same shape as the
firmware-sync helpers. controller.py imports
``_MDNS_HOSTNAME_RESOLVE_TIMEOUT`` / ``_SOURCE_PRIORITY`` back
for its remaining inline uses (``refresh_mdns`` and the
``apply()`` family).

**controller.py wiring**: ``__init__`` now builds
``self._ping = PingSource(self)``; ``start()`` schedules
``asyncio.create_task(self._ping.run())``. ``refresh_mdns``'s
single ``_apply_resolved_addresses`` call becomes
``apply_resolved_addresses(self, ...)``.

**Test redirects** (mechanical):

* ``monitor._ping_loop()`` → ``monitor._ping.run()``.
* ``monitor._ping_sweep`` → ``monitor._ping._ping_sweep``.
* ``monitor._ping_device`` → ``monitor._ping._ping_device``.
* ``monitor._should_ping`` → ``shared.should_ping(monitor, ...)``.
* ``monitor._resolve_non_api_mdns_targets`` →
  ``shared.resolve_non_api_mdns_targets(monitor)``.
* Patches against ``controller.icmp_ping`` /
  ``controller._PING_BOOTSTRAP_DELAY`` /
  ``controller.asyncio.sleep`` redirect to the ``ping`` module.
* ``caplog.at_level(..., logger=state_monitor_module.__name__)``
  redirects to ``ping_module.__name__`` for the two log-line
  assertions whose log now fires from ping.py.
* ``__new__``-built test fixtures now wire
  ``monitor._ping = PingSource(monitor)`` after constructing
  ``MonitorState``.

controller.py: 1453 → 1143 lines.
Copilot AI review requested due to automatic review settings May 14, 2026 15:53
@github-actions github-actions Bot added the refactor Code refactor with no user-visible behaviour change label May 14, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.32%. Comparing base (7c3dabb) to head (7daf17f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #850   +/-   ##
=======================================
  Coverage   99.31%   99.32%           
=======================================
  Files         181      183    +2     
  Lines       13378    13413   +35     
=======================================
+ Hits        13287    13322   +35     
  Misses         91       91           
Flag Coverage Δ
py3.12 99.29% <100.00%> (+<0.01%) ⬆️
py3.14 99.32% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...er/controllers/_device_state_monitor/controller.py 98.31% <100.00%> (-0.34%) ⬇️
..._builder/controllers/_device_state_monitor/ping.py 100.00% <100.00%> (ø)
...uilder/controllers/_device_state_monitor/shared.py 100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 14, 2026

Merging this PR will not alter performance

✅ 25 untouched benchmarks


Comparing split-monitor-ping (7daf17f) with main (91380b8)

Open in CodSpeed

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Extracts the ICMP ping loop and shared mDNS/ping precedence helpers out of DeviceStateMonitor, reducing controller size while preserving behavior.

Changes:

  • Adds PingSource for ping-loop scheduling, target selection, DNS pre-resolution, ICMP probing, and RTT recording.
  • Adds shared.py for source precedence, resolved-address application, and non-API mDNS active resolution.
  • Redirects tests and monkeypatches to the new module boundaries.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
esphome_device_builder/controllers/_device_state_monitor/controller.py Wires PingSource and delegates shared resolved-address handling.
esphome_device_builder/controllers/_device_state_monitor/ping.py New ping source implementation extracted from the controller.
esphome_device_builder/controllers/_device_state_monitor/shared.py New shared helper module for mDNS/ping cross-cutting logic.
tests/test_dns_cache.py Redirects ping sweep and ICMP patches to PingSource/ping.py.
tests/test_non_api_mdns_resolve.py Redirects non-API mDNS resolve tests to shared.py.
tests/test_ping_loop_pause.py Redirects ping loop pause tests to PingSource.run.
tests/test_probe_device.py Updates manual monitor fixtures to include PingSource.
tests/test_state_monitor_lifecycle.py Redirects lifecycle ping patches/log assertions to ping.py.
tests/test_state_monitor_reachability.py Redirects ping-device tests to PingSource.
Comments suppressed due to low confidence (2)

esphome_device_builder/controllers/_device_state_monitor/ping.py:53

  • CLAUDE.md lines 34-43 require multi-line docstrings to put their content on the line after the opening triple quotes. This class docstring spans multiple lines but starts the summary inline, so it doesn't follow the repository docstring convention.
    """ICMP ping loop owning the periodic sweep and per-device probe.

esphome_device_builder/controllers/_device_state_monitor/shared.py:93

  • CLAUDE.md lines 34-43 require multi-line docstrings to put their content on the line after the opening triple quotes. This function docstring spans multiple lines but starts the summary inline, so it doesn't follow the repository docstring convention.
    """Actively resolve ``.local`` hostnames for non-API devices.

@@ -0,0 +1,243 @@
"""ICMP ping fallback source for the device-state monitor.
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.

Fixed both sites in 7daf17f — moved the module docstring summary and the PingSource class docstring summary to the line after """. These were new content I authored in this PR, so CLAUDE.md applies cleanly.

The two function docstrings on this file (run, _ping_sweep, _select_ping_targets, _ping_device) are byte-for-byte moves from controller.py and keep their pre-existing inline-summary style — left as-is for this structural-split PR per feedback_split_then_clean_strict; they'll get tightened in the per-file cleanup PR that follows (same cadence as the firmware-cleanup arc #810 etc.).

Comment thread esphome_device_builder/controllers/_device_state_monitor/shared.py Outdated
Comment thread esphome_device_builder/controllers/_device_state_monitor/shared.py
Per CLAUDE.md "Code style" section: multi-line docstrings put content
on the line after ``"""``, not inline with the opening triple-quote.

Fixes the three new-content docstrings I authored in this split PR
(ping.py module, shared.py module, ``PingSource`` class). The
moved function docstrings stay byte-for-byte for now — pre-existing
inline-summary style; defer to the cleanup PR per the strict
split-then-clean cadence.

Per Copilot review on PR #850.
@bdraco bdraco merged commit 804d1bb into main May 14, 2026
13 checks passed
@bdraco bdraco deleted the split-monitor-ping branch May 14, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Code refactor with no user-visible behaviour change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants