Skip to content

refactor(management): add composable connection-based procedure cores - #1832

Open
kewde wants to merge 11 commits into
mainfrom
kewde/refactor-procedures-2
Open

refactor(management): add composable connection-based procedure cores#1832
kewde wants to merge 11 commits into
mainfrom
kewde/refactor-procedures-2

Conversation

@kewde

@kewde kewde commented May 19, 2026

Copy link
Copy Markdown
Contributor

Description

Adds composable "conn" variants of two management procedures, so several procedures can be chained over one already-open P2PConnection instead of each one opening and closing its own:

  • dm_restart_r_co(conn) — the actual KNX v02.01.02 - Management Procedures 03.05.02 - §3.7.3 procedure name for the connection-based variant of DM_Restart. dm_restart(xknx, address) now just opens a connection and delegates to it.
  • nm_individual_address_check_conn(conn) — an xknx-only naming convention (not a KNX spec name) for the connection-based core of nm_individual_address_check. nm_individual_address_write now calls this directly on its own already-open connections instead of duplicating the DeviceDescriptorRead/timeout handling inline.

All existing top-level procedures (dm_restart, nm_individual_address_check, nm_individual_address_write, nm_individual_address_read, nm_individual_address_serial_number_read/_write) keep their familiar (xknx, ...) signature — no breaking change to any of those call sites.

Also:

  • Add P2PConnection.send_data(payload, wait_for_ack=True), replacing the near-duplicate send_data_no_ack/private _send_data pair, so dm_restart_r_co and the request-based procedures share one send path instead of two copies of the same TDataConnected construction.
  • Remove the nm_invididual_address_write typo alias — the misspelled name is no longer exported. This is the only breaking change in this PR.
  • Add CLAUDE.md documenting how to cite the KNX Standard in code (document + version, since different KNX documents version independently), the management procedure naming convention, changelog conventions, and PR template usage.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The documentation has been adjusted accordingly
  • Tests have been added that prove the fix is effective or that the feature works
  • The changes are documented in the changelog (docs/changelog.md)

@kewde
kewde requested a review from farmio May 19, 2026 15:29
Comment thread test/management_tests/procedures/network/test_nm_individual_address_write.py Outdated

