Skip to content

Fix intermittent reconnect failures and missing charge point ID extraction#752

Open
rishabhvaish wants to merge 2 commits intomobilityhouse:masterfrom
rishabhvaish:fix/751-reconnect-charge-point-id
Open

Fix intermittent reconnect failures and missing charge point ID extraction#752
rishabhvaish wants to merge 2 commits intomobilityhouse:masterfrom
rishabhvaish:fix/751-reconnect-charge-point-id

Conversation

@rishabhvaish
Copy link

Summary

  • Add extract_charge_point_id() utility for robust WebSocket path parsing — handles nested paths, query strings, empty/malformed paths
  • Make ChargePoint.start() catch connection exceptions gracefully instead of crashing, with informative log output
  • Update v16 and v201 examples to validate charge point ID before creating instances, and handle ConnectionClosed cleanly

Problem

When chargers intermittently disconnect and reconnect, two issues arise:

  1. Path parsing is fragile: The examples use websocket.request.path.strip("/"), which breaks for nested paths like /ocpp/CP001 (returns ocpp/CP001 instead of CP001) and silently produces empty strings for root paths
  2. No connection lifecycle handling: ChargePoint.start() calls await self._connection.recv() in a bare loop — if the WebSocket closes, the exception propagates unhandled, leaving no diagnostic information

Changes

ocpp/charge_point.py

  • New function extract_charge_point_id(path): Safely extracts the last path segment as the charge point ID, handling edge cases (empty paths, query strings, fragments, multiple slashes)
  • ChargePoint.start() resilience: Wraps recv() in a try/except that logs the disconnection reason and exits the loop cleanly

examples/v16/central_system.py and examples/v201/central_system.py

  • Use extract_charge_point_id() instead of path.strip("/")
  • Validate the extracted ID (close connection if None)
  • Log connect/disconnect events
  • Wrap cp.start() in a ConnectionClosed handler

tests/test_charge_point_connection.py (new)

  • 14 parametric tests for extract_charge_point_id() covering normal paths, nested paths, empty/None inputs, query strings, special characters
  • 4 async tests for ChargePoint.start() verifying clean exit on connection close, message processing before close, disconnection logging, and reconnection with new instances

Test plan

  • All 182 tests pass (164 existing + 18 new)
  • Manual test with a real charger simulator connecting/disconnecting
  • Verify log output shows charge point ID on connect and disconnect

@a-alak
Copy link
Contributor

a-alak commented Feb 13, 2026

I like your thoughts generally!

I just have couple of comments:

extract_charge_point_id() seems a bit out of scope for this lib. One could imagine various validation checks that consumers of this lib wanted on the charge point id. Of course those could be added independently, and extract_charge_point_id() is definitely in accordance with min requirements of OCPP spec. So could be a nice utility to make available.

The idea of better connection error handling I think is great. However there is couple of issues with the implementation. First of all a bit of bad practice catching all exceptions. Of course one could argue that it makes sense for logging, but I think this should be more specific.
Logging every error as info I will say is semantically incorrect. As minimum this should be a warning, and probably people want to inspect the exception, so I think logging.exception is more fitting. Of course we could do both, to inform a disconnection, but I think it should be warn when the disconnection is due to an exception.
In the example you show a very relevant handling of the ConnectionClosed exception for the consumer. However the error handling in charge_point.start() will silence this error unfortunately. It is never re-raised. And since this is both a central system and a charge point implementation we have to account for the consumer wanting to handle stuff like re-connection logic, so they should be able to catch these errors. (also easy to imagine a central system implementing different handling of different errors, like creating a notification or what ever).

Even if we improve on this points I think we might end up creating some limitations. This library is currently coupled with the websockets library interface, which is fine. However I imagine people wrapping other websocket interfaces in compatible objects to be able to use FastAPI or AIOHTTP websockets with this library. These websockets will raise different errors, so behaviour will be different. I don't think that it should be even more coupled with websockets.

Just my quick thoughts.

- Remove bare `except Exception` in ChargePoint.start() so exceptions
  propagate to consumers, enabling custom reconnection logic and
  error-specific handling without coupling the library to websockets
- Use `logging.warning` instead of `logging.info` for disconnections
  in examples, and log the exception details
- Update tests to verify exceptions propagate rather than being silenced

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rishabhvaish
Copy link
Author

Thanks for the thoughtful review, @a-alak! All great points. I've pushed a commit addressing each one:

On extract_charge_point_id() — Agreed it's on the edge of scope, but as you noted it aligns with the minimum OCPP spec requirements (charge point ID as the last path segment). I think it earns its place as a small utility that saves every consumer from re-implementing the same path parsing with the same edge cases. Happy to discuss further if you feel strongly about removing it.

On exception handling in ChargePoint.start() — You're absolutely right on all counts. I've removed the try/except block from start() entirely. Exceptions now propagate directly to the caller, which:

  • Eliminates the bare except Exception (bad practice ✅)
  • Stops silencing errors consumers need to handle (reconnection logic, error differentiation ✅)
  • Avoids adding any coupling with the websockets library in the core code — people using FastAPI/AIOHTTP websockets will get their native exceptions propagated as-is ✅

On logging levels — Updated the examples to use logging.warning instead of logging.info for disconnections, and the exception object is now logged so users can inspect it.

The examples still demonstrate ConnectionClosed handling, but that's purely example-level code showing one possible consumer pattern — the library itself is now completely transport-agnostic on this front.

All 181 tests pass.

@a-alak
Copy link
Contributor

a-alak commented Feb 17, 2026

Great, I think the PR looks good now!

@rishabhvaish
Copy link
Author

Thank you for the thorough review and feedback, @a-alak! I really appreciate you taking the time to help improve the implementation. Your suggestions made the PR much cleaner and more focused. 🙏

@rishabhvaish
Copy link
Author

Hi! Just checking in — this PR was approved by @a-alak on Feb 17. Is there anything else needed before it can be merged? Happy to address any additional feedback. Thanks!

@rishabhvaish
Copy link
Author

Thanks for the thorough review @a-alak! Glad the PR looks good now. Let me know if there's anything else needed for merging.

@a-alak
Copy link
Contributor

a-alak commented Mar 1, 2026

I am not a maintainer, just a consumer (and have minimal contributions). I can't approve or merge the PR.

@rishabhvaish
Copy link
Author

Ah, sorry about that @a-alak — I mistakenly assumed you were a maintainer. Thanks for clarifying and for the thorough review earlier!

@mobilityhouse/developers — could a maintainer take a look when you get a chance? All review feedback from @a-alak has been addressed and the PR is ready to merge.

@wafa-yah
Copy link
Contributor

wafa-yah commented Mar 2, 2026

Ah, sorry about that @a-alak — I mistakenly assumed you were a maintainer. Thanks for clarifying and for the thorough review earlier!

@mobilityhouse/developers — could a maintainer take a look when you get a chance? All review feedback from @a-alak has been addressed and the PR is ready to merge.

@mdwcrft @proelke can one of you please have a look ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants