Conversation
Add bookmarks for each title in PDF spec
Add actutators / SystemTable
Feat/system table data
9ae4fb2 to
bc28c93
Compare
|
I see, I knew about the API docs but hadn't really noticed the possibility of several devices being connected. In HA, this could maybe be built as subdevices. As the whole library is currently relying on nodes and also, as I understand it, there will be no node without an actuator entry in the system table, I'd rather like to see the single nodes extended with the information from the system table instead of having a parallel structure. Otherwise you'll just have to match actuator and node IDs later on to be able to actually use them. Unless I am missing something. |
| class ActuatorList(list): | ||
| """a useless class for MyPy.""" | ||
|
|
||
| def __init__(self, init: list[Actuator]) -> None: | ||
| """Init a list.""" | ||
| self.acts: list[Actuator] = init | ||
|
|
||
| def __getitem__(self, key: int) -> Actuator: # type: ignore[override] | ||
| """Get an item.""" | ||
| return super().__getitem__(key) | ||
|
|
||
| def __setitem__(self, key: int, value: Actuator) -> None: # type: ignore[override] | ||
| """Set an item.""" | ||
| self.acts[key] = value | ||
|
|
||
|
|
||
| class FrameGetSystemTableNotification(FrameBase): | ||
| """Frame for scene list notification.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| """Init Frame.""" | ||
| super().__init__(Command.GW_CS_GET_SYSTEMTABLE_DATA_NTF) | ||
| self.actuators = ActuatorList([]) |
There was a problem hiding this comment.
why can't this be simply this:
| class ActuatorList(list): | |
| """a useless class for MyPy.""" | |
| def __init__(self, init: list[Actuator]) -> None: | |
| """Init a list.""" | |
| self.acts: list[Actuator] = init | |
| def __getitem__(self, key: int) -> Actuator: # type: ignore[override] | |
| """Get an item.""" | |
| return super().__getitem__(key) | |
| def __setitem__(self, key: int, value: Actuator) -> None: # type: ignore[override] | |
| """Set an item.""" | |
| self.acts[key] = value | |
| class FrameGetSystemTableNotification(FrameBase): | |
| """Frame for scene list notification.""" | |
| def __init__(self) -> None: | |
| """Init Frame.""" | |
| super().__init__(Command.GW_CS_GET_SYSTEMTABLE_DATA_NTF) | |
| self.actuators = ActuatorList([]) | |
| class FrameGetSystemTableNotification(FrameBase): | |
| """Frame for scene list notification.""" | |
| def __init__(self) -> None: | |
| """Init Frame.""" | |
| super().__init__(Command.GW_CS_GET_SYSTEMTABLE_DATA_NTF) | |
| self.actuators: list[Actuator] = [] |
There was a problem hiding this comment.
It just breaks MyPy :
https://github.com/Julius2342/pyvlx/actions/runs/22038005236/job/63674093952?pr=529
| def get_payload(self) -> bytes: | ||
| """Return Payload.""" | ||
| # TODO Paquet are limited to 200 bytes so KLF200 would never send more that 10 entries at once | ||
| ret = bytes([len(self.actuators)]) | ||
| for i in self.actuators: | ||
| ret += bytes(self.actuators[i].idx) | ||
| ret += self.actuators[i].address | ||
| ret += bytes(self.actuators[i].subtype.value) | ||
| ret += bytes(self.actuators[i].turn_around_time.value + self.actuators[i].rf * 16 | ||
| + self.actuators[i].io * 32 + self.actuators[i].power_save_mode.value * 64) | ||
| ret += bytes(self.actuators[i].manufactor.value) | ||
| ret += self.actuators[i].backbone | ||
| ret += bytes([self.remaining_objects]) | ||
| return ret |
There was a problem hiding this comment.
Am I missing something why this cannot be like this:
| def get_payload(self) -> bytes: | |
| """Return Payload.""" | |
| # TODO Paquet are limited to 200 bytes so KLF200 would never send more that 10 entries at once | |
| ret = bytes([len(self.actuators)]) | |
| for i in self.actuators: | |
| ret += bytes(self.actuators[i].idx) | |
| ret += self.actuators[i].address | |
| ret += bytes(self.actuators[i].subtype.value) | |
| ret += bytes(self.actuators[i].turn_around_time.value + self.actuators[i].rf * 16 | |
| + self.actuators[i].io * 32 + self.actuators[i].power_save_mode.value * 64) | |
| ret += bytes(self.actuators[i].manufactor.value) | |
| ret += self.actuators[i].backbone | |
| ret += bytes([self.remaining_objects]) | |
| return ret | |
| def get_payload(self) -> bytes: | |
| """Return Payload.""" | |
| # TODO Paquet are limited to 200 bytes so KLF200 would never send more that 10 entries at once | |
| ret = bytes([len(self.actuators)]) | |
| for actuator in self.actuators: | |
| ret += actuator.idx | |
| ret += actuator.address | |
| ... |
There was a problem hiding this comment.
This proposition breaks also CI/CD checks (most of them)
There was a problem hiding this comment.
I don't think it does if you combine it with my suggestion above (https://github.com/Julius2342/pyvlx/pull/529/changes#r2809249738)
| ) | ||
| match NodeType(floor(frame.node_type.value / 64)): | ||
| case NodeType.VENETIAN_BLIND | NodeType.BLIND: # 1, 10 | ||
| # this seems wrong : VENITIAN_BLIND excepts FP1, FP2 and FP3 |
There was a problem hiding this comment.
true, there seems to be something missing. OTOH, I haven't seen any complaint yet, so it could also be a mistake in the docs. Good choice to leave it as is.
For the rest: I personally prefer the formatting as is, longer but easier to read IMHO.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the FrameGetSystemTableRequest/Confirmation/Notification API to retrieve system table data from the KLF200 gateway. The system table provides information about actuators including their backbone address (electrical connection identifier), which helps identify which devices share the same Velux Optima or KUX 110 connection.
However, the PR also includes numerous unrelated changes including critical bug fixes, refactoring, dependency updates, and documentation improvements, making it a mixed-purpose PR.
Changes:
- Added system table API support with new Actuator and Actuators classes to query device backbone addresses
- Fixed critical bug in DimmableDevice where turn_on/turn_off intensity percentages were inverted
- Refactored node_helper.py to use Python 3.10+ match statements for cleaner node type handling
- Updated GitHub Actions workflow dependencies and switched to setuptools-scm for dynamic versioning
- Fixed multiple typos and corrected enum values (LINAR→LINEAR, SLUTS→SLATS, LOUVER→LOUVRE)
- Improved logging and documentation across examples and README
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| pyvlx/actuator.py | New Actuator class representing system table entries with backbone address |
| pyvlx/actuators.py | New collection class for managing actuators retrieved from system table |
| pyvlx/api/get_system_table.py | API event handler for system table requests |
| pyvlx/api/frames/frame_get_systemtable.py | Frame definitions for system table request/response protocol |
| pyvlx/api/frames/frame_command_send.py | Added run_status and status_reply fields to command notifications |
| pyvlx/dimmable_device.py | Renamed from LighteningDevice, fixed turn_on/off intensity bug, added device types |
| pyvlx/node_helper.py | Refactored to use match statements and handle new device types |
| pyvlx/parameter.py | Enhanced Intensity class with proper percent conversion and documentation |
| pyvlx/const.py | Added TurnAround and Manufactor enums, fixed typos, added missing handlers |
| pyvlx/pyvlx.py | Integrated actuators collection with load_actuators method |
| test/frame_get_systemtable_test.py | New tests for system table frame parsing |
| test/intensity_test.py | Comprehensive tests for Intensity parameter handling |
| test/node_helper_test.py | Updated tests for new device types and refactored logic |
| pyproject.toml | Switched to dynamic versioning with setuptools-scm |
| .github/workflows/*.yml | Updated action versions and added manual trigger support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PowerMode(payload[(i * 11 + 7)] >> 6 & 3), # bit 0-1 power_save_mode | ||
| bool(payload[(i * 11 + 7)] >> 5 & 1), # bit2 io membership | ||
| bool(payload[(i * 11 + 7)] >> 4 & 1), # bit3 rf support | ||
| # bit4-5 reserved | ||
| TurnAround(payload[(i * 11 + 7)] % 4), # bit6-7 actuatortime |
There was a problem hiding this comment.
The bit operation comment "bit 0-1 power_save_mode" is misleading. The operation >> 6 & 3 shifts right by 6 bits and masks the lowest 2 bits, which extracts bits 6-7 (not bits 0-1). The comments on lines 91-94 appear to have the bit positions reversed - they should describe the actual bit positions in the byte (bits 6-7 for turnaround, bits 4-5 for RF/IO).
| PowerMode(payload[(i * 11 + 7)] >> 6 & 3), # bit 0-1 power_save_mode | |
| bool(payload[(i * 11 + 7)] >> 5 & 1), # bit2 io membership | |
| bool(payload[(i * 11 + 7)] >> 4 & 1), # bit3 rf support | |
| # bit4-5 reserved | |
| TurnAround(payload[(i * 11 + 7)] % 4), # bit6-7 actuatortime | |
| PowerMode(payload[(i * 11 + 7)] >> 6 & 3), # bit6-7 power_save_mode | |
| bool(payload[(i * 11 + 7)] >> 5 & 1), # bit5 io membership | |
| bool(payload[(i * 11 + 7)] >> 4 & 1), # bit4 rf support | |
| # bit2-3 reserved | |
| TurnAround(payload[(i * 11 + 7)] % 4), # bit0-1 actuatortime |
There was a problem hiding this comment.
Obviously wrong :(
Copilot don't know about big little indian
There was a problem hiding this comment.
Endian-ness has nothing to do with the order of bits within a byte, only with the order of bytes in longer data types. So Copilot is not wrong here. I am not sure if the spec is wrong/misleading (which is equivalent to your documentation) or the code, though. Please check again if the spec is misleading or if the code is wrong.
| def test_notify_empty_bytes(self): | ||
| """Test FrameGetSystemTableNotification.""" | ||
| frame = FrameGetSystemTableNotification() | ||
| self.assertEqual(bytes(frame), b"\x00\x05\x01\x02\x00\x00\x06") |
There was a problem hiding this comment.
Missing test coverage: The get_payload() method is tested only with an empty actuators list (line 49), which doesn't exercise the loop body where the critical bugs exist (lines 67-74 in frame_get_systemtable.py). A test should be added that creates a FrameGetSystemTableNotification with actuators and verifies that bytes(frame) produces the correct output. This would catch the iteration and bytes() constructor bugs.
| class ActuatorList(list): | ||
| """a useless class for MyPy.""" | ||
|
|
||
| def __init__(self, init: list[Actuator]) -> None: | ||
| """Init a list.""" | ||
| self.acts: list[Actuator] = init | ||
|
|
||
| def __getitem__(self, key: int) -> Actuator: # type: ignore[override] | ||
| """Get an item.""" | ||
| return super().__getitem__(key) | ||
|
|
||
| def __setitem__(self, key: int, value: Actuator) -> None: # type: ignore[override] | ||
| """Set an item.""" | ||
| self.acts[key] = value |
There was a problem hiding this comment.
The ActuatorList class has an unused acts attribute that is set during initialization but never used. The class inherits from list and should call super().__init__(init) to properly initialize the parent list, rather than storing the items in a separate attribute. The current implementation doesn't actually populate the list itself, making the custom __setitem__ ineffective.
…lx into feat/SystemTableData
Co-authored-by: wollew <wollew@users.noreply.github.com>
Co-authored-by: wollew <wollew@users.noreply.github.com>
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: wollew <wollew@users.noreply.github.com>
Co-authored-by: wollew <wollew@users.noreply.github.com>
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
…lx into feat/SystemTableData
I've added support for FrameGetSystemTableRequest / FrameGetSystemTableConfirmation / FrameGetSystemTableNotification API paquets.
its looks similar to Nodes, but have an extra backbone address attribute. backbone stands for electric connection : each product sharing the backbone address are connected to the same Velux Otima (or a KUX 110)
Ex: in my setup, index 0 (a velux window) share backbone address with index 4 (a roller)