Fix OCPP subprotocol negotiation and version handling across reconnects - #2012
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe integration adds OCPP version configuration, server-ordered websocket negotiation, version-specific ChargePoint construction, reconnection replacement on version changes, and an existing-entry reconfiguration flow. ChangesOCPP version support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Charger
participant CentralSystem
participant ChargePoint
Charger->>CentralSystem: Offer websocket subprotocols
CentralSystem->>CentralSystem: Select compatible configured subprotocol
CentralSystem->>ChargePoint: Build version-specific ChargePoint
CentralSystem->>ChargePoint: Start or reconnect session
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/api.py`:
- Around line 337-363: Update the ChargePoint replacement flow around
charge_point.stop() so a connection-close failure cannot skip cancellation of
the old instance’s background tasks. Ensure ChargePoint.stop() performs task
cancellation in a finally-style cleanup path, or explicitly invoke the existing
task-cancellation logic after a suppressed stop failure, before replacing the
instance in self.charge_points.
🪄 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 Plus
Run ID: 8a567e15-745c-4ffa-9b68-2f0218f178c9
📒 Files selected for processing (8)
custom_components/ocpp/api.pycustom_components/ocpp/config_flow.pycustom_components/ocpp/const.pycustom_components/ocpp/translations/en.jsoncustom_components/ocpp/translations/i-default.jsontests/const.pytests/test_api_paths.pytests/test_config_flow.py
16b67c9 to
7223ea1
Compare
|
The CodeRabbit finding about suppressing exceptions around A note on the root cause, since it isn't specific to this PR:
# custom_components/ocpp/chargepoint.py:560
async def stop(self):
"""Close connection and cancel ongoing tasks."""
self.status = STATE_UNAVAILABLE
if self._connection.state is State.OPEN:
await self._connection.close() # if this raises...
for task in self.tasks: # ...this never runs
task.cancel()So any caller whose I've handled it defensively at the new call site in Happy to do either of the following, whichever you prefer:
Just say which and I'll follow up. |
7223ea1 to
6383787
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_config_flow.py (1)
244-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the duplicated local
CONF_OCPP_VERSIONimport to the top of the file.Both new tests import
CONF_OCPP_VERSIONlocally; consolidating into the existing top-level import fromcustom_components.ocpp.constavoids the repeated inline import.Also applies to: 283-283
🤖 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 `@tests/test_config_flow.py` at line 244, Move the duplicated local CONF_OCPP_VERSION imports from both new tests into the existing top-level import from custom_components.ocpp.const, and remove the inline imports while preserving each test’s use of the constant.tests/test_api_paths.py (1)
592-631: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider also covering the
tasks is Nonecancellation path.This test exercises
stop()failing with a populatedself.taskslist. The PR description notes the fix also covers the case whereself.tasksis stillNone(viagetattr(charge_point, "tasks", None) or []), but no test setsself.tasks = None(or omits it) to exercise that fallback explicitly.🤖 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 `@tests/test_api_paths.py` around lines 592 - 631, Extend test_on_connect_cancels_tasks_when_stop_fails to cover a stale ChargePoint whose tasks attribute is None or absent, exercising the fallback cancellation path without raising. Keep the existing assertions that on_connect replaces the charge point and starts the new instance, while verifying the None/omitted tasks case completes safely.custom_components/ocpp/config_flow.py (1)
148-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEntry title isn't updated when
CONF_CSIDchanges via reconfigure.
STEP_USER_CS_DATA_SCHEMA(reused here) still requiresCONF_CSID, and the original entry's title is set from it at creation (title=self._data[CONF_CSID]inasync_step_user). Reconfigure only updatesentry.data, so if a user changes the CSID here the entry title in the UI will drift out of sync with the actualcsidvalue.💡 Proposed fix
self.hass.config_entries.async_update_entry( - entry, data={**entry.data, **user_input} + entry, + title=user_input[CONF_CSID], + data={**entry.data, **user_input}, )🤖 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/config_flow.py` around lines 148 - 180, Update async_step_reconfigure so changing CONF_CSID also updates the config entry title to the new CSID while preserving the existing entry data update and reload behavior. Use the submitted user_input value when calling the config-entry update API, and keep the current reconfigure_successful abort flow unchanged.
🤖 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.
Nitpick comments:
In `@custom_components/ocpp/config_flow.py`:
- Around line 148-180: Update async_step_reconfigure so changing CONF_CSID also
updates the config entry title to the new CSID while preserving the existing
entry data update and reload behavior. Use the submitted user_input value when
calling the config-entry update API, and keep the current reconfigure_successful
abort flow unchanged.
In `@tests/test_api_paths.py`:
- Around line 592-631: Extend test_on_connect_cancels_tasks_when_stop_fails to
cover a stale ChargePoint whose tasks attribute is None or absent, exercising
the fallback cancellation path without raising. Keep the existing assertions
that on_connect replaces the charge point and starts the new instance, while
verifying the None/omitted tasks case completes safely.
In `@tests/test_config_flow.py`:
- Line 244: Move the duplicated local CONF_OCPP_VERSION imports from both new
tests into the existing top-level import from custom_components.ocpp.const, and
remove the inline imports while preserving each test’s use of the constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59dbc0fb-1421-4590-b37e-8a51767271e1
📒 Files selected for processing (8)
custom_components/ocpp/api.pycustom_components/ocpp/config_flow.pycustom_components/ocpp/const.pycustom_components/ocpp/translations/en.jsoncustom_components/ocpp/translations/i-default.jsontests/const.pytests/test_api_paths.pytests/test_config_flow.py
🚧 Files skipped from review as they are similar to previous changes (4)
- custom_components/ocpp/translations/i-default.json
- custom_components/ocpp/const.py
- custom_components/ocpp/translations/en.json
- custom_components/ocpp/api.py
|
The CodeRabbit finding about suppressing exceptions around A note on the root cause, since it isn't specific to this PR:
# custom_components/ocpp/chargepoint.py:560
async def stop(self):
"""Close connection and cancel ongoing tasks."""
self.status = STATE_UNAVAILABLE
if self._connection.state is State.OPEN:
await self._connection.close() # if this raises...
for task in self.tasks: # ...this never runs
task.cancel()So any caller whose I've handled it defensively at the new call site in Happy to do either of the following, whichever you prefer:
Just say which and I'll follow up. |
GitHub Actions
Inactive
Thanks, a separate PR is preferred for the stop() fix |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2012 +/- ##
==========================================
+ Coverage 94.96% 95.03% +0.07%
==========================================
Files 12 12
Lines 2962 3004 +42
==========================================
+ Hits 2813 2855 +42
Misses 149 149 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A charge point that supports several OCPP versions can break the integration in multiple ways, observed live with a FoxESS A022KP1 (issue lbbrhzn#2008): - After a version switch the charger makes a short-lived ocpp1.6 probe connection before its real connection. The probe caused a v1.6 ChargePoint to be built and cached for the cp_id; the real 2.0.1 connection then reused the cached object and validated its 2.0.1 BootNotification against the 1.6 schema, raising FormatViolationError on every reconnect until the config entry was reloaded. - select_subprotocol iterated a set() of the charger's offer, so when a charger offered several subprotocols the negotiated version depended on set-iteration order and varied between handshakes. Changes: - select_subprotocol: iterate the server's ordered subprotocol list, which is the documented behaviour of the websockets default this function overrides ('pick the first one in the list declared the server'). The override closely mirrors that default but iterated the client's offer rather than the server's declared list, which is what made selection non-deterministic. The other deviation from the default - returning None instead of raising when the client offers no subprotocol - is deliberate and is preserved, since it is what allows a charger offering no subprotocol to default to OCPP 1.6. This restores the intended deterministic behaviour; chargers offering a single subprotocol are unaffected, while a dual-stack charger now consistently negotiates the first entry of DEFAULT_SUBPROTOCOLS (ocpp1.6) instead of an arbitrary one. - on_connect: when a charger reconnects negotiating a different OCPP version than the cached ChargePoint was built with, stop the stale object and rebuild it from the current handshake's subprotocol instead of reusing it. - New config-flow option 'OCPP version' (default 'auto'). Pinning a version restricts negotiation to that version's subprotocol, so mixed-version probes are rejected at the websocket handshake and never reach the charge point cache, and it is how a charger is held to a version other than the default preference. When pinned to a 2.x version, connections offering no subprotocol at all are rejected too (previously they silently became v1.6 charge points). Note the pin applies to the whole central system entry (one websocket port): mixed-version installations should keep 'auto' or use separate ports. - Reconfigure flow: existing entries can now change central-system settings (including the new OCPP version pin) without deleting and re-adding the integration. - Debug log of the charger's offered subprotocols to aid diagnosis. Fixes lbbrhzn#2008
6383787 to
0f3ddb3
Compare
GitHub Actions
Inactive
* Always cancel charge point tasks when stopping ChargePoint.stop() closed the websocket before cancelling self.tasks, so a close that raised (or was cancelled) skipped cancellation entirely and left monitor_connection running against a connection the charge point no longer owns - duplicate pings and metric writes from an instance that is supposed to be stopped. Move the cancellation into a finally block so it runs on every path, and tolerate self.tasks still being None: stop() is reachable before run() has populated it, where it previously raised TypeError. The close exception still propagates, so callers learn the close failed. Split out of #2012 at the maintainer's request. * Test that a cancelled websocket close still cancels tasks CancelledError derives from BaseException, so it is the one close failure an "except Exception" guard at a call site cannot catch - only the finally block covers it. Pin that path explicitly. Addresses CodeRabbit review on #2013. --------- Co-authored-by: KingHavok <shane@shanewilliams.com.au>
2 participants
Fixes #2008
Background
Setting up a FoxESS A-series charger (A022KP1) against this integration was
unreliable until the charger was locked to OCPP 1.6J on the charger side; switching
it to 2.0.1 (to explore ISO 15118/SoC support) produced the
FormatViolationErrorloop in #2008, and switching back to 1.6 did not recover — only a config-entry
reload did. To isolate whether the charger or the integration was at fault, the
charger was pointed at a standalone test CSMS and its handshakes captured in all
three of its modes (1.6J / 2.0.1 / auto). The charger turned out to be well-behaved
(it honours whatever subprotocol is negotiated and holds a stable 2.0.1 session);
the captures identified the integration-side root causes fixed here.
Problem
A charge point that supports several OCPP versions can wedge the integration into a
FormatViolationErrorloop that only a config-entry reload clears. Root cause wasconfirmed on the wire against a FoxESS A022KP1 using a standalone test CSMS:
After an OCPP version switch, the charger makes a short-lived
ocpp1.6probeconnection (~3 s, self-dropped) before its real connection. The probe caused a
v1.6
ChargePointto be built and cached for thecp_id. The real 2.0.1connection then reused the cached v16 object, and its 2.0.1
BootNotification(
chargingStation/reason) was validated against the 1.6 schema:This repeats on every reconnect (both directions: 1.6→2.0.1 and 2.0.1→1.6);
charger power-cycles don't clear it, only reloading the config entry does.
Independently,
select_subprotocoliterated aset()of the charger's offer, sowhen a charger offered several subprotocols (this charger's "auto" mode offers
ocpp2.0.1, ocpp1.6) the negotiated version depended on set-iteration order andcould vary between handshakes.
Changes
on_connect: when a charger reconnects negotiating a different OCPP versionthan the cached
ChargePointwas built with, stop the stale object and rebuild itfrom the current handshake's subprotocol instead of reusing it. Same-version
reconnects keep the existing fast
reconnect()path.select_subprotocol: iterate the server's ordered subprotocol list, which isthe documented behaviour of the
websocketsdefault this function overrides("pick the first one in the list declared the server"). The override closely
mirrors that default but iterated the client's offer rather than the server's
declared list, which is what made selection non-deterministic. The other deviation
from the default — returning
Noneinstead of raising when the client offers nosubprotocol — is deliberate and is preserved, since it is what allows a charger
offering no subprotocol to default to OCPP 1.6. This restores the intended
deterministic behaviour. Chargers offering a single subprotocol are unaffected; a
dual-stack charger now consistently negotiates the first entry of
DEFAULT_SUBPROTOCOLS(ocpp1.6) instead of an arbitrary one, so a user whoseprocess happened to select 2.0.1 through the old unordered set may see the
negotiated version change — the new OCPP-version option is the explicit lever for
that.
auto, preserving currentbehaviour). Pinning a version restricts negotiation to that version's subprotocol,
so mixed-version probes are rejected at the websocket handshake and never reach the
charge point cache. When pinned to a 2.x version, connections offering no
subprotocol are rejected too (previously they silently became v1.6 charge points).
async_step_reconfigure): existing entries can changecentral-system settings — including the new pin — without deleting and re-adding
the integration.
Scope note
The pin applies to the whole central-system entry (one websocket port). Mixed-version
installations should keep
auto(which now self-heals thanks to the rebuild) or useseparate ports/entries.
Validation
auto) via a standalone CSMS, which is how the transient 1.6 probe and the per-mode
subprotocol offers were identified.
charger between 1.6J, 2.0.1 and auto — including six connections inside one minute —
logged a rebuild on every version change and completed post-connect setup on the
first attempt each time, with no
FormatViolationErrorand no reconnect loop.orders select 1.6), pin overriding that preference, pin restriction +
no-subprotocol rejection,
on_connectrebuild-on-version-change andreuse-when-unchanged, reconfigure flow round-trip.
no-subprotocol rejection were additionally verified through real loopback websocket
connections.
Note
A companion fix for the OCPP 2.0.1
units_changed/ connector-count symptom alsodescribed in #2008 is being prepared as a separate PR against
ocppv201.py; the twochanges touch disjoint files and are independent.
Summary by CodeRabbit
New Features
Bug Fixes
Tests