Add retry logic with exponential backoff to SetChargingProfile - #2000
Add retry logic with exponential backoff to SetChargingProfile#2000Mark-Hetherington wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOCPP 1.6 and 2.0.1 charge-rate updates now retry ChangesCharge rate retry handling
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
custom_components/ocpp/ocppv16.py (1)
419-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the retry outcomes.
Mock
self.callto cover: exception/exception/accepted with sleeps of 0.5s and 1.0s; immediate rejection with no retry; and three exceptions with exactly one notification and aFalseresult.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/ocpp/ocppv16.py` around lines 419 - 437, Add focused tests for the SetChargingProfile retry flow, mocking self.call to verify exception/exception/accepted succeeds after 0.5s and 1.0s sleeps, an immediate rejected response returns False without retrying, and three exceptions produce exactly one notification and a False result. Anchor the tests to the method containing the shown retry loop and assert call, sleep, and notify_ha counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/ocpp/ocppv16.py`:
- Around line 420-424: Adjust the retry scope around the SetChargingProfile
operation in the enclosing method so int(conn_id) and
call.SetChargingProfile(...) execute outside the retry block. Restrict retries
to the awaited self.call(req) network operation, while preserving the existing
retry and notification behavior for call failures.
---
Nitpick comments:
In `@custom_components/ocpp/ocppv16.py`:
- Around line 419-437: Add focused tests for the SetChargingProfile retry flow,
mocking self.call to verify exception/exception/accepted succeeds after 0.5s and
1.0s sleeps, an immediate rejected response returns False without retrying, and
three exceptions produce exactly one notification and a False result. Anchor the
tests to the method containing the shown retry loop and assert call, sleep, and
notify_ha counts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a6147ef-3cbb-4c6a-8aba-edc8d7c901bf
📒 Files selected for processing (1)
custom_components/ocpp/ocppv16.py
2ffa9a6 to
5293bd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/ocpp/ocppv201.py`:
- Around line 362-380: Separate response-status validation from the
retry-catching block in the SetChargingProfile flow around the identified loop,
so only exceptions from self.call are retried and non-accepted charger responses
raise HomeAssistantError once afterward. Apply the same change at
custom_components/ocpp/ocppv201.py lines 362-380 and 416-436, preserving the
existing rejection error details and successful return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6593e68d-ba45-47f5-917f-25cd46b5ca42
📒 Files selected for processing (2)
custom_components/ocpp/ocppv16.pycustom_components/ocpp/ocppv201.py
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/ocpp/ocppv16.py
5293bd4 to
de30beb
Compare
de30beb to
e037007
Compare
e037007 to
8653d10
Compare
Fixes lbbrhzn#1999 When SetChargingProfile times out due to transient network issues or charger processing delay, retry up to 3 times with exponential backoff (0.5s, 1s) before notifying the user. Immediate charger rejections still fail fast. This prevents dynamic charge rate adjustments from silently failing on brief network hiccups, which is common in IoT environments. - Adds asyncio import for sleep() during retry backoff - Retries up to 3 times for exceptions (transient failures) - Fails immediately for active rejections (charger policy) - Only notifies user if all retries exhausted - Includes debug logging for retry attempts
8653d10 to
732fffa
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
A more generalised approach would be preferred ie if a command timesout place it in a queue, fail the websocket connection and then on re-establishing the websocket connection process the queue in fifo order. |
|
Closing this PR. redesigning the timeout handling from ad-hoc per-call retries to a general mechanism. |
Fixes #1999
When SetChargingProfile times out due to transient network issues or charger processing delay, retry up to 3 times with exponential backoff (0.5s, 1s) before notifying the user. Immediate charger rejections still fail fast.
This prevents dynamic charge rate adjustments from silently failing on brief network hiccups.
Changes
Summary by CodeRabbit