Issue fixes for initial release of multiple connector support. #1708
Conversation
…xceptions when changing max current (Fixes lbbrhzn#1706 and lbbrhzn#1703). Use proper connector id for extra attr (phase data) (Fixes lbbrhzn#1704). Clear IdTag and reset transaction id to 0 on stop/unplug (Fixes lbbrhzn#1702). Add more tests.
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1708 +/- ##
==========================================
+ Coverage 94.16% 94.25% +0.09%
==========================================
Files 12 12
Lines 2673 2751 +78
==========================================
+ Hits 2517 2593 +76
- Misses 156 158 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Please test with one-connector chargers to make sure set_charge_rate works, and that phase attributes land on the sensor. |
![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)
tests/test_charge_point_v16.py (1)
2622-2829: Bug: using cp_id instead of cpid in CentralSystem lookups can break on custom CPIDsCentralSystem.get_extra_attr expects the HA device id (cpid), not the websocket cp_id. Several assertions here pass cp_id, which will fail when CONF_CPID != cp_id.
@@ async def test_current_import_phase_extra_attrs_single_and_multi_connector( - srv_cp: ServerCP = cs.charge_points[cp_id] + srv_cp: ServerCP = cs.charge_points[cp_id] + cpid = srv_cp.settings.cpid @@ - attrs = cs.get_extra_attr(cp_id, "Current.Import", connector_id=None) + attrs = cs.get_extra_attr(cpid, "Current.Import", connector_id=None) @@ - attrs1 = cs.get_extra_attr(cp_id, "Current.Import", connector_id=1) + attrs1 = cs.get_extra_attr(cpid, "Current.Import", connector_id=1) @@ - attrs1 = cs.get_extra_attr(cp_id, "Current.Import", connector_id=1) - attrs2 = cs.get_extra_attr(cp_id, "Current.Import", connector_id=2) + attrs1 = cs.get_extra_attr(cpid, "Current.Import", connector_id=1) + attrs2 = cs.get_extra_attr(cpid, "Current.Import", connector_id=2)
🧹 Nitpick comments (9)
custom_components/ocpp/api.py (1)
370-374: Good normalization of empty values to enable fallbacks.Treating blank unit strings and empty extra_attr dicts as None is correct and aligns with multi-connector fallback logic. Consider a tiny DRY helper (e.g., _none_if_blank) to avoid repetition across get_unit/get_ha_unit/get_extra_attr.
Also applies to: 386-389, 412-416, 428-431, 454-457, 470-472
custom_components/ocpp/number.py (1)
35-37: Avoid setting logger level in module code.Hard-coding the domain logger to INFO can override user HA logger settings. Recommend removing the level set and letting HA config control verbosity.
-_LOGGER: logging.Logger = logging.getLogger(__package__) -logging.getLogger(DOMAIN).setLevel(logging.INFO) +_LOGGER: logging.Logger = logging.getLogger(__package__)custom_components/ocpp/ocppv16.py (3)
328-356: Deterministic profile IDs: nice; tighten type safety to avoid typos.Passing purpose as a free-form string risks mismatches. Consider typing purpose as ChargingProfilePurposeType and deriving the code/map from the Enum to prevent string drift.
- def _profile_ids_for( - self, conn_id: int, purpose: str, tx_id: int | None = None + def _profile_ids_for( + self, + conn_id: int, + purpose: ChargingProfilePurposeType, + tx_id: int | None = None, ) -> tuple[int, int]: @@ - PURPOSE_CODE = { - "ChargePointMaxProfile": 1, - "TxDefaultProfile": 2, - "TxProfile": 3, - } - if purpose == "ChargePointMaxProfile": + PURPOSE_CODE = { + ChargingProfilePurposeType.charge_point_max_profile: 1, + ChargingProfilePurposeType.tx_default_profile: 2, + ChargingProfilePurposeType.tx_profile: 3, + } + if purpose == ChargingProfilePurposeType.charge_point_max_profile: @@ - if purpose == "TxProfile" and tx_id is not None: + if purpose == ChargingProfilePurposeType.tx_profile and tx_id is not None:And adapt call sites accordingly when constructing profiles.
394-415: Profile application order and pre-clearing by id are solid; improve unit detection.Attempt sequence (CPMax → TxDefault → TxProfile) and targeted Clear by id reduce conflicts. One small robustness tweak: parse the allowed units token-wise and case-insensitively to avoid substring matches (e.g., "Power" containing "ow").
Would you like a follow-up patch to tokenize resp_units and select amps/watts reliably?
Also applies to: 416-435, 437-475
480-496: Availability errors are swallowed; consider notifying HA like other flows.Other service paths notify HA on failure. Mirror that here to surface issues to users.
- except TimeoutError as ex: + except TimeoutError as ex: _LOGGER.debug("ChangeAvailability timed out (conn=%s): %s", conn, ex) - return False + await self.notify_ha(f"ChangeAvailability timed out (conn={conn})") + return False except Exception as ex: _LOGGER.debug("ChangeAvailability failed (conn=%s): %s", conn, ex) - return False + await self.notify_ha(f"ChangeAvailability failed (conn={conn}): {ex}") + return False @@ - return status in ( + ok = status in ( AvailabilityStatus.accepted, AvailabilityStatus.scheduled, ) + if not ok: + await self.notify_ha( + f"Set availability not accepted (conn={conn}, status={status})" + ) + return okAlso applies to: 497-503
custom_components/ocpp/chargepoint.py (2)
649-660: Use the canonical self.num_connectors for target connector resolutionRelying on getattr(self, CONF_NUM_CONNECTORS, …) risks drift if the constant ever changes or isn’t set as an attribute on this instance. You already maintain self.num_connectors; use that directly.
- n_connectors = getattr(self, CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) or 1 + n_connectors = int(getattr(self, "num_connectors", DEFAULT_NUM_CONNECTORS) or 1)
691-733: Minor readability nit: direct equality for voltage measurand checkNo functional change, but this reads simpler.
- if metric in [Measurand.voltage.value]: + if metric == Measurand.voltage.value:tests/test_charge_point_v16.py (2)
2857-2888: Nit: keep comments consistent and in EnglishSmall language mix in the assertion comment.
- assert ok is False # timeout-grenen ska returnera False + assert ok is False # the timeout branch should return False
2708-2714: Optional: avoid reusing TCP ports across parametrized tests to reduce flakiness under parallel runsSeveral tests share ports used elsewhere (e.g., 9077). If these ever run in parallel (xdist), consider allocating unique ports per test or deriving from an offset.
Would you like a quick helper to scan for duplicate port literals across tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.json(1 hunks)custom_components/ocpp/api.py(6 hunks)custom_components/ocpp/chargepoint.py(5 hunks)custom_components/ocpp/number.py(3 hunks)custom_components/ocpp/ocppv16.py(6 hunks)tests/test_charge_point_v16.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-02T06:05:56.424Z
Learnt from: drc38
PR: lbbrhzn/ocpp#1527
File: custom_components/ocpp/chargepoint.py:391-391
Timestamp: 2025-03-02T06:05:56.424Z
Learning: In the OCPP component, post_connect initialization has been refactored to only happen in response to boot notifications rather than automatically on startup, preventing duplicate initialization if a boot notification occurs close to the initial connection.
Applied to files:
custom_components/ocpp/chargepoint.py
🧬 Code graph analysis (5)
custom_components/ocpp/number.py (1)
custom_components/ocpp/api.py (1)
set_max_charge_rate_amps(539-550)
custom_components/ocpp/api.py (2)
custom_components/ocpp/chargepoint.py (5)
unit(94-96)unit(99-101)ha_unit(104-106)extra_attr(109-111)extra_attr(114-116)tests/test_api_paths.py (1)
test_get_units_and_attrs_fallbacks(196-221)
custom_components/ocpp/ocppv16.py (2)
custom_components/ocpp/chargepoint.py (10)
value(84-86)value(89-91)clear_profile(387-389)notify_ha(865-876)set_availability(401-403)get_energy_kwh(735-739)unit(94-96)unit(99-101)extra_attr(109-111)extra_attr(114-116)custom_components/ocpp/ocppv201.py (2)
clear_profile(335-343)set_availability(422-440)
custom_components/ocpp/chargepoint.py (3)
custom_components/ocpp/ocppv16.py (4)
get_number_of_connectors(108-147)set_availability(478-508)trigger_boot_notification(253-263)trigger_status_notification(265-289)custom_components/ocpp/ocppv201.py (4)
get_number_of_connectors(249-252)set_availability(422-440)trigger_status_notification(321-333)ChargePoint(77-881)tests/test_api_paths.py (1)
set_availability(43-48)
tests/test_charge_point_v16.py (5)
custom_components/ocpp/api.py (2)
CentralSystem(98-670)get_extra_attr(445-485)custom_components/ocpp/chargepoint.py (6)
ChargePoint(225-876)value(84-86)value(89-91)get(172-175)set_availability(401-403)set_charge_rate(391-399)custom_components/ocpp/ocppv16.py (3)
ChargePoint(83-1106)set_availability(478-508)set_charge_rate(358-476)custom_components/ocpp/ocppv201.py (3)
ChargePoint(77-881)set_availability(422-440)set_charge_rate(345-420)tests/charge_point_test.py (1)
wait_ready(91-94)
🔇 Additional comments (8)
custom_components/ocpp/number.py (1)
220-241: Revert UI on failure in async_set_native_value
Capture the old _attr_native_value before updating, then if set_max_charge_rate_amps returns false or throws, restore the previous value and call async_write_ha_state() to keep the UI in sync.custom_components/ocpp/ocppv16.py (4)
314-327: clear_profile now robust to exceptions and treats Unknown as success — good.
366-378: Custom profile path handled with guarded SetChargingProfile — good.Good defensive logging and early return on accepted.
821-842: Per-connector EAIR updates during active transactions — good improvement.
1036-1038: Clearing id_tag and transaction_id on stop — good and consistent with #1702.custom_components/ocpp/chargepoint.py (1)
330-335: post_connect hardening: initialization order and defensive availability handling look goodFetching number of connectors first, initializing connector slots, then best‑effort availability (with CancelledError passthrough) is sensible and reduces brittle failures. Debug‑level logging for non‑fatal aborts is appropriate.
Also applies to: 357-363, 369-371
tests/test_charge_point_v16.py (2)
46-46: Added imports for AvailabilityType and Phase are correctThese are used by new tests; imports look right.
Also applies to: 56-56
2997-3031: SetAvailability and SetChargingProfile tests cover key branches wellGood coverage of timeout/exception paths, happy paths, and unit/attempt fallbacks for set_charge_rate. Assertions validate both payload shape and outcomes.
Also applies to: 3041-3075, 3085-3153, 3164-3226, 3236-3292, 3302-3343
| }, | ||
| "python.testing.pytestArgs": [ | ||
| "tests", | ||
| "--cov=custom_components", | ||
| "--cov-report=term-missing", | ||
| "--cov-report=xml:coverage.xml", | ||
| "--timeout=30" | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pytest args look fine; verify plugins and consider centralizing config.
- The added flags require pytest-cov and pytest-timeout; ensure they are present in dev requirements or pyproject.
- Prefer pytest.ini/pyproject.toml to make these defaults across all environments, not just VS Code.
Run to verify presence of plugins and centralized config:
🏁 Script executed:
#!/bin/bash
# Check for pytest config and plugins
rg -nP '^\s*\[tool.pytest.ini_options\]|\[pytest\]' -g 'pyproject.toml|pytest.ini|setup.cfg' -n
rg -nP 'pytest-(cov|timeout)\b' -g 'requirements*.txt|dev-requirements*.txt|pyproject.toml|setup.cfg' -n -C2Length of output: 297
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Locating pytest config files ==="
find . -maxdepth 3 -type f \( -name "pyproject.toml" -o -name "pytest.ini" -o -name "setup.cfg" \)
for cfg in pyproject.toml pytest.ini setup.cfg; do
if [ -f "$cfg" ]; then
echo
echo "=== Contents of $cfg matching pytest config ==="
grep -nE '^\s*\[tool\.pytest\.ini_options\]|\[pytest\]' "$cfg" || echo "No pytest config block in $cfg"
fi
done
echo
echo "=== Checking for pytest-cov and pytest-timeout in dependency files ==="
find . -type f -name "*requirements*.txt" -print -exec grep -nE 'pytest-(cov|timeout)' {} \; || echo "No matches in requirements files"
grep -R -nE 'pytest-(cov|timeout)' pyproject.toml setup.cfg || echo "No plugin deps in pyproject.toml/setup.cfg"Length of output: 344
Add missing pytest plugins and centralize configuration
- Add pytest-cov and pytest-timeout to your development dependencies (e.g. a
requirements-dev.txtor inpyproject.toml) so the VS Code pytest args work correctly. - Move these pytest flags into a central pytest config (create
pytest.inior add a[tool.pytest.ini_options]block inpyproject.tomlor under[pytest]insetup.cfg) to apply them across all environments.
🤖 Prompt for AI Agents
In .vscode/settings.json around lines 13 to 20 the VS Code pytest args reference
plugins and flags that should be managed centrally and installed as dev
dependencies; add pytest-cov and pytest-timeout to your development dependencies
(e.g. requirements-dev.txt, dev-dependencies in pyproject.toml, or setup.cfg)
and remove plugin-specific flags from the editor settings; instead create a
central pytest config (pytest.ini or a [tool.pytest.ini_options] block in
pyproject.toml or [pytest] in setup.cfg) and move the flags
--cov=custom_components, --cov-report=term-missing,
--cov-report=xml:coverage.xml, and --timeout=30 into that config so they apply
in all environments and the VS Code settings simply reference the test discovery
path.
| self._metrics[(target_cid, measurand)].unit = unit | ||
| self._metrics[(target_cid, measurand)].extra_attr[om.unit.value] = unit | ||
| self._metrics[(target_cid, measurand)].extra_attr[phase] = value | ||
| self._metrics[(target_cid, measurand)].extra_attr[om.context.value] = ( | ||
| context | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard extra_attr against None to avoid TypeError and use a local alias
Metric.extra_attr can be None (tests simulate this). Ensure a dict before item assignment and reuse a local alias for clarity.
- self._metrics[(target_cid, measurand)].unit = unit
- self._metrics[(target_cid, measurand)].extra_attr[om.unit.value] = unit
- self._metrics[(target_cid, measurand)].extra_attr[phase] = value
- self._metrics[(target_cid, measurand)].extra_attr[om.context.value] = (
- context
- )
+ m = self._metrics[(target_cid, measurand)]
+ m.unit = unit
+ if not isinstance(m.extra_attr, dict):
+ m.extra_attr = {}
+ m.extra_attr[om.unit.value] = unit
+ m.extra_attr[phase] = value
+ m.extra_attr[om.context.value] = context📝 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.
| self._metrics[(target_cid, measurand)].unit = unit | |
| self._metrics[(target_cid, measurand)].extra_attr[om.unit.value] = unit | |
| self._metrics[(target_cid, measurand)].extra_attr[phase] = value | |
| self._metrics[(target_cid, measurand)].extra_attr[om.context.value] = ( | |
| context | |
| ) | |
| m = self._metrics[(target_cid, measurand)] | |
| m.unit = unit | |
| if not isinstance(m.extra_attr, dict): | |
| m.extra_attr = {} | |
| m.extra_attr[om.unit.value] = unit | |
| m.extra_attr[phase] = value | |
| m.extra_attr[om.context.value] = context |
🤖 Prompt for AI Agents
In custom_components/ocpp/chargepoint.py around lines 680 to 685,
Metric.extra_attr may be None which causes a TypeError when assigning keys;
create or ensure a dict before assigning and use a local alias for the Metric
instance or its extra_attr to avoid repeated lookups. Concretely, retrieve the
metric into a local variable, if metric.extra_attr is None set it to {} (or
assign a new dict to a local extra_attr and assign back), then set metric.unit
and assign unit, phase and context entries into that dict, and finally reassign
metric.extra_attr if you used a local dict.
| if status == ChargePointStatus.available: | ||
| self._metrics[(connector_id or 1, cstat.id_tag.value)].value = "" | ||
| self._metrics[(connector_id or 1, csess.transaction_id.value)].value = 0 | ||
|
|
There was a problem hiding this comment.
Don't clear connector 1 on station-level (connectorId=0) status.
Using (connector_id or 1) clears connector 1 when a station-level Available arrives. Limit clearing to real connector ids > 0 (or intentionally clear all).
- if status == ChargePointStatus.available:
- self._metrics[(connector_id or 1, cstat.id_tag.value)].value = ""
- self._metrics[(connector_id or 1, csess.transaction_id.value)].value = 0
+ if status == ChargePointStatus.available and connector_id not in (None, 0):
+ self._metrics[(connector_id, cstat.id_tag.value)].value = ""
+ self._metrics[(connector_id, csess.transaction_id.value)].value = 0
+ # Optionally, to clear all on station-level Available:
+ # elif connector_id in (None, 0):
+ # for cid in list(self._active_tx.keys()):
+ # self._metrics[(cid, cstat.id_tag.value)].value = ""
+ # self._metrics[(cid, csess.transaction_id.value)].value = 0📝 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 status == ChargePointStatus.available: | |
| self._metrics[(connector_id or 1, cstat.id_tag.value)].value = "" | |
| self._metrics[(connector_id or 1, csess.transaction_id.value)].value = 0 | |
| if status == ChargePointStatus.available and connector_id not in (None, 0): | |
| self._metrics[(connector_id, cstat.id_tag.value)].value = "" | |
| self._metrics[(connector_id, csess.transaction_id.value)].value = 0 | |
| # Optionally, to clear all on station-level Available: | |
| # elif connector_id in (None, 0): | |
| # for cid in list(self._active_tx.keys()): | |
| # self._metrics[(cid, cstat.id_tag.value)].value = "" | |
| # self._metrics[(cid, csess.transaction_id.value)].value = 0 |
🤖 Prompt for AI Agents
In custom_components/ocpp/ocppv16.py around lines 928-931, the current logic
uses (connector_id or 1) which causes station-level messages (connector_id == 0
or None) to clear connector 1 metrics; change the code to only clear metrics
when connector_id is a real connector (> 0). Concretely, check if connector_id
is truthy and > 0 before touching self._metrics for that connector id (skip
clearing when connector_id is 0 or None); do not fallback to 1 for station-level
messages unless you intentionally want to clear all connectors.
Update EAIR continuously (Fixes #1707). Handle clear profile exceptions when changing max current (Fixes #1706 and fixes #1703). Use proper connector id for extra attr (phase data) (Fixes #1704). Clear IdTag and reset transaction id to 0 on stop/unplug (Fixes #1702). Add more tests. Test coverage settings.
Summary by CodeRabbit