Implement proper modbus response checks

custom_components/goodwe/goodwe/dt.py

@@ -9,8 +9,8 @@
 class DT(Inverter):
     """Class representing inverter of DT, D-NS and XS families"""
 
-    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x7531, 0x0028, 87)
-    _READ_DEVICE_RUNNING_DATA: ProtocolCommand = ModbusReadCommand(0x7594, 0x0049, 153)
+    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x7531, 0x0028)
+    _READ_DEVICE_RUNNING_DATA: ProtocolCommand = ModbusReadCommand(0x7594, 0x0049)
 
     __sensors: Tuple[Sensor, ...] = (
         Timestamp("timestamp", 0, "Timestamp"),

custom_components/goodwe/goodwe/eh.py

@@ -9,11 +9,11 @@
 class EH(Inverter):
     """Class representing inverter of EH family"""
 
-    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x88b8, 0x0021, 146)
-    _READ_DEVICE_RUNNING_DATA1: ProtocolCommand = ModbusReadCommand(0x891c, 0x007d, 257)
-    _READ_DEVICE_RUNNING_DATA2: ProtocolCommand = ModbusReadCommand(0x8ca0, 0x001b, 61)
-    _READ_BATTERY_INFO: ProtocolCommand = ModbusReadCommand(0x9088, 0x000b, 29)
-    _GET_WORK_MODE: ProtocolCommand = ModbusReadCommand(0xb798, 0x0001, 9)
+    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x88b8, 0x0021)
+    _READ_DEVICE_RUNNING_DATA1: ProtocolCommand = ModbusReadCommand(0x891c, 0x007d)
+    _READ_DEVICE_RUNNING_DATA2: ProtocolCommand = ModbusReadCommand(0x8ca0, 0x001b)
+    _READ_BATTERY_INFO: ProtocolCommand = ModbusReadCommand(0x9088, 0x000b)
+    _GET_WORK_MODE: ProtocolCommand = ModbusReadCommand(0xb798, 0x0001)
 
     __sensors: Tuple[Sensor, ...] = (
         Voltage("vpv1", 6, "PV1 Voltage", Kind.PV),
@@ -123,7 +123,7 @@ class EH(Inverter):
 
     async def read_device_info(self):
         response = await self._read_from_socket(self._READ_DEVICE_VERSION_INFO)
-        response = response[12:22]
+        response = response[5:-2]
         self.modbus_version = read_unsigned_int(response, 0)
         self.rated_power = read_unsigned_int(response, 2)
         self.ac_output_type = read_unsigned_int(response, 4)
@@ -150,7 +150,7 @@ async def set_work_mode(self, work_mode: int):
 
     async def set_ongrid_battery_dod(self, dod: int):
         if 0 <= dod <= 89:
-            await self._read_from_socket(ModbusWriteCommand(0xb12c, 100 - dod, 10))
+            await self._read_from_socket(ModbusWriteCommand(0xb12c, 100 - dod))
 
     @classmethod
     def sensors(cls) -> Tuple[Sensor, ...]:

custom_components/goodwe/goodwe/et.py

@@ -9,11 +9,11 @@
 class ET(Inverter):
     """Class representing inverter of ET family"""
 
-    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x88b8, 0x0021, 73)
-    _READ_RUNNING_DATA: ProtocolCommand = ModbusReadCommand(0x891c, 0x007d, 257)
-    _READ_METER_DATA: ProtocolCommand = ModbusReadCommand(0x8ca0, 0x0011, 41)
-    _READ_BATTERY_INFO: ProtocolCommand = ModbusReadCommand(0x9088, 0x000b, 29)
-    _GET_WORK_MODE: ProtocolCommand = ModbusReadCommand(0xb798, 0x0001, 9)
+    _READ_DEVICE_VERSION_INFO: ProtocolCommand = ModbusReadCommand(0x88b8, 0x0021)
+    _READ_RUNNING_DATA: ProtocolCommand = ModbusReadCommand(0x891c, 0x007d)
+    _READ_METER_DATA: ProtocolCommand = ModbusReadCommand(0x8ca0, 0x0011)
+    _READ_BATTERY_INFO: ProtocolCommand = ModbusReadCommand(0x9088, 0x000b)
+    _GET_WORK_MODE: ProtocolCommand = ModbusReadCommand(0xb798, 0x0001)
 
     # Modbus registers from offset 0x891c (35100), count 0x7d (125)
     __sensors: Tuple[Sensor, ...] = (
@@ -208,7 +208,7 @@ async def read_runtime_data(self, include_unknown_sensors: bool = False) -> Dict
 
     async def read_settings(self, setting_id: str) -> Any:
         setting = {s.id_: s for s in self.settings()}.get(setting_id)
-        raw_data = await self._read_from_socket(ModbusReadCommand(setting.offset, 1, 9))
+        raw_data = await self._read_from_socket(ModbusReadCommand(setting.offset, 1))
         with io.BytesIO(raw_data[5:-2]) as buffer:
             return setting.read_value(buffer)
 
@@ -225,7 +225,7 @@ async def set_work_mode(self, work_mode: int):
 
     async def set_ongrid_battery_dod(self, dod: int):
         if 0 <= dod <= 89:
-            await self._read_from_socket(ModbusWriteCommand(0xb12c, 100 - dod, 10))
+            await self._read_from_socket(ModbusWriteCommand(0xb12c, 100 - dod))
 
     @classmethod
     def sensors(cls) -> Tuple[Sensor, ...]:

custom_components/goodwe/goodwe/modbus.py

@@ -1,5 +1,9 @@
+import logging
+
 from typing import Union
 
+logger = logging.getLogger(__name__)
+
 # default inverter modbus address
 _INVERTER_ADDRESS = 0xf7
 
@@ -69,14 +73,19 @@ def append_modbus_checksum(payload: str) -> bytes:
     return bytes(data)
 
 
-def validate_modbus_response(data: bytes, response_len: int) -> bool:
+def validate_modbus_response(data: bytes) -> bool:
     """
     Validate the modbus response.
     data[0:1] is header
     data[2:3] is response type
-    data[4] is response payload length ??
+    data[4] is response payload length
     data[-2:] is crc-16 checksum
     """
-    if len(data) <= 4 or (response_len != 0 and response_len != len(data)):
+    if len(data) <= 4 or (data[4] > len(data) - 7):
+        logger.debug(f'Response has unexpected length: {len(data)}, expected {data[4] + 7}.')
+        return False
+    checksum_offset = data[4] + 5
+    if _modbus_checksum(data[2:checksum_offset]) != ((data[checksum_offset + 1] << 8) + data[checksum_offset]):
+        logger.debug(f'Response CRC-16 checksum does not match.')
         return False
-    return _modbus_checksum(data[2:-2]) == ((data[-1] << 8) + data[-2])
+    return True

custom_components/goodwe/goodwe/protocol.py

@@ -163,12 +163,17 @@ def _validate_response(data: bytes, response_type: str) -> bool:
                 or len(data) != data[6] + 9
                 or (response_type and int(response_type, 16) != int.from_bytes(data[4:6], byteorder="big", signed=True))
         ):
