Skip to content

Logaddress issue testing code #332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Logaddress issue testing code #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f89ed4a
expose logaddress and create set-function to update logaddress
dirixmjm Sep 5, 2025
333acdf
bump testpypi version number
dirixmjm Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plugwise_usb/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ class EnergyStatistics:
hour_production_reset: datetime | None = None
day_production: float | None = None
day_production_reset: datetime | None = None
current_logaddress: int | None = None


@dataclass(frozen=True)
Expand Down
11 changes: 9 additions & 2 deletions plugwise_usb/messages/requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ def __init__(
mac: bytes,
dt: datetime,
protocol_version: float,
reset: bool = False,
reset: int | bool = False,
) -> None:
"""Initialize CircleLogDataRequest message object."""
super().__init__(send_fn, mac)
Expand All @@ -754,7 +754,14 @@ def __init__(
this_date = DateTime(dt.year, dt.month, month_minutes)
this_time = Time(dt.hour, dt.minute, dt.second)
day_of_week = Int(dt.weekday(), 2)
if reset:
if isinstance(reset, int):
self._args += [
this_date,
LogAddr(reset, 8, False),
this_time,
day_of_week,
]
elif reset:
self._args += [
this_date,
Comment on lines +757 to 766
Copy link

@coderabbitai coderabbitai bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Bug: bool is a subclass of int — default reset=False currently resets logs

isinstance(reset, int) matches True/False. With the current order, reset=True becomes LogAddr(1, …) and reset=False becomes LogAddr(0, …), unintentionally resetting by default. Reorder the checks and validate input.

Apply:

-        if isinstance(reset, int):
-            self._args += [
-                this_date,
-                LogAddr(reset, 8, False),
-                this_time,
-                day_of_week,
-            ]
-        elif reset:
-            self._args += [
-                this_date,
-                LogAddr(0, 8, False),
-                this_time,
-                day_of_week,
-            ]
-        else:
-            self._args += [this_date, String("FFFFFFFF", 8), this_time, day_of_week]
+        if isinstance(reset, bool):
+            # True -> reset to address 0; False -> no reset (FFFFFFFF sentinel)
+            if reset:
+                self._args += [this_date, LogAddr(0, 8, False), this_time, day_of_week]
+            else:
+                self._args += [this_date, String("FFFFFFFF", 8), this_time, day_of_week]
+        elif isinstance(reset, int):
+            if reset < 0:
+                raise MessageError(f"Invalid logaddress {reset}: must be >= 0")
+            self._args += [
+                this_date,
+                LogAddr(reset, 8, False),
+                this_time,
+                day_of_week,
+            ]
+        else:
+            raise MessageError(f"Invalid type for reset: {type(reset)!r}. Expected bool or int.")

Check call sites and add a focused test for the int path:

🏁 Script executed:

#!/bin/bash
rg -nP '\bCircleClockSetRequest\(' -C2

Length of output: 1464

We need to inspect the CircleClockSetRequest constructor to verify default values and signature before concluding. Please run:

🏁 Script executed:

#!/bin/bash
grep -R -n 'class CircleClockSetRequest' plugwise_usb/messages/requests.py
grep -R -n 'def __init__' plugwise_usb/messages/requests.py

Length of output: 887

🏁 Script executed:

#!/bin/bash
# Extract the CircleClockSetRequest class implementation to inspect its __init__ method
sed -n '723,800p' plugwise_usb/messages/requests.py

Length of output: 2471

Fix reset logic in CircleClockSetRequest to distinguish bool from int
Invert the isinstance(reset, …) checks so that bool is handled first—default reset=False now produces the no‐reset sentinel, and only True or a non‐negative int triggers a log reset. Raise MessageError on negative or invalid types.

-        if isinstance(reset, int):
-            self._args += [
-                this_date,
-                LogAddr(reset, 8, False),
-                this_time,
-                day_of_week,
-            ]
-        elif reset:
-            self._args += [
-                this_date,
-                LogAddr(0, 8, False),
-                this_time,
-                day_of_week,
-            ]
-        else:
-            self._args += [this_date, String("FFFFFFFF", 8), this_time, day_of_week]
+        if isinstance(reset, bool):
+            # True → reset to address 0; False → no reset (FFFFFFFF sentinel)
+            if reset:
+                self._args += [this_date, LogAddr(0, 8, False), this_time, day_of_week]
+            else:
+                self._args += [this_date, String("FFFFFFFF", 8), this_time, day_of_week]
+        elif isinstance(reset, int):
+            if reset < 0:
+                raise MessageError(f"Invalid logaddress {reset}: must be >= 0")
+            self._args += [
+                this_date,
+                LogAddr(reset, 8, False),
+                this_time,
+                day_of_week,
+            ]
+        else:
+            raise MessageError(f"Invalid type for reset: {type(reset)!r}. Expected bool or int.")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if isinstance(reset, int):
self._args += [
this_date,
LogAddr(reset, 8, False),
this_time,
day_of_week,
]
elif reset:
self._args += [
this_date,
if isinstance(reset, bool):
# True → reset to address 0; False → no reset (FFFFFFFF sentinel)
if reset:
self._args += [this_date, LogAddr(0, 8, False), this_time, day_of_week]
else:
self._args += [this_date, String("FFFFFFFF", 8), this_time, day_of_week]
elif isinstance(reset, int):
if reset < 0:
raise MessageError(f"Invalid logaddress {reset}: must be >= 0")
self._args += [
this_date,
LogAddr(reset, 8, False),
this_time,
day_of_week,
]
else:
raise MessageError(f"Invalid type for reset: {type(reset)!r}. Expected bool or int.")
🤖 Prompt for AI Agents 
In plugwise_usb/messages/requests.py around lines 757 to 766, the reset handling
in CircleClockSetRequest incorrectly treats bool as int; update the branch order
so bool is checked first: if reset is False (default) append the no-reset
sentinel args, if reset is True append the reset-without-address args, if reset
is an int and >= 0 append the reset-with-address args using LogAddr(reset,...),
and raise MessageError for negative ints or any other invalid types; ensure the
isinstance checks are inverted and add the explicit validation and MessageError
for invalid values.

LogAddr(0, 8, False),
Expand Down
111 changes: 90 additions & 21 deletions plugwise_usb/nodes/circle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from ..constants import (
DAY_IN_HOURS,
DEFAULT_CONS_INTERVAL,
LOGADDR_MAX,
MAX_TIME_DRIFT,
MINIMAL_POWER_UPDATE,
NO_PRODUCTION_INTERVAL,
Expand Down Expand Up @@ -1030,26 +1031,11 @@ async def node_info_update(
node_info = await node_request.send()

if node_info is None:
_LOGGER.debug("No response for node_info_update() for %s", self.mac)
await self._available_update_state(False)
return None

await super().node_info_update(node_info)
await self._relay_update_state(
node_info.relay_state, timestamp=node_info.timestamp
)
if self._current_log_address is not None and (
self._current_log_address > node_info.current_logaddress_pointer
or self._current_log_address == 1
):
# Rollover of log address
_LOGGER.debug(
"Rollover log address from %s into %s for node %s",
self._current_log_address,
node_info.current_logaddress_pointer,
self._mac_in_str,
)

if self._current_log_address != node_info.current_logaddress_pointer:
self._current_log_address = node_info.current_logaddress_pointer

return self._node_info

Expand All @@ -1059,14 +1045,29 @@ async def update_node_details(
) -> bool:
"""Process new node info and return true if all fields are updated."""
if node_info.relay_state is not None:
self._relay_state = replace(
self._relay_state,
state=node_info.relay_state,
timestamp=node_info.timestamp,
await self._relay_update_state(
node_info.relay_state, timestamp=node_info.timestamp
)

if (
node_info.current_logaddress_pointer is not None
and self._current_log_address is not None
and (
self._current_log_address < node_info.current_logaddress_pointer
or self._current_log_address == 1
)
):
# Rollover of log address
_LOGGER.debug(
"Rollover log address from %s into %s for node %s",
self._current_log_address,
node_info.current_logaddress_pointer,
self._mac_in_str,
)

if node_info.current_logaddress_pointer is not None:
self._current_log_address = node_info.current_logaddress_pointer
self._energy_counters.set_current_logaddres(self._current_log_address)

return await super().update_node_details(node_info)

Expand Down Expand Up @@ -1363,3 +1364,71 @@ async def energy_reset_request(self) -> None:
"Node info update after energy-reset successful for %s",
self._mac_in_str,
)

async def energy_logaddr_setrequest(self, logaddr: int) -> None:
"""Set the logaddress to a specific value."""
if self._node_protocols is None:
raise NodeError("Unable to energy-reset when protocol version is unknown")

if logaddr < 1 or logaddr >= LOGADDR_MAX:
raise ValueError("Set logaddress out of range for {self._mac_in_str}")
Comment on lines +1370 to +1374
Copy link

@coderabbitai coderabbitai bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix exception text and formatting (missing f-string, wrong action).

  • Uses a literal {self._mac_in_str} instead of f-string.
  • Message says “energy-reset” instead of “set logaddress”.
-        if self._node_protocols is None:
-            raise NodeError("Unable to energy-reset when protocol version is unknown")
+        if self._node_protocols is None:
+            raise NodeError("Unable to set logaddress when protocol version is unknown")
@@
-        if logaddr < 1 or logaddr >= LOGADDR_MAX:
-            raise ValueError("Set logaddress out of range for {self._mac_in_str}")
+        if logaddr < 1 or logaddr >= LOGADDR_MAX:
+            raise ValueError(
+                f"Set logaddress out of range for {self._mac_in_str} "
+                f"(got {logaddr}, allowed 1..{LOGADDR_MAX-1})"
+            )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if self._node_protocols is None:
raise NodeError("Unable to energy-reset when protocol version is unknown")
if logaddr < 1 or logaddr >= LOGADDR_MAX:
raise ValueError("Set logaddress out of range for {self._mac_in_str}")
if self._node_protocols is None:
raise NodeError("Unable to set logaddress when protocol version is unknown")
if logaddr < 1 or logaddr >= LOGADDR_MAX:
raise ValueError(
f"Set logaddress out of range for {self._mac_in_str} "
f"(got {logaddr}, allowed 1..{LOGADDR_MAX-1})"
)
🤖 Prompt for AI Agents 
In plugwise_usb/nodes/circle.py around lines 1370-1374, update the two exception
messages: change the NodeError text to "Unable to set logaddress when protocol
version is unknown" (replace the incorrect "energy-reset" wording), and change
the ValueError to use an f-string so the MAC is interpolated (e.g. f"Set
logaddress out of range for {self._mac_in_str}" — optionally include the
attempted logaddr and valid range for clarity).

request = CircleClockSetRequest(
self._send,
self._mac_in_bytes,
datetime.now(tz=UTC),
self._node_protocols.max,
logaddr,
)
if (response := await request.send()) is None:
raise NodeError(f"Logaddress set for {self._mac_in_str} failed")

if response.ack_id != NodeResponseType.CLOCK_ACCEPTED:
raise MessageError(
f"Unexpected NodeResponseType {response.ack_id!r} received as response to CircleClockSetRequest"
)

_LOGGER.warning("Logaddress set for Node %s successful", self._mac_in_str)

# Follow up by an energy-intervals (re)set
interval_request = CircleMeasureIntervalRequest(
self._send,
self._mac_in_bytes,
DEFAULT_CONS_INTERVAL,
NO_PRODUCTION_INTERVAL,
)
if (interval_response := await interval_request.send()) is None:
raise NodeError("No response for CircleMeasureIntervalRequest")

if (
interval_response.response_type
!= NodeResponseType.POWER_LOG_INTERVAL_ACCEPTED
):
raise MessageError(
f"Unknown NodeResponseType '{interval_response.response_type.name}' received"
)
_LOGGER.warning("Resetting energy intervals to default (= consumption only)")

# Clear the cached energy_collection
if self._cache_enabled:
self._set_cache(CACHE_ENERGY_COLLECTION, "")
_LOGGER.warning(
"Energy-collection cache cleared successfully, updating cache for %s",
self._mac_in_str,
)
await self.save_cache()

# Clear PulseCollection._logs
self._energy_counters.reset_pulse_collection()
_LOGGER.warning("Resetting pulse-collection")

# Request a NodeInfo update
if await self.node_info_update() is None:
_LOGGER.warning(
"Node info update failed after energy-reset for %s",
self._mac_in_str,
)
else:
_LOGGER.warning(
"Node info update after energy-reset successful for %s",
self._mac_in_str,
)
Comment on lines +1425 to +1434
Copy link

@coderabbitai coderabbitai bot Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Bypass 30s NodeInfo skip; fix log messages.

node_info_update() has a 30s debounce; right after a logaddress change we want an immediate refresh. Also correct the message text.

-        # Request a NodeInfo update
-        if await self.node_info_update() is None:
-            _LOGGER.warning(
-                "Node info update failed after energy-reset for %s",
-                self._mac_in_str,
-            )
-        else:
-            _LOGGER.warning(
-                "Node info update after energy-reset successful for %s",
-                self._mac_in_str,
-            )
+        # Request a fresh NodeInfo update (bypass 30s skip window)
+        node_request = NodeInfoRequest(self._send, self._mac_in_bytes)
+        node_info = await node_request.send()
+        if node_info is None:
+            _LOGGER.warning(
+                "Node info update failed after logaddress set for %s",
+                self._mac_in_str,
+            )
+        else:
+            await super().node_info_update(node_info)
+            _LOGGER.info(
+                "Node info update after logaddress set successful for %s",
+                self._mac_in_str,
+            )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if await self.node_info_update() is None:
_LOGGER.warning(
"Node info update failed after energy-reset for %s",
self._mac_in_str,
)
else:
_LOGGER.warning(
"Node info update after energy-reset successful for %s",
self._mac_in_str,
)
# Request a fresh NodeInfo update (bypass 30s skip window)
node_request = NodeInfoRequest(self._send, self._mac_in_bytes)
node_info = await node_request.send()
if node_info is None:
_LOGGER.warning(
"Node info update failed after logaddress set for %s",
self._mac_in_str,
)
else:
await super().node_info_update(node_info)
_LOGGER.info(
"Node info update after logaddress set successful for %s",
self._mac_in_str,
)
🤖 Prompt for AI Agents 
In plugwise_usb/nodes/circle.py around lines 1425-1434, the code calls
node_info_update() which is debounced for 30s so it won't refresh immediately
after a logaddress change; change the call to force an immediate refresh (e.g.
await self.node_info_update(force=True) or invoke the internal immediate refresh
helper if the API differs) so the update is not skipped, and also correct the
log messages to read "Node info update failed after logaddress change for %s"
and "Node info update after logaddress change successful for %s" respectively.

11 changes: 11 additions & 0 deletions plugwise_usb/nodes/helpers/counter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ def __init__(self, mac: str) -> None:
self._mac = mac
self._calibration: EnergyCalibration | None = None
self._counters: dict[EnergyType, EnergyCounter] = {}
self._current_logaddress: int | None = None
for energy_type in ENERGY_COUNTERS:
self._counters[energy_type] = EnergyCounter(energy_type, mac)
self._pulse_collection = PulseCollection(mac)
Expand All @@ -67,6 +68,10 @@ def collected_logs(self) -> int:
"""Total collected logs."""
return self._pulse_collection.collected_logs

def set_current_logaddres(self, address: int) -> None:
"""Update current logaddress value."""
self._current_logaddress = address

def add_empty_log(self, address: int, slot: int) -> None:
"""Add empty energy log record to mark any start of beginning of energy log collection."""
self._pulse_collection.add_empty_log(address, slot)
Expand Down Expand Up @@ -127,6 +132,11 @@ def log_addresses_missing(self) -> list[int] | None:
"""Return list of addresses of energy logs."""
return self._pulse_collection.log_addresses_missing

@property
def current_logaddress(self) -> int | None:
"""Return current registered logaddress value."""
return self._current_logaddress

@property
def log_rollover(self) -> bool:
"""Indicate if new log is required due to rollover."""
Expand All @@ -149,6 +159,7 @@ def update(self) -> None:
self._pulse_collection.recalculate_missing_log_addresses()
if self._calibration is None:
return
self._energy_statistics.current_logaddress = self._current_logaddress
self._energy_statistics.log_interval_consumption = (
self._pulse_collection.log_interval_consumption
)
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"

[project]
name = "plugwise_usb"
version = "0.45.0"
version = "0.45.1a0"
license = "MIT"
keywords = ["home", "automation", "plugwise", "module", "usb"]
classifiers = [
Expand Down
2 changes: 2 additions & 0 deletions tests/test_usb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,7 @@ async def fake_get_missing_energy_logs(address: int) -> None:
hour_production_reset=None,
day_production=None,
day_production_reset=None,
current_logaddress=20,
)
# energy_update is not complete and should return none
utc_now = dt.now(UTC)
Expand All @@ -964,6 +965,7 @@ async def fake_get_missing_energy_logs(address: int) -> None:
hour_production_reset=None,
day_production=None,
day_production_reset=None,
current_logaddress=20,
)
await stick.disconnect()

Expand Down