Support for multiple connectors per charger #1689
Conversation
WalkthroughAdds multi‑connector support across the OCPP integration: connector-aware metric storage and resolution, connector-scoped APIs and control flows, per-connector entity creation (sensors/switches/numbers/buttons), protocol handler updates (v1.6 & v2.0.1), tests expanded, new config key for connector count, and developer script/.gitignore adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant HA as Home Assistant
participant CS as CentralSystem
participant CP as ChargePoint
rect rgba(230,240,255,0.5)
note right of HA: Read per-connector metric
User->>HA: request entity/state
HA->>CS: get_metric(cpid, measurand, connector_id)
alt metric present for connector
CS->>CP: get_ha_metric(measurand, connector_id)
else fallback path
CS->>CP: get_ha_metric(measurand, 0 or legacy or conn1..N)
end
CP-->>CS: metric/unit/attrs
CS-->>HA: resolved state
end
rect rgba(235,255,235,0.5)
note right of HA: Per-connector control
User->>HA: set / toggle (with connector_id)
HA->>CS: set_charger_state / set_max_charge_rate_amps(..., connector_id)
CS->>CP: set_availability/start_transaction/set_charge_rate(connector_id)
CP-->>CS: ack/result
CS-->>HA: result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
custom_components/ocpp/ocppv201.py (3)
641-686: Per-connector metrics are keyed with Enum objects instead of strings; breaks lookupsSeveral
_metricsaccesses usecsess.*without.value, creating separate keys of Enum type. This prevents downstream retrieval via string keys and effectively hides meter_start for v2.x.Apply this diff:
- if (tx_event_type == TransactionEventEnumType.started.value) or ( - (tx_event_type == TransactionEventEnumType.updated.value) - and (self._metrics[(global_idx, csess.meter_start)].value is None) - ): + if (tx_event_type == TransactionEventEnumType.started.value) or ( + (tx_event_type == TransactionEventEnumType.updated.value) + and (self._metrics[(global_idx, csess.meter_start.value)].value is None) + ): energy_measurand = MeasurandEnumType.energy_active_import_register.value for meter_value in converted_values: for measurand_item in meter_value: if measurand_item.measurand == energy_measurand: energy_value = cp.get_energy_kwh(measurand_item) energy_unit = HA_ENERGY_UNIT if measurand_item.unit else None - self._metrics[ - (global_idx, csess.meter_start) - ].value = energy_value - self._metrics[ - (global_idx, csess.meter_start) - ].unit = energy_unit + self._metrics[(global_idx, csess.meter_start.value)].value = energy_value + self._metrics[(global_idx, csess.meter_start.value)].unit = energy_unit
488-535: StatusNotification per-connector writes depend on mapping; handle early notificationsIf StatusNotification arrives before inventory (and thus before
_build_connector_map()), per-connector writes can collapse onto unintended indices (see earlier mapping comment). Ensure the map is constructed or a unique index is assigned before writing metrics, to avoid cross-connector contamination.Linking to the
_pair_to_globalfix will address this throughout. Consider invoking_build_connector_map()here ifself._inventoryexists but maps are empty.
706-772: TransactionEvent: Fix remaining Enum-key usages, timestamp parsing, and session duration
- Add
.valueto allcsess.*enum keys in_metrics:
csess.meter_start → csess.meter_start.valuecsess.session_time → csess.session_time.value- Parse ISO timestamps safely (handle trailing
Z):
- Replace
t = datetime.fromisoformat(timestamp)
with
t = datetime.fromisoformat(timestamp.replace("Z", "+00:00"))- Compute session duration using total_seconds (includes multi-day spans):
- Use
(t - start).total_seconds()instead of.secondsApply this diff:
--- a/custom_components/ocpp/ocppv201.py +++ b/custom_components/ocpp/ocppv201.py @@ -669,7 +669,7 @@ def on_transaction_event(...): if ( tx_event_type == TransactionEventEnumType.updated.value - and (self._metrics[(global_idx, csess.meter_start)].value is None) + and (self._metrics[(global_idx, csess.meter_start.value)].value is None) ): # ... @@ -708,7 +708,8 @@ def on_transaction_event(...): - t = datetime.fromisoformat(timestamp) + # tolerate trailing 'Z' in ISO timestamps + t = datetime.fromisoformat(timestamp.replace("Z", "+00:00")) @@ -750,9 +751,9 @@ def on_transaction_event(...): if event_type == TransactionEventEnumType.started.value: self._tx_start_time[global_idx] = t tx_id: str = transaction_info["transaction_id"] - self._metrics[(global_idx, csess.transaction_id.value)].value = tx_id - self._metrics[(global_idx, csess.session_time)].value = 0 - self._metrics[(global_idx, csess.session_time)].unit = UnitOfTime.MINUTES + self._metrics[(global_idx, csess.transaction_id.value)].value = tx_id + self._metrics[(global_idx, csess.session_time.value)].value = 0 + self._metrics[(global_idx, csess.session_time.value)].unit = UnitOfTime.MINUTES @@ -757,9 +758,11 @@ def on_transaction_event(...): if self._tx_start_time.get(global_idx): - duration_minutes: int = ( - (t - self._tx_start_time[global_idx]).seconds + 59 - ) // 60 + elapsed = (t - self._tx_start_time[global_idx]).total_seconds() + duration_minutes: int = int((elapsed + 59) // 60) -- - self._metrics[(global_idx, csess.session_time)].value = duration_minutes - self._metrics[ - (global_idx, csess.session_time) - ].unit = UnitOfTime.MINUTES + self._metrics[(global_idx, csess.session_time.value)].value = duration_minutes + self._metrics[(global_idx, csess.session_time.value)].unit = UnitOfTime.MINUTES
🧹 Nitpick comments (25)
.gitignore (1)
132-133: Tighten patterns: root-anchored config and simpler DS_Store.
- Anchor the config directory ignore to the repo root for clarity.
- The DS_Store pattern can be simplified; a bare .DS_Store matches in any directory.
-# HA Development -config/ -**/.DS_Store +# HA Development +/config/ +.DS_Storescripts/develop (3)
15-16: Guard against dangling symlink; fail fast if source integration is missingIf custom_components/ocpp doesn’t exist, ln will create a dangling symlink and HA will fail later in a non-obvious way. Add an existence check and a clearer error.
Apply this diff:
-# Link the ocpp integration (overwrite if exists) -ln -sfn "${PWD}/custom_components/ocpp" "${PWD}/config/custom_components/ocpp" +# Link the ocpp integration (overwrite if exists) +src="${PWD}/custom_components/ocpp" +dst="${PWD}/config/custom_components/ocpp" +if [[ ! -d "$src" ]]; then + echo "Missing integration sources at: $src" >&2 + exit 1 +fi +ln -sfn "$src" "$dst"Optional portability note (if this script is ever run on macOS/BSD): ln -n is GNU-specific. On BSD/macOS, prefer -h. A simple pattern is:
- Linux: ln -sfn
- macOS/BSD: ln -sfh
Would you like me to add a small detection block to switch -n vs -h based on the platform?
18-19: Install debugpy only if missing; avoid masking errors with|| trueThe current command tries to install on every run and ignores all failures, which can lead to a later crash at startup. Check for the module first and only install if absent.
Apply this diff:
-# Install debugpy if missing -/home/vscode/.local/ha-venv/bin/python -m pip install --quiet debugpy || true +# Install debugpy if missing +if ! /home/vscode/.local/ha-venv/bin/python -c 'import debugpy' >/dev/null 2>&1; then + /home/vscode/.local/ha-venv/bin/python -m pip install --quiet debugpy +fiNit (optional): for stricter bash safety, consider set -euo pipefail near the top. This is optional in a dev script but helps catch typos and unset vars.
21-23: Fallback cleanly when debugpy isn’t available; bind debugger safely by default
- If debugpy can’t be installed (e.g., offline), the current exec will fail and stop the dev flow. Fallback to running HA without the debugger.
- Binding to 0.0.0.0 exposes the debug port to the whole network. Defaulting to 127.0.0.1 is safer, with an env override when remote attach is desired.
Apply this diff:
-# Start Home Assistant with debugger -exec /home/vscode/.local/ha-venv/bin/python -m debugpy --listen 0.0.0.0:5678 \ - -m homeassistant --config "${PWD}/config" --debug +# Start Home Assistant (prefer debugger if available) +DEBUGPY_LISTEN="${DEBUGPY_LISTEN:-127.0.0.1:5678}" +if /home/vscode/.local/ha-venv/bin/python -c 'import debugpy' >/dev/null 2>&1; then + exec /home/vscode/.local/ha-venv/bin/python -m debugpy --listen "${DEBUGPY_LISTEN}" \ + -m homeassistant --config "${PWD}/config" --debug +else + echo "debugpy not available; starting Home Assistant without debugger" >&2 + exec /home/vscode/.local/ha-venv/bin/python -m homeassistant --config "${PWD}/config" --debug +fiPlease confirm whether exposing the debugger on 0.0.0.0 is actually required in your dev container setup. If yes, you can set DEBUGPY_LISTEN=0.0.0.0:5678 when needed.
custom_components/ocpp/translations/nl.json (1)
25-27: Minor wording improvement in DutchSuggest making the term fully Dutch for consistency with other labels:
- "num_connectors": "Aantal connectoren per Charge point" + "num_connectors": "Aantal connectoren per laadpunt"custom_components/ocpp/const.py (1)
143-158: Use DEFAULT_NUM_CONNECTORS in dataclass default to avoid duplicationMinor consistency nit: reference the module-level default to keep defaults in one place.
Apply this diff:
class ChargerSystemSettings: @@ - num_connectors: int = 1 + num_connectors: int = DEFAULT_NUM_CONNECTORStests/test_config_flow.py (1)
117-121: Avoid hardcoding connector count in expected flow outputTo keep the test resilient if the default changes, use DEFAULT_NUM_CONNECTORS instead of a magic 1.
Apply this diff within this block:
- flow_output[CONF_CPIDS][-1]["test_cp_id"][CONF_NUM_CONNECTORS] = 1 + flow_output[CONF_CPIDS][-1]["test_cp_id"][CONF_NUM_CONNECTORS] = DEFAULT_NUM_CONNECTORSAlso add this import at the top of the file:
from custom_components.ocpp.const import DEFAULT_NUM_CONNECTORStests/const.py (2)
119-125: Populate connectors=2 in MOCK_CONFIG_DATA_1: LGTMThis exercises the multi-connector code paths in tests. If multiple test fixtures start needing this, consider a small helper to build CP configs parametrically to reduce duplication.
127-149: Reduce duplication between MOCK_CONFIG_DATA_1 and MOCK_CONFIG_DATA_1_MCBoth fixtures define a single CP with CONF_NUM_CONNECTORS=2 and largely identical fields. Consider consolidating to one, or generate them via a small builder to keep test data DRY.
For example, add a helper in this module:
def make_cp(cpid: str, auto_cfg: bool, n_connectors: int = 1, skip_schema: bool = False, smart: bool = True) -> dict: return { CONF_CPID: cpid, CONF_IDLE_INTERVAL: 900, CONF_MAX_CURRENT: 32, CONF_METER_INTERVAL: 60, CONF_MONITORED_VARIABLES: DEFAULT_MONITORED_VARIABLES, CONF_MONITORED_VARIABLES_AUTOCONFIG: auto_cfg, CONF_NUM_CONNECTORS: n_connectors, CONF_SKIP_SCHEMA_VALIDATION: skip_schema, CONF_FORCE_SMART_CHARGING: smart, }Then use:
MOCK_CONFIG_DATA_1 = { **MOCK_CONFIG_DATA, CONF_CSID: "test_csid_1", CONF_PORT: 9001, CONF_CPIDS: [{"CP_1_nosub": make_cp("test_cpid_9001", auto_cfg=False, n_connectors=2, skip_schema=True)}], }tests/charge_point_test.py (2)
31-36: Entity ID prefix logic for per-connector switches: LGTM with a minor extensibility noteThe special-case for "availability" is fine for now. If more station-level switches appear, consider parameterizing connector_id (e.g., connector_id: int | None = 1) and deriving the prefix from that to avoid string comparisons on keys.
42-48: Pass numeric value to number.set_value instead of strHome Assistant’s number.set_value expects a numeric payload. Passing a string works in many cases but is unnecessary and can be brittle.
Apply this diff:
async def set_number(hass: HomeAssistant, cpid: str, key: str, value: int): """Set a numeric slider.""" await hass.services.async_call( NUMBER_DOMAIN, "set_value", - service_data={"value": str(value)}, + service_data={"value": value}, blocking=True, target={ATTR_ENTITY_ID: f"{NUMBER_DOMAIN}.{cpid}_connector_1_{key}"}, )custom_components/ocpp/config_flow.py (1)
96-97: Constrain num_connectors to be at least 1Prevent a misconfiguration (e.g., 0 or negative) that would later suppress entity creation loops and cause subtle behavior differences across platforms. Validate and coerce here.
Apply this diff:
- vol.Required(CONF_NUM_CONNECTORS, default=DEFAULT_NUM_CONNECTORS): int, + vol.Required(CONF_NUM_CONNECTORS, default=DEFAULT_NUM_CONNECTORS): vol.All( + vol.Coerce(int), vol.Range(min=1) + ),custom_components/ocpp/number.py (2)
148-156: Expose connector-aware availability for numbers (optional)Numbers currently stay available unconditionally and only gate writes in async_set_native_value. Consider re-enabling available with connector-specific logic for better UI feedback.
Apply this diff:
- # @property - # def available(self) -> bool: - # """Return if entity is available.""" - # if not ( - # Profiles.SMART & self.central_system.get_supported_features(self.cpid) - # ): - # return False - # return self.central_system.get_available(self.cpid) # type: ignore [no-any-return] + @property + def available(self) -> bool: + """Return if entity is available.""" + if not (Profiles.SMART & self.central_system.get_supported_features(self.cpid)): + return False + return bool(self.central_system.get_available(self.cpid, self.connector_id))
160-165: Use connector-specific availability when setting valuesGating on charger-level availability can allow setting a connector that is unavailable. Check the target connector explicitly.
Apply this diff:
- if self.central_system.get_available( - self.cpid - ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid): + if self.central_system.get_available( + self.cpid, self.connector_id + ) and Profiles.SMART & self.central_system.get_supported_features(self.cpid): resp = await self.central_system.set_max_charge_rate_amps( self.cpid, num_value, self.connector_id )custom_components/ocpp/switch.py (1)
13-14: Remove unused UnitOfPower import and constantPOWER_KILO_WATT isn’t used; trim dead code to reduce noise.
Apply this diff:
-from homeassistant.const import UnitOfPower from homeassistant.helpers.entity import DeviceInfo @@ -POWER_KILO_WATT = UnitOfPower.KILO_WATTAlso applies to: 37-39
custom_components/ocpp/sensor.py (3)
51-56: Strip and de-duplicate configured measurands to avoid empty/whitespace metricsSplitting on commas can leave leading/trailing spaces and empty tokens (e.g., trailing comma). That would produce odd entity keys/names and potentially duplicate entities. Normalize by stripping and filtering empties before de-dup.
Apply this diff:
- measurands = list( - set( - cp_id_settings[CONF_MONITORED_VARIABLES].split(",") - + list(HAChargerSession) - ) - ) + # Normalize configured measurands: strip whitespace and drop empties + configured = [ + m.strip() + for m in cp_id_settings[CONF_MONITORED_VARIABLES].split(",") + if m and m.strip() + ] + measurands = list(set(configured + list(HAChargerSession)))
80-86: Make description factory resilient to Enum and whitespace inputsMetric entries can be Enum[str] or strings. Ensure consistent key/name generation by coercing to str and stripping whitespace.
Apply this diff:
- def _mk_desc(metric: str, *, cat_diag: bool = False) -> OcppSensorDescription: - return OcppSensorDescription( - key=metric.lower(), - name=metric.replace(".", " "), - metric=metric, + def _mk_desc(metric: str, *, cat_diag: bool = False) -> OcppSensorDescription: + ms = str(metric).strip() + return OcppSensorDescription( + key=ms.lower(), + name=ms.replace(".", " "), + metric=ms, entity_category=EntityCategory.DIAGNOSTIC if cat_diag else None, )
175-179: Unique ID should test connector_id explicitly for NoneUsing a truthy check will skip conn 0 (even if you ever decide to surface a connector 0/aggregate entity). Explicit None check is clearer and future-proof.
Apply this diff:
- if self.connector_id: + if self.connector_id is not None: parts.insert(2, f"conn{self.connector_id}")custom_components/ocpp/api.py (1)
292-297: Helper_get_metricsis handy; consider using it for get_metric/unit/extraYou already factored a safe accessor. Reusing it in get_metric/get_unit/get_ha_unit/get_extra_attr would reduce boilerplate and unify missing-CP handling.
If you want, I can provide a consolidated helper that returns (cp, metrics, n_connectors) and wire the getters to it.
custom_components/ocpp/ocppv16.py (2)
596-724: Strong per-connector MeterValues handling; minor readability nits
- The restoration of meter_start/transaction_id per-connector and use of HA restore is solid.
- Main meter (EAIR) routing to connector 0 when txId is absent is aligned with API semantics.
One small readability suggestion: the “Write main meter value … om this message” comment has a typo (“om” -> “if”).
If you’d like, I can open a follow-up PR to sweep minor comment typos.
856-914: StopTransaction connector resolution has a sensible fallback; consider explicit guardResolving conn via _active_tx and falling back to 1 is okay for tests. If you want to make this stricter in prod, consider logging the last known active connector or skipping zeroing when unknown.
I can draft a guard that skips metric writes when tx_id can’t be mapped, to avoid mutating an arbitrary connector.
custom_components/ocpp/ocppv201.py (2)
369-384: Return early if no active transaction is foundIf
tx_idremains empty, issuingRequestStopTransactionis unnecessary. Bail out early.Apply this diff:
await self._get_inventory() tx_id = "" total = self._total_connectors() or 1 for g in range(1, total + 1): val = self._metrics[(g, csess.transaction_id.value)].value if val: tx_id = val break + if not tx_id: + _LOGGER.info("No active transaction found to stop") + return False req: call.RequestStopTransaction = call.RequestStopTransaction( transaction_id=tx_id )
332-351: Availability targeting logic: sanity-check mapping before using EVSE-level targetingFor connector-scoped requests, mapping relies on
_global_to_pair. If the map isn't ready yet, this path falls back to station-level availability, which may not be intended.Add a guard to (a) build the map if inventory is available, and (b) log and fall back to station-level only if mapping cannot be resolved. This is consistent with the proposed
_pair_to_globalfix and avoids mis-targeted availability changes.custom_components/ocpp/chargepoint.py (2)
118-204: Connector-aware metrics map: solid approach; minor polish on docstringsThe
_ConnectorAwareMetricsmapping is a good compatibility layer and keeps the rest of the code clean. Minor nit: docstrings contain non-English terms (“Returnerar”, “Platt”, “Hjälpmetoder”). Prefer English for consistency.Apply this diff to the docstrings/comments:
class _ConnectorAwareMetrics(MutableMapping): - """Backwards compatible mapping for metrics. - - - m["Power.Active.Import"] -> Metric for connector 0 (flat access) - - m[(2, "Power.Active.Import")] -> Metric for connector 2 (per connector) - - m[2] -> dict[str -> Metric] for connector 2 - - Iteration, len, keys(), items() etc act like flat dict (connector 0). - """ + """Backwards compatible mapping for metrics. + + - m["Power.Active.Import"] -> Metric for connector 0 (flat access) + - m[(2, "Power.Active.Import")] -> Metric for connector 2 (per connector) + - m[2] -> dict[str, Metric] for connector 2 + + Iteration, len, keys(), values(), items() operate on connector 0 (flat view). + """ @@ - # --- flat (default connector 0) & connector-indexerad access --- + # --- flat (default connector 0) & connector-indexed access --- @@ - # Returnerar dict[str -> Metric] för connectorn + # Return dict[str, Metric] for the connector @@ - # Platt: returnera Metric i connector 0 + # Flat access: return Metric in connector 0 @@ - # Platt + # Flat @@ - # Iterera som ett platt dict (connector 0) + # Iterate like a flat dict (connector 0) @@ - # Storlek som platt dict (connector 0) + # Size as flat dict (connector 0) @@ - # Hjälpmetoder i dict-stil + # dict-like helpers
576-608: Entity refresh strategy may be heavy; consider relying on dispatcher signalThe BFS over devices and scheduling an update for every entity, then sending
DATA_UPDATED, may double-trigger updates and scales O(N^2) over devices for larger setups.
- Prefer signaling only via
async_dispatcher_send(DATA_UPDATED)and let entities subscribe.- If direct updates are needed, consider gathering entity ids once (no BFS through all devices each update), or guard with rate limiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (24)
.gitignore(1 hunks)custom_components/ocpp/api.py(6 hunks)custom_components/ocpp/button.py(4 hunks)custom_components/ocpp/chargepoint.py(15 hunks)custom_components/ocpp/config_flow.py(4 hunks)custom_components/ocpp/const.py(3 hunks)custom_components/ocpp/number.py(5 hunks)custom_components/ocpp/ocppv16.py(15 hunks)custom_components/ocpp/ocppv201.py(14 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(5 hunks)custom_components/ocpp/translations/de.json(1 hunks)custom_components/ocpp/translations/en.json(1 hunks)custom_components/ocpp/translations/es.json(1 hunks)custom_components/ocpp/translations/i-default.json(1 hunks)custom_components/ocpp/translations/nl.json(1 hunks)scripts/develop(1 hunks)tests/charge_point_test.py(2 hunks)tests/const.py(2 hunks)tests/test_charge_point_v16.py(10 hunks)tests/test_charge_point_v201.py(3 hunks)tests/test_charge_point_v201_multi.py(1 hunks)tests/test_config_flow.py(2 hunks)tests/test_init.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-28T09:21:24.150Z
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: tests/test_config_flow.py:124-124
Timestamp: 2025-01-28T09:21:24.150Z
Learning: The OCPP integration does not use options flow for configuration updates. Error handling in the config flow is primarily focused on measurand selection validation with the "no_measurands_selected" error.
Applied to files:
custom_components/ocpp/config_flow.py
🧬 Code Graph Analysis (10)
tests/test_charge_point_v201.py (1)
custom_components/ocpp/api.py (1)
get_metric(298-343)
tests/test_init.py (3)
custom_components/ocpp/__init__.py (2)
async_setup(87-95)async_migrate_entry(138-201)custom_components/ocpp/api.py (1)
CentralSystem(98-667)tests/conftest.py (1)
bypass_get_data_fixture(36-46)
custom_components/ocpp/const.py (1)
custom_components/ocpp/enums.py (2)
HAChargerDetails(41-53)ConfigurationKey(119-173)
tests/test_charge_point_v201_multi.py (4)
custom_components/ocpp/enums.py (1)
HAChargerServices(6-22)custom_components/ocpp/ocppv201.py (1)
ChargePoint(76-771)tests/charge_point_test.py (4)
create_configuration(67-75)run_charge_point_test(108-135)remove_configuration(81-86)wait_ready(92-95)custom_components/ocpp/api.py (3)
get_metric(298-343)get_unit(361-400)set_charger_state(549-576)
custom_components/ocpp/sensor.py (3)
custom_components/ocpp/enums.py (3)
HAChargerSession(56-62)HAChargerStatuses(25-38)HAChargerDetails(41-53)custom_components/ocpp/switch.py (1)
available(134-136)custom_components/ocpp/api.py (4)
get_available(483-525)get_extra_attr(442-481)get_metric(298-343)get_ha_unit(402-440)
custom_components/ocpp/ocppv16.py (1)
custom_components/ocpp/ocppv201.py (6)
get_number_of_connectors(170-173)get_configuration(418-439)set_availability(332-350)start_transaction(352-367)stop_transaction(369-383)on_heartbeat(484-486)
custom_components/ocpp/switch.py (4)
custom_components/ocpp/enums.py (2)
HAChargerServices(6-22)HAChargerStatuses(25-38)custom_components/ocpp/sensor.py (2)
available(196-198)async_setup_entry(41-149)custom_components/ocpp/__init__.py (1)
async_setup_entry(98-135)custom_components/ocpp/api.py (3)
get_available(483-525)get_metric(298-343)set_charger_state(549-576)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/ocppv16.py (6)
get_number_of_connectors(108-115)set_charge_rate(289-370)clear_profile(276-287)set_availability(372-391)start_transaction(393-407)on_meter_values(597-725)
custom_components/ocpp/api.py (3)
custom_components/ocpp/enums.py (1)
HAChargerStatuses(25-38)custom_components/ocpp/ocppv16.py (6)
set_charge_rate(289-370)set_availability(372-391)start_transaction(393-407)stop_transaction(409-431)reset(433-443)unlock(445-454)custom_components/ocpp/ocppv201.py (5)
set_charge_rate(266-330)set_availability(332-350)start_transaction(352-367)stop_transaction(369-383)reset(385-395)
tests/test_charge_point_v16.py (2)
custom_components/ocpp/ocppv16.py (1)
get_supported_features(182-219)custom_components/ocpp/api.py (7)
get_metric(298-343)get_unit(361-400)get_ha_unit(402-440)get_extra_attr(442-481)get_supported_features(527-534)set_max_charge_rate_amps(536-547)set_charger_state(549-576)
🔇 Additional comments (46)
.gitignore (2)
131-131: LGTM: Helpful section header.Clear sectioning improves maintainability of the ignore list.
132-133: No tracked files under config/ or .DS_Store remain
OK: No tracked files matched the new.gitignorerules.scripts/develop (2)
9-9: Good call: initialize Home Assistant config on first runRunning ensure_config via python -m homeassistant is the right way to bootstrap config in dev.
12-14: LGTM: proactively ensure custom_components existsPre-creating the directory prevents later surprises when linking the integration.
custom_components/ocpp/translations/en.json (1)
30-31: Additions look correct and JSON remains validKey name and placement are consistent with existing config flow strings. No trailing comma after the last property; JSON parses cleanly.
custom_components/ocpp/translations/i-default.json (1)
30-31: Fallback translation added correctlyThe new key matches the English base string and aligns with config flow usage. Structure and commas are correct.
custom_components/ocpp/translations/es.json (1)
25-27: Spanish translation is accurate and well-placed“Número de conectores por punto de carga” is clear and idiomatic. Comma usage is correct.
custom_components/ocpp/translations/de.json (1)
30-31: German translation reads well; commas and ordering are fine“Anzahl der Anschlüsse pro Ladestation” is clear. No JSON issues.
custom_components/ocpp/const.py (2)
28-28: Add CONF_NUM_CONNECTORS constant: LGTMGood, clear naming and consistent with the rest of the config keys.
49-49: Add DEFAULT_NUM_CONNECTORS: LGTMDefaulting to 1 preserves backward compatibility. Consider reusing this constant where the dataclass sets its default (see below) to avoid drift.
tests/test_config_flow.py (1)
12-20: Import of CONF_NUM_CONNECTORS for tests: LGTMMatches the new config surface and keeps the test expectations explicit.
tests/test_charge_point_v201.py (3)
1061-1067: Explicitly scoping station-level metrics via connector_id=0: LGTM
connector_id=0aligns with the new connector-aware metric model and the lookup semantics in api.get_metric.
1167-1174: Features metric assertion with connector_id=0: LGTMConsistent with the per-connector access pattern and station-level semantics.
1219-1221: Unsupported base report features check with connector_id=0: LGTMMatches the new connector-aware retrieval path.
tests/const.py (1)
14-15: Expose CONF_NUM_CONNECTORS in tests: LGTMKeeps test configs aligned with the integration’s configuration surface.
custom_components/ocpp/config_flow.py (2)
23-24: New num_connectors constants correctly wired into the flowImports for CONF_NUM_CONNECTORS and DEFAULT_NUM_CONNECTORS look correct and consistent with the rest of the PR.
Also applies to: 43-44
112-112: MINOR_VERSION bump is appropriateRaising MINOR_VERSION to 1 matches the addition of a new field in the entry data schema.
tests/test_init.py (2)
13-18: MC test data import looks goodImporting MOCK_CONFIG_DATA_1_MC aligns with the new multi-connector scenario in tests.const.
62-96: Solid setup/reload/unload test for multi-connector entriesThis mirrors the single-connector test with minor_version=1 and validates lifecycle behavior. Good coverage for the new configuration.
custom_components/ocpp/button.py (3)
25-27: per_connector flag added to button descriptions is clearThis cleanly distinguishes charger-level vs per-connector buttons and keeps the setup logic straightforward.
101-118: Unique ID and per-connector DeviceInfo are well-structuredIncluding the connector segment in unique_id and creating a child device (via_device -> CP) matches HA best practices and the rest of the integration.
122-130: Connector-aware availability and action routing are correctavailable() and async_press() correctly pass connector_id through to CentralSystem.
custom_components/ocpp/number.py (2)
64-91: Per-connector number entities created as expectedThe loop 1..num_connectors and embedding connector_id in the description/entity ensure distinct per-connector sliders. Using cp_id_settings[CONF_MAX_CURRENT] to bound initial/max is sensible.
107-131: Unique ID and per-connector DeviceInfo are consistent with other platformsThe unique_id composition and child device creation mirror button/switch, keeping registry relationships coherent.
custom_components/ocpp/switch.py (2)
73-90: Connector-aware entity creation logic looks goodEntities are spawned per-connector for connector-level switches and once for charger-level ones. Fallback normalization of num_connectors with “or 1” is correct.
136-149: State derivation uses connector-aware metrics correctlyis_on() queries the appropriate metric with connector_id and respects metric_condition when supplied. This matches CentralSystem.get_metric semantics.
custom_components/ocpp/sensor.py (3)
100-130: Per-connector entity spawning for multi-outlet chargers looks solidUsing connector-scoped metrics for num_connectors > 1 and adding a single root EAIR is a sensible balance between granularity and avoiding duplication. Diagnostic categorization for connector status/error is also appropriate.
181-191: Child DeviceInfo wiring is correctUsing via_device to attach connector devices under the charge point and unique per-connector identifiers will make the device tree clean and discoverable.
295-306: Good: immediate refresh after restore and dispatcher hookupScheduling an immediate HA state update after restore + dispatcher registration improves perceived responsiveness and avoids stale values on startup.
tests/test_charge_point_v16.py (3)
643-724: Great coverage for multi-connector v1.6 pathEnd-to-end exercising of 2 connectors, including per-connector meter values and status notifications, validates the new connector-aware API. The assertions target connector_id=1 explicitly, which avoids ambiguity with fallback logic.
729-737: Background task management in test CP is safeTracking spawned asyncio tasks in a set and discarding on completion prevents leaks while allowing trigger_message to be non-blocking. This mirrors realistic devices and avoids flakiness.
999-1024: StatusNotification defaults are sensible for testingMapping connector_id 0→SuspendedEV, 1→Charging, else→Available provides variation to validate per-connector availability paths without hardcoding excessive branches in tests.
custom_components/ocpp/api.py (4)
94-96: Utility normalizer_normis appropriate hereLowercasing and stripping non-alphanumerics simplifies status comparisons in availability checks. Good choice.
483-526: Connector-aware availability reads are robust with legacy fallbackThe layered approach (per-connector metric → flat with per-connector extra_attr → charge point status) minimizes false “unavailable” readings across device variants.
536-547: set_max_charge_rate_amps forwards conn_id correctlyMapping HA’s connector_id to v16/v201’s semantics via set_charge_rate(conn_id=...) preserves the 0=CP profile vs >0=TxDefault distinction.
549-576: Connector-aware state control path is consistentForwarding connector_id to availability, start, and unlock calls aligns services with per-connector entities.
custom_components/ocpp/ocppv16.py (3)
68-81: String→MessageTrigger normalization is a nice usability boostAllowing both enums and strings (case/spacing agnostic) for trigger_custom_message reduces friction for callers and tests.
233-258: Status trigger loop adjusts connector count gracefullyIterating 0..N and clamping Connectors metric when the CP rejects higher indexes is a pragmatic way to auto-correct misreported connector counts.
742-756: Good separation between CP-level and connector-level status/errorUsing connector 0 for CP aggregate and per-connector keys for individual outlets matches the retrieval logic in CentralSystem and tests.
tests/test_charge_point_v201_multi.py (3)
58-66: Test CP scaffolding is compact and focusedThe minimal MultiConnectorChargePoint with an event to signal inventory readiness keeps the scenario deterministic without overfitting to internals.
110-187: Full inventory publisher is realistic and exhaustivePublishing EVSE status, connector Enabled, SmartCharging availability, and TxUpdatedMeasurands gives the server enough context to build maps and enable smart charging paths.
329-361: End-to-end assertions cover status, energy accounting, units, and start routing
- Verifying Status.Connector for 1..3 anchors the availability path.
- EAIR/session values and kWh units validate per-connector metric wiring.
- Checking last_start_evse_id confirms the global→EVSE mapping logic.
custom_components/ocpp/ocppv201.py (1)
256-265: ClearChargingProfile: payload purpose key aligns with the library expectationsUsing
"charging_profile_purpose"with.valuelooks correct for v2.x. Good improvement aligning v2 payload with the library’s dataclasses.custom_components/ocpp/chargepoint.py (3)
323-337: Pre-initializing per-connector metrics is the right callEagerly touching per-connector keys ensures entities can be created immediately. This is important because v2.x cannot reliably create connector child devices after-the-fact. Looks good.
452-456: Latency metrics and units: nice touchExplicitly setting units to "ms" and publishing latency metrics is helpful for diagnostics. LGTM.
Also applies to: 483-485, 491-492
285-291: Base metrics initialization on connector 0 is consistentInitializing identifiers, time/energy units, and reconnect counters on connector 0 aligns with the new connector-aware design. Good baseline.
jthun
had a problem deploying
to
continuous-integration
GitHub Actions
Failure
|
Great addition @jthun, however in my view it should not be implemented as a breaking change. If there is only 1 connector the additional functionality should not change sensors, buttons etc that are there now. The extra connector entity and entities specific to this should only appear if there are 2 or more connectors. |
Thanks! I absolutely agree on the breaking change. I'm trying to remove that now. |
…tion. CodeRabbit fixes.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
custom_components/ocpp/button.py (1)
58-59: Zero-connectors normalization: resolved (matches other platforms)The guard int(... or 1) aligns with sensor/number/switch behavior and prevents accidental 0 suppressing entities. Nothing further to do here.
custom_components/ocpp/api.py (1)
198-206: Good: centralize connector-id normalization.Using
_norm_conn()everywhere prevents tuple key mismatches when callers pass strings (e.g.,"1") and aligns lookups across all getters.custom_components/ocpp/chargepoint.py (1)
733-812: Fix confirmed: OCPP 2.x Energy.Session now readsmeter_start.value.The branch correctly uses
csess.meter_start.value, resolving the earlier bug where the Enum itself was used as a key.custom_components/ocpp/ocppv201.py (2)
114-157: Connector-index mapping: solid and collision-free (addresses past concern).The new mapping helpers and the unique-index fallback in
_pair_to_global()avoid collisions when inventory isn’t ready. This directly addresses the earlier review about global-index collisions during early StatusNotification bursts. Good call also on building from inventory when available.
337-340: Enum serialization fix in ClearChargingProfile payload: resolved.Using
.valueforcharging_profile_purposematches the v2.x schema expectations.
🧹 Nitpick comments (16)
custom_components/ocpp/enums.py (1)
65-75: Optional: pin explicit bit values to avoid future accidental renumberingIf the member order ever changes, auto() could shift bit assignments and break persisted values or assumptions in downstream code. Consider assigning explicit values.
Apply this diff if you want to pin the bits:
-class Profiles(IntFlag): +class Profiles(IntFlag): """Flags to indicate supported feature profiles.""" - NONE = 0 - CORE = auto() # Core - FW = auto() # FirmwareManagement - SMART = auto() # SmartCharging - RES = auto() # Reservation - REM = auto() # RemoteTrigger - AUTH = auto() # LocalAuthListManagement + NONE = 0 + CORE = 1 << 0 # Core + FW = 1 << 1 # FirmwareManagement + SMART = 1 << 2 # SmartCharging + RES = 1 << 3 # Reservation + REM = 1 << 4 # RemoteTrigger + AUTH = 1 << 5 # LocalAuthListManagementscripts/develop (2)
7-10: Run ensure_config when config exists but is empty/staleOnly running ensure_config on a missing config dir can miss required defaults if the directory exists but lacks files. Prefer checking for configuration.yaml presence.
Apply this diff:
-# Create config dir if not present -if [[ ! -d "${PWD}/config" ]]; then - mkdir -p "${PWD}/config" - /home/vscode/.local/ha-venv/bin/python -m homeassistant --config "${PWD}/config" --script ensure_config -fi +# Create/prepare config +mkdir -p "${PWD}/config" +if [[ ! -f "${PWD}/config/configuration.yaml" ]]; then + /home/vscode/.local/ha-venv/bin/python -m homeassistant --config "${PWD}/config" --script ensure_config +fi
30-36: Default debugger bind should be localhost for safetyBinding debugpy to 0.0.0.0 by default can expose the debugger when run outside a container. Default to 127.0.0.1 and let users opt into remote access.
Apply this diff:
-DEBUGPY_LISTEN="${DEBUGPY_LISTEN:-0.0.0.0:5678}" +DEBUGPY_LISTEN="${DEBUGPY_LISTEN:-127.0.0.1:5678}"custom_components/ocpp/button.py (1)
39-45: Use a neutral device_class for Unlock (UPDATE is misleading)ButtonDeviceClass.UPDATE is meant for firmware/update actions. For an Unlock action, omit the device_class to avoid confusing UI semantics.
Apply this diff:
- OcppButtonDescription( + OcppButtonDescription( key="unlock", name="Unlock", - device_class=ButtonDeviceClass.UPDATE, + device_class=None, entity_category=EntityCategory.CONFIG, press_action=HAChargerServices.service_unlock.name, per_connector=True, ),custom_components/ocpp/number.py (1)
158-161: Minor: _attr_available is unused when providing an available propertyYou define a dynamic available property below; setting _attr_available here is redundant.
Apply this diff:
- self._attr_should_poll = False - self._attr_available = True + self._attr_should_poll = Falsecustom_components/ocpp/switch.py (1)
175-203: Prefer explicit station-level connector_id=0 for non per-connector actionsPassing None relies on downstream normalization. Using 0 for charger-level actions is clearer and avoids surprises if lower layers don’t normalize None.
Apply this diff:
async def async_turn_on(self, **kwargs): """Turn the switch on.""" - target_conn = ( - self.connector_id if self.entity_description.per_connector else None - ) + target_conn = self.connector_id if self.entity_description.per_connector else 0 self._state = await self.central_system.set_charger_state( self.cpid, self.entity_description.on_action, True, connector_id=target_conn ) async def async_turn_off(self, **kwargs): """Turn the switch off.""" - target_conn = ( - self.connector_id if self.entity_description.per_connector else None - ) + target_conn = self.connector_id if self.entity_description.per_connector else 0custom_components/ocpp/api.py (1)
472-513: Clarify availability fallbacks and unknown statuses.Logic is sound: normalize connector id, try per-connector status (including flattened aggregate extra_attr), and fall back to CP status. Consider logging a debug line when falling back to CP-level because per-connector status is missing; helps diagnose chargers with partial data. Optional.
custom_components/ocpp/ocppv16.py (1)
332-371: Optional: compute “default profile” relative to chosen unit.
is_default = (limit_amps >= 32) and (limit_watts >= 22000)can clear profiles unintentionally when the unused branch retains its default. Consider basing this solely on the actually used unit:- is_default = (limit_amps >= 32) and (limit_watts >= 22000) + is_default = (use_amps and limit_amps >= 32) or (not use_amps and limit_watts >= 22000)tests/test_charge_point_v201_multi.py (1)
355-361: Nice coverage: RequestStartTransaction mapping to EVSE ID.Asserting
last_start_evse_id == 2validates the global→EVSE mapping logic for connector 3. Consider adding one negative test (e.g., out-of-range connector) to assert graceful fallback.If you want, I can add a parametrized case to exercise invalid connector ids and confirm we fall back to CP-level availability.
custom_components/ocpp/chargepoint.py (1)
569-601: Nit: remove or use debug counters.
updated_entitiesandfound_childrenare accumulated but unused. Either log them at debug or drop them to avoid dead code.- updated_entities = 0 - found_children = 0 + # counters available for debugging if needed + # updated_entities = 0 + # found_children = 0 @@ - updated_entities += 1 + # updated_entities += 1 @@ - found_children += 1 + # found_children += 1custom_components/ocpp/ocppv201.py (6)
182-200: EVSE→v1.6 status mapping: consider station-level aggregation semantics.Right now,
_report_evse_status(evse_id, v16)updates the “global” connector (index 0) with the latest EVSE’s status, so the last update wins. If multiple EVSEs exist, you likely want a deterministic station status (e.g., faulted if any EVSE faulted; otherwise unavailable if any unavailable; otherwise preparing if any preparing/charging; else available). This keeps the charger-level entity stable and meaningful.Would you like a small helper that computes a precedence-based aggregate across
self._connector_statusand updates(0, status_connector)once per change?
342-407: Support EVSE-specific SetChargingProfile whenconn_id> 0; avoid hard-coding CS-wide (evse_id=0).Currently
conn_idis unused and profiles are always targeted at the Charging Station. Enable per-EVSE targeting by resolving the(evse_id, connector_id)pair from the global connector index. Keep CS-wide as default for 0/None. Also, consider avoiding hard-coded 32A/22kW thresholds; if feasible, derive the physical limits from inventory/config (nice-to-have).async def set_charge_rate( self, limit_amps: int | None = None, limit_watts: int | None = None, conn_id: int = 0, profile: dict | None = None, ): - """Set a charging profile with defined limit (OCPP 2.x).""" + """Set a charging profile with defined limit (OCPP 2.x). + + Notes: + - conn_id=0 (default) targets the Charging Station (evse_id=0). + - conn_id>0 targets the specific EVSE corresponding to the global connector index. + """ + # Resolve target EVSE: 0=CS-wide; >0 per-EVSE + evse_target = 0 + if conn_id and conn_id > 0: + with contextlib.suppress(Exception): + evse_target, _ = self._global_to_pair(int(conn_id)) if profile is not None: - req = call.SetChargingProfile(0, profile) + req = call.SetChargingProfile(evse_target, profile) resp: call_result.SetChargingProfile = await self.call(req) if resp.status != ChargingProfileStatusEnumType.accepted: raise HomeAssistantError( translation_domain=DOMAIN, translation_key="set_variables_error", translation_placeholders={ "message": f"{str(resp.status)}: {str(resp.status_info)}" }, ) return @@ - charging_profile: dict = { + charging_profile: dict = { "id": 1, "stack_level": 0, "charging_profile_purpose": ChargingProfilePurposeEnumType.charging_station_max_profile.value, "charging_profile_kind": ChargingProfileKindEnumType.relative.value, "charging_schedule": [schedule], } - req: call.SetChargingProfile = call.SetChargingProfile(0, charging_profile) + req: call.SetChargingProfile = call.SetChargingProfile(evse_target, charging_profile)Optional follow-up:
- Instead of fixed 32A/22kW, read limits from inventory (e.g., EVSE/Connector capabilities) or allow them to be HA-configurable caps.
408-427: ChangeAvailability targeting: LGTM with clear CS-wide vs per-EVSE semantics.Passing only
statusfor CS-wide andevse={"id": ...}for per-EVSE is correct. Thecontextlib.suppressaround_global_to_pairavoids spurious failures for bad input. Consider logging a debug whenconnector_id> 0 but fails to resolve to improve observability.
428-444: Start transaction targeting: LGTM; consider uniqueremote_start_id.Mapping global connector index to
evse_idis correct. Generating a uniqueremote_start_id(e.g., monotonic counter) improves traceability and avoids clashes across repeated starts.- req: call.RequestStartTransaction = call.RequestStartTransaction( - evse_id=evse_id, + # TODO: derive a monotonic remote_start_id + req: call.RequestStartTransaction = call.RequestStartTransaction( + evse_id=evse_id, id_token={ "id_token": self._remote_id_tag, "type": IdTokenEnumType.central.value, }, - remote_start_id=1, + remote_start_id=1, )
446-463: Stop transaction: first-hit across connectors is pragmatic; consider preferring most recent.Scanning connectors to find a tx_id works. If multiple EVSEs can transact concurrently, you may want to stop the most recently active transaction or accept a connector/global index param to target a specific EVSE.
798-867: Session timing and cleanup: clear_tx_start_timeon end to avoid stale entries.Great handling of session timing with minute rounding. On
ended, also remove the stored start time to avoid leaking per-connector entries across sessions.if event_type == TransactionEventEnumType.ended.value: - self._metrics[(global_idx, csess.transaction_id.value)].value = "" - self._metrics[(global_idx, cstat.id_tag.value)].value = "" + self._metrics[(global_idx, csess.transaction_id.value)].value = "" + self._metrics[(global_idx, cstat.id_tag.value)].value = "" + self._tx_start_time.pop(global_idx, None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
.gitignore(1 hunks)custom_components/ocpp/api.py(6 hunks)custom_components/ocpp/button.py(4 hunks)custom_components/ocpp/chargepoint.py(15 hunks)custom_components/ocpp/config_flow.py(4 hunks)custom_components/ocpp/const.py(3 hunks)custom_components/ocpp/enums.py(2 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/ocppv16.py(14 hunks)custom_components/ocpp/ocppv201.py(13 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(4 hunks)custom_components/ocpp/translations/nl.json(1 hunks)scripts/develop(1 hunks)tests/charge_point_test.py(2 hunks)tests/test_charge_point_v16.py(10 hunks)tests/test_charge_point_v201_multi.py(1 hunks)tests/test_config_flow.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- custom_components/ocpp/translations/nl.json
- tests/test_config_flow.py
- custom_components/ocpp/const.py
- .gitignore
- tests/test_charge_point_v16.py
- custom_components/ocpp/config_flow.py
- tests/charge_point_test.py
🧰 Additional context used
🧬 Code graph analysis (10)
custom_components/ocpp/button.py (4)
custom_components/ocpp/sensor.py (2)
async_setup_entry(41-139)available(186-188)custom_components/ocpp/number.py (2)
async_setup_entry(56-114)available(176-183)custom_components/ocpp/switch.py (2)
async_setup_entry(65-99)available(147-152)custom_components/ocpp/api.py (2)
get_available(472-513)set_charger_state(537-564)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/chargepoint.py (14)
clear(186-187)get(171-175)value(83-85)value(88-90)update(569-600)set_charge_rate(377-385)clear_profile(373-375)set_availability(387-389)start_transaction(391-393)values(180-181)get_energy_kwh(711-715)unit(93-95)unit(98-100)process_measurands(717-817)
custom_components/ocpp/sensor.py (2)
custom_components/ocpp/enums.py (3)
HAChargerSession(56-62)HAChargerStatuses(25-38)HAChargerDetails(41-53)custom_components/ocpp/api.py (4)
get_available(472-513)get_extra_attr(435-470)get_metric(303-343)get_ha_unit(398-433)
custom_components/ocpp/ocppv16.py (1)
custom_components/ocpp/chargepoint.py (18)
get(171-175)get_number_of_connectors(290-292)get_configuration(425-427)value(83-85)value(88-90)notify_ha(881-892)values(180-181)extra_attr(108-110)extra_attr(113-115)get_ha_metric(840-855)MeasurandValue(214-222)unit(93-95)unit(98-100)get_energy_kwh(711-715)process_measurands(717-817)update(569-600)stop_transaction(395-401)items(183-184)
custom_components/ocpp/chargepoint.py (3)
custom_components/ocpp/ocppv201.py (3)
get_number_of_connectors(246-249)start_transaction(428-443)ChargePoint(76-866)custom_components/ocpp/ocppv16.py (3)
get_number_of_connectors(112-119)start_transaction(431-445)ChargePoint(83-981)custom_components/ocpp/api.py (3)
get_metric(303-343)get_extra_attr(435-470)get_unit(361-396)
scripts/develop (1)
custom_components/ocpp/__init__.py (1)
async_setup(87-95)
custom_components/ocpp/number.py (4)
custom_components/ocpp/sensor.py (1)
available(186-188)custom_components/ocpp/button.py (1)
available(135-137)custom_components/ocpp/api.py (3)
get_supported_features(515-522)get_available(472-513)set_max_charge_rate_amps(524-535)custom_components/ocpp/enums.py (1)
Profiles(65-74)
tests/test_charge_point_v201_multi.py (4)
custom_components/ocpp/enums.py (1)
HAChargerServices(6-22)custom_components/ocpp/ocppv201.py (1)
ChargePoint(76-866)tests/charge_point_test.py (4)
create_configuration(67-75)run_charge_point_test(108-135)remove_configuration(81-86)wait_ready(92-95)custom_components/ocpp/api.py (3)
get_metric(303-343)get_unit(361-396)set_charger_state(537-564)
custom_components/ocpp/switch.py (2)
custom_components/ocpp/api.py (3)
get_available(472-513)get_metric(303-343)set_charger_state(537-564)custom_components/ocpp/enums.py (2)
HAChargerServices(6-22)HAChargerStatuses(25-38)
custom_components/ocpp/api.py (3)
custom_components/ocpp/enums.py (1)
HAChargerStatuses(25-38)custom_components/ocpp/chargepoint.py (18)
get(171-175)get_metric(824-838)value(83-85)value(88-90)get_unit(873-875)unit(93-95)unit(98-100)get_ha_unit(877-879)ha_unit(103-105)get_extra_attr(857-871)extra_attr(108-110)extra_attr(113-115)set_charge_rate(377-385)set_availability(387-389)start_transaction(391-393)stop_transaction(395-401)reset(403-405)unlock(407-409)custom_components/ocpp/ocppv201.py (5)
set_charge_rate(342-406)set_availability(408-426)start_transaction(428-443)stop_transaction(445-462)reset(464-474)
🔇 Additional comments (21)
custom_components/ocpp/enums.py (1)
3-3: Good move: switch Profiles to IntFlag with auto bitmasksUsing IntFlag enables clean bitwise checks (e.g., features & Profiles.SMART). The members defined with auto() will yield power-of-two values suitable for combination. Looks consistent with usages across the PR.
Also applies to: 65-75
custom_components/ocpp/button.py (2)
60-92: Per-connector entity creation logic avoids single-connector breakage — niceCreating per-connector buttons only when num_connectors > 1 preserves existing entity topology for single-outlet chargers, addressing the reviewer’s concern about breaking changes.
117-132: Unique IDs and device graph are consistent and stableIncluding connN in unique_id only when a connector is present keeps single-connector unique_ids unchanged; via_device links connector devices under the main CP device properly.
custom_components/ocpp/number.py (2)
142-157: Stable unique_id and device mapping — goodFor single-connector setups you omit the connector segment, preserving existing unique_ids. For multi-connector setups you add connN and set via_device to the CP device, which yields a clean device tree in HA.
175-183: Availability gating correctly requires SMART profileChecking Profiles.SMART before exposing the slider matches the feature semantics. Looks good.
custom_components/ocpp/switch.py (1)
70-75: Entity creation flow is solid and consistent with button/number platformsReading CONF_NUM_CONNECTORS with or 1 and using flatten_single mirrors the other platforms and should yield predictable entity sets across chargers.
Also applies to: 76-87
custom_components/ocpp/sensor.py (2)
101-138: Nice: no connector child-devices for single-outlet chargers (avoids breaking change).Creating connector child entities only when
num_connectors > 1preserves the legacy single-device layout. This directly addresses the review feedback from drc38.
165-169: Verify unique_id scheme change won’t churn existing entities.Unique IDs now include an extra segment for connectors (
conn{N}) and a different ordering. For single-connector setups you omit the segment, which may still differ from older releases if the slugging or ordering changed. Please confirm this doesn’t cause entity renames or duplicates on upgrade; if it does, add a migration.Run to list existing entity unique_ids at runtime and compare after upgrade.
custom_components/ocpp/ocppv16.py (1)
635-764: Robust and clear meter handling.Parsing sampled values, normalizing Wh→kWh/W→kW, writing EAIR without txId to conn 0, and deriving Energy.Session when applicable looks correct and aligns with the new per-connector model.
tests/test_charge_point_v201_multi.py (1)
336-354: Solid assertions for per-connector EAIR and session kWh.The test validates per-connector status, energy values, and HA units, which catches most regressions in the 2.0.1 path.
custom_components/ocpp/chargepoint.py (1)
118-196: Great abstraction:_ConnectorAwareMetricspreserves flat access while enabling per-connector keys.Backwards-compatible indexing (
m["X"]→ conn 0;m[(n,"X")]→ connector n) simplifies the migration. Iteration semantics (flat view) are sensible for HA-facing code.custom_components/ocpp/ocppv201.py (10)
201-210: Buffered StatusNotifications: good defensive handling.Deferring application until the connector map is ready and then flushing is the right approach to avoid misattribution.
211-216: Total connector computation: LGTM.Summing
connector_countwith a safe default is correct.
235-245: Inventory retrieval flow: LGTM.Gracefully handling NotImplemented and OCPP errors, waiting on the event, and building the map post-inventory is sound.
246-250: Expose number of connectors: LGTM.Delegates cleanly to inventory and totalizer.
567-570: Global status update on (0, status_connector): confirm consumer expectations.Writing EVSE-derived v1.6 status into
(0, status_connector)is fine if HA treats 0 as charger-level status. If 0 is strictly “aggregate,” consider computing a true station-level status (see earlier comment).Would you like a short script to check where
(0, status_connector)is consumed in the integration to ensure semantics align?
573-587: StatusNotification buffering + apply: LGTM.The buffering path gracefully returns an ACK and eventually reconciles once mapping exists, followed by an HA update. Good.
590-608: Handler scaffolding for v2.x events: LGTM.Responding with empty ACKs for firmware/log/notify is acceptable until richer handling is implemented.
610-717: Inventory parsing: robust coverage of EVSE/Connector/Controllers.
- Correctly updates EVSE count and per-EVSE connector maxima.
- Parses
TxUpdatedMeasurandsfrom characteristics’values_list.- Triggers map build and flushes pending notifications only when
tbcis false.
733-779: Per-connector meter values: correct unit handling and meter_start initialization.
- Properly expands multipliers and converts Wh→kWh for EAIR.
- Initializes
meter_startatstartedor firstupdatedif missing.
781-797: End-of-transaction measurand cleanup: LGTM.Zeroing non-energy measurands absent at
transaction_endkeeps HA entities sane.
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
lbbrhzn
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1689 +/- ##
==========================================
+ Coverage 92.64% 94.16% +1.51%
==========================================
Files 12 12
Lines 2053 2673 +620
==========================================
+ Hits 1902 2517 +615
- Misses 151 156 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…d for debugging). Fix Charge Control naming when only one connector. More CodeRabbit suggestions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/ocppv201.py (1)
65-74: Critical: InventoryReport uses class-level mutable defaults (state leaks across chargers).connector_count and tx_updated_measurands are class variables initialized to the same list for all instances. In-place mutations (+=, append, etc.) will leak between ChargePoint instances. Convert to a dataclass with per-instance defaults.
@@ -from datetime import datetime, UTC +from datetime import datetime, UTC +from dataclasses import dataclass, field @@ -class InventoryReport: - """Cached full inventory report for a charger.""" - - evse_count: int = 0 - connector_count: list[int] = [] - smart_charging_available: bool = False - reservation_available: bool = False - local_auth_available: bool = False - tx_updated_measurands: list[MeasurandEnumType] = [] +@dataclass +class InventoryReport: + """Cached full inventory report for a charger.""" + evse_count: int = 0 + connector_count: list[int] = field(default_factory=list) + smart_charging_available: bool = False + reservation_available: bool = False + local_auth_available: bool = False + tx_updated_measurands: list[MeasurandEnumType] = field(default_factory=list)
♻️ Duplicate comments (6)
scripts/develop (1)
23-27: Bug: symlink to custom_components/ocpp is never created (integration won’t load).You remove any existing destination at $dst but don’t actually create the new symlink. As written, Home Assistant will start without the ocpp integration linked into config/custom_components.
Fix by adding the ln step after removal:
# Remove existing dir/symlink/file at destination if [[ -e "$dst" || -L "$dst" ]]; then rm -rf -- "$dst" fi +# Create the symlink +ln -s "$src" "$dst"custom_components/ocpp/number.py (1)
129-140: Per-connector writes are correctly targeted via _op_connector_id.The constructor derives _op_connector_id appropriately, and async_set_native_value now forwards connector_id=self._op_connector_id, fixing the earlier “always EVSE 0” issue.
Also applies to: 187-191
custom_components/ocpp/ocppv201.py (3)
139-158: Collision-free connector mapping implemented — matches prior guidance._pair_to_global now avoids collisions and builds/allocates deterministically. Good improvement for early notifications before inventory is ready.
167-173: List-of-lists aliasing bug fixed when growing _connector_status — nice.Using a comprehension ensures each EVSE gets its own list.
333-342: SetChargingProfile payload correctness — using .value for enums looks good.charging_profile_purpose is now serialized to its string value; request construction aligns with the OCPP 2.0.1 schema.
Also applies to: 399-407
custom_components/ocpp/switch.py (1)
123-128: Single-connector stability preserved: no conn1 in unique_idConditionally appending
conn{N}only whennot self._flatten_singleresolves the prior breaking-change risk for single-outlet chargers. Nice.
🧹 Nitpick comments (8)
scripts/develop (1)
28-41: Optional: streamline debugger install/use and noisy output.
- Consider adding --disable-pip-version-check to quiet pip.
- Since you install debugpy unconditionally when unavailable, you can skip the second import check and just exec through debugpy.
Purely optional polish; current behavior is correct.
-if ! /home/vscode/.local/ha-venv/bin/python -c 'import debugpy' >/dev/null 2>&1; then - /home/vscode/.local/ha-venv/bin/python -m pip install --quiet debugpy +if ! /home/vscode/.local/ha-venv/bin/python -c 'import debugpy' >/dev/null 2>&1; then + /home/vscode/.local/ha-venv/bin/python -m pip install --quiet --disable-pip-version-check debugpy ficustom_components/ocpp/number.py (2)
63-71: Guard against missing CONF_MAX_CURRENT to avoid KeyError; coerce to numeric.If CONF_MAX_CURRENT isn’t present in cp_id_settings (e.g., legacy entries), this will raise. Use .get with DEFAULT_MAX_CURRENT and coerce to float.
- if desc.key == "maximum_current": - ent_initial = cp_id_settings[CONF_MAX_CURRENT] - ent_max = cp_id_settings[CONF_MAX_CURRENT] + if desc.key == "maximum_current": + max_cur = float(cp_id_settings.get(CONF_MAX_CURRENT, DEFAULT_MAX_CURRENT)) + ent_initial = max_cur + ent_max = max_cur
184-186: Docstring now misleading (can be per-connector, not only EVSE 0).The docstring still says “station-wide max current (EVSE 0)”, but the code targets self._op_connector_id (per-connector when >1). Update the docstring for clarity.
- """Set new value for station-wide max current (EVSE 0).""" + """Set new value for max current (station-wide when _op_connector_id==0, otherwise per-connector)."""custom_components/ocpp/ocppv201.py (1)
285-318: Nit: fix docstring to reflect features, not measurands.The function returns supported feature profiles but the docstring says “measurands”.
- async def get_supported_features(self) -> Profiles: - """Get comma-separated list of measurands supported by the charger.""" + async def get_supported_features(self) -> Profiles: + """Get feature profiles supported by the charger."""custom_components/ocpp/sensor.py (1)
49-57: Configured measurands parsing is robust, but consider a safe default set.You now rely entirely on CONF_MONITORED_VARIABLES. If earlier behavior implicitly included a baseline set (e.g., session metrics), consider augmenting with a minimal default to avoid empty CONNECTOR_ONLY when config is blank.
- measurands = sorted(configured) + # Keep configured list, but ensure a minimal default set when empty + default_measurands: list[str] = [] + measurands = sorted(configured or default_measurands)If you intentionally want “no extra sensors unless configured,” ignore this suggestion.
custom_components/ocpp/switch.py (3)
36-62: Charge-control “on” conditions: confirm intended semanticsCurrently “on” is true for charging and both suspended states. If you want “preparing” or “finishing” to keep the switch on as well (common UX for “actively in a session”), consider adding those. If not, current mapping is fine—just confirming intent.
Would you like me to align these conditions with whatever you decided for buttons/sensors (e.g., the per-connector button or sensor states) for consistency?
144-147: Ensure boolean return from available()Home Assistant expects
availableto be a bool.get_available()can returnNone; make it explicitly boolean to avoid edge cases.Apply this diff:
- return self.central_system.get_available(self.cpid, target_conn) + return bool(self.central_system.get_available(self.cpid, target_conn))
177-193: Off action handling is sound; optional clarity improvementLogic for same-action toggles (availability) vs. distinct off-action (stop transaction) is correct. For clarity, you could pass
state=Trueexplicitly in the distinct off-action branch, even though it’s ignored for stop/unlock.Apply this tiny diff if you prefer explicitness:
- resp = await self.central_system.set_charger_state( - self.cpid, self.entity_description.off_action, connector_id=target_conn - ) + resp = await self.central_system.set_charger_state( + self.cpid, + self.entity_description.off_action, + True, + connector_id=target_conn, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
custom_components/ocpp/button.py(4 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/ocppv201.py(13 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(4 hunks)scripts/develop(1 hunks)tests/charge_point_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- custom_components/ocpp/button.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/charge_point_test.py (1)
custom_components/ocpp/chargepoint.py (2)
value(83-85)value(88-90)
custom_components/ocpp/sensor.py (2)
custom_components/ocpp/enums.py (2)
HAChargerDetails(41-53)HAChargerStatuses(25-38)custom_components/ocpp/api.py (4)
get_available(472-513)get_extra_attr(435-470)get_metric(303-343)get_ha_unit(398-433)
custom_components/ocpp/switch.py (1)
custom_components/ocpp/api.py (3)
get_available(472-513)get_metric(303-343)set_charger_state(537-564)
custom_components/ocpp/number.py (8)
custom_components/ocpp/chargepoint.py (5)
get(171-175)get_supported_features(306-308)value(83-85)value(88-90)get_number_of_connectors(201-203)custom_components/ocpp/sensor.py (2)
native_unit_of_measurement(271-282)available(186-188)custom_components/ocpp/button.py (1)
available(135-137)custom_components/ocpp/switch.py (1)
available(142-147)custom_components/ocpp/ocppv201.py (1)
get_supported_features(285-317)custom_components/ocpp/api.py (3)
get_supported_features(515-522)get_available(472-513)set_max_charge_rate_amps(524-535)custom_components/ocpp/ocppv16.py (2)
get_supported_features(186-223)get_number_of_connectors(89-91)custom_components/ocpp/enums.py (1)
Profiles(65-74)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/chargepoint.py (9)
get(171-175)value(83-85)value(88-90)update(569-600)values(180-181)get_energy_kwh(711-715)unit(93-95)unit(98-100)process_measurands(717-817)
🔇 Additional comments (11)
custom_components/ocpp/number.py (2)
72-92: Non-breaking behavior preserved for single-connector setups — good call.Creating per-connector Number entities only when num_connectors > 1 keeps existing entity model intact for single-outlet chargers while enabling multi-connector control when needed.
Also applies to: 94-112
174-182: Confirm synchronous contract forget_supported_featuresThe call to
central_system.get_supported_features(self.cpid)innumber.availablecorrectly invokes the synchronous method defined incustom_components/ocpp/api.py(line 515). An async-only implementation is not in use here, and existing tests cover both sync and async behavior, so the availability check won’t be inadvertently blocked.custom_components/ocpp/ocppv201.py (1)
420-439: Availability control semantics are sensible.conn_id None/0 targets station, >0 resolves EVSE via the global-to-pair mapping; behavior matches user expectations for per-connector availability.
tests/charge_point_test.py (1)
39-47: Passing numeric values as ints to number.set_value — correct and future-proof.Home Assistant’s number.set_value expects a numeric type; removing str(...) avoids implicit conversions and aligns with the rest of the tests.
custom_components/ocpp/sensor.py (3)
89-99: Per-connector entity spawning and root device wiring look correct and non-breaking.
- Root/charger-only metrics always attach to the base device.
- Connector metrics use per-connector devices with via_device set to the base cp.
- Single-connector setups keep connector_id=None to preserve legacy flattening.
All align with the PR goal to avoid breaking single-connector users.
Also applies to: 101-120, 121-137
165-181: Stable unique IDs and device info per connector — good structure.Including connN in unique_id and device identifiers ensures deterministic migration when moving from 1→N connectors and prevents collisions across connectors.
186-189: Connector-aware data paths are consistent across availability, attributes, value, and units.All central_system calls pass connector_id consistently, and an initial update is scheduled on add — solid.
Also applies to: 199-204, 263-269, 271-283, 295-296
custom_components/ocpp/switch.py (4)
31-33: Good extension: list-based conditions and per-connector flagSwitching
metric_conditiontolist[str]and introducingper_connectoron the description cleanly model multi-connector semantics. No issues spotted.
129-139: Connector sub-devices only for multi-connector: correct approachCreating per-connector DeviceInfo only when
not self._flatten_singleavoids migrating existing single-connector entities to a new device. This addresses the earlier concern about breaking changes while still yielding clear naming for multi-connector setups via device names.
170-175: Per-connector targeting for turn_on looks correctPassing
connector_idfor per-connector switches and 0 for charger-level availability aligns with the API expectations (0 = charge point). Looks good.
70-75: Ignore alternative CPIDs parsing suggestionAfter reviewing all usages of
entry.data[CONF_CPIDS]in the config flow, setup routines, migration logic, and tests, each charger entry is always a single-key dict mapping the charger’s ID to its settings dict—which itself containsCONF_CPID. There is no evidence of a flat shape whereCONF_CPIDwould appear at the top level of the charger dict. The existing extraction vialist(charger.values())[0]and then indexingCONF_CPIDis therefore correct. Please disregard the previous suggestion to support an unsupported data shape.Likely an incorrect or invalid review comment.
custom_components/ocpp/switch.py
Outdated
| metric_conn = ( | ||
| self.connector_id | ||
| if ( | ||
| self.entity_description.metric_state | ||
| == HAChargerStatuses.status_connector.value | ||
| or self.entity_description.per_connector | ||
| ) | ||
| else None | ||
| ) | ||
| resp = self.central_system.get_metric( | ||
| self.cpid, self.entity_description.metric_state | ||
| self.cpid, self.entity_description.metric_state, metric_conn | ||
| ) | ||
| if resp in self.entity_description.metric_condition: | ||
| self._state = True | ||
| else: | ||
| self._state = False | ||
| return self._state # type: ignore [no-any-return] | ||
| if self.entity_description.metric_condition is not None: | ||
| self._state = resp in self.entity_description.metric_condition | ||
| return self._state |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t overwrite state when metric is unknown (preserve default/last-known)
If get_metric() returns None, the code currently sets _state to False due to None in [...] being False. This negates the “use default to set initial state” behavior and can cause flicker on startup. Only update _state when a metric is present.
Apply this diff:
resp = self.central_system.get_metric(
self.cpid, self.entity_description.metric_state, metric_conn
)
- if self.entity_description.metric_condition is not None:
- self._state = resp in self.entity_description.metric_condition
+ if resp is None:
+ return self._state
+ if self.entity_description.metric_condition is not None:
+ self._state = resp in self.entity_description.metric_condition
return self._state📝 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.
| metric_conn = ( | |
| self.connector_id | |
| if ( | |
| self.entity_description.metric_state | |
| == HAChargerStatuses.status_connector.value | |
| or self.entity_description.per_connector | |
| ) | |
| else None | |
| ) | |
| resp = self.central_system.get_metric( | |
| self.cpid, self.entity_description.metric_state | |
| self.cpid, self.entity_description.metric_state, metric_conn | |
| ) | |
| if resp in self.entity_description.metric_condition: | |
| self._state = True | |
| else: | |
| self._state = False | |
| return self._state # type: ignore [no-any-return] | |
| if self.entity_description.metric_condition is not None: | |
| self._state = resp in self.entity_description.metric_condition | |
| return self._state | |
| metric_conn = ( | |
| self.connector_id | |
| if ( | |
| self.entity_description.metric_state | |
| == HAChargerStatuses.status_connector.value | |
| or self.entity_description.per_connector | |
| ) | |
| else None | |
| ) | |
| resp = self.central_system.get_metric( | |
| self.cpid, self.entity_description.metric_state, metric_conn | |
| ) | |
| if resp is None: | |
| return self._state | |
| if self.entity_description.metric_condition is not None: | |
| self._state = resp in self.entity_description.metric_condition | |
| return self._state |
🤖 Prompt for AI Agents
In custom_components/ocpp/switch.py around lines 154 to 168, the code updates
self._state by evaluating resp in self.entity_description.metric_condition even
when resp is None (causing False and overwriting default/last-known state);
change the logic so you only update self._state when a metric value is present:
call resp = self.central_system.get_metric(...), then if
self.entity_description.metric_condition is not None and resp is not None, set
self._state = (resp in self.entity_description.metric_condition); otherwise
leave self._state unchanged to preserve the default/last-known state.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
custom_components/ocpp/number.py (1)
188-195: Thanks for fixing: per-connector write now targets the correct connectorUsing
connector_id=self._op_connector_idresolves the earlier issue where all sliders targeted EVSE 0. This aligns the UI entity with the intended backend target.custom_components/ocpp/button.py (1)
58-58: Guard against zero connectors: normalization matches other platformsUsing
int(... or 1)avoids the “0 connectors” edge case and keeps behavior consistent with sensor/number/switch.custom_components/ocpp/switch.py (2)
123-139: Unique ID/device info respects flatten_single to avoid breaking changesOnly adding
connNand connector device metadata when not flattening single connector aligns with the requirement to maintain existing entities for single-connector chargers.
163-168: Don’t overwrite state when metric is unknown (preserve default/last-known)If
get_metric()returnsNone,resp in [...]evaluates toFalseand clobbers your default/last-known state. Update only when a value is present.- if self.entity_description.metric_condition is not None: - self._state = resp in self.entity_description.metric_condition + if resp is None: + return self._state + if self.entity_description.metric_condition is not None: + self._state = resp in self.entity_description.metric_conditioncustom_components/ocpp/ocppv201.py (3)
141-159: Collision-free global-index allocation looks good.The explicit lookup, inventory-driven build, and unique index allocation remove collision risks noted earlier. Good improvement.
165-176: List-of-lists growth avoids aliasing — fix confirmed.Switch to list comprehension instead of
[[]] * nprevents inner-list aliasing across EVSEs. Nicely done.
340-341: Correct: use enum .value in SetChargingProfile purpose.This fixes the payload serialization issue raised earlier.
🧹 Nitpick comments (6)
custom_components/ocpp/number.py (1)
149-151: Optional: Disambiguate names for multi-connector UIsConsider prefixing entity names with the connector index only when there are 2+ connectors to improve readability in global views. No impact on unique_id stability for single-connector setups.
- self._attr_name = self.entity_description.name + self._attr_name = self.entity_description.name + if self.connector_id: + self._attr_name = f"Connector {self.connector_id} {self._attr_name}"custom_components/ocpp/button.py (2)
135-138: Return a strict boolean from available()
get_available(...)can beNone; HA expects a bool foravailable. Wrap withbool(...)for consistency with other platforms.- return self.central_system.get_available(self.cpid, self._op_connector_id) + return bool(self.central_system.get_available(self.cpid, self._op_connector_id))
121-127: Optional: Clarify names for multi-connector buttonsFor chargers with 2+ connectors, consider prefixing names for better discoverability in the Entities list. Keep flat names for single-connector installs.
self._attr_name = self.entity_description.name - if self.connector_id: + if self.connector_id: + self._attr_name = f"Connector {self.connector_id} {self._attr_name}" self._attr_device_info = DeviceInfo(custom_components/ocpp/sensor.py (1)
187-190: Return a strict boolean from available()
get_available(...)may beNone. Cast toboolto satisfy HA expectations and mirror switch/number platforms.- return self.central_system.get_available(self.cpid, self.connector_id) + return bool(self.central_system.get_available(self.cpid, self.connector_id))custom_components/ocpp/switch.py (1)
128-133: Optional: Improve entity naming for multi-connector chargersFor 2+ connectors, prefix the name with the connector index; keep the current flat name when
flatten_singleis True. This improves UX in global views without breaking single-connector setups.self._attr_name = self.entity_description.name - if self.connector_id and not self._flatten_single: + if self.connector_id and not self._flatten_single: + self._attr_name = ( + f"Connector {self.connector_id} {self._attr_name}" + ) self._attr_device_info = DeviceInfo(custom_components/ocpp/ocppv201.py (1)
180-184: Guard metric writes to avoid KeyError when metric is not yet registered.Depending on initialization order,
(global_idx, cstat.status_connector.value)might not exist inself._metrics. Consider a safe update pattern or anensure_metric(...)helper to avoid runtime KeyErrors under early notifications.Example:
- self._metrics[ - (global_idx, cstat.status_connector.value) - ].value = ConnectorStatusEnumType(connector_status).value + key = (global_idx, cstat.status_connector.value) + if key in self._metrics: + self._metrics[key].value = ConnectorStatusEnumType(connector_status).valueIf a helper exists to lazily create metrics, use that instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
custom_components/ocpp/button.py(4 hunks)custom_components/ocpp/number.py(4 hunks)custom_components/ocpp/ocppv201.py(15 hunks)custom_components/ocpp/sensor.py(5 hunks)custom_components/ocpp/switch.py(4 hunks)scripts/develop(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/develop
🧰 Additional context used
🧬 Code graph analysis (5)
custom_components/ocpp/button.py (5)
custom_components/ocpp/sensor.py (2)
async_setup_entry(41-140)available(187-189)custom_components/ocpp/enums.py (1)
HAChargerServices(6-22)custom_components/ocpp/number.py (2)
async_setup_entry(56-117)available(178-185)custom_components/ocpp/switch.py (2)
async_setup_entry(65-99)available(142-147)custom_components/ocpp/api.py (2)
get_available(472-513)set_charger_state(537-564)
custom_components/ocpp/switch.py (3)
custom_components/ocpp/api.py (4)
CentralSystem(98-655)get_available(472-513)get_metric(303-343)set_charger_state(537-564)custom_components/ocpp/enums.py (2)
HAChargerServices(6-22)HAChargerStatuses(25-38)custom_components/ocpp/__init__.py (1)
async_setup_entry(98-135)
custom_components/ocpp/sensor.py (3)
custom_components/ocpp/enums.py (2)
HAChargerDetails(41-53)HAChargerStatuses(25-38)custom_components/ocpp/chargepoint.py (7)
get(171-175)value(83-85)value(88-90)get_extra_attr(857-871)get_metric(824-838)get_ha_unit(877-879)Metric(70-112)custom_components/ocpp/api.py (4)
get_available(472-513)get_extra_attr(435-470)get_metric(303-343)get_ha_unit(398-433)
custom_components/ocpp/number.py (3)
custom_components/ocpp/api.py (2)
get_available(472-513)set_max_charge_rate_amps(524-535)custom_components/ocpp/ocppv16.py (2)
get_number_of_connectors(89-91)ChargePoint(65-811)custom_components/ocpp/enums.py (1)
Profiles(65-74)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/chargepoint.py (9)
get(171-175)value(83-85)value(88-90)update(569-600)values(180-181)get_energy_kwh(711-715)unit(93-95)unit(98-100)process_measurands(717-817)
🔇 Additional comments (17)
custom_components/ocpp/number.py (3)
63-96: Non-breaking single-connector behavior preserved; multi-connector spawning looks correctGood normalization with
or 1, per-connector entity creation only whennum_connectors > 1, and a flattened single-connector path that keeps the legacy shape. This aligns with the PR objective to avoid breaking changes for single-outlet chargers.
145-156: Unique ID/device hierarchy correctly isolates connectors without breaking single-connector installsAdding
connNonly whenconnector_idis set and creating a per-connector DeviceInfo withvia_devicekeeps existing unique_ids/names intact for single-connector chargers while properly nesting connector devices for multi-connector setups.
177-185: Availability gating by SMART profile and connector availability is appropriateThe slider is available only if SMART is supported and the target connector/charger is available. Matches expected semantics for “Maximum Current.”
custom_components/ocpp/button.py (2)
117-133: Connector-aware unique_id/device info without breaking single-connector installs
connNsegment appears only when a realconnector_idis present; single-connector remains flat. Propervia_devicelinkage is set.
141-145: Per-connector action routing looks correctPassing
connector_id=self._op_connector_idensures Unlock targets the intended connector; single-connector path remains flat (no conn segment in unique_id).custom_components/ocpp/sensor.py (4)
49-58: Config parsing fixed to avoid Enum/string mixing; dedup and sorting are sensibleUsing
str(...).split(",")with guards prevents the previous Enum/string mismatch issue and keeps measurands predictable.
102-121: Per-connector sensor spawning is correct and constrained to multi-connector setupsConnector sensors are created for each connector only when
num_connectors > 1; otherwise, a single flattened entity is created. This matches the non-breaking goal.
166-170: Unique IDs use “is not None” check—good catch for connector_id=0 semanticsUsing
is not Noneavoids accidental falsy checks and ensures future-proofing if 0 ever appears.
296-300: LGTM: immediate update on addScheduling an immediate state update after wiring the dispatcher improves initial UI responsiveness.
custom_components/ocpp/switch.py (2)
70-97: Entity spawning honors non-breaking single-connector behavior
flatten_single = num_connectors == 1while still instantiating a single connector entity ensures legacy unique_ids/names remain unchanged for single-outlet chargers.
144-148: Availability calculation is connector-aware and coerced to boolSelecting
target_connbyper_connectorand returningbool(...)matches API semantics and HA expectations.custom_components/ocpp/ocppv201.py (6)
204-213: Buffered status notifications flushing is correct.The map check and deferred replay ensure deterministic updates once inventory-derived mapping is ready.
422-441: Availability per EVSE via global connector id looks sound.Mapping the global connector id to
evse_idand defaulting to CS-wide when falsy is aligned with v2.0.1 semantics.
591-599: Deferring StatusNotification until mapping exists is the right call.This prevents early writes to misindexed metrics and reduces the chance of remapping issues once inventory is processed.
821-827: Per-connector transaction tracking looks correct; timekeeping and cleanup are consistent.
- Correctly derives
global_idxfrom (evse_id, connector_id).- Starts/stops session timer and clears id_tag/transaction_id on
ended.Also applies to: 856-877
784-792: Ignore the import adjustment—cp.get_energy_kwhis correct as used
get_energy_kwhis defined on theChargePointclass with@staticmethodincustom_components/ocpp/chargepoint.py(lines 710–713), so calling it via thecpalias works as intended. No import or call-site changes are needed.Likely an incorrect or invalid review comment.
709-721: No changes needed forvalues_listextractionWe’ve confirmed that both the OCPP library and our test fixtures use snake_case for this field:
- All occurrences in tests (
tests/test_charge_point_v201_multi.py:173,tests/test_charge_point_v201.py:404) usevalues_list(novaluesListpresent).VariableCharacteristicsTypeis defined and instantiated with avalues_list=parameter, meaning incoming JSON is normalized to snake_case before our handler sees it.Since there’s no evidence of a raw
valuesListkey reaching this code path, adding a fallback for camelCase isn’t necessary.
| g = 1 | ||
| self._evse_to_global.clear() | ||
| self._global_to_evse.clear() | ||
| for evse_id in range(1, self._inventory.evse_count + 1): | ||
| count = 0 | ||
| if len(self._inventory.connector_count) >= evse_id: | ||
| count = int(self._inventory.connector_count[evse_id - 1] or 0) | ||
| for conn_id in range(1, count + 1): | ||
| self._evse_to_global[(evse_id, conn_id)] = g | ||
| self._global_to_evse[g] = (evse_id, conn_id) | ||
| g += 1 | ||
| return bool(self._evse_to_global) | ||
|
|
||
| def _ensure_connector_map(self) -> bool: | ||
| if self._evse_to_global and self._global_to_evse: | ||
| return True | ||
| return self._build_connector_map() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve existing connector indices when inventory arrives; don’t clear maps.
_build_connector_map() clears both maps and rebuilds from inventory. If any TransactionEvent/MeterValues arrived earlier (before inventory) and created provisional indices via _pair_to_global, clearing the maps will change indices and break lookups (e.g., stop_transaction scans by global_idx). Prefer completing the map without destroying existing assignments. Also, _ensure_connector_map() should opportunistically (re)build to fill missing pairs even when some mapping exists.
Apply this diff:
@@
- g = 1
- self._evse_to_global.clear()
- self._global_to_evse.clear()
+ # Preserve existing assignments; only assign indices for missing pairs.
+ g = max(self._global_to_evse.keys(), default=0) + 1
for evse_id in range(1, self._inventory.evse_count + 1):
count = 0
if len(self._inventory.connector_count) >= evse_id:
count = int(self._inventory.connector_count[evse_id - 1] or 0)
for conn_id in range(1, count + 1):
- self._evse_to_global[(evse_id, conn_id)] = g
- self._global_to_evse[g] = (evse_id, conn_id)
- g += 1
+ if (evse_id, conn_id) not in self._evse_to_global:
+ self._evse_to_global[(evse_id, conn_id)] = g
+ self._global_to_evse[g] = (evse_id, conn_id)
+ g += 1
return bool(self._evse_to_global)
@@
- if self._evse_to_global and self._global_to_evse:
- return True
- return self._build_connector_map()
+ # Always try to complete the map once inventory is available.
+ if self._inventory:
+ self._build_connector_map()
+ return bool(self._evse_to_global and self._global_to_evse)This keeps provisional indices stable and adds missing ones post-inventory, preventing mismatches between previously recorded metrics and the rebuilt map.
📝 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.
| g = 1 | |
| self._evse_to_global.clear() | |
| self._global_to_evse.clear() | |
| for evse_id in range(1, self._inventory.evse_count + 1): | |
| count = 0 | |
| if len(self._inventory.connector_count) >= evse_id: | |
| count = int(self._inventory.connector_count[evse_id - 1] or 0) | |
| for conn_id in range(1, count + 1): | |
| self._evse_to_global[(evse_id, conn_id)] = g | |
| self._global_to_evse[g] = (evse_id, conn_id) | |
| g += 1 | |
| return bool(self._evse_to_global) | |
| def _ensure_connector_map(self) -> bool: | |
| if self._evse_to_global and self._global_to_evse: | |
| return True | |
| return self._build_connector_map() | |
| def _build_connector_map(self) -> bool: | |
| # Preserve existing assignments; only assign indices for missing pairs. | |
| g = max(self._global_to_evse.keys(), default=0) + 1 | |
| for evse_id in range(1, self._inventory.evse_count + 1): | |
| count = 0 | |
| if len(self._inventory.connector_count) >= evse_id: | |
| count = int(self._inventory.connector_count[evse_id - 1] or 0) | |
| for conn_id in range(1, count + 1): | |
| if (evse_id, conn_id) not in self._evse_to_global: | |
| self._evse_to_global[(evse_id, conn_id)] = g | |
| self._global_to_evse[g] = (evse_id, conn_id) | |
| g += 1 | |
| return bool(self._evse_to_global) | |
| def _ensure_connector_map(self) -> bool: | |
| # Always try to complete the map once inventory is available. | |
| if self._inventory: | |
| self._build_connector_map() | |
| return bool(self._evse_to_global and self._global_to_evse) |
🤖 Prompt for AI Agents
In custom_components/ocpp/ocppv201.py around lines 123–140, _build_connector_map
currently clears both maps and rebuilds from inventory which loses provisional
indices; change it to preserve existing entries and only add missing
(evse_id,conn_id) -> global mappings: compute the next free global id by taking
max(self._global_to_evse.keys(), default=0)+1, do not clear self._evse_to_global
or self._global_to_evse, iterate inventory connectors and for each pair only
assign a new global id if that pair is absent and ensure the chosen global id is
unused; update _ensure_connector_map to still call _build_connector_map
opportunistically when mappings are incomplete (e.g., when some expected pairs
from inventory are missing) so missing pairs are filled without destroying
existing mappings.
| evse_target = 0 | ||
| if conn_id and conn_id > 0: | ||
| with contextlib.suppress(Exception): | ||
| evse_target, _ = self._global_to_pair(int(conn_id)) | ||
| if profile is not None: | ||
| req = call.SetChargingProfile(evse_target, profile) | ||
| resp: call_result.SetChargingProfile = await self.call(req) | ||
| if resp.status != ChargingProfileStatusEnumType.accepted: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="set_variables_error", | ||
| translation_placeholders={ | ||
| "message": f"{str(resp.status)}: {str(resp.status_info)}" | ||
| }, | ||
| ) | ||
| return |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SetChargingProfile: double-check type of resp.status; normalize comparisons.
Elsewhere you compare enum instances directly (no .value), but here and in a few other places you compare to .value. Inconsistency can cause false negatives depending on what the OCPP client returns.
Run this quick check to find and standardize status comparisons:
If resp.status is an enum instance (typical for ocpp v201 call_result dataclasses), prefer comparing to the enum, not .value. If it’s a string, use .value consistently. Consider adding a small helper for robust comparisons if different code paths return differing types.
🏁 Script executed:
#!/bin/bash
# Find comparisons against OCPP v201 status enums using either raw enum or .value
rg -nP -C2 --type=py '\bresp\.status\s*[!=]=\s*([A-Za-z_]+\.){1,4}[A-Za-z_]+(\.value)?' custom_components/ocppLength of output: 7325
Normalize resp.status comparisons in OCPP v2.0.1 code
Our grep confirms inconsistent status checks in custom_components/ocpp/ocppv201.py: some handlers compare the raw JSON string (resp.status) to enum instances, others to .value. Since the library populates resp.status from the JSON payload as a string, mixing these styles can always evaluate to false and silently break functionality. Please refactor to one consistent pattern:
• In your two SetChargingProfile blocks (around lines 363–366 and 411–415), change
if resp.status != ChargingProfileStatusEnumType.accepted:
…to
if resp.status != ChargingProfileStatusEnumType.accepted.value:
…• In request_start_transaction() (lines 455–460) and stop_transaction() (lines 474–478), replace
return resp.status == RequestStartStopStatusEnumType.accepted.valuewith either
– Direct string comparison (recommended):
return resp.status == RequestStartStopStatusEnumType.accepted.value– Or convert to enum first and compare instances:
return RequestStartStopStatusEnumType(resp.status) is RequestStartStopStatusEnumType.accepted• In reset() (lines 480–484), similarly switch
if resp.status != ResetStatusEnumType.accepted.value:
…(or convert to enum first as above).
For DRYness and clarity, consider adding a helper:
def _is_accepted(status: str, enum_type: Type[Enum]) -> bool:
return status == enum_type.accepted.valueand using it everywhere you check for “accepted.”
| """Request remote stop of current transaction (default EVSE 1).""" | ||
| await self._get_inventory() | ||
| tx_id = "" | ||
| total = self._total_connectors() or 1 | ||
| for g in range(1, total + 1): | ||
| val = self._metrics[(g, csess.transaction_id.value)].value | ||
| if val: | ||
| tx_id = val | ||
| break | ||
| if not tx_id: | ||
| _LOGGER.info("No active transaction found to stop") | ||
| return False | ||
| req: call.RequestStopTransaction = call.RequestStopTransaction( | ||
| transaction_id=self._metrics[csess.transaction_id.value].value | ||
| transaction_id=tx_id | ||
| ) | ||
| resp: call_result.RequestStopTransaction = await self.call(req) | ||
| return resp.status == RequestStartStopStatusEnumType.accepted.value |
There was a problem hiding this comment.
🛠️ Refactor suggestion
stop_transaction can miss active sessions after map rebuild; scan metrics by key.
When inventory arrives later, _build_connector_map() (current behavior) can reindex connectors. The loop for g in range(1, total + 1) then scans only the new contiguous [1..N] set and may miss transactions stored under provisional indices created pre-inventory.
Apply this diff to scan actual metric keys for transaction_id instead of assuming a contiguous index range:
- tx_id = ""
- total = self._total_connectors() or 1
- for g in range(1, total + 1):
- val = self._metrics[(g, csess.transaction_id.value)].value
- if val:
- tx_id = val
- break
+ tx_id = ""
+ # Scan existing metric keys to avoid assumptions about contiguous indices.
+ for key in list(self._metrics.keys()):
+ if isinstance(key, tuple) and len(key) == 2 and key[1] == csess.transaction_id.value:
+ val = self._metrics[key].value
+ if val:
+ tx_id = val
+ breakThis remains correct even if indices were provisionally assigned and later completed.
📝 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.
| """Request remote stop of current transaction (default EVSE 1).""" | |
| await self._get_inventory() | |
| tx_id = "" | |
| total = self._total_connectors() or 1 | |
| for g in range(1, total + 1): | |
| val = self._metrics[(g, csess.transaction_id.value)].value | |
| if val: | |
| tx_id = val | |
| break | |
| if not tx_id: | |
| _LOGGER.info("No active transaction found to stop") | |
| return False | |
| req: call.RequestStopTransaction = call.RequestStopTransaction( | |
| transaction_id=self._metrics[csess.transaction_id.value].value | |
| transaction_id=tx_id | |
| ) | |
| resp: call_result.RequestStopTransaction = await self.call(req) | |
| return resp.status == RequestStartStopStatusEnumType.accepted.value | |
| """Request remote stop of current transaction (default EVSE 1).""" | |
| await self._get_inventory() | |
| tx_id = "" | |
| # Scan existing metric keys to avoid assumptions about contiguous indices. | |
| for key in list(self._metrics.keys()): | |
| if isinstance(key, tuple) and len(key) == 2 and key[1] == csess.transaction_id.value: | |
| val = self._metrics[key].value | |
| if val: | |
| tx_id = val | |
| break | |
| if not tx_id: | |
| _LOGGER.info("No active transaction found to stop") | |
| return False | |
| req: call.RequestStopTransaction = call.RequestStopTransaction( | |
| transaction_id=tx_id | |
| ) | |
| resp: call_result.RequestStopTransaction = await self.call(req) | |
| return resp.status == RequestStartStopStatusEnumType.accepted.value |
🤖 Prompt for AI Agents
In custom_components/ocpp/ocppv201.py around lines 460-476, the code assumes
connector indices are contiguous and loops range(1, total+1), which can miss
transactions after map rebuild; instead iterate the actual self._metrics keys
and check entries whose metric name equals the transaction_id metric, using
those metric values to find tx_id. Replace the for-loop with a scan over
self._metrics.items() (or keys()) filtering on the metric name for
transaction_id, pick the first truthy .value as tx_id, then proceed to build the
RequestStopTransaction and call it as before.
|
@jthun can you look into adding some tests to make sure the new functionality is covered? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
custom_components/ocpp/switch.py (2)
124-127: Breaking change concern: connector ID in unique_id for single-connector chargers.When
flatten_singleis True (single connector), you still append the connector ID only if it's False. However, for multiple connectors, the connector ID is always appended, preserving backward compatibility for single-connector chargers as requested by the reviewer.
164-168: Preserve state when metric is unavailable.When
get_metric()returnsNone, the code still updates_statebased on the condition check. SinceNone in [...]is always False, this can cause the switch to incorrectly show as "off" when the metric is temporarily unavailable.Apply this fix to preserve the last-known state:
resp = self.central_system.get_metric( self.cpid, self.entity_description.metric_state, metric_conn ) + if resp is None: + return self._state if self.entity_description.metric_condition is not None: self._state = resp in self.entity_description.metric_conditioncustom_components/ocpp/ocppv16.py (1)
285-301: ClearChargingProfile implementation looks good!The implementation correctly passes the enum value (not the enum object) and handles both accepted and unknown statuses appropriately, addressing the previous review concern.
custom_components/ocpp/chargepoint.py (1)
803-804: Fix incorrect enum key access for meter_start.The code uses
csess.meter_startinstead ofcsess.meter_start.valuewhen accessing the metrics dictionary, which will cause the lookup to fail and ms_val to always be None.ms_val = self._metrics[ - (connector_id, csess.meter_start) + (connector_id, csess.meter_start.value) ].valuecustom_components/ocpp/ocppv201.py (6)
142-160: Ensure connector mapping remains collision-free.The
_pair_to_globalmethod now includes proper collision avoidance by allocating unique indices when needed, addressing the previous concern about EVSE collisions.
339-342: Fix enum value access in ClearChargingProfile.Good - the code correctly uses
.valueto pass the string value instead of the enum object.
82-89: Fix mutable class-level default for _connector_status.The
_connector_statusis declared at the class level, which means all ChargePoint instances will share the same list, causing cross-instance state corruption.- _connector_status: list[list[ConnectorStatusEnumType | None]] + _connector_status: list[list[ConnectorStatusEnumType | None]] # ... in __init__: self._tx_start_time = {} self._global_to_evse: dict[int, tuple[int, int]] = {} self._evse_to_global: dict[tuple[int, int], int] = {} self._pending_status_notifications: list[tuple[str, str, int, int]] = [] + self._connector_status = []
170-172: Fix list aliasing bug in _connector_status growth.Using
[[]] * ncreates n references to the same inner list, which will corrupt EVSE status tracking.if evse_id > len(self._connector_status): - needed = evse_id - len(self._connector_status) - self._connector_status.extend([[] for _ in range(needed)]) + needed = evse_id - len(self._connector_status) + self._connector_status.extend([[] for _ in range(needed)])
405-405: Fix SetChargingProfile enum value.The
charging_profile_purposefield needs the string value, not the enum object.- "charging_profile_purpose": ChargingProfilePurposeEnumType.charging_station_max_profile.value, + "charging_profile_purpose": ChargingProfilePurposeEnumType.charging_station_max_profile.value,
464-469: Consider scanning actual metric keys instead of assuming contiguous indices.The current implementation assumes connector indices are contiguous [1..N], which may miss transactions if indices were provisionally assigned before inventory arrived.
tx_id = "" -total = self._total_connectors() or 1 -for g in range(1, total + 1): - val = self._metrics[(g, csess.transaction_id.value)].value - if val: - tx_id = val - break +# Scan existing metric keys to avoid assumptions about contiguous indices +for key in list(self._metrics.keys()): + if isinstance(key, tuple) and len(key) == 2 and key[1] == csess.transaction_id.value: + val = self._metrics[key].value + if val: + tx_id = val + break
🧹 Nitpick comments (14)
tests/test_connector_aware_metrics.py (2)
8-10: Consider documenting the test helper function.The
Mhelper creates Metric instances for testing. While the implementation is straightforward, consider adding a docstring to clarify thatvandunitparameters match the Metric constructor signature.def M(v=None, unit=None): - """Help to create a Metric with a value and a None timestamp.""" + """Help to create a Metric with a value and unit for testing. + + Args: + v: The metric value (default: None) + unit: The unit of measurement (default: None) + + Returns: + Metric: A Metric instance with the specified value and unit. + """ return Metric(v, unit)
37-40: Add assertions to verify the connector mapping’s default-factory behaviorThe implementation of _ConnectorAwareMetrics indeed uses a nested defaultdict(lambda: defaultdict(lambda: Metric(None, None))) to back new connector maps. To ensure tests fully cover this, please update test_get_whole_connector_mapping_and_assign_full_mapping (right after
conn1_map = m[1]) to:• Import defaultdict at the top of the test file:
from collections import defaultdict• Replace the existing assertions:
- assert isinstance(conn1_map, dict) - assert conn1_map == {} + # It should be a defaultdict that produces new Metric(None, None) on key access + assert isinstance(conn1_map, defaultdict) + assert dict(conn1_map) == {} # still empty when viewed as a plain dict + + # Accessing an unknown measurement yields a fresh Metric with None fields + new_metric = conn1_map["Nonexistent"] + assert isinstance(new_metric, Metric) + assert new_metric.value is None + assert new_metric.unit is None + # The key has now been inserted by the default factory + assert "Nonexistent" in conn1_mapThis will explicitly verify both that m[1] is a defaultdict and that its factory auto-creates Metric(None, None).
custom_components/ocpp/ocppv16.py (1)
108-110: Consider documenting the connector ID allocation strategy.The
_profile_ids_for_connectormethod generates unique profile IDs per connector. Document why profile IDs start at 1000 + connector_id to avoid conflicts with other profiles.def _profile_ids_for_connector(self, conn_id: int) -> tuple[int, int]: - """Return (profile_id, stack_level) that is stable and unique per connector.""" + """Return (profile_id, stack_level) that is stable and unique per connector. + + Profile IDs start at 1000 to avoid conflicts with system-level profiles. + Each connector gets a unique ID: 1000 + connector_id. + """ return 1000 + max(1, int(conn_id)), 1tests/test_charge_point_v16.py (11)
651-706: Avoid race with post_connect by awaiting boot completion before per-connector trafficRight now BootNotification, status triggers, start_transaction, and meter values are fired concurrently, which can race the server’s post_connect (where it learns connector count and initializes metrics). This can intermittently drop early per-connector metrics. Make Boot → wait_ready a separate phase, then send the rest.
- async with websockets.connect( + async with websockets.connect( f"ws://127.0.0.1:{port}/{cp_id}", subprotocols=["ocpp1.5", "ocpp1.6"], ) as ws: - cp = ChargePoint(f"{cp_id}_client", ws, no_connectors=num_connectors) - - cp_task = asyncio.create_task(cp.start()) - - phase1 = [ - cp.send_boot_notification(), + cp = ChargePoint(f"{cp_id}_client", ws, no_connectors=num_connectors) + cp_task = asyncio.create_task(cp.start()) + # Phase 0: ensure the server finished post_connect & knows connector count + await cp.send_boot_notification() + await wait_ready(cs.charge_points[cp_id]) + + phase1 = [ cp.send_authorize(), cp.send_heartbeat(), cp.send_security_event(), cp.send_firmware_status(), cp.send_data_transfer(), cp.send_status_for_all_connectors(), cp.send_start_transaction(12345), ]
713-724: Exercise connector 2 as well to validate per-connector isolationYou only assert connector 1. Since the test dispatches periodic data for both connectors, add at least one assertion for connector 2 (e.g., EAIR value and unit) to prove we don’t mistakenly collapse into connector 1.
assert cs.get_unit(cpid, "Power.Reactive.Import", connector_id=1) == "var" assert cs.get_metric("unknown_cpid", "Energy.Active.Import.Register") is None + # Sanity for connector 2 path as well + assert int(cs.get_metric(cpid, "Energy.Active.Import.Register", connector_id=2)) == int(1305570 / 1000) + assert cs.get_unit(cpid, "Energy.Active.Import.Register", connector_id=2) == "kWh"
668-670: Naming consistency: use num_connectors (not no_connectors) to match config terminologyThe test module uses “num_connectors” everywhere else (config and prose). Prefer the same name here to reduce cognitive overhead.
- num_connectors = 2 + num_connectors = 2And in the ChargePoint class constructor and usages below (see separate comment) switch the parameter name to num_connectors.
760-872: Type check uses PEP 604 union in isinstance — this always falls into except
isinstance(original_features, set | list | tuple)raisesTypeErrorbecauseisinstanceexpects a type or tuple of types, not atypes.UnionType. You then fall back to 0 via the except, losing the intent to preserve container type. Use a tuple of types.- if isinstance(original_features, set | list | tuple): + if isinstance(original_features, (set, list, tuple)): new_val = tp() # empty same container type else: new_val = 0 # fall back to "no features"
1339-1399: Minor cleanups in monitor_connection timeout test
_NeverFinishesis defined twice; keep one.callsdict is initialized twice; keep one.
These reduce noise and make the intention clearer.- calls = {"n": 0} - - class _NeverFinishes: - def __await__(self): - fut = asyncio.get_event_loop().create_future() - return fut.__await__() - - calls = {"n": 0} + calls = {"n": 0} + + class _NeverFinishes: + def __await__(self): + fut = asyncio.get_event_loop().create_future() + return fut.__await__() async def fake_wait_for(awaitable, timeout): calls["n"] += 1 if inspect.iscoroutine(awaitable): awaitable.close() if calls["n"] == 1: - - class _NeverFinishes: - def __await__(self): - fut = asyncio.get_event_loop().create_future() - return fut.__await__() - return _NeverFinishes() raise TimeoutError
1711-1780: Drop redundant local re-import of MetricYou already import
Metric as Mat module scope. The re-import inside the test adds clutter and can confuse readers.- from custom_components.ocpp.chargepoint import Metric as M - meas = "Energy.Active.Import.Register"
1785-1797: Align constructor/attribute naming with config: num_connectorsThe test harness uses
no_connectorswhile config/tests talk aboutnum_connectors. Unify onnum_connectorsto reduce confusion and grep friction.- def __init__(self, id, connection, response_timeout=30, no_connectors=1): + def __init__(self, id, connection, response_timeout=30, num_connectors=1): """Init extra variables for testing.""" super().__init__(id, connection) - self.no_connectors = int(no_connectors) + self.num_connectors = int(num_connectors)And update references:
- {"key": key[0], "readonly": False, "value": f"{self.no_connectors}"} + {"key": key[0], "readonly": False, "value": f"{self.num_connectors}"}- for cid in range(0, max(1, self.no_connectors) + 1): + for cid in range(0, max(1, self.num_connectors) + 1):- cp = ChargePoint(f"{cp_id}_client", ws, no_connectors=num_connectors) + cp = ChargePoint(f"{cp_id}_client", ws, num_connectors=num_connectors)
1960-1988: Swallowing all exceptions hides legitimate issues in trigger_message handlerThe bare
except Exception: passcan mask test bugs. Either restrict to import errors or at least log the exception at DEBUG.- except Exception: - pass + except ImportError: + # MessageTrigger not available; skip background scheduling in this environment + pass + except Exception as e: + logging.debug("trigger_message scheduling failed: %r", e)
2070-2095: Spec nicety: avoid sending transaction_id=0 in clock samplesFor “clock” samples, OCPP 1.6 allows omitting transactionId when not part of a session. You currently send
transaction_id=0. It works with our server, but to be closer to the spec, skip the field whenactive_transactionId == 0.- request = call.StatusNotification( + request = call.StatusNotification( connector_id=connector_id, error_code=ChargePointErrorCode.no_error, status=status, timestamp=datetime.now(tz=UTC).isoformat(), info="Test info", vendor_id="The Mobility House", vendor_error_code="Test error", ) await self.call(request)And in send_meter_clock_data:
- request = call.MeterValues( - connector_id=connector_id, - transaction_id=self.active_transactionId, + kwargs = {"connector_id": connector_id} + if self.active_transactionId: + kwargs["transaction_id"] = self.active_transactionId + request = call.MeterValues( + **kwargs, meter_value=[ { "timestamp": "2021-06-21T16:15:09Z", "sampledValue": [
15-15: Metric alias M shadows tests’ helper name in another moduleUsing
from custom_components.ocpp.chargepoint import Metric as Mis fine here, but it shares a name with the helpertests/test_connector_aware_metrics.M. Not a bug, just a note. If both get imported into the same scope in future, prefer a distinct alias likeHA_Metric.
190-209: Wire upnum_connectorsin thesetup_config_entryfixtureIntegration reads
num_connectorsfrom each charger’s settings underentry.data[CONF_CPIDS], so we should honor the"num_connectors"value passed viarequest.paramwhen building the mock config entry.Locations to update:
- In
tests/test_charge_point_v16.py, inside thesetup_config_entryfixture (around lines 190–209), modify how you append toconfig_data[CONF_CPIDS]. For example:- config_data[CONF_CPIDS].append( - { request.param["cp_id"]: MOCK_CONFIG_CP_APPEND.copy() } - ) + # Build per-CP settings and inject num_connectors from test params + cp_settings = MOCK_CONFIG_CP_APPEND.copy() + cp_settings[CONF_NUM_CONNECTORS] = request.param.get("num_connectors", DEFAULT_NUM_CONNECTORS) + config_data[CONF_CPIDS].append( + { request.param["cp_id"]: cp_settings } + )This ensures that any downstream logic in
switch.py,sensor.py,number.py, orbutton.py—which all readcp_settings.get(CONF_NUM_CONNECTORS, 1)—will see the correct number of connectors in your v1.6 tests.Let me know if you’d like me to open a follow-up patch to implement this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
custom_components/ocpp/chargepoint.py(16 hunks)custom_components/ocpp/ocppv16.py(15 hunks)custom_components/ocpp/ocppv201.py(14 hunks)custom_components/ocpp/switch.py(4 hunks)tests/test_charge_point_v16.py(12 hunks)tests/test_connector_aware_metrics.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_connector_aware_metrics.py (1)
custom_components/ocpp/chargepoint.py (11)
_ConnectorAwareMetrics(118-195)Metric(73-115)unit(93-95)unit(98-100)value(83-85)value(88-90)keys(177-178)values(180-181)items(183-184)clear(186-187)get(171-175)
custom_components/ocpp/switch.py (4)
custom_components/ocpp/api.py (4)
CentralSystem(98-655)get_available(472-513)get_metric(303-343)set_charger_state(537-564)custom_components/ocpp/enums.py (1)
HAChargerStatuses(25-38)custom_components/ocpp/button.py (2)
available(135-137)async_setup_entry(49-94)custom_components/ocpp/number.py (2)
available(178-185)async_setup_entry(56-117)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/chargepoint.py (10)
ChargePoint(225-894)get(171-175)value(83-85)value(88-90)get_number_of_connectors(290-292)values(180-181)get_energy_kwh(713-717)unit(93-95)unit(98-100)process_measurands(719-819)
custom_components/ocpp/ocppv16.py (2)
custom_components/ocpp/chargepoint.py (24)
get(171-175)get_number_of_connectors(290-292)value(83-85)value(88-90)trigger_custom_message(366-371)clear_profile(373-375)notify_ha(883-894)set_charge_rate(377-385)set_availability(387-389)start_transaction(391-393)values(180-181)get_diagnostics(419-421)extra_attr(108-110)extra_attr(113-115)get_ha_metric(842-857)MeasurandValue(214-222)unit(93-95)unit(98-100)get_energy_kwh(713-717)process_measurands(719-819)update(571-602)stop_transaction(395-401)items(183-184)data_transfer(423-425)custom_components/ocpp/sensor.py (1)
ChargePointMetric(85-222)
tests/test_charge_point_v16.py (5)
custom_components/ocpp/chargepoint.py (19)
Metric(73-115)ChargePoint(225-894)start(503-505)get_metric(826-840)get_unit(875-877)get_ha_unit(879-881)get_extra_attr(859-873)get_supported_features(306-308)value(83-85)value(88-90)unit(93-95)unit(98-100)get(171-175)extra_attr(108-110)extra_attr(113-115)monitor_connection(447-494)update_firmware(411-417)get_diagnostics(419-421)data_transfer(423-425)tests/test_connector_aware_metrics.py (1)
M(8-10)custom_components/ocpp/ocppv16.py (5)
ChargePoint(83-992)get_supported_features(186-223)update_firmware(487-519)get_diagnostics(521-540)data_transfer(542-564)custom_components/ocpp/api.py (7)
get_metric(303-343)get_unit(361-396)get_ha_unit(398-433)get_extra_attr(435-470)get_supported_features(515-522)set_max_charge_rate_amps(524-535)set_charger_state(537-564)tests/charge_point_test.py (1)
wait_ready(91-94)
custom_components/ocpp/chargepoint.py (4)
custom_components/ocpp/ocppv16.py (3)
get_number_of_connectors(112-119)start_transaction(424-438)ChargePoint(83-992)custom_components/ocpp/ocppv201.py (3)
get_number_of_connectors(250-253)start_transaction(443-458)ChargePoint(78-882)custom_components/ocpp/api.py (4)
get_metric(303-343)get_extra_attr(435-470)get_unit(361-396)get_ha_unit(398-433)custom_components/ocpp/sensor.py (1)
ChargePointMetric(85-222)
🔇 Additional comments (21)
tests/test_connector_aware_metrics.py (4)
1-10: LGTM!Proper test structure and imports for testing the connector-aware metrics functionality.
13-30: Excellent test coverage for flat access semantics!The test thoroughly validates that flat access (connector 0) is independent from per-connector access, ensuring backward compatibility. The assertions correctly verify that the flat view's length and keys remain unaffected by per-connector metrics.
86-111: Comprehensive type enforcement tests!Excellent coverage of type validation for all three access patterns (flat, per-connector tuple, and connector dict). The test ensures type safety is maintained throughout the mapping interface.
223-240: Well-designed test for plain dict behavior!This test effectively validates that when a connector's inner mapping is replaced with a plain dict (not defaultdict), the
get()method correctly returns the provided default without inserting anything. This is the expected behavior that should apply to all cases.custom_components/ocpp/switch.py (2)
31-33: Good update to support multiple metric conditions!Changing
metric_conditionfrom a single string to a list allows switches to respond to multiple acceptable states, which is essential for connector status handling.
43-49: Verify inclusion of all “active” charging statesThe OCPP 1.6
ChargePointStatusenum actually defines the following members (name → value):
- available → “Available”
- preparing → “Preparing”
- charging → “Charging”
- suspended_evse → “SuspendedEVSE”
- suspended_ev → “SuspendedEV”
- finishing → “Finishing”
- reserved → “Reserved”
- unavailable → “Unavailable”
- faulted → “Faulted”
(verified against ocpp v2.1.0)Currently, in
custom_components/ocpp/switch.py(lines 43–49), you only include:metric_condition=[ ChargePointStatus.charging.value, ChargePointStatus.suspended_evse.value, ChargePointStatus.suspended_ev.value, ],• If your use case treats the “Finishing” state (handshake after charging) as active, add
ChargePointStatus.finishing.value.
• If “Preparing” (readying the session but not yet drawing current) should count as “on” for your metrics, includeChargePointStatus.preparing.value.Please confirm which of these additional states—
Finishingand/orPreparing—should be considered “charging on” and updatemetric_conditionaccordingly.custom_components/ocpp/ocppv16.py (3)
68-81: Good addition of the message trigger helper!The
_to_message_triggerfunction provides a robust way to handle both string and MessageTrigger inputs, with proper normalization and mapping.
718-724: Good energy normalization in meter values!The code correctly converts EAIR (Energy.Active.Import.Register) values to kWh and stores them in the aggregated connector 0 metrics when no transaction ID is present. This maintains backward compatibility while supporting per-connector metrics.
868-870: Robust transaction ID generation.Using Unix timestamp for transaction IDs is a good approach, providing unique, monotonically increasing IDs. The per-connector tracking in
_active_txdictionary properly manages multiple simultaneous transactions.custom_components/ocpp/chargepoint.py (3)
118-196: Well-designed connector-aware metrics implementation!The
_ConnectorAwareMetricsclass provides excellent backward compatibility with flat access while supporting per-connector metrics. The type checking in__setitem__ensures data integrity.
322-329: Good initialization of per-connector metrics!The post_connect method properly initializes metric scaffolding for all connectors (1 to N), ensuring that per-connector metrics are ready before any data arrives.
571-602: Efficient hierarchical device update!The update method efficiently traverses both the root device and all child connector devices, ensuring all entities are updated when metrics change. The use of
async_dispatcher_sendproperly notifies listeners.tests/test_charge_point_v16.py (9)
882-931: Clear profile happy path test looks goodCapturing
last_clear_profile_kwargson the fake CP and asserting it’s a dict is adequate for the current handler. No issues spotted.
932-1103: Robust coverage for stop_transaction branchesThe three scenarios (compute delta, EAIR→kWh, kWh passthrough, and “do not overwrite”) thoroughly exercise the server code paths and the post-stop zeroing. Nicely structured.
1104-1211: Good targeted on_meter_values path coverageThe crafted MeterValues with empty strings and Wh→kWh conversion asserts the coercion and unit normalization behavior. Looks solid.
1213-1329: Restore paths test is precise and self-containedMonkeypatching
get_ha_metricto drive both restore branches and then verifying aggregate EAIR attrs is a clean approach. No issues.
1478-1513: Firmware update happy path assertions are on pointAsserting the client received UpdateFirmware with a correctly formatted
retrieve_dateguards exactly what can regress. LGTM.
1529-1564: Invalid URL branch is correctly validated via caplogThe test ensures server-side validation rejects bad URLs and doesn’t attempt the RPC. Looks good.
1566-1610: RPC failure path is well coveredMonkeypatching
call()to raise ensures the error logging and False return are exercised. No issues.
1659-1709: Unit fallback test is preciseSeeding (0) and (1) with no units and (2) with “kW” mirrors the API fallback order and proves connector ≥2 fallback works. LGTM.
743-881: Diagnostics/DataTransfer test is comprehensiveCaptures payloads on the fake CP, asserts Accepted/Rejected paths, and checks invalid-URL logging and FW-profile-not-supported branch. Nicely done.
jthun
had a problem deploying
to
continuous-integration
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_charge_point_v16.py (1)
1708-1717: Fix isinstance() check using PEP 604 unions (TypeError) and remove broad try/exceptUsing isinstance(original_features, set | list | tuple) is invalid; isinstance expects a class or a tuple of classes. The try/except masks the problem and can hide real errors.
Apply:
- # Try to blank out features regardless of type (set/list/tuple/int) - try: - tp = type(original_features) - if isinstance(original_features, set | list | tuple): - new_val = tp() # empty same container type - else: - new_val = 0 # fall back to "no features" - setattr(cpobj, "_attr_supported_features", new_val) - except Exception: - setattr(cpobj, "_attr_supported_features", 0) + # Try to blank out features regardless of type (set/list/tuple/int) + tp = type(original_features) + if isinstance(original_features, (set, list, tuple)): + new_val = tp() # empty same container type + else: + new_val = 0 # fall back to "no features" + setattr(cpobj, "_attr_supported_features", new_val)
🧹 Nitpick comments (6)
tests/test_charge_point_v16.py (6)
383-404: Prefer broadcasting via a dedicated helper to respect the configured connector countsend_status_notification() currently hardcodes notifications for connectors 0, 1, and 2. In tests that advertise a single connector (default) this sends an extra, potentially misleading notification for connector 2. Use the new broadcast helper instead so we only notify 0..no_connectors.
Apply:
@@ - await cp.send_status_notification() + await cp.send_status_for_all_connectors()And likewise in the multi-connector test:
@@ - await cp.send_status_notification() + await cp.send_status_for_all_connectors()Also applies to: 686-691
2191-2193: Bound the busy-wait loops to reduce CI flakinessThese helpers spin until a transaction starts with an unbounded wait. If a prior step fails, tests will hang until the pytest timeout. Cap the retries (as you already did in send_meter_periodic_data) so failures surface quickly.
@@ async def send_meter_line_voltage(self, connector_id: int = 1): - while self.active_transactionId == 0: - await asyncio.sleep(1) + n = 0 + while self.active_transactionId == 0 and n < 3: + await asyncio.sleep(1) + n += 1 @@ async def send_meter_err_phases(self, connector_id: int = 1): - while self.active_transactionId == 0: - await asyncio.sleep(1) + n = 0 + while self.active_transactionId == 0 and n < 3: + await asyncio.sleep(1) + n += 1 @@ async def send_meter_energy_kwh(self, connector_id: int = 1): - while self.active_transactionId == 0: - await asyncio.sleep(1) + n = 0 + while self.active_transactionId == 0 and n < 3: + await asyncio.sleep(1) + n += 1 @@ async def send_main_meter_clock_data(self, connector_id: int = 1): - while self.active_transactionId == 0: - await asyncio.sleep(1) + n = 0 + while self.active_transactionId == 0 and n < 3: + await asyncio.sleep(1) + n += 1If CI instability persists, I can provide a small “eventually(assert_fn)” helper for these tests.
Also applies to: 2233-2235, 2267-2269, 2292-2294
772-773: Avoid magic sleeps; await the HA loop insteadThe 50 ms sleeps can be tight on CI and may cause flakes. Since the service calls use blocking=True, prefer awaiting Home Assistant’s loop drain.
- await asyncio.sleep(0.05) + await hass.async_block_till_done()Same change applies to the later sleep after DataTransfer.
Also applies to: 1669-1670
1556-1577: Redundant re-import of Metric as M - use the module-level importThere’s already a module-level import of Metric as M. Re-importing inside the function is unnecessary.
- from custom_components.ocpp.chargepoint import Metric as M
1741-1745: Constructor parameter response_timeout is unusedThe response_timeout parameter is accepted but not used. Consider dropping it to avoid confusion, unless you plan to wire it in later.
- def __init__(self, id, connection, response_timeout=30, no_connectors=1): + def __init__(self, id, connection, no_connectors=1):
2004-2037: Optional: align send_status_notification semantics with per-connector helpersRight now send_status_notification() always sends three notifications (0/1/2). Consider evolving it to accept an optional connector_id and delegate broadcasting to send_status_for_all_connectors(). This keeps the old behavior but removes the implicit assumption “2 connectors exist”.
If you choose to proceed:
- async def send_status_notification(self): - """Send a status notification.""" + async def send_status_notification(self, connector_id: int | None = None): + """Send a status notification. If connector_id is None, broadcast to all.""" + if connector_id is None: + return await self.send_status_for_all_connectors() request = call.StatusNotification( - connector_id=0, + connector_id=connector_id, error_code=ChargePointErrorCode.no_error, - status=ChargePointStatus.suspended_ev, + status=ChargePointStatus.suspended_ev if connector_id == 0 else ChargePointStatus.charging, timestamp=datetime.now(tz=UTC).isoformat(), info="Test info", vendor_id="The Mobility House", vendor_error_code="Test error", ) resp = await self.call(request) - request = call.StatusNotification( - connector_id=1, - ... - ) - resp = await self.call(request) - request = call.StatusNotification( - connector_id=2, - ... - ) - resp = await self.call(request) assert resp is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/test_charge_point_v16.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_charge_point_v16.py (2)
custom_components/ocpp/ocppv16.py (2)
ChargePoint(65-811)on_status_notification(667-700)tests/charge_point_test.py (1)
run_charge_point_test(107-134)
🔇 Additional comments (3)
tests/test_charge_point_v16.py (3)
1777-1779: Dynamic number_of_connectors wired correctlyGood change: returning self.no_connectors via GetConfiguration mirrors the new multi-connector configuration.
1908-1910: Capturing ClearChargingProfile kwargs for assertions is solidStoring the kwargs enables straightforward, deterministic assertions in tests. Nicely done.
1925-1944: Persisting last_ fields (firmware/diagnostics/data) improves observability in tests*These hooks make the tests precise without brittle log scraping. Approve.
| async def send_status_for_all_connectors(self): | ||
| """Send StatusNotification for all connectors.""" | ||
| await self.send_status_notification() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement send_status_for_all_connectors() to iterate 0..no_connectors
The current implementation simply calls send_status_notification(), which still emits a fixed set (0,1,2). Make this helper drive the exact range based on self.no_connectors to avoid accidental over-reporting when no_connectors != 2.
- async def send_status_for_all_connectors(self):
- """Send StatusNotification for all connectors."""
- await self.send_status_notification()
+ async def send_status_for_all_connectors(self):
+ """Send StatusNotification for CP-level (0) and all physical connectors."""
+ # CP level (0): suspended_ev is fine for “ready but not charging”
+ req0 = call.StatusNotification(
+ connector_id=0,
+ error_code=ChargePointErrorCode.no_error,
+ status=ChargePointStatus.suspended_ev,
+ timestamp=datetime.now(tz=UTC).isoformat(),
+ info="Test info",
+ vendor_id="The Mobility House",
+ vendor_error_code="Test error",
+ )
+ await self.call(req0)
+ # Physical connectors: pick a reasonable default status
+ for cid in range(1, self.no_connectors + 1):
+ status = (
+ ChargePointStatus.charging if cid == 1 else ChargePointStatus.available
+ )
+ req = call.StatusNotification(
+ connector_id=cid,
+ error_code=ChargePointErrorCode.no_error,
+ status=status,
+ timestamp=datetime.now(tz=UTC).isoformat(),
+ info="Test info",
+ vendor_id="The Mobility House",
+ vendor_error_code="Test info",
+ )
+ await self.call(req)📝 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.
| async def send_status_for_all_connectors(self): | |
| """Send StatusNotification for all connectors.""" | |
| await self.send_status_notification() | |
| async def send_status_for_all_connectors(self): | |
| """Send StatusNotification for CP-level (0) and all physical connectors.""" | |
| # CP level (0): suspended_ev is fine for “ready but not charging” | |
| req0 = call.StatusNotification( | |
| connector_id=0, | |
| error_code=ChargePointErrorCode.no_error, | |
| status=ChargePointStatus.suspended_ev, | |
| timestamp=datetime.now(tz=UTC).isoformat(), | |
| info="Test info", | |
| vendor_id="The Mobility House", | |
| vendor_error_code="Test error", | |
| ) | |
| await self.call(req0) | |
| # Physical connectors: pick a reasonable default status | |
| for cid in range(1, self.no_connectors + 1): | |
| status = ( | |
| ChargePointStatus.charging if cid == 1 else ChargePointStatus.available | |
| ) | |
| req = call.StatusNotification( | |
| connector_id=cid, | |
| error_code=ChargePointErrorCode.no_error, | |
| status=status, | |
| timestamp=datetime.now(tz=UTC).isoformat(), | |
| info="Test info", | |
| vendor_id="The Mobility House", | |
| vendor_error_code="Test info", | |
| ) | |
| await self.call(req) |
🤖 Prompt for AI Agents
In tests/test_charge_point_v16.py around lines 2039-2042, the helper
send_status_for_all_connectors currently calls send_status_notification() with a
fixed set; change it to iterate connector IDs from 0 through self.no_connectors
(inclusive) and call await self.send_status_notification(connector_id=connector)
for each ID so the emitted StatusNotification matches the configured number of
connectors; ensure you use range(self.no_connectors + 1) and await each call.
At least the tests passed now. I'll write a few more to get better coverage. |
Hi @jthun it would be good to have a bit more of a description on the logic you have used in adding extra connectors to the codebase. For example in some functions the default connector is 0 in others 1. I assume 0 is used to refer to the EVSE itself where a function could relate to the EVSE or a specific connector? |
| self._attr_icon = ICON | ||
| self._attr_native_unit_of_measurement = None | ||
|
|
||
| @property | ||
| def available(self) -> bool: | ||
| """Return if sensor is available.""" | ||
| return self.central_system.get_available(self.cpid) | ||
| return self.central_system.get_available(self.cpid, self.connector_id) |
There was a problem hiding this comment.
A number of sensors are showing as unavailable? Eg
There was a problem hiding this comment.
Hi @jthun, looks like you're making good progress with this. However, the above (unavailable sensors) is still an issue on your latest commit.
There was a problem hiding this comment.
I believe I removed those three in a later commit. Are they still there if you re-install from the latest?
There was a problem hiding this comment.
What do you mean by remove? Why would the integration no longer provide these sensors?
There was a problem hiding this comment.
I removed them from the charger level, but forgot to add them to the connector level again. Fixed now locally, so please check that this works after next commit.
| alphabet = string.ascii_uppercase + string.digits | ||
| self._remote_id_tag = "".join(secrets.choice(alphabet) for i in range(20)) | ||
| self.num_connectors: int = 1 |
There was a problem hiding this comment.
Why not initialise with user configured number of connectors? eg self.settings.num_connectors
There was a problem hiding this comment.
The num_connectors currently gets set in post_connect and the logic is OCPP version dependent, so I just set it to 1 in init.
Ideally, the charger should self-report the number of connectors, but I couldn't get that to work properly since all entity platforms (switch/button/number/sensor) are created before that information is available. My hope is that someone can point me in the right direction for this, and then the integration can get rid of the user num_connector setting.
There was a problem hiding this comment.
See the lines from
ocpp/custom_components/ocpp/chargepoint.py
Line 238 in e4d587c
If the number of connectors differs from 1 then you need to update the config_entry with the new value. This will then cause HA to reload the integration (with extra sensors for extra connectors) causing post_connect to run again. So need to make sure there's logic so it doesn't keep looping.
Once confirmed this works you can avoid asking for num connectors at setup and simply default to 1 in the config entry.
custom_components/ocpp/sensor.py
Outdated
| DATA_UPDATED, | ||
| DEFAULT_CLASS_UNITS_HA, | ||
| DOMAIN, | ||
| ICON, | ||
| Measurand, | ||
| ) | ||
| from .enums import HAChargerDetails, HAChargerSession, HAChargerStatuses | ||
| from .enums import HAChargerDetails, HAChargerStatuses |
There was a problem hiding this comment.
HAChargerSession sensor metrics missing?
There was a problem hiding this comment.
I accidently removed them from the connector(s) when I removed them from the charger level... Back now, so they will be back after next commit.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Also |
|
Caution CodeRabbit failed during planning: Script execution failed: Command failed with exit code 127: jk_chrootlaunch --jail /inmem/6545/436acbe5-90d3-41bf-ada0-8a8152082198 --group jailuser --user jailuser --exec /usr/bin/env -- '/bin/bash --norc --noprofile -c "cd '''/home/jailuser/git''' && bash -x /.coderabbit_commands_7e3b1f5d-9303-4094-8a1c-bd0359bd5613.sh"'
|
Hi! I've tried to understand the OCPP spec and use the relevant connector_id for the various usages. As you say, sometimes 0 is used when targeting the EVSE level, and other times 1 or the actual connector. Here's also one of my big unknowns - how does single-connectors handle this? Are they consistent in using 1 for connector-related stuff and 0 for the EVSE level? I don't have access to any other chargers, so I can't test with a physical unit. Some cases are a bit more complex, like set_max_charge_rate_amps. There 0 can/should be used to set EVSE-level profiles, while the actual connector can be used to set connector/tx profiles. Here's an overview of the various calls and their usages of connectorId from OCPP 1.6: 
|
…s. Clear charger-level entities after connector creation. Add num_connector changes to reload logic. Remove not used functions from chargepoint.py. Revert to original config flow due to automatic discovery of number of connectors. Add more tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/sensor.py (1)
272-300: Fix device_class checks using Measurand Enum values.self.metric is a string; comparing against Enum members will fail. Use .value or string prefixes.
Apply:
- elif self.metric in [ - Measurand.frequency, - Measurand.rpm, - ] or self.metric.lower().startswith("frequency"): + elif self.metric in [ + getattr(Measurand, "frequency", None) and getattr(Measurand, "frequency").value, + getattr(Measurand, "rpm", None) and getattr(Measurand, "rpm").value, + ] or self.metric.lower().startswith("frequency"): device_class = SensorDeviceClass.FREQUENCYAlternatively, drop the Enum comparison entirely and rely on the string prefixes:
- elif self.metric in [ - Measurand.frequency, - Measurand.rpm, - ] or self.metric.lower().startswith("frequency"): + elif self.metric.lower().startswith(("frequency", "rpm")): device_class = SensorDeviceClass.FREQUENCY
♻️ Duplicate comments (4)
custom_components/ocpp/switch.py (2)
153-170: Single-connector stability preserved (no conn1 suffix/name) — niceunique_id/name/device_info now avoid “conn1” when flattening single-connector setups, addressing the breaking-change risk.
188-205: Don’t overwrite state when metric is missing (preserve default/last-known)If get_metric returns None, current code flips state to False, causing startup flicker.
- if self.entity_description.metric_condition is not None: - self._state = resp in self.entity_description.metric_condition - else: - self._state = bool(resp) + if resp is None: + return self._state + if self.entity_description.metric_condition is not None: + self._state = resp in self.entity_description.metric_condition + else: + self._state = bool(resp)custom_components/ocpp/number.py (1)
218-223: Fix applied: numbers now target the intended connectorasync_set_native_value uses the operational connector id, avoiding the EVSE 0 bug for multi-connector sliders.
custom_components/ocpp/button.py (1)
66-77: Normalize num_connectors and avoid duplicated lookup logic.
- Treat 0 or missing as 1 for consistency with other platforms and to avoid odd edge cases.
- Consider factoring this repeated CPID→num_connectors lookup into a shared helper.
Apply:
- num_connectors = int( - cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) - ) + num_connectors = int( + cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) or 1 + )
🧹 Nitpick comments (14)
tests/test_api_paths.py (2)
206-214: Avoid brittle private-attribute hacks in testsSetting Metric.dict["_ha_unit"] won’t affect the ha_unit property (it derives from unit). Drop these lines; rely on unit values for assertions.
Apply:
- cp._metrics[(3, meas)].__dict__["_ha_unit"] = "W" cp._metrics[(3, meas)].extra_attr = {"ctx": "Sample.Periodic"}
216-221: Same here: remove unused _ha_unit injectionThis write is a no-op and can mislead future readers.
Apply:
cp._metrics[(1, meas)] = M(11.0, "kW") - cp._metrics[(3, meas)].__dict__["_ha_unit"] = "kW" cp._metrics[(1, meas)].extra_attr = {"src": "conn1"}custom_components/ocpp/switch.py (2)
83-95: Simplify connector count lookup (remove O(n^2) scan)You already have cp_settings; use it directly.
- num_connectors = 1 - for item in entry.data.get(CONF_CPIDS, []): - for _, cfg in item.items(): - if cfg.get(CONF_CPID) == cpid: - num_connectors = int( - cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) - ) - break - else: - continue - break - flatten_single = num_connectors == 1 + num_connectors = int(cp_settings.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS)) + flatten_single = num_connectors == 1
170-175: Consider not setting entity_id explicitlyHome Assistant prefers deriving entity_id from unique_id/name and letting users rename. Manually setting entity_id can fight the registry and user overrides.
tests/test_sensor.py (2)
53-55: Replace sleep with deterministic sync to reduce flakinessUse HA helpers to wait for setup instead of arbitrary delays.
- # Wait for setup to complete - await asyncio.sleep(1) + # Wait for setup to complete + await hass.async_block_till_done()
100-102: Same: avoid arbitrary delay in multi-connector testDeterministic wait improves CI stability.
- # Give HA a tick to register entities - await asyncio.sleep(0.5) + await hass.async_block_till_done()tests/test_charge_point_core.py (1)
131-132: Unnecessary component setupdevice_registry access doesn’t require device_tracker. You can drop this to speed tests.
- await async_setup_component(hass, "device_tracker", {})custom_components/ocpp/number.py (1)
68-79: Simplify connector count lookup (mirror switch.py)Use cp_id_settings directly; avoid scanning all CPIDs.
- num_connectors = 1 - for item in entry.data.get(CONF_CPIDS, []): - for _, cfg in item.items(): - if cfg.get(CONF_CPID) == cpid: - num_connectors = int( - cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) - ) - break - else: - continue - break + num_connectors = int(cp_id_settings.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS))custom_components/ocpp/sensor.py (1)
54-65: Normalize num_connectors and de-duplicate the lookup.
- Use “or 1” to guard against 0/missing.
- Extract the CPID→num_connectors resolution into a shared helper to keep platforms in sync.
Apply:
- num_connectors = int( - cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) - ) + num_connectors = int( + cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) or 1 + )custom_components/ocpp/ocppv16.py (3)
335-434: SetChargingProfile improvements are solid; one nit.
- Station-level vs TxDefault split and retry-on-stack-level are good.
- Nit: limit_watts branch uses watts (as expected). Consider documenting expectations (W vs kW) for callers.
519-552: Firmware update: input validation is good; minor robustness nit.Consider logging the validated URL and return False on non-accepted status explicitly (currently any non-exception returns True).
Apply:
- resp = await self.call(req) - _LOGGER.info("UpdateFirmware response: %s", resp) - return True + resp = await self.call(req) + _LOGGER.info("UpdateFirmware response: %s", resp) + return bool(resp)
677-809: MeterValues handling is connector-aware and careful; two small suggestions.
- Converting unparsable values to 0 may hide data issues; consider skipping such samples.
- Session time derivation based on tx_id-as-epoch is clever; ensure tx_id is always epoch-derived.
Apply:
- try: - v = float(v) - except (ValueError, TypeError): - v = 0.0 + try: + v = float(v) + except (ValueError, TypeError): + continue # skip invalid samplescustom_components/ocpp/chargepoint.py (2)
323-348: post_connect updates config_entry; guard against reload loops.Looks safe due to equality check. Consider short-circuiting async_update_entry when nothing changed.
Apply:
- self.hass.config_entries.async_update_entry(self.entry, data=updated_entry) + if updated_entry != self.entry.data: + self.hass.config_entries.async_update_entry(self.entry, data=updated_entry)
453-486: Connection monitor and update() dispatcher: pragmatic approach; note perf.
- Latency metrics units set upfront; good.
- update() BFS over devices is fine but could be heavy on large registries; consider throttling or targeting only impacted devices.
Also applies to: 546-549, 577-609
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
custom_components/ocpp/button.py(3 hunks)custom_components/ocpp/chargepoint.py(16 hunks)custom_components/ocpp/number.py(5 hunks)custom_components/ocpp/ocppv16.py(15 hunks)custom_components/ocpp/sensor.py(6 hunks)custom_components/ocpp/switch.py(3 hunks)tests/test_api_paths.py(1 hunks)tests/test_charge_point_core.py(1 hunks)tests/test_sensor.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
tests/test_charge_point_core.py (3)
custom_components/ocpp/chargepoint.py (16)
ChargePoint(225-860)OcppVersion(198-203)Metric(74-116)_ConnectorAwareMetrics(119-195)MeasurandValue(214-222)value(84-86)value(89-91)_get_specific_response(441-451)async_update_device_info(550-569)get_ha_metric(832-847)unit(94-96)unit(99-101)process_phases(641-716)process_measurands(725-825)_handle_call(502-507)get(172-175)custom_components/ocpp/const.py (2)
CentralSystemSettings(160-180)ChargerSystemSettings(144-156)custom_components/ocpp/enums.py (2)
HAChargerDetails(41-53)HAChargerSession(56-62)
tests/test_api_paths.py (3)
custom_components/ocpp/api.py (18)
select_subprotocol(207-226)get_metric(303-343)get_unit(361-396)get_ha_unit(398-433)get_extra_attr(435-470)get_available(472-513)device_info(566-570)set_max_charge_rate_amps(524-535)set_charger_state(537-564)handle_clear_profile(600-602)handle_trigger_custom_message(594-597)handle_update_firmware(605-609)handle_get_diagnostics(612-615)handle_data_transfer(618-623)handle_set_charge_rate(626-640)handle_configure(643-648)handle_get_configuration(651-655)del_metric(345-359)custom_components/ocpp/chargepoint.py (22)
Metric(74-116)SetVariableResult(206-210)set_charge_rate(383-391)set_availability(393-395)start_transaction(397-399)stop_transaction(401-407)reset(409-411)unlock(413-415)trigger_custom_message(372-377)clear_profile(379-381)update_firmware(417-423)get_diagnostics(425-427)data_transfer(429-431)configure(437-439)value(84-86)value(89-91)get_configuration(433-435)get(172-175)unit(94-96)unit(99-101)extra_attr(109-111)extra_attr(114-116)tests/test_connector_aware_metrics.py (1)
M(8-10)
tests/test_sensor.py (1)
tests/charge_point_test.py (2)
remove_configuration(80-85)create_configuration(66-74)
custom_components/ocpp/button.py (5)
custom_components/ocpp/api.py (3)
CentralSystem(98-655)get_available(472-513)set_charger_state(537-564)custom_components/ocpp/sensor.py (2)
async_setup_entry(43-175)available(227-229)custom_components/ocpp/number.py (2)
async_setup_entry(58-140)available(206-213)custom_components/ocpp/switch.py (2)
async_setup_entry(73-129)available(177-182)custom_components/ocpp/__init__.py (1)
async_setup_entry(98-135)
custom_components/ocpp/number.py (3)
custom_components/ocpp/ocppv201.py (1)
get_number_of_connectors(131-134)custom_components/ocpp/api.py (2)
get_available(472-513)set_max_charge_rate_amps(524-535)custom_components/ocpp/enums.py (1)
Profiles(65-74)
custom_components/ocpp/sensor.py (6)
custom_components/ocpp/api.py (4)
get_available(472-513)get_extra_attr(435-470)get_metric(303-343)get_ha_unit(398-433)custom_components/ocpp/enums.py (2)
HAChargerStatuses(25-38)HAChargerSession(56-62)custom_components/ocpp/button.py (1)
available(167-169)custom_components/ocpp/number.py (1)
available(206-213)custom_components/ocpp/switch.py (1)
available(177-182)custom_components/ocpp/ocppv201.py (2)
_report_evse_status(405-414)ChargePoint(75-659)
custom_components/ocpp/ocppv16.py (1)
custom_components/ocpp/chargepoint.py (20)
get(172-175)get_number_of_connectors(297-299)value(84-86)value(89-91)clear_profile(379-381)notify_ha(849-860)set_charge_rate(383-391)set_availability(393-395)start_transaction(397-399)values(180-181)extra_attr(109-111)extra_attr(114-116)get_ha_metric(832-847)MeasurandValue(214-222)unit(94-96)unit(99-101)get_energy_kwh(719-723)process_measurands(725-825)stop_transaction(401-407)data_transfer(429-431)
custom_components/ocpp/switch.py (2)
custom_components/ocpp/api.py (4)
CentralSystem(98-655)get_available(472-513)get_metric(303-343)set_charger_state(537-564)custom_components/ocpp/enums.py (2)
HAChargerServices(6-22)HAChargerStatuses(25-38)
custom_components/ocpp/chargepoint.py (2)
custom_components/ocpp/ocppv16.py (6)
get_number_of_connectors(112-151)get_heartbeat_interval(153-155)get_supported_measurands(157-205)set_standard_configuration(207-216)start_transaction(456-470)ChargePoint(83-1024)custom_components/ocpp/ocppv201.py (7)
get_number_of_connectors(250-253)get_supported_measurands(268-286)set_standard_configuration(255-266)start_transaction(443-458)ChargePoint(78-882)ChargePoint(75-659)_set_meter_values(548-599)
🔇 Additional comments (24)
tests/test_sensor.py (1)
103-116: Good coverage for per-connector entity creationAsserts for connector 1/2 presence, 3 absence, and root sensor existence look solid.
tests/test_charge_point_core.py (1)
215-240: Phase processing and unit normalization tests are thoroughCovers l–l→l–n conversion and W→kW roll-up paths; assertions are precise.
custom_components/ocpp/number.py (1)
205-213: Available correctly gates on SMART profile and connector availabilityGood check combining feature flag and per-connector availability.
custom_components/ocpp/button.py (3)
87-119: Per-connector instantiation logic looks correct (flatten for single, fan-out for multi).Creation, stale-entity cleanup, and op_connector_id wiring align with PR goal and avoid breaking single-connector setups. Nice.
145-164: Unique ID and DeviceInfo scheme is coherent.Connector child-device identifiers and entity_ids are stable and match the pattern used elsewhere.
167-177: Availability/action routing is correct.
- Reset uses charger-level (connector_id=None).
- Unlock uses targeted connector via _op_connector_id.
custom_components/ocpp/sensor.py (5)
66-73: Configured measurands handling is safe.Strings only, trimmed, with a clear default path. Good.
117-124: Stale flat entity cleanup for multi-connector is correct.Uses entity_registry lookup with the new unique_id scheme. Good.
125-135: Entity fan-out logic matches the non-breaking requirement.
- Charger-only metrics at root.
- Connector metrics flattened for single-connector, expanded for multi-connector.
Also applies to: 137-173
226-233: Data-paths to CentralSystem are correct and connector-aware.available/native_value/unit delegate to CentralSystem with connector context. Good.
Also applies to: 302-325
333-341: Immediate state update on add is fine.Dispatcher wiring and initial schedule align with the new update() signaling.
custom_components/ocpp/ocppv16.py (8)
68-81: Robust TriggerMessage parsing.Accepts both enum and strings; mapping looks good.
269-293: Status trigger loop logic makes sense.Handles connector 0..N and clamps count when charger rejects. Good.
295-311: Trigger custom message: LGTM.Graceful fallback for unsupported message names.
312-334: ClearChargingProfile schema fix and handling are correct.Uses purpose.value and proper connector targeting. Good.
435-456: Availability, start, and stop flows look right.Connector-aware, with sensible fallbacks when tx id/conn is missing.
Also applies to: 456-471, 472-495
496-518: Reset/unlock behavior OK.Resets reconnect counter and uses correct request types.
553-573: Diagnostics gating on FW profile is fine.Early exit when unsupported; good URL validation.
856-863: Firmware/heartbeat status updates: consistent use of connector 0 aggregator.Matches the new metrics layout.
Also applies to: 1010-1025
custom_components/ocpp/chargepoint.py (5)
119-176: _ConnectorAwareMetrics: solid and backwards-compatible.
- Flat view on connector 0 with per-connector access via (conn, meas).
- get()/contains implemented correctly to avoid defaultdict auto-creation.
285-296: Initialize connector slots once per connector.Units pre-seeded; good for HA UI consistency.
641-717: Measurand processing and unit normalization are correct.
- Wh→kWh and W→kW conversions are consistent.
- Phase aggregation uses sensible averaging/summing.
Also applies to: 725-826
832-848: get_ha_metric helper mirrors v16 behavior.Good for restoring state after HA restarts.
335-345: Migration path reminder: bump existing entries to include num_connectors.Ensure async_migrate_entry in init.py adds CONF_NUM_CONNECTORS with default for existing users to avoid breaking changes.
Would you like me to add a targeted migration diff in init.py?
| async def get_number_of_connectors(self) -> int: | ||
| """Return number of connectors on this charger.""" | ||
| return await self.get_configuration(ckey.number_of_connectors.value) | ||
| resp = None | ||
|
|
||
| try: | ||
| req = call.GetConfiguration(key=["NumberOfConnectors"]) | ||
| resp = await self.call(req) | ||
| except Exception: | ||
| resp = None | ||
|
|
||
| cfg = None | ||
| if resp is not None: | ||
| cfg = getattr(resp, "configuration_key", None) | ||
|
|
||
| if ( | ||
| cfg is None | ||
| and isinstance(resp, list | tuple) | ||
| and len(resp) >= 3 | ||
| and isinstance(resp[2], dict) | ||
| ): | ||
| cfg = resp[2].get("configurationKey") or resp[2].get( | ||
| "configuration_key" | ||
| ) | ||
|
|
||
| if cfg: | ||
| for kv in cfg: | ||
| k = getattr(kv, "key", None) | ||
| v = getattr(kv, "value", None) | ||
| if k is None and isinstance(kv, dict): | ||
| k = kv.get("key") | ||
| v = kv.get("value") | ||
| if k == "NumberOfConnectors" and v not in (None, ""): | ||
| try: | ||
| n = int(str(v).strip()) | ||
| if n > 0: | ||
| return n | ||
| except (ValueError, TypeError): | ||
| pass | ||
|
|
||
| return 1 |
There was a problem hiding this comment.
Fix isinstance union misuse (runtime TypeError).
isinstance(x, list | tuple) is invalid at runtime. Use a type tuple.
Apply:
- and isinstance(resp, list | tuple)
+ and isinstance(resp, (list, tuple))📝 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.
| async def get_number_of_connectors(self) -> int: | |
| """Return number of connectors on this charger.""" | |
| return await self.get_configuration(ckey.number_of_connectors.value) | |
| resp = None | |
| try: | |
| req = call.GetConfiguration(key=["NumberOfConnectors"]) | |
| resp = await self.call(req) | |
| except Exception: | |
| resp = None | |
| cfg = None | |
| if resp is not None: | |
| cfg = getattr(resp, "configuration_key", None) | |
| if ( | |
| cfg is None | |
| and isinstance(resp, list | tuple) | |
| and len(resp) >= 3 | |
| and isinstance(resp[2], dict) | |
| ): | |
| cfg = resp[2].get("configurationKey") or resp[2].get( | |
| "configuration_key" | |
| ) | |
| if cfg: | |
| for kv in cfg: | |
| k = getattr(kv, "key", None) | |
| v = getattr(kv, "value", None) | |
| if k is None and isinstance(kv, dict): | |
| k = kv.get("key") | |
| v = kv.get("value") | |
| if k == "NumberOfConnectors" and v not in (None, ""): | |
| try: | |
| n = int(str(v).strip()) | |
| if n > 0: | |
| return n | |
| except (ValueError, TypeError): | |
| pass | |
| return 1 | |
| async def get_number_of_connectors(self) -> int: | |
| """Return number of connectors on this charger.""" | |
| resp = None | |
| try: | |
| req = call.GetConfiguration(key=["NumberOfConnectors"]) | |
| resp = await self.call(req) | |
| except Exception: | |
| resp = None | |
| cfg = None | |
| if resp is not None: | |
| cfg = getattr(resp, "configuration_key", None) | |
| if ( | |
| cfg is None | |
| and isinstance(resp, (list, tuple)) | |
| and len(resp) >= 3 | |
| and isinstance(resp[2], dict) | |
| ): | |
| cfg = resp[2].get("configurationKey") or resp[2].get( | |
| "configuration_key" | |
| ) | |
| if cfg: | |
| for kv in cfg: | |
| k = getattr(kv, "key", None) | |
| v = getattr(kv, "value", None) | |
| if k is None and isinstance(kv, dict): | |
| k = kv.get("key") | |
| v = kv.get("value") | |
| if k == "NumberOfConnectors" and v not in (None, ""): | |
| try: | |
| n = int(str(v).strip()) | |
| if n > 0: | |
| return n | |
| except (ValueError, TypeError): | |
| pass | |
| return 1 |
🤖 Prompt for AI Agents
In custom_components/ocpp/ocppv16.py around lines 112 to 151, the code uses
isinstance(resp, list | tuple) which raises a TypeError at runtime because
isinstance requires a tuple of types, not a union expression; change that check
to isinstance(resp, (list, tuple)) and ensure any similar isinstance usages in
this block use a tuple of types; keep the existing logic for accessing resp[2]
and its dict keys unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_charge_point_v201.py (1)
1238-1238: Avoid hardcoded WS port; use pytest’s unused_tcp_port to eliminate flakesParallel CI or residual listeners can collide with fixed ports. Use the fixture-provided free port.
Apply:
- config_data[CONF_PORT] = 9080 + config_data[CONF_PORT] = unused_tcp_portAnd update the test signature:
# before async def test_cms_responses_v201(hass, socket_enabled): # after async def test_cms_responses_v201(hass, socket_enabled, unused_tcp_port):Consider the same pattern for the later 9011 assignment in this file for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/test_charge_point_v201.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_charge_point_v201.py (3)
custom_components/ocpp/api.py (1)
get_metric(303-343)custom_components/ocpp/chargepoint.py (3)
value(84-86)value(89-91)async_update_device_info(448-467)custom_components/ocpp/sensor.py (1)
ChargePointMetric(85-222)
🔇 Additional comments (1)
tests/test_charge_point_v201.py (1)
1061-1066: Connector-0 metrics writes confirmed—no test changes neededI’ve verified that
async_update_device_infonow writes all of the required metrics at connector 0:
- At line 320 in
custom_components/ocpp/chargepoint.py,featuresis stored under the key(0, cdet.features.value).- At lines 555–558,
model,vendor,firmware_version, andserialare each stored under(0, …)as well.Since production now populates
(0, <metric>)for all of the tested keys, the existing assertions intests/test_charge_point_v201.pywill pass as written. No changes to the tests orget_metricfallbacks are required.
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
Hi @jthun, test coverage is looking good. There are still some sensors missing though and the bumping of config entry version to go: |
Are the sensors still unavailable after a charger reset? I’ve noticed that early reset (as done in the code) is often rejected. |
The sensors remain unavailable after a reset. In terms of the num_connector setting you are still adding it indirectly in https://github.com/lbbrhzn/ocpp/pull/1689/files#diff-b6d6970d2b3d2f80bf2d8977874f1298c8399909b25296a8ffeb4d0213f58266R344 which is hard to follow and also this will not be stored to disk potentially creating issues when rebooting or if further changes are made to the config entry structure. Better approach is as per before where you bump minor config entry version to 1, add the default explicitly in the discovery config flow and include the same in a migration from minor_version 0 to 1. |
|
Hi @jthun, I haven't had any issues with the changes. So once the bits already mentioned are resolved it will be good to merge. |
…tors to config entry. Migration to config 2.1.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
custom_components/ocpp/ocppv201.py (5)
141-160: Collision‑free_pair_to_global— LGTMExact‑match, inventory rebuild, and unique provisional index cover early notifications safely.
340-341: SetChargingProfile purpose serialized correctly — LGTMUsing
.valuefixes payload type; avoids invalid request bodies.
123-140: Don’t clear connector maps on rebuild; preserve indices to avoid data mismatchesClearing
_evse_to_global/_global_to_evsewill reindex connectors and break lookups for metrics/transactions stored under provisional indices. Preserve existing entries and only add missing pairs; have_ensure_connector_map()opportunistically complete the map.def _build_connector_map(self) -> bool: - if not self._inventory or self._inventory.evse_count == 0: - return False - if self._evse_to_global and self._global_to_evse: - return True - - g = 1 - self._evse_to_global.clear() - self._global_to_evse.clear() + if not self._inventory or self._inventory.evse_count == 0: + return False + # Preserve existing assignments; only add missing pairs + g = max(self._global_to_evse.keys(), default=0) + 1 for evse_id in range(1, self._inventory.evse_count + 1): count = 0 if len(self._inventory.connector_count) >= evse_id: count = int(self._inventory.connector_count[evse_id - 1] or 0) for conn_id in range(1, count + 1): - self._evse_to_global[(evse_id, conn_id)] = g - self._global_to_evse[g] = (evse_id, conn_id) - g += 1 - return bool(self._evse_to_global) + if (evse_id, conn_id) not in self._evse_to_global: + self._evse_to_global[(evse_id, conn_id)] = g + self._global_to_evse[g] = (evse_id, conn_id) + g += 1 + return bool(self._evse_to_global and self._global_to_evse) def _ensure_connector_map(self) -> bool: - if self._evse_to_global and self._global_to_evse: - return True - return self._build_connector_map() + # Always try to complete the map when inventory is available. + if self._inventory: + self._build_connector_map() + return bool(self._evse_to_global and self._global_to_evse)
365-372: Normalize status comparisons (enum vs string) to avoid false negativesMixed comparisons risk treating Accepted responses as failures. Use
.valueconsistently (or accept both).@@ def set_charge_rate(...): - if resp.status != ChargingProfileStatusEnumType.accepted: + if resp.status != ChargingProfileStatusEnumType.accepted.value: raise HomeAssistantError( @@ - if resp.status != ChargingProfileStatusEnumType.accepted: + if resp.status != ChargingProfileStatusEnumType.accepted.value: raise HomeAssistantError( @@ def get_configuration(...): - if result["attribute_status"] != GetVariableStatusEnumType.accepted: + if result["attribute_status"] not in ( + GetVariableStatusEnumType.accepted, + GetVariableStatusEnumType.accepted.value, + ): raise HomeAssistantError( @@ def configure(...): - if result["attribute_status"] == SetVariableStatusEnumType.accepted: + if result["attribute_status"] in ( + SetVariableStatusEnumType.accepted, + SetVariableStatusEnumType.accepted.value, + ): return SetVariableResult.accepted - elif result["attribute_status"] == SetVariableStatusEnumType.reboot_required: + elif result["attribute_status"] in ( + SetVariableStatusEnumType.reboot_required, + SetVariableStatusEnumType.reboot_required.value, + ): return SetVariableResult.reboot_requiredRun to find any remaining inconsistent checks:
#!/bin/bash # Find status comparisons using enums with/without .value rg -nP -C2 --type=py '\b(resp|result)\.status|\battribute_status\b' custom_components/ocpp/ocppv201.pyAlso applies to: 412-420, 526-532, 549-555
459-471: stop_transaction may miss active sessions after map changes; scan actual metric keysIterating
range(1, total+1)assumes contiguous indices and can fail post‑reindex. Scan existing metric keys fortransaction_id.- tx_id = "" - total = self._total_connectors() or 1 - for g in range(1, total + 1): - val = self._metrics[(g, csess.transaction_id.value)].value - if val: - tx_id = val - break + tx_id = "" + for key in list(self._metrics.keys()): + if isinstance(key, tuple) and len(key) == 2 and key[1] == csess.transaction_id.value: + val = self._metrics[key].value + if val: + tx_id = val + breakcustom_components/ocpp/sensor.py (1)
74-104: Resolved: enums now converted to strings via .value (no mixed types).This addresses prior lookup issues raised earlier.
custom_components/ocpp/chargepoint.py (1)
172-176: dict.get semantics preserved (fix confirmed)
get()now avoids defaultdict auto-creation and returns the provided default when missing. This addresses the prior concern.
🧹 Nitpick comments (17)
tests/test_init.py (1)
62-96: Strengthen the multi‑connector lifecycle assertionsAdd assertions that the config entry actually carries the expected connector count to guard against silent regressions in migration/defaulting.
@@ async def test_setup_unload_and_reload_entry_multiple_connectors( hass: AsyncGenerator[HomeAssistant, None], bypass_get_data: None ): @@ assert await hass.config_entries.async_setup(config_entry.entry_id) await hass.async_block_till_done() assert DOMAIN in hass.data assert config_entry.entry_id in hass.data[DOMAIN] assert type(hass.data[DOMAIN][config_entry.entry_id]) is CentralSystem + # Verify connector count persisted in entry data + from custom_components.ocpp.const import CONF_CPIDS, CONF_NUM_CONNECTORS + cpids = config_entry.data[CONF_CPIDS] + assert cpids and isinstance(cpids[0], dict) + cp_id, cp_data = list(cpids[0].items())[0] + assert cp_data[CONF_NUM_CONNECTORS] == 2custom_components/ocpp/__init__.py (1)
197-226: Migration to v2.1 adds CONF_NUM_CONNECTORS — works; simplify duplicate updateLogic correctly backfills
CONF_NUM_CONNECTORSand bumpsminor_version. The two branches both callasync_update_entryidentically; simplify.- if changed: - data[CONF_CPIDS] = cpids - hass.config_entries.async_update_entry( - config_entry, - data=data, - version=2, - minor_version=1, - ) - else: - hass.config_entries.async_update_entry( - config_entry, - data=data, - version=2, - minor_version=1, - ) + # Always bump minor version; write back data (possibly unchanged) + data[CONF_CPIDS] = cpids + hass.config_entries.async_update_entry( + config_entry, + data=data, + version=2, + minor_version=1, + )custom_components/ocpp/ocppv201.py (2)
581-584: Also update EVSE‑levelStatusmetric for parity with v1.6You set
(0, Status.Connector); mirror to(0, Status)to keep dashboards/automations consistent across 1.6/2.x.def _report_evse_status(self, evse_id: int, evse_status_v16: ChargePointStatusv16): """Report EVSE-level status on the global connector.""" self._metrics[(0, cstat.status_connector.value)].value = evse_status_v16.value + self._metrics[(0, cstat.status.value)].value = evse_status_v16.value self.hass.async_create_task(self.update(self.settings.cpid))
780-792: Always set meter_start unit to HA kWh when convertingYou convert to kWh; set the unit unconditionally for clarity and downstream math.
- energy_unit = HA_ENERGY_UNIT if measurand_item.unit else None + energy_unit = HA_ENERGY_UNITtests/test_config_flow.py (1)
123-126: Add a test that honors discovery-provided num_connectors (non-default).Current test only asserts the default. Add a discovery test passing, e.g., 2, and assert it’s persisted into the CP entry.
Example (new test function):
async def test_discovery_uses_detected_num_connectors(hass, bypass_get_data): from custom_components.ocpp.const import CONF_NUM_CONNECTORS, DOMAIN entry = MockConfigEntry(domain=DOMAIN, data=MOCK_CONFIG_CS, entry_id="x", title="x", version=2, minor_version=0) if hass.data.get(DOMAIN) is None: hass.data.setdefault(DOMAIN, {}) entry.add_to_hass(hass) assert await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() info = {"cp_id": "cp2", "entry": entry, CONF_NUM_CONNECTORS: 2} result_disc = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY}, data=info ) result_disc["discovery_info"] = info cp_input = MOCK_CONFIG_CP.copy() res_cp = await hass.config_entries.flow.async_configure(result_disc["flow_id"], user_input=cp_input) assert res_cp["type"] == data_entry_flow.FlowResultType.ABORT updated = hass.config_entries._entries.get_entries_for_domain(DOMAIN)[0].data assert updated["cpids"][-1]["cp2"][CONF_NUM_CONNECTORS] == 2custom_components/ocpp/config_flow.py (2)
147-150: Coerce and clamp discovered connector count (defensive parsing).Avoids 0/negative/str values accidentally entering config.
- self._detected_num_connectors = discovery_info.get( - CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS - ) + try: + self._detected_num_connectors = max( + 1, + int( + discovery_info.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) + or DEFAULT_NUM_CONNECTORS + ), + ) + except (TypeError, ValueError): + self._detected_num_connectors = DEFAULT_NUM_CONNECTORS
166-171: cp_data merge looks good; consider copying the CPIDS list to avoid aliasing.Shallow copying entry.data keeps the same CPIDS list reference. Prefer recreating the list before appending to avoid in-place aliasing with the old entry snapshot.
Outside this hunk, update where self._data is built:
# replace: self._data = {**self._entry.data} # with: self._data = {**self._entry.data, CONF_CPIDS: [*self._entry.data.get(CONF_CPIDS, [])]}custom_components/ocpp/sensor.py (4)
54-65: Simplify: read num_connectors directly from cp_id_settings.No need to rescan CPIDS; also clamp to >=1.
- num_connectors = 1 - for item in entry.data.get(CONF_CPIDS, []): - for _, cfg in item.items(): - if cfg.get(CONF_CPID) == cpid: - num_connectors = int( - cfg.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS) - ) - break - else: - continue - break + num_connectors = max( + 1, int(cp_id_settings.get(CONF_NUM_CONNECTORS, DEFAULT_NUM_CONNECTORS)) + )
114-121: Avoid duplicating unique_id construction logic._uid must track ChargePointMetric’s logic; centralize it to a single helper to prevent drift.
Example: add a @staticmethod on ChargePointMetric:
@staticmethod def make_uid(cpid: str, key: str, connector_id: int | None) -> str: parts = [DOMAIN, cpid, key.lower(), SENSOR_DOMAIN] if connector_id is not None: parts.insert(2, f"conn{connector_id}") return ".".join(parts)Then call ChargePointMetric.make_uid(...) here and in init.
122-129: Nice: cleanup stale charger-level connector entities when N>1. Consider migrating instead.Entity-registry deletions during platform setup work, but doing this in async_migrate_entry keeps platform code lean and makes upgrades atomic.
234-234: Ensure boolean for availability.Guard against None from central_system when CP not yet ready.
- return self.central_system.get_available(self.cpid, self.connector_id) + return bool(self.central_system.get_available(self.cpid, self.connector_id))custom_components/ocpp/chargepoint.py (6)
189-196: Avoid implicit creation of connector 0 in membership checks
__contains__usesself._by_conn[0]for flat keys, which will instantiate connector 0 even for pure membership tests. Low impact, but easy to avoid.- return key in self._by_conn[0] + return key in self._by_conn.get(0, {})
337-350: Config-entry update loop control and migration noteUpdating
CONF_NUM_CONNECTORSandCONF_MONITORED_VARIABLESviaasync_update_entryis correct, and the inequality guard prevents reload loops. Make sure:
- Config entry version bump +
async_migrate_entryhandle introducingCONF_NUM_CONNECTORSfor existing users so it’s present from the first reload.- Config flow defaults to 1 and does not expose
num_connectorsunless absolutely necessary (device-reported probe should win when available).
579-611: Entity update fan-out may double notify; consider choosing one pathYou call
entity_component.async_update_entity(...)for every entity and also broadcastDATA_UPDATED. This can trigger duplicate updates depending on subscribers.
- Prefer a single signaling mechanism (dispatcher) and let entities subscribe, or keep the direct updates and remove the broadcast if not needed.
- If you keep breadth-first traversal, consider early-exiting if
root_devhas no entities to reduce work on large registries.
643-673: Per-phase aggregation looks good; minor attr consistencyPhase extras add
unit, per-phase values, andcontext. Consider also addinglocationto the phase extras (when present) for consistency with the non-phase path.- self._metrics[(connector_id, measurand)].extra_attr[om.unit.value] = ( - unit - ) + self._metrics[(connector_id, measurand)].extra_attr[om.unit.value] = unit self._metrics[(connector_id, measurand)].extra_attr[phase] = value self._metrics[(connector_id, measurand)].extra_attr[ om.context.value ] = context + if item.location is not None: + self._metrics[(connector_id, measurand)].extra_attr[om.location.value] = item.location
768-771: Session-energy source detection likely never triggers
_charger_reports_session_energyis set only whenmeter_start == 0. Given_init_connector_slotsleaves it asNoneand v2.x typically sets a non-zero meter_start at Tx start, this flag may never turn True, forcing the subtraction path always.
- Consider setting this flag explicitly in the v2.x handler when you detect that the charger reports session energy directly (e.g., when EAIR at transaction_begin is ~0 and grows from there, or via device capability/inventory).
- Alternatively, treat “meter_start is None at transaction updates” as a hint to accept EAIR directly until a non-None meter_start is observed.
455-459: Latency metric units and defaultsStoring ping/pong latency with “ms” is fine. Optional: use a shared constant or Unit enum to keep unit spelling consistent across the integration.
Also applies to: 486-495
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
custom_components/ocpp/__init__.py(3 hunks)custom_components/ocpp/chargepoint.py(16 hunks)custom_components/ocpp/config_flow.py(6 hunks)custom_components/ocpp/enums.py(3 hunks)custom_components/ocpp/ocppv201.py(14 hunks)custom_components/ocpp/sensor.py(6 hunks)tests/test_config_flow.py(2 hunks)tests/test_init.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-28T09:21:24.150Z
Learnt from: drc38
PR: lbbrhzn/ocpp#1498
File: tests/test_config_flow.py:124-124
Timestamp: 2025-01-28T09:21:24.150Z
Learning: The OCPP integration does not use options flow for configuration updates. Error handling in the config flow is primarily focused on measurand selection validation with the "no_measurands_selected" error.
Applied to files:
custom_components/ocpp/config_flow.py
🧬 Code graph analysis (7)
custom_components/ocpp/enums.py (1)
custom_components/ocpp/ocppv16.py (1)
get_supported_features(158-195)
custom_components/ocpp/chargepoint.py (2)
custom_components/ocpp/ocppv201.py (2)
get_number_of_connectors(249-252)start_transaction(442-457)custom_components/ocpp/ocppv16.py (2)
get_number_of_connectors(112-151)start_transaction(456-470)
custom_components/ocpp/__init__.py (2)
custom_components/ocpp/ocppv16.py (3)
get_number_of_connectors(89-91)trigger_status_notification(209-233)on_status_notification(667-700)custom_components/ocpp/number.py (1)
async_setup_entry(55-71)
custom_components/ocpp/config_flow.py (1)
custom_components/ocpp/ocppv16.py (1)
get_number_of_connectors(89-91)
custom_components/ocpp/ocppv201.py (1)
custom_components/ocpp/chargepoint.py (12)
ChargePoint(225-862)get(172-175)value(84-86)value(89-91)update(579-610)get_number_of_connectors(297-299)set_availability(395-397)values(180-181)get_energy_kwh(721-725)unit(94-96)unit(99-101)process_measurands(727-827)
tests/test_init.py (3)
custom_components/ocpp/__init__.py (1)
async_setup(89-97)tests/conftest.py (1)
bypass_get_data_fixture(36-46)tests/charge_point_test.py (1)
create_configuration(66-74)
custom_components/ocpp/sensor.py (3)
custom_components/ocpp/api.py (4)
get_available(472-513)get_extra_attr(435-470)get_metric(303-343)get_ha_unit(398-433)custom_components/ocpp/enums.py (3)
HAChargerStatuses(25-38)HAChargerDetails(41-53)HAChargerSession(56-62)custom_components/ocpp/ocppv16.py (2)
data_transfer(574-596)on_status_notification(667-700)
🔇 Additional comments (19)
tests/test_init.py (2)
13-18: Multi‑connector fixture import — LGTMImporting
MOCK_CONFIG_DATA_1_MCcleanly scopes the new scenario.
125-125: Migration expectation updated to minor_version == 1 — LGTMMatches the new migration step added in the integration.
custom_components/ocpp/enums.py (1)
65-81: Profiles → IntFlag with labels(): good upgrade
- IntFlag is appropriate for combinable feature profiles.
labels()returns a concise, readable string and handles NONE.custom_components/ocpp/__init__.py (1)
28-28: Expose CONF_NUM_CONNECTORS defaults — LGTMConstants are correctly imported for use across the integration.
Also applies to: 49-49
tests/test_config_flow.py (1)
10-14: LGTM: importing connector constants is correct.These imports line up with the updated flow output and keep the test self-contained.
custom_components/ocpp/config_flow.py (2)
120-121: Good: per-flow storage for detected connector count.Keeps cp_user step simple and source-agnostic.
111-111: async_migrate_entry injects DEFAULT_NUM_CONNECTORS; no change requiredThe migration in custom_components/ocpp/init.py already adds DEFAULT_NUM_CONNECTORS for any entry missing CONF_NUM_CONNECTORS (lines 205–209).
custom_components/ocpp/sensor.py (4)
142-178: Single-connector preserved; multi-connector expands per-connector entities.This removes the breaking change while adding N-connector support. Good balance.
223-228: Manually setting entity_id: verify necessity and collisions.If this is for backward compatibility, fine. Otherwise, prefer letting the registry derive IDs from unique_id to avoid accidental name clashes.
310-322: LGTM: feature flag rendering is user-friendly.Gracefully handles IntFlag.labels() or falls back to str(value).
352-353: LGTM: immediate update scheduled after wiring dispatcher.Reduces “unknown” window on first load.
custom_components/ocpp/chargepoint.py (8)
119-131: Solid backward-compatible metrics wrapperThe
_ConnectorAwareMetricsabstraction cleanly preserves flat access while enabling per-connector keys. Good use ofMutableMappingwith explicit behaviors documented.
275-286: Metrics init and default connector countGood initialization of core metrics and sane default for
num_connectors. No issues.
287-296: Ensure connector-scoped slots are ready before usePre-seeding units and keys per connector is correct and prevents None-unit surprises downstream.
297-300: Confirm override of get_number_of_connectors in versioned subclassesBase implementation returns
self.num_connectors. Verify v1.6/v2.x subclasses still override to probe the device and that the base fallback is only used when probing is unavailable.
317-324: Feature metrics update + loggingSetting
cdet.featuresand logging labels is fine. Ensurelabels()is present on allProfilesvariants to avoid AttributeError across versions.
708-719: Normalization and unit setting are correctConverting Wh→kWh and W→kW with HA units is correct. No issues.
548-549: Reconnect counter updateIncrementing
reconnectson the root device is sensible and initialized to 0 at startup; safe against None.
552-561: Device info propagation looks correctMetrics update plus registry population with both identifiers (
cpid,id) ensures stable device linkage.
| def get_ha_metric(self, measurand: str, connector_id: int | None = None): | ||
| """Return last known value in HA for given measurand, or None if not available.""" | ||
| base = self.settings.cpid.lower() | ||
| meas_slug = measurand.lower().replace(".", "_") | ||
|
|
||
| candidates: list[str] = [] | ||
| if connector_id and connector_id > 0: | ||
| candidates.append(f"sensor.{base}_connector_{connector_id}_{meas_slug}") | ||
| else: | ||
| candidates.append(f"sensor.{base}_{meas_slug}") | ||
|
|
||
| for entity_id in candidates: | ||
| st = self.hass.states.get(entity_id) | ||
| if st and st.state not in (STATE_UNAVAILABLE, STATE_UNKNOWN, None): | ||
| return st.state | ||
| return None | ||
|
|
There was a problem hiding this comment.
Fallback to aggregate entity for single-connector compatibility
When a caller passes connector_id=1 on a single-connector setup, only the per-connector entity is checked. If entities were created in legacy (aggregate) form, this returns None and breaks state recovery (e.g., after reset). Add aggregate fallback.
def get_ha_metric(self, measurand: str, connector_id: int | None = None):
"""Return last known value in HA for given measurand, or None if not available."""
base = self.settings.cpid.lower()
meas_slug = measurand.lower().replace(".", "_")
- candidates: list[str] = []
- if connector_id and connector_id > 0:
- candidates.append(f"sensor.{base}_connector_{connector_id}_{meas_slug}")
- else:
- candidates.append(f"sensor.{base}_{meas_slug}")
+ candidates: list[str] = []
+ if connector_id and connector_id > 0:
+ candidates.append(f"sensor.{base}_connector_{connector_id}_{meas_slug}")
+ # Always include aggregate fallback for backward compatibility (single-connector)
+ candidates.append(f"sensor.{base}_{meas_slug}")
for entity_id in candidates:
st = self.hass.states.get(entity_id)
if st and st.state not in (STATE_UNAVAILABLE, STATE_UNKNOWN, None):
return st.state
return None📝 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.
| def get_ha_metric(self, measurand: str, connector_id: int | None = None): | |
| """Return last known value in HA for given measurand, or None if not available.""" | |
| base = self.settings.cpid.lower() | |
| meas_slug = measurand.lower().replace(".", "_") | |
| candidates: list[str] = [] | |
| if connector_id and connector_id > 0: | |
| candidates.append(f"sensor.{base}_connector_{connector_id}_{meas_slug}") | |
| else: | |
| candidates.append(f"sensor.{base}_{meas_slug}") | |
| for entity_id in candidates: | |
| st = self.hass.states.get(entity_id) | |
| if st and st.state not in (STATE_UNAVAILABLE, STATE_UNKNOWN, None): | |
| return st.state | |
| return None | |
| def get_ha_metric(self, measurand: str, connector_id: int | None = None): | |
| """Return last known value in HA for given measurand, or None if not available.""" | |
| base = self.settings.cpid.lower() | |
| meas_slug = measurand.lower().replace(".", "_") | |
| candidates: list[str] = [] | |
| if connector_id and connector_id > 0: | |
| candidates.append(f"sensor.{base}_connector_{connector_id}_{meas_slug}") | |
| # Always include aggregate fallback for backward compatibility (single-connector) | |
| candidates.append(f"sensor.{base}_{meas_slug}") | |
| for entity_id in candidates: | |
| st = self.hass.states.get(entity_id) | |
| if st and st.state not in (STATE_UNAVAILABLE, STATE_UNKNOWN, None): | |
| return st.state | |
| return None |
🤖 Prompt for AI Agents
In custom_components/ocpp/chargepoint.py around lines 834 to 850, the
get_ha_metric function only checks the per-connector entity when connector_id==1
which misses legacy aggregate entities; update the candidates logic so that when
connector_id is 1 you still append the aggregate entity name
(sensor.{base}_{meas_slug}) as a fallback (keep per-connector first, then
aggregate) so the loop will check the per-connector state and then the legacy
aggregate state before returning None.
drc38
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
* add ocpp versions supported (lbbrhzn#1678) * build(deps): bump actions/checkout from 4 to 5 (lbbrhzn#1685) * build(deps): bump pre-commit from 4.2.0 to 4.3.0 (lbbrhzn#1686) * build(deps): bump ruff from 0.12.2 to 0.12.9 (lbbrhzn#1687) * Correct custom component name for debuggging (lbbrhzn#1697) * Support for multiple connectors per charger (lbbrhzn#1689) * Update .gitignore for HA dev. Use symlinks for integration. * Handle multiple connectors, initial framework. * Fix sensor updates and placement on proper devices. * Add test for v2x multi connector. Handle meter values properly. max_current working per connector. * Fix per connector set charge rate. Clean up swenglish comments. * Remove separate connector device breaking change. Fix test race condition. CodeRabbit fixes. * Fix max_current per connector bug. Remove ChargerSession sensors (used for debugging). Fix Charge Control naming when only one connector. More CodeRabbit suggestions. * Fix Connector naming. More CodeRabbit suggestions. * Add more tests. More CodeRabbit suggestions. * Test fixes. CodeRabbit suggestions. * Fix tests. * Split stop transaction test in separate tests. * Change entity id to include charger. Add session sensors to connectors. Clear charger-level entities after connector creation. Add num_connector changes to reload logic. Remove not used functions from chargepoint.py. Revert to original config flow due to automatic discovery of number of connectors. Add more tests. * Change port on test to fix crash. * Add missing sensors. Convert features value in sensor. Add num_connectors to config entry. Migration to config 2.1. --------- Co-authored-by: Jan Thunqvist <[email protected]> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: drc38 <[email protected]> * build(deps): bump jsonschema from 4.24.0 to 4.25.1 (lbbrhzn#1690) Bumps [jsonschema](https://github.com/python-jsonschema/jsonschema) from 4.24.0 to 4.25.1. - [Release notes](https://github.com/python-jsonschema/jsonschema/releases) - [Changelog](https://github.com/python-jsonschema/jsonschema/blob/main/CHANGELOG.rst) - [Commits](python-jsonschema/jsonschema@v4.24.0...v4.25.1) --- updated-dependencies: - dependency-name: jsonschema dependency-version: 4.25.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: drc38 <[email protected]> * Added Rolec EVO to docs (lbbrhzn#1695) * Added Rolec EVO to docs * Update docs/user-guide.md Fix unit of power Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: drc38 <[email protected]> * build(deps): bump ruff from 0.12.9 to 0.12.11 (lbbrhzn#1696) Bumps [ruff](https://github.com/astral-sh/ruff) from 0.12.9 to 0.12.11. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.12.9...0.12.11) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.12.11 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump actions/stale from 9 to 10 (lbbrhzn#1699) Bumps [actions/stale](https://github.com/actions/stale) from 9 to 10. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v9...v10) --- updated-dependencies: - dependency-name: actions/stale dependency-version: '10' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump actions/setup-python from 5.6.0 to 6.0.0 (lbbrhzn#1700) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5.6.0 to 6.0.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5.6.0...v6.0.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: drc38 <[email protected]> * Update EAIR continuously (Fixes lbbrhzn#1707). Handle clear profile exceptions 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. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mutesplash <[email protected]> Co-authored-by: Jan <[email protected]> Co-authored-by: Jan Thunqvist <[email protected]> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: Mark Gibson <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* add ocpp versions supported (lbbrhzn#1678) * build(deps): bump actions/checkout from 4 to 5 (lbbrhzn#1685) * build(deps): bump pre-commit from 4.2.0 to 4.3.0 (lbbrhzn#1686) * build(deps): bump ruff from 0.12.2 to 0.12.9 (lbbrhzn#1687) * Correct custom component name for debuggging (lbbrhzn#1697) * Support for multiple connectors per charger (lbbrhzn#1689) * Update .gitignore for HA dev. Use symlinks for integration. * Handle multiple connectors, initial framework. * Fix sensor updates and placement on proper devices. * Add test for v2x multi connector. Handle meter values properly. max_current working per connector. * Fix per connector set charge rate. Clean up swenglish comments. * Remove separate connector device breaking change. Fix test race condition. CodeRabbit fixes. * Fix max_current per connector bug. Remove ChargerSession sensors (used for debugging). Fix Charge Control naming when only one connector. More CodeRabbit suggestions. * Fix Connector naming. More CodeRabbit suggestions. * Add more tests. More CodeRabbit suggestions. * Test fixes. CodeRabbit suggestions. * Fix tests. * Split stop transaction test in separate tests. * Change entity id to include charger. Add session sensors to connectors. Clear charger-level entities after connector creation. Add num_connector changes to reload logic. Remove not used functions from chargepoint.py. Revert to original config flow due to automatic discovery of number of connectors. Add more tests. * Change port on test to fix crash. * Add missing sensors. Convert features value in sensor. Add num_connectors to config entry. Migration to config 2.1. --------- Co-authored-by: Jan Thunqvist <[email protected]> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: drc38 <[email protected]> * build(deps): bump jsonschema from 4.24.0 to 4.25.1 (lbbrhzn#1690) Bumps [jsonschema](https://github.com/python-jsonschema/jsonschema) from 4.24.0 to 4.25.1. - [Release notes](https://github.com/python-jsonschema/jsonschema/releases) - [Changelog](https://github.com/python-jsonschema/jsonschema/blob/main/CHANGELOG.rst) - [Commits](python-jsonschema/jsonschema@v4.24.0...v4.25.1) --- updated-dependencies: - dependency-name: jsonschema dependency-version: 4.25.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: drc38 <[email protected]> * Added Rolec EVO to docs (lbbrhzn#1695) * Added Rolec EVO to docs * Update docs/user-guide.md Fix unit of power Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: drc38 <[email protected]> * build(deps): bump ruff from 0.12.9 to 0.12.11 (lbbrhzn#1696) Bumps [ruff](https://github.com/astral-sh/ruff) from 0.12.9 to 0.12.11. - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.12.9...0.12.11) --- updated-dependencies: - dependency-name: ruff dependency-version: 0.12.11 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump actions/stale from 9 to 10 (lbbrhzn#1699) Bumps [actions/stale](https://github.com/actions/stale) from 9 to 10. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v9...v10) --- updated-dependencies: - dependency-name: actions/stale dependency-version: '10' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump actions/setup-python from 5.6.0 to 6.0.0 (lbbrhzn#1700) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5.6.0 to 6.0.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5.6.0...v6.0.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: drc38 <[email protected]> * fixes for initial release of multiple connector support Co-authored-by: Jan Thunqvist <[email protected]> * Average Current values. Handle cases where meter values bucket contains several EAIR with different contexts (Fixes lbbrhzn#1709). Set energy_session initally to 0 to avoid spike. More tests. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: mutesplash <[email protected]> Co-authored-by: Jan <[email protected]> Co-authored-by: Jan Thunqvist <[email protected]> Co-authored-by: lbbrhzn <[email protected]> Co-authored-by: Mark Gibson <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Add support for multiple connectors per charger.
Breaking change is that Connector devices are now always added to a ChargePoint, even if there's only one outlet. I couldn't figure out a solid way of creating Connector devices/sensors after a charger has reported the number of connectors, so there is a new config entry num_connectors that defaults to 1.
Tested on a ChargeStorm Connected v1 (OCPP v 1.6) with two outlets.
Summary by CodeRabbit
New Features
Chores
Tests