async def nm_individual_address_write(
xknx: XKNX, individual_address: IndividualAddressableType
manager: ConnectionManager,

@kewde kewde May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Problem: procedures that need multiple connections

Some procedures require sequential A_Connects to different addresses:

  • NM_IndividualAddress_Write — 2 serial A_Connects (occupancy check + verify/restart).
  • NM_Router_Scan — loop of up to 256 A_Connects, one per subnet address 0–255.
  • NM_DomainAddress_Scan / NM_DomainAddress_Scan2 — same pattern.

These can't take conn: P2PConnection — they need a connection factory.

Options

1. Move address into connect()

P2PConnection becomes a reusable session object; address is passed at connect-time:

# procedure signature
async def nm_router_scan(conn: P2PConnection) -> list[IndividualAddress]:
    for i in range(256):
        async with conn.connect(IndividualAddress(f"1.{i}.0")):
            ...

# caller
conn = P2PConnection(xknx)
await nm_router_scan(conn)

2. Pass ConnectionManager to scanning procedures

Remove the raise guard in Management.connect() — allow reconnecting to the same or a new
address — and let scanning procedures own their loop:

async def nm_router_scan(manager: ConnectionManager) -> list[IndividualAddress]:
    for i in range(256):
        async with manager.connection(IndividualAddress(f"1.{i}.0")) as conn:
            ...

# caller
await nm_router_scan(xknx.management)

This is already the pattern for nm_individual_address_write — scanning procedures are just
another case of a procedure that manages its own connection lifecycle.

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.

In my opinion Option 2 is a cleaner API.
Regarding removing the raise:

if address in self._connections:
            raise ManagementConnectionError(f"Connection to {address} already exists.")

We remove the adddress from self._connections on disconnect. Parallel connections aren't possible iirc. So I think we can - and should - keep this. Connections to new addresses are possible anyway.

I could imagine to have some kind of backoff retry mechanism if an address is already connected by another procedure. But that's out of scope for this PR. And maybe better handled by the user of the API.

Comment thread xknx/management/management.py Outdated
self._response_waiter.set_result(telegram)
self._expected_sequence_number = self._expected_sequence_number + 1 & 0xF

async def send_data_no_ack(self, payload: APCI) -> None:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

to provide a clean way to do restarts

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.79%. Comparing base (f5ab709) to head (96a0b31).

Files with missing lines Patch % Lines
.../procedures/network/nm_individual_address_check.py 93.75% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1832      +/-   ##
==========================================
+ Coverage   97.76%   97.79%   +0.03%     
==========================================
  Files         181      181              
  Lines       13195    13202       +7     
==========================================
+ Hits        12900    12911      +11     
+ Misses        295      291       -4     
Files with missing lines Coverage Δ
xknx/management/management.py 92.30% <100.00%> (+1.08%) ⬆️
xknx/management/procedures/__init__.py 100.00% <100.00%> (ø)
xknx/management/procedures/device/__init__.py 100.00% <100.00%> (ø)
...nx/management/procedures/device/dm_restart_r_co.py 100.00% <100.00%> (ø)
xknx/management/procedures/network/__init__.py 100.00% <100.00%> (ø)
.../procedures/network/nm_individual_address_write.py 100.00% <100.00%> (+7.14%) ⬆️
.../procedures/network/nm_individual_address_check.py 96.15% <93.75%> (+1.41%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@farmio

farmio commented Jul 21, 2026

Copy link
Copy Markdown
Member

Claude Code 🤖:

I like where this is heading — splitting "procedure logic" from "connection lifecycle" is the right move, and dm_restart(conn) reads much better than the old telegram-crafting inline. A few things I'd change before this lands, all with "what does the call site look like for a user?" as the yardstick.

1. Drop protocols.py — annotate with the real classes

Re: the Protocol discussion above — I'm with @farmio here. Concretely:

  • xknx.management.protocols.P2PConnection and xknx.management.management.P2PConnection are two different things with the same name. Anyone reading conn: P2PConnection now has to check which import it is. That alone is a strong argument.
  • The protocols are @runtime_checkable, but nothing does isinstance() on them — and for P2PConnection (which has a data member address) issubclass() wouldn't work anyway. So the decorator buys nothing.
  • "third parties can bring their own Management" is a real use case, but it's already served by subclassing Management — and if we ever do need structural typing, converting ManagementProtocol later is a typing-only change. It's not a decision we have to make now, and making it now costs indirection on every procedure and one more file to keep in sync whenever request() or connection() changes.

So: conn: P2PConnection, management: Management — the actual classes.

2. Never pass the same object twice

nm_individual_address_write(xknx.management, xknx.management, individual_address_new)

This is the clearest signal that the split is at the wrong seam. Management is both the connection manager and the broadcaster; the two-argument form exists only to describe the protocol split from (1). With concrete types this collapses to:

await nm_individual_address_write(xknx.management, "1.2.3")

Same for nm_individual_address_read(xknx.management, ...) etc. — one management argument everywhere a procedure owns its own connections, one conn argument where it operates on an established one. That's a rule a user can hold in their head.

3. No legacy layer, please — let's just break it

The management procedures have very few users, and we're happy to cut a major release for an API change here. That's much cheaper for everyone than what's currently in the PR:

  • the _legacy modules are more code than the procedures themselves, and we'd have to carry and test them for a couple of releases;
  • they force a second public name for every procedure;
  • and today from xknx.management.procedures import dm_restart and from xknx.management.procedures.device import dm_restart are two different functions with the same name and incompatible signatures, differing only by import path — that's a trap no IDE or traceback will help with.

So: drop device/legacy.py, network/legacy.py and the module-level aliasing, keep one public name per procedure with the new signature, and describe the change in docs/changelog.md with before/after snippets. While we're at it, the nm_invididual_address_write typo alias can go too.

4. nm_individual_address_check(conn) leaks its own semantics

The procedure is specified as "is IA_new occupied?", and per 03.05.02 §2.3 an A_Disconnect counts as occupied. After the split, part of that answer lives outside the function — the ManagementConnectionRefused from connect()/disconnect() — which is why the legacy wrapper has to re-implement it:

except ManagementConnectionRefused:
    return True

and why nm_individual_address_write needs this:

try:
    async with manager.connection(individual_address) as conn:
        address_found = await nm_individual_address_check(conn)
except ManagementConnectionRefused:
    pass

Two callers, two different handlings of the same condition, and in the second one address_found is only bound if the exception came from disconnect() in the finally — if it ever comes from anywhere earlier, the next line raises UnboundLocalError. Even if that path isn't reachable today, it shouldn't depend on where inside the context manager the exception originates.

Suggestion: nm_individual_address_check keeps owning its connection (management, address), so "refused == occupied" stays in one place. If a raw "does this device answer on an open connection" primitive is useful for scanning, give it its own name (device_responds(conn) / dd_read_check(conn)) rather than overloading the spec-named procedure. At minimum, set address_found = True in that except and drop the reliance on the earlier assignment.

5. P2PConnection.send_data_no_ack() — 👍

Good addition, this is exactly the kind of thing that belongs on the connection rather than being open-coded in two procedures. Only bikeshed: send_data_no_ack next to the private _send_data reads a bit like an internal; send_unacknowledged() or send_data(payload, wait_for_ack=False) might age better. Your call.

Suggested target shape

# procedures that talk to one device and own the connection
await dm_restart(xknx.management, "1.2.3")
await nm_individual_address_check(xknx.management, "1.2.3")
await nm_individual_address_write(xknx.management, "1.2.3")

# broadcast-only
await nm_individual_address_read(xknx.management)

# power users / multiple procedures on one connection
async with xknx.management.connection(IndividualAddress("1.2.3")) as conn:
    await dm_restart_r_co(conn)

i.e. the conn-taking functions stay (they're the composable core, and scanning procedures like NM_Router_Scan will want them), but the spec-named public procedures keep the one-liner ergonomics. On your NM_Router_Scan question above: option 2 (pass Management, loop over manager.connection(...)) — a connection object that can be re-pointed at another address (option 1) means conn.address is no longer stable, and every procedure taking conn would have to care about that.

@farmio

farmio commented Jul 21, 2026

Copy link
Copy Markdown
Member

Personally I'd drop the Protocols and the Legacy shims. It should be fine to break these APIs and do a major release when we are ready.

@kewde
kewde force-pushed the kewde/refactor-procedures-2 branch from 4d4e09f to 26036e5 Compare July 28, 2026 07:59
kewde added a commit that referenced this pull request Jul 28, 2026
… CLAUDE.md

Finish adding the Management Procedures 03.05.02 spec version to the
remaining files PR #1832 didn't touch (nm_individual_address_read,
nm_individual_address_serial_number_read/write, and their tests).

Add CLAUDE.md documenting the "KNX v<version> - <Document Title>
<number> - <paragraph>" citation format, why the version matters
(different KNX documents carry different versions - Transport Layer is
v01.02.02, Management Procedures is v02.01.02), and the hard rule to
never guess a document's version/title/number - ask first. Lists the
documents with confirmed versions so far and the ones still needing
input (Application Layer 03.03.07, LTE Application Layer, Routing
3.8.5, Communication Medium KNX IP 3.2.6).
@kewde
kewde force-pushed the kewde/refactor-procedures-2 branch from f4e3fb3 to 7c0150c Compare July 28, 2026 19:50
@kewde kewde changed the title refactor(management): clean protocol API for procedures refactor(management): add composable connection-based procedure cores Jul 28, 2026
kewde added a commit that referenced this pull request Jul 28, 2026
… CLAUDE.md

Finish adding the Management Procedures 03.05.02 spec version to the
remaining files PR #1832 didn't touch (nm_individual_address_read,
nm_individual_address_serial_number_read/write, and their tests).

Add CLAUDE.md documenting the "KNX v<version> - <Document Title>
<number> - <paragraph>" citation format, why the version matters
(different KNX documents carry different versions - Transport Layer is
v01.02.02, Management Procedures is v02.01.02), and the hard rule to
never guess a document's version/title/number - ask first. Lists the
documents with confirmed versions so far and the ones still needing
input (Application Layer 03.03.07, LTE Application Layer, Routing
3.8.5, Communication Medium KNX IP 3.2.6).
@kewde
kewde force-pushed the kewde/refactor-procedures-2 branch from 3d4c9b7 to 574e855 Compare August 1, 2026 07:56
kewde added a commit that referenced this pull request Aug 1, 2026
… CLAUDE.md

Finish adding the Management Procedures 03.05.02 spec version to the
remaining files PR #1832 didn't touch (nm_individual_address_read,
nm_individual_address_serial_number_read/write, and their tests).

