Skip to content

Avoid TC address cache overwrite when setting extended timeout - #768

Merged
puddly merged 1 commit into
zigpy:devfrom
puddly:puddly/avoid-tc-addr-cache-overwrite
Oct 23, 2026
Merged

puddly merged 1 commit into
zigpy:devfrom
puddly:puddly/avoid-tc-addr-cache-overwrite

Conversation

@puddly

@puddly puddly commented Oct 15, 2026

Copy link
Copy Markdown
Member

See #765 and NabuCasa/silabs-firmware-builder#236 for more context on the firmware side.

The address table actually contains the trust center address cache as a trailer so there's a small chance we can override an entry. Instead of randomly selecting entries, we round robin instead.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 20:08
@codecov

codecov Bot commented Oct 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.58%. Comparing base (aea9248) to head (afcd5d5).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #768   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          64       64           
  Lines        4331     4332    +1     
=======================================
+ Hits         4313     4314    +1     
  Misses         18       18           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is narrow, preserves configured table bounds, and includes tests for sequential allocation and wraparound.

0 open findings

What changed in this PR

Changes extended-timeout address-table allocation from random selection to bounded round-robin, complementing the related firmware work.

Changes:

  • Adds a persistent cursor for sequential slot selection.
  • Updates tests to verify sequential allocation and wraparound.
File Description
tests/​test_ezsp_v4.py Verifies sequential selection and wraparound.
tests/​test_ezsp_v14.py Updates the expected initial slot.
bellows/​ezsp/​v4/​__init__.py Replaces random selection with round-robin allocation.
bellows/​ezsp/​protocol.py Initializes the address-table cursor.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is narrow, preserves allocation bounds, and includes focused regression coverage with no unresolved findings.

No open issues 🎉

Review effort: Balanced

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. Replacing the random slot with a cursor is right for the host path: bellows is the only thing allocating application address table slots, and getConfigurationValue(CONFIG_ADDRESS_TABLE_SIZE) returns only the application slots, so the cursor walks free slots first and only then evicts round-robin (oldest first), which beats random eviction.

I checked out the head (afcd5d5) and ran tests/test_ezsp_v4.py, tests/test_ezsp_v5.py and tests/test_ezsp_v14.py: 286 passed. The new test_set_extended_timeout_table_wraps covers the wrap-around, and dropping the random patching makes the existing tests simpler.

Two non-blocking notes:

  • The cursor lives on the protocol object and starts at 0 each time one is created, which is fine as long as a new protocol object always means a fresh NCP address table (the host path is stateless otherwise). If an EZSP reconnect ever keeps NCP state while recreating the protocol, the first writes would evict live entries again instead of using free slots.
  • The cursor advances even if replaceAddressTableEntry returns a non-success status (its result is ignored, as before), so a failed write burns a slot position. Harmless in practice.

As the issue in #765 notes, the firmware-side % sli_zigbee_address_table_size pick in the XNCP combined send is separate and still needs its own change (NabuCasa/silabs-firmware-builder#236).

@puddly
puddly merged commit 289daa3 into zigpy:dev Oct 23, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix This PR fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants