Logaddress issue testing code #332
Conversation
WalkthroughIntroduces current_logaddress support across API, counters, and tests; updates CircleClockSetRequest to accept int|bool reset; adds PlugwiseCircle.energy_logaddr_setrequest flow including validation, clock set, interval reset, cache clears, and NodeInfo refresh; enhances circle node availability handling and rollover logging; bumps version to 0.45.1a0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Circle as PlugwiseCircle
participant Msg as CircleClockSetRequest
participant Stick as USB Stick
participant Ctr as EnergyCounters
participant NI as NodeInfo
Caller->>Circle: energy_logaddr_setrequest(logaddr)
Note right of Circle: Validate protocol_version and 1 ≤ logaddr < LOGADDR_MAX
Circle->>Msg: build(reset=logaddr, dt, mac, protocol)
Msg-->>Circle: request
Circle->>Stick: send(request)
Stick-->>Circle: CLOCK_ACCEPTED / error
alt accepted
Circle->>Stick: CircleMeasureIntervalRequest(DEFAULT, NO_PROD)
Stick-->>Circle: POWER_LOG_INTERVAL_ACCEPTED
Circle->>Ctr: set_current_logaddres(logaddr)
Circle->>Circle: clear energy/pulse caches
Circle->>NI: update_node_details()
NI-->>Circle: node_info (may include pointer)
Note over Circle,Ctr: Rollover detection and propagation
else error
Note over Circle: Raise/handle NodeError
end
sequenceDiagram
autonumber
actor Caller
participant Msg as CircleClockSetRequest
Caller->>Msg: __init__(..., reset)
alt reset is int
Note right of Msg: Append date, LogAddr(reset,8), time, weekday
else reset is True
Note right of Msg: Append date, LogAddr(0,8), time, weekday
else reset is False
Note right of Msg: Append date, String("FFFFFFFF",8), time, weekday
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
- Coverage 81.30% 80.94% -0.37%
==========================================
Files 36 36
Lines 8063 8099 +36
==========================================
Hits 6556 6556
- Misses 1507 1543 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
620-647: Bug: slots_empty may be set False for outdated records.
cache_updatedis set True whenever a slot has non-None data, even if considered outdated and not stored. This contradicts the method’s contract (“recent, non-empty record was stored”) and can suppress backfill. Set the flag only when a recent record is actually stored.- cache_updated = False + any_recent = False for _slot in range(4, 0, -1): log_timestamp, log_pulses = response.log_data[_slot] _LOGGER.debug( "In slot=%s: pulses=%s, timestamp=%s", _slot, log_pulses, log_timestamp ) address = response.log_address if log_timestamp is None or log_pulses is None: self._energy_counters.add_empty_log(address, _slot) else: log_timestamp = log_timestamp.replace(tzinfo=UTC) if self._check_timestamp_is_recent(address, _slot, log_timestamp): self._energy_counters.add_pulse_log( address, _slot, log_timestamp, log_pulses, import_only=True, ) - cache_updated = True + any_recent = True @@ - if cache_updated: + if any_recent: slots_empty = False await self._energy_log_records_save_to_cache() if save_cache: _LOGGER.debug("Saving energy cache for %s", self._mac_in_str) await self.save_cache() return result, slots_empty
🧹 Nitpick comments (6)
plugwise_usb/api.py (1)
263-264: Add field documentation for EnergyStatistics.current_logaddressPlease extend the EnergyStatistics docstring to describe current_logaddress (semantics, units, and None behavior).
plugwise_usb/messages/requests.py (1)
724-731: Docstring no longer matches behavior — clarify tri-state resetPlease update the docstring to describe False/True/int semantics.
- """Set internal clock of node and flash address. - - reset=True, will reset all locally stored energy logs - - Supported protocols : 1.0, 2.0 - Response message : NodeResponse - """ + """Set internal clock of node and optionally adjust energy logaddress. + + reset parameter: + - False (default): do not change logaddress (send FFFFFFFF sentinel) + - True: reset to logaddress 0 + - int: set explicit logaddress value (>= 0) + + Supported protocols : 1.0, 2.0 + Response message : NodeResponse + """plugwise_usb/nodes/helpers/counter.py (1)
71-74: Add correctly spelled setter alias and input validation
- In plugwise_usb/nodes/helpers/counter.py, update set_current_logaddres to validate
address >= 0and raise aValueErrorfor negatives.- Add a new set_current_logaddress method with the same implementation and docstring as the alias.
- Optionally update the call in plugwise_usb/nodes/circle.py (line 1070) to use set_current_logaddress instead of the misspelled name.
plugwise_usb/nodes/circle.py (3)
1052-1066: Fix rollover detection condition (invert comparison).Rollover occurs when the new pointer wraps below the previous value, not when it’s higher. Adjust comparison to prevent false “rollover” logs.
- 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 + and self._current_log_address is not None + and ( + node_info.current_logaddress_pointer < self._current_log_address + or node_info.current_logaddress_pointer == 1 + ) + ): + _LOGGER.debug( + "Logaddress rollover detected for %s: %s -> %s", + self._mac_in_str, + self._current_log_address, + node_info.current_logaddress_pointer, + )
1390-1410: Use INFO level for success; optionally update local state after ACK.
- Success shouldn’t be logged as WARNING.
- Optionally set
_current_log_addressand counters immediately after ACK to reduce inconsistency window until NodeInfo refresh.- _LOGGER.warning("Logaddress set for Node %s successful", self._mac_in_str) + _LOGGER.info("Logaddress set for node %s successful", self._mac_in_str) + # Optional: reflect new address locally after ACK + self._current_log_address = logaddr + self._energy_counters.set_current_logaddres(logaddr)
1411-1423: Trigger fresh collection after reset.After clearing caches and pulse collection, ensure the “last energy log requested” bootstrap runs again.
# Clear PulseCollection._logs self._energy_counters.reset_pulse_collection() - _LOGGER.warning("Resetting pulse-collection") + _LOGGER.warning("Resetting pulse-collection") + self._last_energy_log_requested = False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
plugwise_usb/api.py(1 hunks)plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/nodes/circle.py(4 hunks)plugwise_usb/nodes/helpers/counter.py(4 hunks)pyproject.toml(1 hunks)tests/test_usb.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/helpers/counter.pyplugwise_usb/nodes/circle.py
📚 Learning: 2025-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (4)
plugwise_usb/api.py (1)
plugwise_usb/nodes/helpers/counter.py (1)
current_logaddress(136-138)
tests/test_usb.py (1)
plugwise_usb/nodes/helpers/counter.py (1)
current_logaddress(136-138)
plugwise_usb/messages/requests.py (1)
plugwise_usb/messages/properties.py (1)
LogAddr(387-407)
plugwise_usb/nodes/circle.py (6)
plugwise_usb/api.py (4)
mac(355-356)node_info(331-332)name(327-328)save_cache(407-410)plugwise_usb/nodes/node.py (7)
mac(235-237)_available_update_state(422-450)node_info(225-227)name(218-222)_set_cache(690-704)save_cache(706-720)node_info_update(452-465)plugwise_usb/messages/responses.py (4)
ack_id(133-135)NodeResponseType(56-72)response_type(236-240)response_type(266-270)plugwise_usb/nodes/helpers/counter.py (3)
timestamp(190-198)set_current_logaddres(71-73)reset_pulse_collection(111-113)plugwise_usb/exceptions.py (2)
NodeError(24-25)MessageError(20-21)plugwise_usb/messages/requests.py (19)
CircleClockSetRequest(723-783)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(774-783)send(807-816)send(847-856)send(879-888)send(911-920)CircleMeasureIntervalRequest(1261-1292)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (7)
pyproject.toml (1)
7-7: Confirm alpha release intent & tooling compatibilityVersion bumped to 0.45.1a0; ensure this pre-release tag is intentional and that your CI/publishing workflows (and Home Assistant pinning) properly support PEP 440 pre-releases.
tests/test_usb.py (1)
950-951: Tests updated to assert current_logaddress — LGTMThe assertions align with the new EnergyStatistics field and expected value 20.
Also applies to: 968-969
plugwise_usb/nodes/helpers/counter.py (1)
60-60: Propagating current_logaddress — looks good
- Private storage, public getter, and propagation into EnergyStatistics on update are consistent with the new API.
Also applies to: 135-139, 163-163
plugwise_usb/nodes/circle.py (4)
30-31: Import of LOGADDR_MAX is appropriate.Used for input validation later. No concerns.
1034-1036: Good: mark node unavailable when node_info is missing.The debug log plus
_available_update_state(False)is the right behavior.
1048-1051: Good: async relay state update via helper.Switching to
await self._relay_update_state(...)keeps cache and subscribers consistent.
1068-1071: Propagate current_logaddress to counters.Good propagation via
set_current_logaddres(...).
| if isinstance(reset, int): | ||
| self._args += [ | ||
| this_date, | ||
| LogAddr(reset, 8, False), | ||
| this_time, | ||
| day_of_week, | ||
| ] | ||
| elif reset: | ||
| self._args += [ | ||
| this_date, |
There was a problem hiding this comment.
💡 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\(' -C2Length 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.pyLength 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.pyLength 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.
| 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.
| 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}") |
There was a problem hiding this comment.
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.
| 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).
| 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, | ||
| ) |
There was a problem hiding this comment.
🛠️ 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.
| 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.
|
Circles cannot update logaddress in a valid way. Logaddresses get truncated. |
Summary by CodeRabbit