Refactor CEMILData.flags into a typed CEMIFlags dataclass - #1884
Conversation
`CEMILData.flags` was a bare 16 bit `int` and `CEMIFlags` a bag of bit constants, so callers had to know the layout of Ctrl1 and Ctrl2 to read or set anything, and `hops` needed a hand written mask/shift property. Make `CEMIFlags` a slotted dataclass holding the control field values that are actually independent state - priority, repeat_on_error, system_broadcast, acknowledge_request, confirm_error and hop_count - and add a `CEMIPriority` enum. `enum.Flag` does not fit: priority is a 2 bit value and the hop count a 3 bit counter, neither of which are flags. Frame Type, Address Type and Extended Frame Format are deliberately *not* stored. Each of them is determined by something the frame already knows: the NPDU length, the type of the destination address, and the fact that only frame format 0 is supported. Deriving them when serializing means `flags` can no longer disagree with what is put on the wire - the class of bug that required the L_Data_Extended fix in the first place - and there is no stale Frame Type left to go wrong when Data Secure replaces the payload of a frame it copied. An `int | None` of raw incoming flags was considered and rejected. Parsing into the dataclass loses only the Frame Type (deliberately) and the reserved Ctrl1 b6, and letting a stored raw value take precedence when serializing would reintroduce exactly the stale-flags problem. The raw octets of a received frame remain available to `CEMIHandler.handle_raw_cemi`. Also: - `hops` is replaced by `flags.hop_count`, validated to 0..7 when serializing instead of silently corrupting Ctrl2. - the raw bit masks move to `xknx.cemi.flags` as per-octet constants; mixing 8 and 16 bit values (0x8000 vs 0x0080) was a standing source of confusion. - `block_0()` and the `SecureData` methods take `dst_is_group_address: bool` instead of `frame_flags: int`. Only Ctrl2's Address Type bit ever survived `B0_AT_FIELD_FLAGS_MASK`, so the mask is gone and the CCM input is unchanged - the AN158 Annex A and real-device vectors still verify and the secured frame byte vectors are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1884 +/- ##
=======================================
Coverage 97.76% 97.77%
=======================================
Files 181 182 +1
Lines 13192 13236 +44
=======================================
+ Hits 12897 12941 +44
Misses 295 295
🚀 New features to boost your workflow:
|
| if _eff := flags & CEMIFlags.EXTENDED_FRAME_FORMAT_MASK: | ||
| if _eff := raw[1] & EXTENDED_FRAME_FORMAT_MASK: |
There was a problem hiding this comment.
I don't like this this check runs on raw. Should we add an enum / dataclasses that holds frametype?
I'd like to be able to hold (log) it for incoming frames while auto-setting it for outgoing. It would be merged with the flags .to_knx() and the address type when sending.
For address type we can see the received value from the type of dst_addr, so that's fine. Original frame format however is lost.
There was a problem hiding this comment.
Claude Code 🤖: Done in c072f10 — both the typed frame type and the parsed check.
CEMIFlags gains two informational fields that hold what was received and are ignored when serializing:
frame_type: CEMIFrameType = CEMIFrameType.STANDARD # EXTENDED / STANDARD
frame_format: CEMIFrameFormat = CEMIFrameFormat.STANDARD # STANDARD / LTE_HEEto_knx() takes the Frame Type as a CEMIFrameType from CEMILData, which derives it from the NPDU length, so self.frame_type is never consulted when sending — exactly the "hold it for incoming, auto-set for outgoing, merge in to_knx()" split you described. test_to_knx_ignores_received_frame_type pins that.
The EFF check no longer touches raw bits:
if flags.frame_format is not CEMIFrameFormat.STANDARD:
raise UnsupportedCEMIMessage(
f"Extended Frame Format not supported: {flags.frame_format.name} ..."
)CEMIFrameFormat._missing_ resolves the whole 01xxb range to LTE_HEE (the 2 lsb are the LTE extended address type, so enumerating them separately would be noise). Reserved encodings raise ConversionError out of CEMIFlags.from_knx, which CEMILData.from_knx converts to UnsupportedCEMIMessage with the source address and raw frame — the same pattern already used for an unsupported TPCI. Both paths are parametrised in cemi_frame_test.py.
One thing I did not do: serialize self.frame_format. to_knx() always writes STANDARD, because a CEMILData with any other format cannot exist — it is rejected during parsing. Storing it is still worth it for the typed check and for logging, but writing it back would only matter if we ever supported LTE.
Address type is unchanged, readable from dst_addr and also exposed as CEMILData.dst_is_group_address.
Also added the compact __str__ you asked for: LOW STANDARD hop_count=6, listing only the booleans that are set, used by CEMILData.__repr__ so debug log lines stay short. The full dataclass __repr__ still shows every field.
The Data Secure byte vectors are unchanged, including 3ce0500104001103f11000254ae1cb67cd98e577b519be47bb.
Addresses review feedback on #1884: the Extended Frame Format check ran on `raw[1]` rather than on a parsed value, and the Frame Type a frame arrived with was discarded, so it could not be logged. Add `CEMIFrameType` (STANDARD / EXTENDED) and `CEMIFrameFormat` (STANDARD / LTE_HEE, with `_missing_` resolving the whole 01xxb range to LTE_HEE) and keep both on `CEMIFlags` as informational fields: they hold what was received and are ignored when serializing. `CEMILData.to_knx()` still passes the Frame Type it derives from the NPDU length, and the Address Type stays out of the dataclass - it is readable from the type of `dst_addr`. `CEMIFlags.from_knx()` now raises `ConversionError` for a reserved frame format, which `CEMILData.from_knx()` turns into `UnsupportedCEMIMessage` with the source address and raw frame, the same way it already handles an unsupported TPCI. The LTE check compares a typed value. Add a compact `CEMIFlags.__str__` - "LOW STANDARD hop_count=6", listing only the boolean flags that are set - and use it in `CEMILData.__repr__` to keep per-frame debug log lines readable. `__repr__` is the full dataclass repr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`CEMIFlags.to_knx()` took a `frame_type` argument that shadowed its own
`frame_type` field, which reads as if the field were the default. Give
`CEMIFrameType` and the new `CEMIAddressType` their own `to_knx()` / `from_knx()`
placing their single bit, and let the frame compose:
self.flags.to_knx() | frame_type.to_knx() | self.address_type.to_knx()
`CEMIFlags.to_knx()` now only sets the bits it owns and takes no arguments, so
nothing shadows anything. Control fields are handled as one 16 bit value
(Ctrl1 << 8 | Ctrl2) to make the composition a plain OR; `to_knx()` returns an
int and `from_knx()` takes one, as `TPCI.to_knx()` already does for a bit level
field. The Frame Type and Address Type bit masks are gone from the module level
constants - each enum places its own bit - which was the only place where the
8/16 bit mixing was actually confusing.
`CEMILData.dst_is_group_address` is replaced by `CEMILData.address_type`.
`block_0()` and the `SecureData` methods take `address_type: CEMIAddressType`
accordingly; the B0 flags octet is literally the Address Type bit plus the
Extended Frame Format, so `address_type.to_knx()` is that octet and the manual
constant is gone. Byte vectors are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3ead45a to
3963bc6
Compare
| sequence_number=sequence_number_bytes, | ||
| address_fields_raw=address_fields_raw, | ||
| frame_flags=frame_flags, | ||
| address_type=address_type, |
There was a problem hiding this comment.
nice, much easier to see what gets included in the MAC 👍
2 participants
Description
Follow-up to #1882, targeted at the next major release.
CEMILData.flagswas a bare 16 bitintandCEMIFlagsa bag of bit constants, so callers had to know the layout of Ctrl1 and Ctrl2 to read or set anything, andhopsneeded a hand written mask/shift property.CEMIFlagsis now a slotted dataclass holding the control field values that are actually independent state:enum.Flagwas considered and does not fit: priority is a 2 bit value and the hop count a 3 bit counter, neither of which are flags. A plain slotted dataclass gives free__eq__, keyword construction and mutability for the hop count, which is what a router would need.What is not stored
Frame Type, Address Type and Extended Frame Format are deliberately not fields. Each is determined by something the frame already knows:
<= 15is a standard frameisinstance(dst_addr, GroupAddress)0; anything else is rejected when parsingCEMIFlags.to_knx()takes the first two as keyword arguments from the frame that knows them:This means
flagscan no longer disagree with what is put on the wire - the class of bug that made #1868 possible - and there is no stale Frame Type left to go wrong when Data Secure replaces the payload of a frame it copied.On the
int | NoneideaStoring raw incoming flags in a private attribute was considered and rejected. Parsing straight into the dataclass loses only the Frame Type (deliberately) and the reserved Ctrl1 b6; everything else round-trips, including for a received frame. Letting a stored raw value take precedence when serializing would reintroduce exactly the stale-flags problem, since a
copy()d frame with a swapped payload would emit the old Frame Type. If you want byte-exact fidelity for debugging, the raw octets of a received frame are already available inCEMIHandler.handle_raw_cemi- keeping a second, partly authoritative copy on the object is the part that does not pay for itself.Happy to add it as a debug-only field if you disagree - it is a small addition on top of this.
Also in this change
CEMILData.hopsis replaced byflags.hop_count, validated to0..7when serializing instead of silently corrupting Ctrl2.CEMILData.dst_is_group_addressis added.xknx.cemi.flagsas per-octet constants. Mixing 8 and 16 bit values (FRAME_TYPE_STANDARD = 0x8000next toDESTINATION_GROUP_ADDRESS = 0x0080) was a standing source of confusion.block_0(),SecureData.init_from_plain_apdu()andSecureData.get_plain_apdu()takedst_is_group_address: boolinstead offrame_flags: int. Only Ctrl2's Address Type bit ever survivedB0_AT_FIELD_FLAGS_MASK, so the mask is gone and the CCM input is unchanged.CEMILData(flags=...)is optional and defaults toCEMIFlags().Verification
The Data Secure byte vectors are the strongest check that nothing moved: the AN158 v07 Annex A example and the real-device frame still verify, and the secured outgoing frames are byte-identical to what
mainproduces, including the extended-frame vector from #1882:New
test/cemi_tests/flags_test.pycovers parsing, the priority encoding from 3/2/2 Figure 28, the derived-field arguments, a Ctrl1/Ctrl2 round trip and hop count validation. Full suite passes (3758);test/integration_tests/routing_multicast_test.pyfails on my machine for lack of multicast routing, identically onmain.Type of change
Checklist
🤖 Generated with Claude Code