From b7423675fd60e8c4ad0c62d29cc16fe02e4342c1 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 14 Apr 2023 00:54:48 +0200 Subject: [PATCH 1/5] tcp: Make firmware strings (in packet suffix) optional Multiple of the following reports are going around: Buffer size too small (85 instead of at least 125 bytes) As it turns out these last 40 bytes contain exactly the `firmware` and `firmware_slave` strings, which are not sent by certain 3000TL inverters. Move the fields to a secondary, optional `Structure` to allow these devices to be read out over TCP, and warn when the length of a reply isn't matching one of the known message sizes so that we can potentially discover more different layouts. --- omnikinverter/models.py | 5 +++-- omnikinverter/tcp.py | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/omnikinverter/models.py b/omnikinverter/models.py index aaabb2b7..f53355c5 100644 --- a/omnikinverter/models.py +++ b/omnikinverter/models.py @@ -14,14 +14,15 @@ class Inverter: serial_number: str | None model: str | None - firmware: str | None - firmware_slave: str | None solar_rated_power: int | None solar_current_power: int | None solar_energy_today: float | None solar_energy_total: float | None alarm_code: str | None = None + firmware: str | None = None + firmware_slave: str | None = None + # TCP only inverter_active: bool | None = None solar_hours_total: int | None = None diff --git a/omnikinverter/tcp.py b/omnikinverter/tcp.py index 08a652b5..21ce3c08 100644 --- a/omnikinverter/tcp.py +++ b/omnikinverter/tcp.py @@ -1,7 +1,7 @@ """Data model and conversions for tcp-based communication with the Omnik Inverter.""" from __future__ import annotations -from ctypes import BigEndianStructure, c_char, c_ubyte, c_uint, c_ushort +from ctypes import BigEndianStructure, c_char, c_ubyte, c_uint, c_ushort, sizeof from typing import TYPE_CHECKING, Any from .const import LOGGER @@ -70,6 +70,12 @@ class _TcpData(BigEndianStructure): ("padding1", c_ubyte * 4), ("unknown0", c_ushort), ("padding2", c_ubyte * 10), + ] + + +class _TcpFirmwareStrings(BigEndianStructure): + _pack_ = 1 + _fields_ = [ ("firmware", c_char * 16), ("padding3", c_ubyte * 4), ("firmware_slave", c_char * 16), @@ -248,7 +254,20 @@ def parse_messages(serial_number: int, data: bytes) -> dict[str, Any]: def _parse_information_reply(data: bytes) -> dict[str, Any]: + if len(data) not in [ + sizeof(_TcpData), + sizeof(_TcpData) + sizeof(_TcpFirmwareStrings), + ]: # pragma: no cover + LOGGER.warning( + "Unrecognized INFORMATION_REPLY size `%s`, are there extra bytes?", + len(data), + ) + tcp_data = _TcpData.from_buffer_copy(data) + tcp_firmware_data = None + # Only parse firmware strings if available + if len(data) >= sizeof(tcp_data) + sizeof(_TcpFirmwareStrings): + tcp_firmware_data = _TcpFirmwareStrings.from_buffer_copy(data, sizeof(tcp_data)) if tcp_data.unknown0 not in [0, UINT16_MAX]: # pragma: no cover LOGGER.warning("Unexpected unknown0 `%s`", tcp_data.unknown0) @@ -261,8 +280,12 @@ def _parse_information_reply(data: bytes) -> dict[str, Any]: # uncovered. for idx in range(1, 5): # pragma: no cover name = f"padding{idx}" - padding = getattr(tcp_data, name) - if sum(padding): + padding = getattr(tcp_data, name, None) or getattr( + tcp_firmware_data, + name, + None, + ) + if padding is not None and sum(padding): LOGGER.warning("Unexpected `%s`: `%s`", name, padding) def list_divide_10(integers: list[int]) -> list[float | None]: @@ -292,8 +315,6 @@ def temperature_to_float(temp: int) -> float | None: "solar_energy_total": 0.1, "solar_hours_total": None, "inverter_active": int_to_bool, - "firmware": None, - "firmware_slave": None, } result: dict[str, Any] = {} @@ -317,4 +338,8 @@ def temperature_to_float(temp: int) -> float | None: result[name] = value + if tcp_firmware_data is not None: + for name in ["firmware", "firmware_slave"]: + result[name] = getattr(tcp_firmware_data, name) + return result From 9a9a1e2e63e79566679e60251c4a6e456542ae7b Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 14 Apr 2023 01:38:12 +0200 Subject: [PATCH 2/5] Refactor TCP packet parsing into `class` methods Makes `_parse_information_reply()` quite a bit slimmer --- omnikinverter/tcp.py | 163 +++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 77 deletions(-) diff --git a/omnikinverter/tcp.py b/omnikinverter/tcp.py index 21ce3c08..52698f93 100644 --- a/omnikinverter/tcp.py +++ b/omnikinverter/tcp.py @@ -72,6 +72,77 @@ class _TcpData(BigEndianStructure): ("padding2", c_ubyte * 10), ] + @classmethod + def parse(cls: _TcpData, data: bytes, offset: int = 0) -> dict[str, Any]: + tcp_data = cls.from_buffer_copy(data, offset) + + if tcp_data.unknown0 not in [0, UINT16_MAX]: # pragma: no cover + LOGGER.warning("Unexpected unknown0 `%s`", tcp_data.unknown0) + + if tcp_data.padding0 != b"\x81\x02\x01": # pragma: no cover + LOGGER.warning("Unexpected padding0 `%s`", tcp_data.padding0) + + # For all data that's expected to be zero, print it if it's not. Perhaps + # there are more interesting fields on different inverters waiting to be + # uncovered. + for idx in range(1, 3): # pragma: no cover + name = f"padding{idx}" + padding = getattr(tcp_data, name) + if sum(padding): + LOGGER.warning("Unexpected `%s`: `%s`", name, padding) + + def list_divide_10(integers: list[int]) -> list[float | None]: + return [None if v == UINT16_MAX else v * 0.1 for v in integers] + + def int_to_bool(num: int) -> bool: + return { + 0: False, + 1: True, + }[num] + + # Set temperature to None if it matches 65326, this is returned + # when the inverter is "offline". + def temperature_to_float(temp: int) -> float | None: + return None if temp == 65326 else temp * 0.1 + + # Only these fields will be extracted from the structure + field_extractors = { + "serial_number": None, + "temperature": temperature_to_float, + "dc_input_voltage": list_divide_10, + "dc_input_current": list_divide_10, + "ac_output_current": list_divide_10, + "ac_output_voltage": list_divide_10, + "ac_output": None, + "solar_energy_today": 0.01, + "solar_energy_total": 0.1, + "solar_hours_total": None, + "inverter_active": int_to_bool, + } + + result = {} + + for name, extractor in field_extractors.items(): + value = getattr(tcp_data, name) + + if name == "ac_output": + # Flatten the list of frequency+power AC objects + + result["ac_output_frequency"] = [o.get_frequency() for o in value] + result["ac_output_power"] = [o.get_power() for o in value] + continue + + if isinstance(extractor, float): + value *= extractor + elif extractor is not None: + value = extractor(value) + elif isinstance(value, bytes): + value = value.decode(encoding="utf-8") + + result[name] = value + + return result + class _TcpFirmwareStrings(BigEndianStructure): _pack_ = 1 @@ -82,6 +153,17 @@ class _TcpFirmwareStrings(BigEndianStructure): ("padding4", c_ubyte * 4), ] + @classmethod + def parse(cls: _TcpFirmwareStrings, data: bytes, offset: int = 0) -> dict[str, Any]: + tcp_firmware_data = cls.from_buffer_copy(data, offset) + + result = {} + + for name in ["firmware", "firmware_slave"]: + result[name] = getattr(tcp_firmware_data, name).decode(encoding="utf-8") + + return result + def _pack_message( message_type: int, @@ -263,83 +345,10 @@ def _parse_information_reply(data: bytes) -> dict[str, Any]: len(data), ) - tcp_data = _TcpData.from_buffer_copy(data) - tcp_firmware_data = None + result = _TcpData.parse(data) + # Only parse firmware strings if available - if len(data) >= sizeof(tcp_data) + sizeof(_TcpFirmwareStrings): - tcp_firmware_data = _TcpFirmwareStrings.from_buffer_copy(data, sizeof(tcp_data)) - - if tcp_data.unknown0 not in [0, UINT16_MAX]: # pragma: no cover - LOGGER.warning("Unexpected unknown0 `%s`", tcp_data.unknown0) - - if tcp_data.padding0 != b"\x81\x02\x01": # pragma: no cover - LOGGER.warning("Unexpected padding0 `%s`", tcp_data.padding0) - - # For all data that's expected to be zero, print it if it's not. Perhaps - # there are more interesting fields on different inverters waiting to be - # uncovered. - for idx in range(1, 5): # pragma: no cover - name = f"padding{idx}" - padding = getattr(tcp_data, name, None) or getattr( - tcp_firmware_data, - name, - None, - ) - if padding is not None and sum(padding): - LOGGER.warning("Unexpected `%s`: `%s`", name, padding) - - def list_divide_10(integers: list[int]) -> list[float | None]: - return [None if v == UINT16_MAX else v * 0.1 for v in integers] - - def int_to_bool(num: int) -> bool: - return { - 0: False, - 1: True, - }[num] - - # Set temperature to None if it matches 65326, this is returned - # when the inverter is "offline". - def temperature_to_float(temp: int) -> float | None: - return None if temp == 65326 else temp * 0.1 - - # Only these fields will be extracted from the structure - field_extractors = { - "serial_number": None, - "temperature": temperature_to_float, - "dc_input_voltage": list_divide_10, - "dc_input_current": list_divide_10, - "ac_output_current": list_divide_10, - "ac_output_voltage": list_divide_10, - "ac_output": None, - "solar_energy_today": 0.01, - "solar_energy_total": 0.1, - "solar_hours_total": None, - "inverter_active": int_to_bool, - } - - result: dict[str, Any] = {} - - for name, extractor in field_extractors.items(): - value = getattr(tcp_data, name) - - if name == "ac_output": - # Flatten the list of frequency+power AC objects - - result["ac_output_frequency"] = [o.get_frequency() for o in value] - result["ac_output_power"] = [o.get_power() for o in value] - continue - - if isinstance(extractor, float): - value *= extractor - elif extractor is not None: - value = extractor(value) - elif isinstance(value, bytes): - value = value.decode(encoding="utf-8") - - result[name] = value - - if tcp_firmware_data is not None: - for name in ["firmware", "firmware_slave"]: - result[name] = getattr(tcp_firmware_data, name) + if len(data) >= sizeof(_TcpData) + sizeof(_TcpFirmwareStrings): + result.update(_TcpFirmwareStrings.parse(data, sizeof(_TcpData))) return result From 7a6168c799cf6bd48dc0e5afbfa62b4be6f4b87d Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Fri, 14 Apr 2023 01:38:36 +0200 Subject: [PATCH 3/5] Test inverter reply without firmware --- tests/fixtures/tcp_reply_no_firmware.data | Bin 0 -> 99 bytes tests/test_tcp_models.py | 38 ++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 tests/fixtures/tcp_reply_no_firmware.data diff --git a/tests/fixtures/tcp_reply_no_firmware.data b/tests/fixtures/tcp_reply_no_firmware.data new file mode 100644 index 0000000000000000000000000000000000000000..f00389224de71c32d85fdc4437187113e6602cf9 GIT binary patch literal 99 zcmc}@b=+V&;gvE7H!?B$`MCHQ85kKD8XFlJ85o-~ykeQp!0`V+0|Uc!Aj$Y22sn;{ gNZ~G!I0P_VWMKT3&A?D`hk+5O5~3c&0D@0q0N(c}V*mgE literal 0 HcmV?d00001 diff --git a/tests/test_tcp_models.py b/tests/test_tcp_models.py index 0a88d645..a552b3f8 100644 --- a/tests/test_tcp_models.py +++ b/tests/test_tcp_models.py @@ -297,6 +297,44 @@ async def test_inverter_tcp() -> None: await server_exit +async def test_inverter_tcp_no_firmware() -> None: + """Test request from an Inverter without firmware reply - TCP source.""" + serial_number = 987654321 + + (server_exit, port) = tcp_server(serial_number, "tcp_reply_no_firmware.data") + + client = OmnikInverter( + host="localhost", + source_type="tcp", + serial_number=serial_number, + tcp_port=port, + ) + + inverter: Inverter = await client.inverter() + + assert inverter + assert inverter.solar_rated_power is None + assert inverter.solar_current_power == 0 + + assert inverter.model is None + assert inverter.serial_number == "NLDN202013212035" + assert inverter.temperature == 23.400000000000002 + assert inverter.dc_input_voltage == [118.30000000000001, 0.0, None] + assert inverter.dc_input_current == [0.0, 15.100000000000001, None] + assert inverter.ac_output_voltage == [224.5, None, None] + assert inverter.ac_output_current == [0.1, None, None] + assert inverter.ac_output_frequency == [50.02, None, None] + assert inverter.ac_output_power == [0, None, None] + assert inverter.solar_energy_today == 7.21 + assert inverter.solar_energy_total == 12861.900000000001 + assert inverter.solar_hours_total == 30940 + assert inverter.inverter_active is True + assert inverter.firmware is None + assert inverter.firmware_slave is None + + await server_exit + + async def test_inverter_tcp_offline() -> None: """Test request from an Inverter (offline) - TCP source.""" serial_number = 1608449224 From 985dd6164613640a13eee3df06197e53d9e7ab65 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 24 May 2023 20:30:02 +0200 Subject: [PATCH 4/5] Drop `classmethod` type annotations and globally ignore `ANN102` --- omnikinverter/tcp.py | 4 ++-- pyproject.toml | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/omnikinverter/tcp.py b/omnikinverter/tcp.py index 52698f93..31265b22 100644 --- a/omnikinverter/tcp.py +++ b/omnikinverter/tcp.py @@ -73,7 +73,7 @@ class _TcpData(BigEndianStructure): ] @classmethod - def parse(cls: _TcpData, data: bytes, offset: int = 0) -> dict[str, Any]: + def parse(cls, data: bytes, offset: int = 0) -> dict[str, Any]: tcp_data = cls.from_buffer_copy(data, offset) if tcp_data.unknown0 not in [0, UINT16_MAX]: # pragma: no cover @@ -154,7 +154,7 @@ class _TcpFirmwareStrings(BigEndianStructure): ] @classmethod - def parse(cls: _TcpFirmwareStrings, data: bytes, offset: int = 0) -> dict[str, Any]: + def parse(cls, data: bytes, offset: int = 0) -> dict[str, Any]: tcp_firmware_data = cls.from_buffer_copy(data, offset) result = {} diff --git a/pyproject.toml b/pyproject.toml index 86e95019..383fde65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -139,6 +139,7 @@ asyncio_mode = "auto" select = ["ALL"] ignore = [ "ANN101", # Self... explanatory + "ANN102", # cls in classmethods "ANN401", # Opinionated warning on disallowing dynamically typed expressions "D203", # Conflicts with other rules "D213", # Conflicts with other rules From e4931cd008c73c5cb7ed7dd27a06b0a335a19122 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Wed, 24 May 2023 20:33:33 +0200 Subject: [PATCH 5/5] Add docstring to `parse()` methods --- omnikinverter/tcp.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/omnikinverter/tcp.py b/omnikinverter/tcp.py index 31265b22..9d5fdb64 100644 --- a/omnikinverter/tcp.py +++ b/omnikinverter/tcp.py @@ -74,6 +74,7 @@ class _TcpData(BigEndianStructure): @classmethod def parse(cls, data: bytes, offset: int = 0) -> dict[str, Any]: + """Parse `data` into all fields described by this C structure.""" tcp_data = cls.from_buffer_copy(data, offset) if tcp_data.unknown0 not in [0, UINT16_MAX]: # pragma: no cover @@ -155,6 +156,7 @@ class _TcpFirmwareStrings(BigEndianStructure): @classmethod def parse(cls, data: bytes, offset: int = 0) -> dict[str, Any]: + """Parse `data` into all fields described by this C structure.""" tcp_firmware_data = cls.from_buffer_copy(data, offset) result = {}