Add CLAUDE.md documenting the "KNX v<version> - <Document Title>
<number> - <paragraph>" citation format, why the version matters
(different KNX documents carry different versions - Transport Layer is
v01.02.02, Management Procedures is v02.01.02), and the hard rule to
never guess a document's version/title/number - ask first. Lists the
documents with confirmed versions so far and the ones still needing
input (Application Layer 03.03.07, LTE Application Layer, Routing
3.8.5, Communication Medium KNX IP 3.2.6).
kewde added 11 commits August 2, 2026 00:21
- Simple procedures (dm_restart, nm_individual_address_check) now take
  conn: P2PConnection directly; callers own the connection lifecycle
- Complex procedures (nm_individual_address_write) keep ConnectionManager
  + Broadcaster signatures to manage their own connection lifecycle
- Legacy wrappers open/close connections and delegate to clean functions
- Add legacy tests for dm_restart and all nm_individual_address_* wrappers
- Document T_Connect-PDU = A_Connect at wire level (KNX 03.03.07 §3.5.1)
- Remove unnecessary ellipsis constants after docstrings in Protocol
  method bodies (W2301)
- Replace reimport aliasing pattern in procedures/__init__.py with
  explicit assignments to avoid W0404
…m_individual_address_write

Add test for the case where the device sends TDisconnect during the
initial address check, causing nm_individual_address_check to return
True internally while the context manager's finally raises
ManagementConnectionRefused from disconnect() — which the outer except
swallows per KNX 03.05.02 §2.3 step 1.
…back

