Skip to content

Recover a consumer's bus_id from its source block - #1757

Merged
bdraco merged 1 commit into
mainfrom
bus-recover-consumer-id
Jun 30, 2026
Merged

Recover a consumer's bus_id from its source block#1757
bdraco merged 1 commit into
mainfrom
bus-recover-consumer-id

Conversation

@bdraco

@bdraco bdraco commented Jun 30, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

The last 3 boards allow-listed by #1755kincony_ag8 (uart), kincony_kc868_e8t
(uart), kincony_mb (i2c) — shipped a full-setup config that wouldn't compile
because their bus was never lifted. The allow-list comments claimed "source has no
top-level block", but the live devices.esphome.io pages (and the cached source)
show the bus is there, with explicit ids, in the same code block as the leaf —
so that was a misdiagnosis. The real cause is an importer bug.

Each board has multiple same-platform leaves with no id: on the consumer
block (2 switch.uart, 2 sensor.bl0939, 16 sensor.ina226), each binding a bus
via uart_id:/i2c_id:. That ref is dropped from the finalized entry (the bus
isn't a featured component yet), so _collect_bus_dep_refs re-finds the source
block via _find_consumer_block — which filters by platform, gets N id-less
blocks, can't disambiguate, and returns None. With the ref lost and multiple
buses present, _materialize_bus can't pick one, so the bus never lifts.

Fix: _Candidate already retains the source block and each finalized entry's id
equals its unique local_id, so stash the source block on the entry at finalize,
read it back in _collect_bus_dep_refs (falling back to _find_consumer_block
for entries built outside the extractor, e.g. unit tests), and strip the transient
key in _make_record before serialization. Each consumer's <bus>_id is now
recovered directly: kincony_mb's 16 ina226 split correctly across bus_a/bus_b,
ag8/e8t wire each leaf to its own uart.

This empties the guard's _UNSATISFIED_BUS_ALLOW_LIST — every imported board's
featured bus dependencies now lift, and validate_definitions.py proves it (0
errors). Regenerated against ESPHome 2026.6.3; the diff is exactly the 3 boards
gaining their uart/i2c entries + requires.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix

Frontend coordination

  • No frontend change needed

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.index.json / definitions/components/*.json have not been hand-edited.
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

@bdraco bdraco added the bugfix Bug fix label Jun 30, 2026
@bdraco
bdraco marked this pull request as ready for review June 30, 2026 01:25
Copilot AI review requested due to automatic review settings June 30, 2026 01:25
@codspeed-hq

codspeed-hq Bot commented Jun 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing bus-recover-consumer-id (b5fcf67) with main (9acd745)

Open in CodSpeed

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (9acd745) to head (b5fcf67).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1757   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         228      228           
  Lines       18203    18203           
=======================================
  Hits        18121    18121           
  Misses         82       82           
Flag Coverage Δ
py3.12 99.52% <ø> (ø)
py3.14 99.54% <ø> (ø)

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes a bug in the devices.esphome.io importer where bus dependencies (i2c/uart) could not be lifted for id-less, same-platform consumers by recovering the consumer’s <bus>_id directly from its original source block.

Changes:

  • Stash each finalized featured entry’s source YAML block transiently and use it to recover <bus>_id during bus dependency collection (then strip before serialization).
  • Update validation/test coverage to treat _UNSATISFIED_BUS_ALLOW_LIST as a per-(board,bus) allow-list and confirm the list can be empty.
  • Regenerate KinCony board manifests/bodies so required i2c/uart components are featured and wired via requires.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
script/sync_esphome_devices.py Stashes _source_block on finalized entries, uses it to recover <bus>_id in _collect_bus_dep_refs, and strips it before writing records.
script/validate_definitions.py Converts the unsatisfied-bus allow-list to a typed, currently-empty frozenset and keeps per-(board,bus) semantics.
tests/test_sync_esphome_devices_bus_dep.py Adds a regression test ensuring bus id recovery works when _find_consumer_block can’t disambiguate id-less same-platform consumers.
tests/test_validate_featured.py Updates tests to monkeypatch the allow-list as (board, bus) pairs (and not rely on real board ids).
esphome_device_builder/definitions/boards/kincony_mb/manifest.yaml Adds featured i2c buses (bus_a/bus_b) and stamps requires on INA226 sensors; updates occupied pins.
esphome_device_builder/definitions/boards/kincony_kc868_e8t/manifest.yaml Adds featured UARTs (uart_1/uart_2) and stamps requires on BL0939 sensors; updates occupied pins.
esphome_device_builder/definitions/boards/kincony_ag8/manifest.yaml Adds featured UART (uart_232) and stamps requires on the UART switch; updates occupied pins.
esphome_device_builder/definitions/board_bodies/kincony_mb.json Regenerated body including i2c buses and per-sensor requires.
esphome_device_builder/definitions/board_bodies/kincony_kc868_e8t.json Regenerated body including UARTs and per-sensor requires.
esphome_device_builder/definitions/board_bodies/kincony_ag8.json Regenerated body including UART and requires.

@esphbot

esphbot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

ag8/e8t/mb have multiple same-platform leaves (2 switch.uart, 2 bl0939, 16
ina226) with no id: on the consumer block, so _find_consumer_block can't tell
them apart, the <bus>_id ref is lost, and with multiple buses present
_materialize_bus can't pick one, so the bus never lifts. _Candidate already
keeps the source block and each entry's id equals its local_id, so stash the
source block on the finalized entry, read it in _collect_bus_dep_refs (falling
back to _find_consumer_block), and strip it in _make_record before serialization.
Empties the guard's allow list; regenerated against ESPHome 2026.6.3.
@bdraco
bdraco force-pushed the bus-recover-consumer-id branch from 433ff1e to b5fcf67 Compare June 30, 2026 01:33
@bdraco

bdraco commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

Thanks @esphbot — applied your suggestion 1 (read-and-discard) in b5fcf678.

_collect_bus_dep_refs is the sole reader of _source_block, so it now pops the key at the top of its loop (before any continue) and uses the popped value, and the separate strip loop in _make_record is gone. The transient key is removed at the moment it's consumed, for every entry, independent of how the entries are later serialized — closing the 'future serialization path leaks it' risk. Updated the _finalize_entry comment to point at the new consumption site.

Behavior-preserving: the regenerated catalog is byte-identical, no _source_block in any board body, validation stays at 0 errors, full suite green.

@esphbot

esphbot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@bdraco: Thanks @esphbot — applied your suggestion 1 (read-and-discard) in b5fcf678.

Verified on PR head. Pop sits above every continue, so all leaf entries strip — infra entries included. _finalize_entry is the sole _source_block producer; both call sites feed _make_record's featured, which always runs _extract_bus_deps. _materialize_bus/hub entries are plain dicts, prepended after the pop, so nothing reintroduces the key. Single-serializer coupling closed. Catalog clean, allow-list empty. LGTM.

@esphbot

esphbot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review — Recover a consumer's bus_id from its source block

Correct, well-scoped importer fix; read-and-discard hardening applied; merge-ready.

Strengths:

  • Accurate root-cause: _find_consumer_block can't disambiguate N id-less same-platform consumers, so each leaf now recovers its own <bus>_id from the stashed source block, with the finder kept as a fallback for entries built outside the extractor.
  • Prior suggestion Add section config API and user preferences #1 resolved in b5fcf678: _collect_bus_dep_refs now pops _source_block at the top of its loop (before any continue), and the separate strip in _make_record is gone — the transient key is removed at the moment it's consumed, independent of serialization path.
  • Regression test asserts observable behavior (lifted bus ids + per-consumer requires), not source text.

Verification performed:

  • _finalize_entry is the only producer of _source_block; both its call sites flow into _make_record's featured, which unconditionally passes through the _extract_bus_deps lift (sole success return is after the loop).

  • The pop runs before the if "." not in component_id: continue, so infra entries are stripped too.

  • _materialize_bus / hub entries are built as plain dicts (no _source_block) and prepended after the pop, so nothing re-introduces the key.

  • _UNSATISFIED_BUS_ALLOW_LIST correctly empties to a typed frozenset(); regenerated KinCony bodies contain no _source_block.

  • No blocking issues.



Checklist

  • Transient key stripped before serialization on all paths (read-and-discard)
  • Bus-dep read recovers id-less consumer's bus correctly
  • No mutation/aliasing of shared source block
  • Generated catalog matches stated scope, no transient key
  • Test coverage asserts behavior, not source

Automated review by Kōan (Claude) HEAD=b5fcf67 3 min 28s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

@bdraco
bdraco merged commit 58006c4 into main Jun 30, 2026
21 checks passed
@bdraco
bdraco deleted the bus-recover-consumer-id branch June 30, 2026 01:40
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants