diff --git a/melnor_bluetooth/device.py b/melnor_bluetooth/device.py index 6ba6e83..f5f1072 100644 --- a/melnor_bluetooth/device.py +++ b/melnor_bluetooth/device.py @@ -6,7 +6,7 @@ import logging import struct from datetime import datetime, time -from typing import List +from typing import List, Optional, Union from bleak.backends.device import BLEDevice from bleak.exc import BleakError @@ -37,7 +37,7 @@ class Valve: """Wrapper class to handle interacting with individual valves on a Melnor timer""" - _device: Device + _device: "Device" _frequency: Frequency _id: int _is_watering: bool @@ -53,39 +53,53 @@ def __init__(self, identifier: int, device) -> None: self._manual_minutes = 20 self._end_time = 0 - def update_state(self, raw_bytes: bytes, uuid: str) -> None: - """Update the state of the valve from the raw bytes""" + def update_state(self, raw_bytes: Optional[bytes], uuid: str) -> None: + """Update the state of the valve from the raw bytes (defensive against empty/short reads).""" + + if not raw_bytes: + _LOGGER.debug("Valve %s: empty payload for UUID %s; skipping update", self._id, uuid) + return + + # helper that validates length before unpack + def _unpack(fmt: str, data: bytes, offset: int = 0): + needed = struct.calcsize(fmt) + if offset < 0 or (offset + needed) > len(data): + _LOGGER.debug( + "Valve %s: insufficient data for %s at offset %s (len=%s) UUID=%s", + self._id, fmt, offset, len(data), uuid + ) + raise ValueError("insufficient data") + return struct.unpack_from(fmt, data, offset) offset = self._id * 5 if uuid == VALVE_MANUAL_SETTINGS_UUID: - # Parses a 5 byte segment from the device and updates the state of the zone - # [ - # 0 - 0x00, # is_watering - boolean - # 1-2 - 0x00, # manual_watering_time - unsigned short - # 3-4 - 0x00, # duplicate of byte 1 - # ] - - self._is_watering = struct.unpack_from(">?", raw_bytes, offset)[0] - self._manual_minutes = struct.unpack_from(">H", raw_bytes, offset + 1)[0] + # payload slice is 5 bytes per valve: + # 0: bool is_watering + # 1-2: uint16 manual_watering_time + # 3-4: duplicate of 1-2 + try: + self._is_watering = _unpack(">?", raw_bytes, offset)[0] + self._manual_minutes = _unpack(">H", raw_bytes, offset + 1)[0] + except ValueError: + return elif uuid == VALVE_MANUAL_STATES_UUID: - # byte segment for manual watering time left - # [ - # 0 - 0x00, # unclear, 0-2 - # 1-4 - 0x00, # timestamp - unsigned int - # ] - - parsed_time = self._end_time = struct.unpack_from( - ">I", raw_bytes, offset + 1 - )[0] - + # payload slice (per valve): + # 0 - unknown (0-2) + # 1-4 - uint32 timestamp end time (epoch seconds) + try: + parsed_time = _unpack(">I", raw_bytes, offset + 1)[0] + except ValueError: + return self._end_time = parsed_time - date.time_shift() if parsed_time != 0 else 0 elif uuid == VALVE_ON_OFF_UUID: - self._is_frequency_schedule_enabled = struct.unpack_from( - ">?", raw_bytes, self._id - )[0] + # per valve 1 byte boolean at index == valve_id + try: + self._is_frequency_schedule_enabled = _unpack(">?", raw_bytes, self._id)[0] + except ValueError: + return elif ( (self._id == 0 and uuid == VALVE_0_MODE_UUID) @@ -93,21 +107,15 @@ def update_state(self, raw_bytes: bytes, uuid: str) -> None: or (self._id == 2 and uuid == VALVE_2_MODE_UUID) or (self._id == 3 and uuid == VALVE_3_MODE_UUID) ): - # byte segment for valve mode - # [ - # 0 - 0x00, # unclear, always 0 - # 1-4 - 0x00, # timestamp - unsigned int - # 5-6 - 0x00, # duration - unsigned short - # 7 - 0x00, # frequency - unsigned char - # ] - + # Let Frequency handle its own validation self._frequency.update_state(raw_bytes) @property - def frequency_bytes(self) -> bytes | None: + def frequency_bytes(self) -> Optional[bytes]: """Returns the frequency bytes""" if self._frequency is not None: return self._frequency.to_bytes() + return None @property def id(self) -> int: @@ -130,22 +138,16 @@ def frequency(self) -> Frequency: return self._frequency @property - def next_cycle(self) -> datetime | None: + def next_cycle(self) -> Optional[datetime]: """Returns the next cycle time""" if self.schedule_enabled: return self._frequency.next_run_time - return None @is_watering.setter @deprecated(version="0.0.18", reason="Use set_is_watering instead") def is_watering(self, value: bool) -> None: - """ - @deprecated - Sets the zone watering state - This isn't necessarily safe on the event loop when used in - conjunction with `Device.push_state` - if other processes are calling `Device.fetch_state`""" + """@deprecated""" self._is_watering = value @bluetooth_lock @@ -156,23 +158,18 @@ async def set_is_watering(self, value: bool) -> None: @property def manual_watering_minutes(self) -> int: - """Returns the number of seconds the zone has been manually watering""" + """Returns the number of minutes the zone has been manually watering""" return self._manual_minutes @manual_watering_minutes.setter @deprecated(version="0.0.18", reason="Use set_manual_watering_minutes instead") def manual_watering_minutes(self, value: int) -> None: - """ - @deprecated - Sets the number of seconds the zone has been manually watering. - This isn't necessarily safe on the event loop when used in - conjunction with `Device.push_state` - if other processes are calling `Device.fetch_state`""" + """@deprecated""" self._manual_minutes = value @bluetooth_lock async def set_manual_watering_minutes(self, value: int) -> None: - """Atomically set the number of seconds the valve should water.""" + """Atomically set the number of minutes the valve should water.""" self._manual_minutes = value await self._device._unsafe_push_state() # pylint: disable=protected-access @@ -205,13 +202,11 @@ async def set_frequency_enabled(self, value: bool) -> None: @property def watering_end_time(self) -> int: - """Read-only unix timestamp in seconds when watering will end. Pulled directly - from the device. To influence the value use `set_manual_watering_minutes`""" + """Read-only unix timestamp in seconds when watering will end.""" return self._end_time def _manual_setting_bytes(self) -> bytes: """Returns the 5 byte payload to be written to the device""" - return struct.pack( ">?HH", self._is_watering, @@ -250,9 +245,7 @@ def __init__(self, ble_device: BLEDevice) -> None: self._mac = ble_device.address self._valves = [] - # The 1 and 2 valve devices still use 4 valve bytes - # So we'll instantiate 4 valves to mimic that behavior - # set of bytes too 🤦‍♂️ + # The 1 and 2 valve devices still use 4 valve bytes, so we mirror that for i in range(4): self._valves.append(Valve(i, self)) @@ -274,7 +267,6 @@ async def _read_model(self): def disconnected_callback(self, client): # pylint: disable=unused-argument """Callback for when the device is disconnected""" - _LOGGER.warning("Disconnected from %s", self._mac) self._is_connected = False @@ -300,9 +292,7 @@ async def connect(self, retry_attempts=4) -> None: self._is_connected = True - # Bluez handles certain types of advertisements poorly - # To work around the missing data we grab it here - # Callers simply need to connect and it'll be populated + # Work around BlueZ advertisement quirks await self._read_model() _LOGGER.debug("Successfully connected to %s", self._mac) @@ -335,49 +325,73 @@ async def fetch_state(self) -> None: ] try: - bytes_array: List[bytes] = await asyncio.gather( + # allow exceptions in results so one bad read doesn't kill the whole pass + results: List[Union[bytes, Exception, None]] = await asyncio.gather( *[self._read(uuid) for uuid in uuids], return_exceptions=True, ) - for i, some_bytes in enumerate(bytes_array): + for i, payload in enumerate(results): uuid = uuids[i] - # This is a little awkward, but it's the only single - # attribute we read regularly. + # Handle exceptions and empty reads gracefully + if isinstance(payload, Exception): + _LOGGER.debug("Read exception for %s: %r", uuid, payload) + continue + if not payload: + _LOGGER.debug("No data for UUID %s; skipping", uuid) + continue + + # Coerce to bytes if backend returns bytearray/memoryview + if not isinstance(payload, (bytes, bytearray, memoryview)): + try: + payload = bytes(payload) # type: ignore[arg-type] + except Exception: + _LOGGER.debug("Failed to coerce payload type for %s; skipping", uuid) + continue + if isinstance(payload, (bytearray, memoryview)): + payload = bytes(payload) + if uuid == BATTERY_UUID: - self._battery = parse_battery_value(some_bytes) + try: + self._battery = parse_battery_value(payload) + except Exception as err: + _LOGGER.debug("Battery parse failed for %s: %r", self._mac, err) + continue + # Update all valves with this UUID's payload for valve in self._valves: - some_bytes = uuids.index(uuid) - valve.update_state(bytes_array[some_bytes], uuids[i]) + try: + valve.update_state(payload, uuid) + except Exception as err: + _LOGGER.debug( + "Valve %s update skipped for UUID %s due to error: %r", + valve.id, uuid, err + ) except BleakError as error: - # Only throw this error if the device is still connected + # Only re-raise if the device is still connected if self._is_connected: raise error - async def _read(self, uuid: str) -> bytes | None: + async def _read(self, uuid: str) -> Optional[bytes]: """Reads the given characteristic from the device""" if not self._is_connected: - return + return None try: return await self._connection.read_gatt_char(uuid) except BleakError: _LOGGER.error("Failed to read %s from %s", uuid, self._mac) + return None async def _unsafe_push_state(self) -> None: - """Pushes the new state of the device to the device. WARNING: This - function runs without an internal lock. Public callers should use `push_state` - instead""" + """Pushes the new state of the device to the device. WARNING: no internal lock.""" if not self._is_connected: return - on_off = self._connection.services.get_characteristic( - VALVE_MANUAL_SETTINGS_UUID - ) + on_off = self._connection.services.get_characteristic(VALVE_MANUAL_SETTINGS_UUID) if on_off is not None: await self._connection.write_gatt_char( @@ -407,27 +421,19 @@ async def _unsafe_push_state(self) -> None: zone_1_frequency_bytes = self._valves[0].frequency_bytes if zone_1_frequency_bytes is not None: - await self._connection.write_gatt_char( - VALVE_0_MODE_UUID, zone_1_frequency_bytes, True - ) + await self._connection.write_gatt_char(VALVE_0_MODE_UUID, zone_1_frequency_bytes, True) zone_2_frequency_bytes = self._valves[1].frequency_bytes if zone_2_frequency_bytes is not None: - await self._connection.write_gatt_char( - VALVE_1_MODE_UUID, zone_2_frequency_bytes, True - ) + await self._connection.write_gatt_char(VALVE_1_MODE_UUID, zone_2_frequency_bytes, True) zone_3_frequency_bytes = self._valves[2].frequency_bytes if zone_3_frequency_bytes is not None: - await self._connection.write_gatt_char( - VALVE_2_MODE_UUID, zone_3_frequency_bytes, True - ) + await self._connection.write_gatt_char(VALVE_2_MODE_UUID, zone_3_frequency_bytes, True) zone_4_frequency_bytes = self._valves[3].frequency_bytes if zone_4_frequency_bytes is not None: - await self._connection.write_gatt_char( - VALVE_3_MODE_UUID, zone_4_frequency_bytes, True - ) + await self._connection.write_gatt_char(VALVE_3_MODE_UUID, zone_4_frequency_bytes, True) updated_at = self._connection.services.get_characteristic(UPDATED_AT_UUID) @@ -440,7 +446,6 @@ async def _unsafe_push_state(self) -> None: async def push_state(self) -> None: """Pushes the new state of the device to the device""" - async with GLOBAL_BLUETOOTH_LOCK: await self._unsafe_push_state() @@ -495,41 +500,43 @@ def zone1(self) -> Valve: return self._valves[0] @property - def zone2(self) -> Valve | None: + def zone2(self) -> Optional[Valve]: """Returns the second zone on the device""" if self._valve_count > 1: return self._valves[1] + return None @property - def zone3(self) -> Valve | None: + def zone3(self) -> Optional[Valve]: """Returns the third zone on the device""" if self._valve_count > 2: return self._valves[2] + return None @property - def zone4(self) -> Valve | None: + def zone4(self) -> Optional[Valve]: """Returns the fourth zone on the device""" - if self._valve_count > 2: + if self._valve_count > 3: return self._valves[3] + return None def update_ble_device(self, ble_device: BLEDevice) -> None: """Updates the cached BLEDevice for the device""" self._ble_device = ble_device def __str__(self) -> str: - string = ( - f"{self.__class__.__name__}(\n battery={self._battery}\n valves=(\n" - ) + string = f"{self.__class__.__name__}(\n battery={self._battery}\n valves=(\n" for valve in self._valves: string += f"{valve}\n" return f"{string} )\n)" - def __getitem__(self, key: str) -> Valve | None: + def __getitem__(self, key: str) -> Optional[Valve]: if key == "zone1": return self.zone1 - elif key == "zone2": + if key == "zone2": return self.zone2 - elif key == "zone3": + if key == "zone3": return self.zone3 - elif key == "zone4": + if key == "zone4": return self.zone4 + return None diff --git a/melnor_bluetooth/utils/battery.py b/melnor_bluetooth/utils/battery.py index fa5cda5..9726c37 100644 --- a/melnor_bluetooth/utils/battery.py +++ b/melnor_bluetooth/utils/battery.py @@ -1,14 +1,27 @@ # See _getBattValue from DataUtils.java -def parse_battery_value(bytes: bytes) -> int: - """Converts the little endian 2 byte array to the battery life %""" - if (bytes[0] & 255 == 238) and (bytes[1] & 255 == 238): +def parse_battery_value(data: bytes) -> int: + """Converts the little endian 2-byte array to the battery life %. + Handles None or malformed inputs gracefully. + """ + # Defensive check: ensure we actually have 2 bytes of data + if not data or len(data) < 2: + # Return a sentinel value or 0 to indicate missing data + # (0 is safe; caller can treat it as unknown or depleted) return 0 - else: - rawVal = ( - ((bytes[0] & 255) + (bytes[1] & 255) / 256) - 2.35 - ) * 181.81818181818187 - if rawVal > 100: - return 100 + b0 = data[0] & 0xFF + b1 = data[1] & 0xFF - return int(rawVal) + # Handle sentinel bytes (0xEE 0xEE means 'no reading yet') + if b0 == 0xEE and b1 == 0xEE: + return 0 + + # Original Melnor conversion formula + rawVal = ((b0 + b1 / 256) - 2.35) * 181.81818181818187 + + if rawVal > 100: + return 100 + elif rawVal < 0: + return 0 + + return int(rawVal)