Per farmio's review on this PR: drop xknx.management.protocols entirely
and annotate procedures with the concrete Management/P2PConnection
classes instead, and drop the XKNX-based legacy wrappers rather than
carrying a deprecation layer - the API can just break with a major
release.

- dm_restart_r_co(conn) keeps its spec name (KNX 03.05.02 §3.7.3); a new
  dm_restart(management, address) convenience wrapper owns the connection.
- nm_individual_address_check(conn) is renamed to
  nm_individual_address_check_conn(conn) - not a spec name, just an xknx
  convention for the composable core - freeing nm_individual_address_check
  for a new (management, address)-owning wrapper that keeps the
  occupied-on-disconnect semantics in one place.
- nm_individual_address_write now takes a single management: Management
  argument instead of (manager, broadcaster), and calls the _conn cores
  directly on its own already-open connections. This also fixes a latent
  UnboundLocalError risk: address_found is now always initialized before
  the try/except that can swallow ManagementConnectionRefused.
- nm_individual_address_read/nm_individual_address_serial_number_read/
  nm_individual_address_serial_number_write retyped from Broadcaster to
  Management (no behavior change, Management already satisfied it).
- Removed device/legacy.py, network/legacy.py, and the
  nm_invididual_address_write typo alias.
- Updated examples and changelog (with before/after snippets) to match.
…procedures

Management.__init__ requires an XKNX instance anyway, so typing the
connection/broadcast-owning procedures narrowly with Management didn't
buy real decoupling - it just made callers write xknx.management
everywhere instead of the xknx they already have. Revert those six
procedures (dm_restart, nm_individual_address_check,
nm_individual_address_write, nm_individual_address_read,
nm_individual_address_serial_number_read/write) back to taking xknx: XKNX,
using xknx.management internally.