+            logger.debug(f'Response has unexpected length: {len(data)}, expected {data[6] + 9}.')
             return False
         else:
             checksum = 0
             for each in data[:-2]:
                 checksum += each
-            return checksum == int.from_bytes(data[-2:], byteorder="big", signed=True)
+            if checksum != int.from_bytes(data[-2:], byteorder="big", signed=True):
+                logger.debug(f'Response checksum does not match.')
+                return False
+            return True
+
 
 
 class ModbusProtocolCommand(ProtocolCommand):
@@ -194,10 +199,10 @@ class ModbusProtocolCommand(ProtocolCommand):
     Last 2 bytes of response is again the CRC-16 of the response.
     """
 
-    def __init__(self, payload: str, response_len: int = 0):
+    def __init__(self, payload: str):
         super().__init__(
             append_modbus_checksum(payload),
-            lambda x: validate_modbus_response(x, response_len),
+            lambda x: validate_modbus_response(x),
         )
 
 
@@ -206,10 +211,10 @@ class ModbusReadCommand(ProtocolCommand):
     Inverter modbus READ command for retrieving <count> modbus registers starting at register # <offset>
     """
 
-    def __init__(self, offset: int, count: int, response_len: int = 0):
+    def __init__(self, offset: int, count: int):
         super().__init__(
             create_modbus_request(MODBUS_READ_CMD, offset, count),
-            lambda x: validate_modbus_response(x, response_len),
+            lambda x: validate_modbus_response(x),
         )
 
 
@@ -218,8 +223,8 @@ class ModbusWriteCommand(ProtocolCommand):
     Inverter modbus WRITE command setting to modbus register # <register> value <value>
     """
 
-    def __init__(self, register: int, value: int, response_len: int = 0):
+    def __init__(self, register: int, value: int):
         super().__init__(
             create_modbus_request(MODBUS_WRITE_CMD, register, value),
-            lambda x: validate_modbus_response(x, response_len),
+            lambda x: validate_modbus_response(x),
         )

custom_components/goodwe/goodwe/sensor.py

@@ -220,8 +220,6 @@ def read_current(buffer: io.BytesIO, offset: int = None) -> float:
     if offset:
         buffer.seek(offset)
     value = int.from_bytes(buffer.read(2), byteorder="big", signed=True)
-    if value > 32768:
-        value = value - 65535
     return float(value) / 10
 
 
@@ -240,8 +238,6 @@ def read_power2(buffer: io.BytesIO, offset: int = None) -> int:
     if offset:
         buffer.seek(offset)
     value = int.from_bytes(buffer.read(2), byteorder="big", signed=True)
-    if value > 32768:
-        value = value - 65535
     return value