Expose calling CirclePlusAllowJoiningRequest through enable_auto_join function #260
Conversation
add call to CirclePlusAllowJoinRequest from enable_auto_join function
WalkthroughThe changes introduce support for a new "circleplus" feature in the Plugwise USB integration. This includes updating feature enumerations, adding feature-to-firmware mapping, and implementing an asynchronous method to enable auto-join functionality for Circle Plus nodes. Additionally, join request handling was removed from the Stick and StickNetwork classes, and related tests were adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlugwiseCirclePlus
participant CirclePlusAllowJoiningRequest
User->>PlugwiseCirclePlus: enable_auto_join()
PlugwiseCirclePlus->>PlugwiseCirclePlus: Check if node is loaded
PlugwiseCirclePlus->>CirclePlusAllowJoiningRequest: Create request (allow_joining=True)
PlugwiseCirclePlus->>PlugwiseCirclePlus: Send request asynchronously
PlugwiseCirclePlus-->>User: Return result (bool)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
- Coverage 81.22% 81.13% -0.09%
==========================================
Files 36 36
Lines 7537 7502 -35
==========================================
- Hits 6122 6087 -35
Misses 1415 1415 ☔ 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: 2
🧹 Nitpick comments (2)
plugwise_usb/api.py (1)
57-58: Enum entry looks fine, but consider keeping alphabetical order
CIRCLEPLUSis correctly added and follows the existing lowercase-string convention.
For long-term readability, you may want to keep the enum list alphabetically sorted – a convention already adopted in several other enums in this file.plugwise_usb/nodes/circle_plus.py (1)
13-14: Imports OK – minor style nitImports follow the local relative-package style, but ordering is now interleaved between stdlib / third-party / local sections. Consider grouping the two local-package imports (
CirclePlusAllowJoiningRequest,raise_not_loaded) together for clarity.Also applies to: 18-19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/circle_plus.py(3 hunks)plugwise_usb/nodes/helpers/firmware.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/helpers/firmware.py (1)
plugwise_usb/api.py (1)
NodeFeature(39-57)
🪛 GitHub Check: codecov/patch
plugwise_usb/nodes/circle_plus.py
[warning] 130-132: plugwise_usb/nodes/circle_plus.py#L130-L132
Added lines #L130 - L132 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (2)
plugwise_usb/api.py (1)
78-88: Addition toPUSHING_FEATURESOK
NodeFeature.CIRCLEPLUSis correctly appended to the push-capable features.plugwise_usb/nodes/helpers/firmware.py (1)
156-172: Firmware mapping updated correctly
NodeFeature.CIRCLEPLUSis mapped to2.0, in line with the other first-generation features.
coderabbitai recommendations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle_plus.py (2)
128-134: Docstring violates PEP 257 / Ruff D413Add a blank line after the “Returns:” header.
- Returns: - bool: True if the request was acknowledged, False otherwise. + Returns: + + bool: True if the request was acknowledged, False otherwise.🧰 Tools
🪛 Ruff (0.11.9)
132-132: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
135-135: Log message lacks node contextStick to the existing pattern and include the MAC address for easier debugging.
- _LOGGER.info("Enabling auto-join for CirclePlus") + _LOGGER.info("Enabling auto-join for CirclePlus %s", self.mac)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/circle_plus.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/api.py
🧰 Additional context used
🪛 Ruff (0.11.9)
plugwise_usb/nodes/circle_plus.py
132-132: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (3)
plugwise_usb/nodes/circle_plus.py (3)
34-42: Feature-set parity fixed – looks good
NodeFeature.CIRCLEPLUSis now present in both cache-restore and fresh-load paths, removing the earlier discrepancy.Also applies to: 71-79
136-138:Details
✅ Verification successful
Confirm
CirclePlusAllowJoiningRequestconstructorMost request classes take
(send_fn, mac_in_bytes, enable); only passingTruemay produce an invalid frame. Please verify.
🏁 Script executed:
#!/bin/bash # Inspect the constructor signature ast-grep --pattern $'class CirclePlusAllowJoiningRequest { $$$ def __init__($$$) { $$$ } $$$ }'Length of output: 108
I’ll locate and inspect the actual constructor signature for CirclePlusAllowJoiningRequest.
🏁 Script executed:
#!/bin/bash # Find where the class is defined rg -n "class CirclePlusAllowJoiningRequest" -n . # Show its __init__ signature rg -n "def __init__" "$(rg -l "CirclePlusAllowJoiningRequest")"Length of output: 606
I’ll inspect the
CirclePlusAllowJoiningRequestclass and its basePlugwiseRequestto confirm the required constructor parameters.
🏁 Script executed:
#!/bin/bash # Show CirclePlusAllowJoiningRequest definition and its __init__ (if any) rg -n -A10 "class CirclePlusAllowJoiningRequest" plugwise_usb/messages/requests.py # Show base class PlugwiseRequest signature for inherited __init__ rg -n -A20 "class PlugwiseRequest" plugwise_usb/messages/requests.pyLength of output: 1288
I’ll inspect how requests are instantiated in
circle_plus.pyaround this call to confirm the intended parameters.
🏁 Script executed:
#!/bin/bash # Show context around the allow-joining call rg -n -C5 "CirclePlusAllowJoiningRequest" plugwise_usb/nodes/circle_plus.py # List other request instantiations for comparison rg -n -C3 "CirclePlus.*Request" plugwise_usb/nodes/circle_plus.pyLength of output: 2322
I’ll retrieve the full
CirclePlusAllowJoiningRequestimplementation to confirm its constructor:
🏁 Script executed:
#!/bin/bash # Show the full class including any __init__ override rg -n -A30 "class CirclePlusAllowJoiningRequest" plugwise_usb/messages/requests.pyLength of output: 1246
No changes required for CirclePlusAllowJoiningRequest constructor
The constructor is defined as__init__(self, send_fn, enable)and callssuper().__init__(send_fn, None), so passing onlyself._sendandTrueis correct.
141-142:Details
❌ Incorrect review comment
Potential enum mismatch & unnecessary cast
- Ensure the ACK constant really is
JOIN_ACCEPTED—older code usesALLOW_JOINING_ACCEPTED.- If
response.ack_idis already aNodeResponseType, the explicit cast is redundant and may raiseValueErrorwhen the id is unknown.- return NodeResponseType(response.ack_id) == NodeResponseType.JOIN_ACCEPTED + return response.ack_id == NodeResponseType.ALLOW_JOINING_ACCEPTEDAdjust the constant name to the actual one defined in
NodeResponseType.
🏁 Script executed:
#!/bin/bash set -e echo "1. Locating NodeResponseType definition..." rg --max-count 1 "class NodeResponseType" -n . echo echo "2. Searching for ALLOW_JOINING_ACCEPTED references..." rg "ALLOW_JOINING_ACCEPTED" -n . echo echo "3. Searching for JOIN_ACCEPTED references..." rg "JOIN_ACCEPTED" -n .Length of output: 387
🏁 Script executed:
#!/bin/bash set -e echo "Displaying NodeResponseType enum members (lines 50–120):" sed -n '50,120p' plugwise_usb/messages/responses.pyLength of output: 2564
🏁 Script executed:
#!/bin/bash set -e echo "1. Searching for ack_id property or usage..." rg "ack_id" -n plugwise_usb/messages/responses.py echo echo "2. Displaying context around NodeResponse class..." start_line=$(grep -n "class NodeResponse" -n plugwise_usb/messages/responses.py | cut -d: -f1) sed -n "$((start_line - 5)),$((start_line + 30))p" plugwise_usb/messages/responses.pyLength of output: 1143
No change needed for JOIN_ACCEPTED & enum cast
NodeResponseTypedefinesJOIN_ACCEPTED(b"00D9")—there is noALLOW_JOINING_ACCEPTEDconstant.response.ack_idis a rawbytesvalue; wrapping it inNodeResponseType(...)is required to map it to the enum before comparison.The existing code is correct as-is.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_usb.py (1)
297-300: Avoidasyncio.sleep()for timestamp separation to keep the test deterministicUsing
await asyncio.sleep(0.001)to ensure different timestamps works most of the time but relies on wall-clock granularity and the event-loop scheduler.
A busy CI runner may occasionally schedule both coroutine wake-ups within the same millisecond, making the ordering assertions flaky.A safer pattern is to bypass the real clock completely and stub the timestamp that the
PlugwiseRequestuses for ordering (e.g. viafreezegun, a parametrisednow=argument, or monkey-patching the attribute after construction).-await asyncio.sleep(0.001) # Ensure timestamp is different -circle_plus_allow_joining_request = pw_requests.CirclePlusAllowJoiningRequest( - self.dummy_fn, True -) +# Force a monotonic timestamp delta instead of relying on sleep +with freeze_time(dt.utcnow() + td(milliseconds=2)): + circle_plus_allow_joining_request = pw_requests.CirclePlusAllowJoiningRequest( + self.dummy_fn, True + )This keeps the tests both fast and fully deterministic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_usb.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_usb.py (1)
plugwise_usb/api.py (1)
NodeFeature(39-57)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (1)
tests/test_usb.py (1)
2498-2503: Feature list update looks correct
NodeFeature.CIRCLEPLUSis now included in the expected feature set for the Circle + node, matching the library change. No further issues spotted here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
5-6: Fix Markdown link syntax and naming consistency
There’s an extra space before the parentheses in the PR link (resulting in a bare URL), and “circle plus interface” should match theCirclePluscasing.- - PR [#260] (https://github.com/plugwise/python-plugwise-usb/pull/260) + - PR [#260](https://github.com/plugwise/python-plugwise-usb/pull/260) - - CirclePlus: Expose Allow Joining Request to circle plus interface + - CirclePlus: Expose Allow Joining Request to CirclePlus interface🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
5-5: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
5-5: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (1)
CHANGELOG.md (1)
3-4: New version entry looks good
The header## v0.44.3 - 2025-06-12follows the established format and is correctly positioned above v0.44.2.
|
Add NodeFeature.CIRCLEPLUS
Add enable_auto_join function which exposes CirclePlusAllowJoingingRequest
Summary by CodeRabbit