This actually makes the top-level, single-device procedures' call
signature identical to the pre-PR API for anyone who was just calling
dm_restart(xknx, address) etc. - the only remaining breaking changes are
nm_individual_address_write dropping its duplicate xknx.management
argument, the protocols module removal, and the _conn/_r_co composable
core renames, all covered in the changelog entry.

The composable conn-based cores (dm_restart_r_co,
nm_individual_address_check_conn) are unaffected - they still take
conn: P2PConnection since they operate on an already-open connection,
not the whole xknx instance.
The occupancy check in nm_individual_address_write was needlessly
duplicating nm_individual_address_check's own connection-open and
ManagementConnectionRefused handling instead of just calling it directly
- a leftover from an earlier iteration of this branch that used to type
this function differently. Call nm_individual_address_check(xknx, address)
directly, matching main's structure exactly for that part.

The changelog was overstating this PR's breaking changes by comparing
against this branch's own earlier, since-reverted commits (Broadcaster/
ConnectionManager protocols, dual manager+broadcaster arguments) rather
than against current main. Diffed every touched procedure file against
main directly: every existing top-level procedure signature is actually
unchanged. The only real breaking change is dropping the
nm_invididual_address_write typo alias; everything else is additive
(dm_restart_r_co, nm_individual_address_check_conn,
P2PConnection.send_data_no_ack) or internal refactoring.
Comments/docstrings referencing "KNX 03.05.02" or "KNX 03.03.07" didn't
specify which release of the KNX Standard those document numbers belong
to. Add "v02.01.02 - <document title> <number>" consistently, scoped to
the files this PR already touches.
…d guards

test_nm_individual_address_write_address_found_other_in_programming_mode
advanced the clock past nm_individual_address_read's 3s broadcast window
before delivering the programming-mode reply, so the reply was silently
dropped and the test actually exercised the "no device in programming
mode" branch instead of the address-conflict branch its name and the
generic pytest.raises(ManagementConnectionError) implied. Deliver the
reply while the broadcast is still listening, then advance the clock via
time_travel to close it out without a real 3s wait, and assert on the
specific error message so this can't silently regress again.

Add test_send_on_disconnected_connection covering the three identical
"if not self._connected: raise ManagementConnectionRefused" guards in
P2PConnection (send_data_no_ack, request, _send_data). _send_data's was
previously only covered incidentally through unrelated procedure tests
(and stopped being covered at all once the write test above was fixed),
which is exactly the kind of coverage that silently breaks when unrelated
tests change; test it directly instead.
…wait_for_ack)

send_data_no_ack sitting right next to the private _send_data read like
an internal helper that leaked into the public API, and the two were
near-duplicates of the same telegram construction. Merge them into one
public send_data(payload, wait_for_ack=True) - the ack-waiting path is
unchanged, the no-ack path just sends and returns.
This branch predates the 3.16.0 and 3.17.0 releases, and rebasing had
kept this PR's changelog entries textually anchored where they were
originally written - which by now was inside the already-shipped
"# 3.16.0 Complex schema" section instead of "# Unreleased changes" at
the top. Move the Breaking Changes/New Features entries to Unreleased,
and drop the stale "Internals" bullet describing the abandoned
Broadcaster/ConnectionManager protocol design (superseded several
commits ago) instead of leaving it duplicated in both places.
kewde added a commit that referenced this pull request Aug 1, 2026
… CLAUDE.md

Finish adding the Management Procedures 03.05.02 spec version to the
remaining files PR #1832 didn't touch (nm_individual_address_read,
nm_individual_address_serial_number_read/write, and their tests).

Add CLAUDE.md documenting the "KNX v<version> - <Document Title>
<number> - <paragraph>" citation format, why the version matters
(different KNX documents carry different versions - Transport Layer is
v01.02.02, Management Procedures is v02.01.02), and the hard rule to
never guess a document's version/title/number - ask first. Lists the
documents with confirmed versions so far and the ones still needing
input (Application Layer 03.03.07, LTE Application Layer, Routing
3.8.5, Communication Medium KNX IP 3.2.6).
@kewde
kewde force-pushed the kewde/refactor-procedures-2 branch from 574e855 to 96a0b31 Compare August 1, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants