Average Current Values. More robust handling of meter values. #1710
Conversation
…ns several EAIR with different contexts (Fixes lbbrhzn#1709). Set energy_session initally to 0 to avoid spike. More tests.
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1710 +/- ##
==========================================
+ Coverage 94.25% 96.03% +1.77%
==========================================
Files 12 12
Lines 2751 2897 +146
==========================================
+ Hits 2593 2782 +189
+ Misses 158 115 -43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @jthun EAIR is still not picking up and reporting Sample.Periodic measurements? |
Can you provide debug logs from a charging session? |
|
|
Is that using the code in the PR or 0.10.1? Are there other sensors (maybe inactive) on the charger device? Can you start a charging session and capture the logs a little longer? I just want to see that the charger is actually sending periodic EAIR updates. |
It's using this PR (although both are equivalent for this issue). Looking at the codebase it is ignoring when an EAIR is sent with a transactionId and connector=1, which are the Sample.Periodic values. For a single connector charger it should assume an EAIR associated with connector 1 is the same as the EAIR for connector 0 unless the value received is less than the previous EAIR (eg ABB chargers start the transaction EAIR at 0 for each session) ie always increasing. |
|
I have some additional changes to EAIR handling that I'm working on. |
…t. More efficient measurand handling on init. Improved log message/handling in trigger_status_notification. Remove excessive logging and notification of SetChargingProfile fails (Fixes lbbrhzn#1713). Handle Power.Factor. More tests.
jthun
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom_components/ocpp/ocppv16.py (1)
122-147: Fix isinstance use:list | tupleraises at runtime.
isinstance(resp, list | tuple)is invalid and will throwTypeError. Use a tuple of types.Apply:
- and isinstance(resp, list | tuple) + and isinstance(resp, (list, tuple))
🧹 Nitpick comments (8)
custom_components/ocpp/api.py (1)
293-305: Make _safe_int reusable and typed.The defensive conversion is good. Consider hoisting
_safe_intto module scope (or a@staticmethod) and adding a return annotation to_get_metricsfor clarity and reuse elsewhere that consumes connector counts.+from typing import Tuple, Mapping, Any + +def _safe_int(value, default: int = 1) -> int: + try: + iv = int(value) + return iv if iv > 0 else default + except Exception: + return default + def _get_metrics(self, id: str): - """Return (cp_id, metrics mapping, cp instance, safe int num_connectors).""" + """Return (cp_id, metrics mapping, cp instance, safe int num_connectors).""" cp_id = self.cpids.get(id, id) cp = self.charge_points.get(cp_id) - - def _safe_int(value, default=1): - try: - iv = int(value) - return iv if iv > 0 else default - except Exception: - return default - - n_connectors = _safe_int(getattr(cp, "num_connectors", 1), default=1) + n_connectors = _safe_int(getattr(cp, "num_connectors", 1), default=1) return ( (cp_id, cp._metrics, cp, n_connectors) if cp is not None else (None, None, None, None) )tests/test_more_coverage_chargepoint.py (2)
89-121: Patch the correct base alias and assert CallError serialization.Nice isolation by patching
custom_components.ocpp.chargepoint.cp(the base alias). This ensures the wrapper path is exercised. Consider asserting the payload shape (e.g., contains"error") in addition to exact equality to decouple from future formatting tweaks.
336-355: Graph traversal test is precise; minor readability nit.Duplicating
childindevices.values()to hit the “skip visited” path is clever. Optionally add a short in-test assert comment on expected visited order to aid future readers.tests/test_additional_charge_point_v16.py (1)
623-674: Test should also verify that meter_start remains None.The test correctly sets up the scenario where float conversion fails for meter_start restoration. Consider also asserting that meter_start remains None (or its original invalid value) after the failed conversion attempt.
resp = await cp.call(mv_no_tx) assert resp is not None - assert srv._metrics[(1, "Energy.Meter.Start")].value is None + # Verify meter_start remains None after failed conversion + assert srv._metrics[(1, "Energy.Meter.Start")].value is Nonecustom_components/ocpp/ocppv16.py (4)
194-203: Normalize unknown measurands to empty string.
get_configuration()can return "Unknown". Propagate an empty string to avoid surfacing a sentinel value upstream.- chgr_csv = await self.get_configuration(key) + chgr_csv = await self.get_configuration(key) or "" + if isinstance(chgr_csv, str) and chgr_csv.strip().lower() == "unknown": + chgr_csv = ""
204-209: Minor: avoid redundant configure after accepted CSV.After an accepted ChangeConfiguration, calling
configure()again is extra I/O. Safe to skip unless you specifically want HA notification side effects.- await self.configure(key, effective_csv) - return effective_csv + # Value already accepted; no need to re-set. + return effective_csv
210-247: Prefer reusingconfigure()for consistency and error handling.Direct ChangeConfiguration bypasses the read-only/unknown handling already implemented in
configure().- try: - resp = await self.call( - call.ChangeConfiguration(key=key, value=desired_csv) - ) - _LOGGER.debug( - "'%s' measurands set manually to %s", self.id, desired_csv - ) - if getattr(resp, "status", None) in cfg_ok: - effective_csv = desired_csv - else: - _LOGGER.debug( - "'%s' manual measurands set not accepted (status=%s); using charger's value", - self.id, - getattr(resp, "status", None), - ) - effective_csv = await self.get_configuration(key) - except Exception as ex: - _LOGGER.debug( - "Manual measurands set failed for '%s': %s; using charger's value", - self.id, - ex, - ) - effective_csv = await self.get_configuration(key) + try: + res = await self.configure(key, desired_csv) + if res in (None, SetVariableResult.accepted, SetVariableResult.reboot_required): + effective_csv = desired_csv + else: + effective_csv = await self.get_configuration(key) + except Exception as ex: + _LOGGER.debug("Manual measurands set failed for '%s': %s; using charger's value", self.id, ex) + effective_csv = await self.get_configuration(key)
967-977: Session metrics derived from tx-bound EAIR: verify assumptions.You assume tx_id encodes epoch seconds (Line 983). That’s true here (see start_transaction), but if a backend changes tx_id semantics, session time will be wrong. Consider persisting an explicit start timestamp metric on StartTransaction.
- tx_start = float( - self._metrics[(connector_id, csess.transaction_id.value)].value - or time.time() - ) + tx_start = float( + self._metrics[(connector_id, csess.transaction_start_ts.value)].value + if (connector_id, csess.transaction_start_ts.value) in self._metrics + else time.time() + )If you want, I can wire up setting
csess.transaction_start_tsinon_start_transaction.Also applies to: 979-1041
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
custom_components/ocpp/api.py(1 hunks)custom_components/ocpp/chargepoint.py(5 hunks)custom_components/ocpp/ocppv16.py(8 hunks)tests/conftest.py(2 hunks)tests/test_additional_charge_point_v16.py(1 hunks)tests/test_api_paths.py(1 hunks)tests/test_charge_point_v16.py(6 hunks)tests/test_charge_point_v201.py(1 hunks)tests/test_more_coverage_chargepoint.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-02T06:05:56.424Z
Learnt from: drc38
PR: lbbrhzn/ocpp#1527
File: custom_components/ocpp/chargepoint.py:391-391
Timestamp: 2025-03-02T06:05:56.424Z
Learning: In the OCPP component, post_connect initialization has been refactored to only happen in response to boot notifications rather than automatically on startup, preventing duplicate initialization if a boot notification occurs close to the initial connection.
Applied to files:
custom_components/ocpp/chargepoint.py
🧬 Code graph analysis (7)
tests/test_more_coverage_chargepoint.py (1)
custom_components/ocpp/chargepoint.py (22)
ChargePoint(225-984)MeasurandValue(214-222)start(537-539)get_number_of_connectors(297-299)get_supported_measurands(305-307)get_supported_features(313-315)set_availability(411-413)start_transaction(415-417)stop_transaction(419-425)reset(427-429)unlock(431-433)get_configuration(451-453)configure(455-457)_handle_call(530-535)run(541-556)update(605-636)values(180-181)value(84-86)value(89-91)process_measurands(819-949)unit(94-96)unit(99-101)
tests/conftest.py (2)
custom_components/ocpp/api.py (1)
CentralSystem(98-679)tests/charge_point_test.py (2)
create_configuration(66-74)remove_configuration(80-85)
tests/test_charge_point_v201.py (2)
custom_components/ocpp/api.py (1)
get_metric(312-352)custom_components/ocpp/chargepoint.py (2)
value(84-86)value(89-91)
tests/test_additional_charge_point_v16.py (4)
custom_components/ocpp/enums.py (2)
HAChargerDetails(41-53)ConfigurationKey(125-179)tests/charge_point_test.py (1)
wait_ready(91-94)custom_components/ocpp/chargepoint.py (17)
ChargePoint(225-984)start(537-539)value(84-86)value(89-91)trigger_status_notification(386-388)get_number_of_connectors(297-299)get_supported_measurands(305-307)trigger_boot_notification(382-384)trigger_custom_message(390-395)clear(186-187)stop_transaction(419-425)update_firmware(435-441)get_configuration(451-453)configure(455-457)get(172-175)unit(94-96)unit(99-101)custom_components/ocpp/ocppv16.py (14)
ChargePoint(83-1266)trigger_status_notification(310-341)get_number_of_connectors(108-147)get_supported_measurands(153-246)trigger_boot_notification(298-308)trigger_custom_message(343-358)_profile_ids_for(380-408)stop_transaction(581-603)update_firmware(628-660)get_configuration(707-723)configure(725-773)on_status_notification(1061-1093)on_start_transaction(1134-1174)on_stop_transaction(1177-1250)
tests/test_charge_point_v16.py (4)
custom_components/ocpp/enums.py (1)
HAChargerDetails(41-53)custom_components/ocpp/chargepoint.py (7)
set_charge_rate(401-409)ChargePoint(225-984)start(537-539)get_energy_kwh(813-817)value(84-86)value(89-91)trigger_status_notification(386-388)custom_components/ocpp/ocppv16.py (3)
set_charge_rate(410-531)ChargePoint(83-1266)trigger_status_notification(310-341)tests/conftest.py (1)
setup_config_entry(71-90)
custom_components/ocpp/chargepoint.py (2)
custom_components/ocpp/ocppv16.py (2)
trigger_boot_notification(298-308)trigger_status_notification(310-341)custom_components/ocpp/ocppv201.py (1)
trigger_status_notification(321-333)
custom_components/ocpp/ocppv16.py (2)
custom_components/ocpp/chargepoint.py (14)
value(84-86)value(89-91)get_configuration(451-453)configure(455-457)notify_ha(973-984)get(172-175)get_ha_metric(956-971)get_energy_kwh(813-817)unit(94-96)unit(99-101)extra_attr(109-111)extra_attr(114-116)MeasurandValue(214-222)process_measurands(819-949)custom_components/ocpp/ocppv201.py (2)
get_configuration(511-532)configure(534-558)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (40)
tests/test_api_paths.py (1)
22-22: Good import consolidation to tests.const.This aligns tests with shared fixtures/constants and reduces brittle relative imports.
tests/test_charge_point_v201.py (1)
609-611: Correct to use phase-averaged currents with tolerance.Switching to
pytest.approxand expecting 2.2 A and 12.2 A (L2 averages) matches the new “Average Current” semantics and avoids brittle FP equality.tests/test_more_coverage_chargepoint.py (4)
29-59: Solid base-behavior coverage with proper cleanup.Good use of a live WS client, explicit boot, and finally-block to cancel the client task and close the socket. The explicit base-class dispatch verifies defaults without depending on v16 overrides.
157-173: Exercise both exception paths in run() with deterministic stop().Clear separation of TimeoutError vs other exceptions, and verifying
stop()is awaited twice is effective. Looks good.
235-256: Registry stubs correctly force early-return path.The fake DR/ER plus patched dispatcher cover the “no root device” branch without side effects. No changes needed.
393-416: v2.x EAIR defaults and session-energy delta validated well.Good direct unit normalization check (Wh→kWh) and delta computation. This test is independent of WS I/O, keeping it fast.
tests/test_additional_charge_point_v16.py (9)
1-24: LGTM! Well-organized test module.The imports are properly structured and the new test module follows the existing test organization pattern in the codebase.
26-72: Test correctly validates the connector count adjustment logic.The test properly verifies that when a TriggerMessage times out on connector 2, the connector count is reduced to 1 (max(1, cid - 1)), and the function returns False. The test setup with monkeypatch is clean and focused.
75-113: Good error handling coverage for GetConfiguration exception.The test verifies that when GetConfiguration raises an exception, the fallback value of 1 is returned as expected.
159-204: Test validates autodetect measurands fallback behavior.The test correctly verifies that when ChangeConfiguration fails during autodetect, the code falls back to reading the value via GetConfiguration.
417-446: Good edge case coverage for profile ID generation.The test verifies that when a non-integer conn_id is provided, the conn_seg defaults to 1, resulting in the expected profile ID of 1012.
482-518: Test validates firmware update invalid wait_time handling.Good coverage of the fallback behavior when wait_time is invalid.
735-789: Good coverage of new transaction reset behavior.The test correctly verifies that when a new transaction ID appears, the per-connector EAIR and meter_start are reset to allow lower values (e.g., 0 Wh) to be accepted, which is essential for handling ABB chargers.
1078-1110: Test validates meter_start conversion failure defaults to 0.0.The test correctly verifies that when meter_start cannot be converted to float, it defaults to 0.0 as expected.
1112-1152: Good test of denied authorization path.The test correctly verifies that when authorization is denied, the transaction ID is 0.
tests/test_charge_point_v16.py (16)
30-30: LGTM! HAChargerDetails alias improves readability.The alias
cdetforHAChargerDetailsmakes the code more concise while maintaining clarity.
2902-2956: Excellent exception handling test with creative use of eq.The test cleverly uses a custom _ExplosiveStatus class that raises on equality checks to verify the exception handling path in set_availability. This ensures the code properly catches and handles exceptions during status comparisons.
3128-3199: Good edge case coverage for pre-clear exceptions.The test properly verifies that when ClearChargingProfile(id=pid) raises an exception, it's swallowed and the function continues with SetChargingProfile attempts.
3209-3212: No notify_ha when all SetChargingProfile attempts raise exceptions.When all SetChargingProfile attempts raise exceptions (not just return rejected status), the function returns False without calling notify_ha. This is consistent with the behavior for internal/periodic failures.
3315-3371: Good test coverage for Transaction.Begin context filtering.The test correctly verifies that Transaction.Begin EAIR entries are ignored when Sample.Periodic is also present in the same bucket, ensuring the higher-priority context is selected.
3373-3455: Test validates EAIR transaction updates and session energy calculation.The test properly verifies that tx-bound EAIR values update the per-connector metrics and that Energy.Session is calculated correctly from the best EAIR in each bucket.
3457-3513: Good priority testing for Transaction.End over Sample.Periodic.The test correctly verifies that Transaction.End context takes priority over Sample.Periodic regardless of order.
3515-3614: Excellent test of sanitization and exception handling.The test thoroughly covers:
- NaN values are ignored
- Negative values are ignored
- get_energy_kwh exceptions are caught and ignored
- Sample.Periodic is finally selected as the valid entry
3675-3719: Single connector trigger status test validates expected behavior.The test correctly verifies that for single connector (n=1), only connector 1 is probed (not connector 0), and success returns True.
3721-3765: Multi-connector happy path test is well structured.The test verifies that for n=2, connectors 0, 1, and 2 are all probed successfully and the connector count remains unchanged.
3914-3960: Test validates connector count adjustment on timeout.The test correctly verifies that when connector 2 times out, the connector count is reduced to 1 and the function returns False, matching the expected behavior.
3963-4010: Good test of post_connect exception handling.The test verifies that when fetch_supported_features raises an exception inside post_connect, it's swallowed and post_connect_success remains False.
4012-4096: Test validates REM trigger execution despite set_availability error.The test correctly verifies that when set_availability raises a generic Exception, it's caught and swallowed, and the REM triggers (boot and status notifications) are still called.
4100-4163: CancelledError propagation test is essential.The test correctly verifies that asyncio.CancelledError is re-raised (not swallowed) to ensure proper cancellation propagation.
4493-4636: Comprehensive ABB transaction reset test.The test thoroughly validates the ABB-specific behavior where a new transaction ID triggers a reset of per-connector EAIR and meter_start, allowing the EAIR to restart from 0 Wh. This is critical for correct handling of ABB chargers.
4638-4758: Excellent EAIR context priority test.The test comprehensively validates the context priority mechanism within buckets:
- Transaction.End (priority 3) beats both Sample.Periodic and Sample.Clock
- Sample.Periodic (priority 2) beats Sample.Clock when Transaction.End is absent
The test structure with multiple buckets is well designed.custom_components/ocpp/chargepoint.py (5)
366-377: Good addition of timeout bounds for trigger notifications.Adding 3-second timeouts to trigger_boot_notification and trigger_status_notification prevents these operations from blocking indefinitely. The exception handling ensures failures don't break the post_connect flow.
478-483: Excellent backstop implementation for non-compliant chargers.The 10-second backstop ensures post_connect runs even if a boot notification is never received, improving robustness for non-compliant chargers.
526-528: Good addition of generic exception handler.The catch-all exception handler ensures the monitor_connection loop exits cleanly on unexpected errors, preventing silent failures.
669-787: Comprehensive per-connector phase aggregation implementation.The refactored process_phases method properly aggregates phase data per connector with appropriate rules:
- Voltage: averaged with proper L-N/L-L handling
- Current: averaged across phases
- Power.Factor: averaged (not summed) as it's dimensionless
- Other Power values: summed across phases
The helper functions _avg_l123 and _sum_l123 improve code clarity.
883-912: Good OCPP 2.x session energy initialization.The code properly bootstraps Energy.Session and Energy.Meter.Start metrics for OCPP 2.x transactions, ensuring proper initialization even when missing. The positive delta calculation guards against counter resets.
custom_components/ocpp/ocppv16.py (4)
790-797: EAIR routing and mirroring rules look solid.Clear docstring; matches the later implementation.
841-857: New transaction detection/reset is correct and prevents false “decrease” rejections.Good call resetting per-connector EAIR and meter_start on new tx.
883-966: EAIR selection logic across contexts: LGTM.Priority ordering and single-bucket best-pick address #1709 multi-EAIR cases; safe handling of missing/invalid values.
526-531: Noise reduction on SetChargingProfile failures: good.Conditional HA notification based on
last_statusavoids spurious alerts.
| latency_pong = self.cs_settings.websocket_ping_timeout * 1000 | ||
| pong_waiter = await asyncio.wait_for( |
There was a problem hiding this comment.
Fix incorrect latency initialization.
Both latency_ping and latency_pong are initialized to the same timeout value before the ping is sent. This means latency_ping won't reflect the actual ping time if the ping succeeds.
Move the latency_pong initialization after the ping completes:
time0 = time.perf_counter()
latency_ping = self.cs_settings.websocket_ping_timeout * 1000
- latency_pong = self.cs_settings.websocket_ping_timeout * 1000
pong_waiter = await asyncio.wait_for(
connection.ping(), timeout=self.cs_settings.websocket_ping_timeout
)
time1 = time.perf_counter()
latency_ping = round(time1 - time0, 3) * 1000
+ latency_pong = self.cs_settings.websocket_ping_timeout * 1000📝 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.
| latency_pong = self.cs_settings.websocket_ping_timeout * 1000 | |
| pong_waiter = await asyncio.wait_for( | |
| time0 = time.perf_counter() | |
| latency_ping = self.cs_settings.websocket_ping_timeout * 1000 | |
| pong_waiter = await asyncio.wait_for( | |
| connection.ping(), timeout=self.cs_settings.websocket_ping_timeout | |
| ) | |
| time1 = time.perf_counter() | |
| latency_ping = round(time1 - time0, 3) * 1000 | |
| latency_pong = self.cs_settings.websocket_ping_timeout * 1000 |
🤖 Prompt for AI Agents
In custom_components/ocpp/chargepoint.py around lines 489-490, latency_ping and
latency_pong are both set to the websocket_ping_timeout before the ping is
actually sent so latency_ping never reflects the real ping round-trip; instead,
record the send timestamp and set latency_ping when the ping is sent/completed
and only initialize latency_pong after the await for the pong finishes (i.e.,
move the latency_pong assignment to after the wait_for returns or raises) so
latency_ping and latency_pong reflect actual measured times and keep the same
timeout usage for the wait_for call.
| n = int(self._metrics[0][cdet.connectors.value].value or 1) | ||
| except Exception: | ||
| nof_connectors = 1 | ||
| for cid in range(0, nof_connectors + 1): | ||
| _LOGGER.debug(f"trigger status notification for connector={cid}") | ||
| req = call.TriggerMessage( | ||
| requested_message=MessageTrigger.status_notification, | ||
| connector_id=int(cid), | ||
| ) | ||
| resp = await self.call(req) | ||
| if resp.status != TriggerMessageStatus.accepted: | ||
| _LOGGER.warning("Failed with response: %s", resp.status) | ||
| _LOGGER.warning( | ||
| "Forcing number of connectors to %d, charger returned %d", | ||
| cid - 1, | ||
| nof_connectors, | ||
| n = 1 | ||
|
|
||
| # Single connector: only probe 1. Multi: probe 0 then 1..n. | ||
| attempts = [1] if n <= 1 else [0] + list(range(1, n + 1)) | ||
|
|
||
| for cid in attempts: | ||
| _LOGGER.debug("trigger status notification for connector=%s", cid) | ||
| try: | ||
| req = call.TriggerMessage( | ||
| requested_message=MessageTrigger.status_notification, | ||
| connector_id=int(cid), | ||
| ) | ||
| self._metrics[0][cdet.connectors.value].value = max(1, cid - 1) | ||
| return_value = cid > 1 | ||
| break | ||
| return return_value | ||
| resp = await self.call(req) | ||
| status = getattr(resp, "status", None) | ||
| except Exception as ex: | ||
| _LOGGER.debug("TriggerMessage failed for connector=%s: %s", cid, ex) | ||
| status = None | ||
|
|
||
| if status != TriggerMessageStatus.accepted: | ||
| if cid > 0: | ||
| _LOGGER.warning("Failed with response: %s", status) | ||
| # Reduce to the last known-good connector index. | ||
| self._metrics[0][cdet.connectors.value].value = max(1, cid - 1) | ||
| return False | ||
| # If connector 0 is rejected, continue probing numbered connectors. | ||
|
|
||
| return True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t shrink connector count on transient errors.
You reduce cdet.connectors when any non-accepted status (including None on exceptions) is returned. This can incorrectly lower the connector count on timeouts or transient failures.
- if status != TriggerMessageStatus.accepted:
+ if status not in (TriggerMessageStatus.accepted,):
if cid > 0:
- _LOGGER.warning("Failed with response: %s", status)
- # Reduce to the last known-good connector index.
- self._metrics[0][cdet.connectors.value].value = max(1, cid - 1)
- return False
+ if status == TriggerMessageStatus.rejected:
+ _LOGGER.warning("Rejected for connector=%s; reducing connector count", cid)
+ self._metrics[0][cdet.connectors.value].value = max(1, cid - 1)
+ else:
+ _LOGGER.debug("Non-accepted status for connector=%s: %s (not reducing count)", cid, status)
+ return False📝 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.
| n = int(self._metrics[0][cdet.connectors.value].value or 1) | |
| except Exception: | |
| nof_connectors = 1 | |
| for cid in range(0, nof_connectors + 1): | |
| _LOGGER.debug(f"trigger status notification for connector={cid}") | |
| req = call.TriggerMessage( | |
| requested_message=MessageTrigger.status_notification, | |
| connector_id=int(cid), | |
| ) | |
| resp = await self.call(req) | |
| if resp.status != TriggerMessageStatus.accepted: | |
| _LOGGER.warning("Failed with response: %s", resp.status) | |
| _LOGGER.warning( | |
| "Forcing number of connectors to %d, charger returned %d", | |
| cid - 1, | |
| nof_connectors, | |
| n = 1 | |
| # Single connector: only probe 1. Multi: probe 0 then 1..n. | |
| attempts = [1] if n <= 1 else [0] + list(range(1, n + 1)) | |
| for cid in attempts: | |
| _LOGGER.debug("trigger status notification for connector=%s", cid) | |
| try: | |
| req = call.TriggerMessage( | |
| requested_message=MessageTrigger.status_notification, | |
| connector_id=int(cid), | |
| ) | |
| self._metrics[0][cdet.connectors.value].value = max(1, cid - 1) | |
| return_value = cid > 1 | |
| break | |
| return return_value | |
| resp = await self.call(req) | |
| status = getattr(resp, "status", None) | |
| except Exception as ex: | |
| _LOGGER.debug("TriggerMessage failed for connector=%s: %s", cid, ex) | |
| status = None | |
| if status != TriggerMessageStatus.accepted: | |
| if cid > 0: | |
| _LOGGER.warning("Failed with response: %s", status) | |
| # Reduce to the last known-good connector index. | |
| self._metrics[0][cdet.connectors.value].value = max(1, cid - 1) | |
| return False | |
| # If connector 0 is rejected, continue probing numbered connectors. | |
| return True | |
| n = int(self._metrics[0][cdet.connectors.value].value or 1) | |
| except Exception: | |
| n = 1 | |
| # Single connector: only probe 1. Multi: probe 0 then 1..n. | |
| attempts = [1] if n <= 1 else [0] + list(range(1, n + 1)) | |
| for cid in attempts: | |
| _LOGGER.debug("trigger status notification for connector=%s", cid) | |
| try: | |
| req = call.TriggerMessage( | |
| requested_message=MessageTrigger.status_notification, | |
| connector_id=int(cid), | |
| ) | |
| resp = await self.call(req) | |
| status = getattr(resp, "status", None) | |
| except Exception as ex: | |
| _LOGGER.debug("TriggerMessage failed for connector=%s: %s", cid, ex) | |
| status = None | |
| if status not in (TriggerMessageStatus.accepted,): | |
| if cid > 0: | |
| if status == TriggerMessageStatus.rejected: | |
| _LOGGER.warning( | |
| "Rejected for connector=%s; reducing connector count", cid | |
| ) | |
| self._metrics[0][cdet.connectors.value].value = max(1, cid - 1) | |
| else: | |
| _LOGGER.debug( | |
| "Non-accepted status for connector=%s: %s (not reducing count)", | |
| cid, | |
| status, | |
| ) | |
| return False | |
| # If connector 0 is rejected, continue probing numbered connectors. | |
| return True |
🤖 Prompt for AI Agents
In custom_components/ocpp/ocppv16.py around lines 313 to 341, the code currently
reduces cdet.connectors whenever status != accepted, including when status is
None from exceptions; change this so transient failures (exceptions/timeouts
yielding None) do not shrink the connector count. Concretely: treat None as a
transient failure and skip adjusting the metric (log and continue probing), and
only update self._metrics[0][cdet.connectors.value] when you received an actual
response (status is not None) that definitively indicates fewer connectors
(e.g., non-accepted response for a numbered connector). Ensure exceptions
continue to be caught and do not trigger the reduction.
| config_data = MOCK_CONFIG_DATA.copy() | ||
| config_data[CONF_CPIDS].append( | ||
| {request.param["cp_id"]: MOCK_CONFIG_CP_APPEND.copy()} | ||
| ) | ||
| config_data[CONF_PORT] = request.param["port"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shared-mutable test state: use deep copies.
MOCK_CONFIG_DATA.copy() is shallow; appending to CONF_CPIDS mutates the original list across tests and can cause order-dependent flakiness. Deep copy both the config and the CP append blob.
+from copy import deepcopy
@@
- config_data = MOCK_CONFIG_DATA.copy()
- config_data[CONF_CPIDS].append(
- {request.param["cp_id"]: MOCK_CONFIG_CP_APPEND.copy()}
- )
+ config_data = deepcopy(MOCK_CONFIG_DATA)
+ config_data[CONF_CPIDS].append(
+ {request.param["cp_id"]: deepcopy(MOCK_CONFIG_CP_APPEND)}
+ )📝 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.
| config_data = MOCK_CONFIG_DATA.copy() | |
| config_data[CONF_CPIDS].append( | |
| {request.param["cp_id"]: MOCK_CONFIG_CP_APPEND.copy()} | |
| ) | |
| config_data[CONF_PORT] = request.param["port"] | |
| from copy import deepcopy | |
| config_data = deepcopy(MOCK_CONFIG_DATA) | |
| config_data[CONF_CPIDS].append( | |
| {request.param["cp_id"]: deepcopy(MOCK_CONFIG_CP_APPEND)} | |
| ) | |
| config_data[CONF_PORT] = request.param["port"] |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 75 to 79, the test builds config_data using a
shallow copy (MOCK_CONFIG_DATA.copy()) and then appends a CP dict created from
MOCK_CONFIG_CP_APPEND, which mutates shared state across tests; replace the
shallow copy with a deep copy of MOCK_CONFIG_DATA and deep copy
MOCK_CONFIG_CP_APPEND when constructing the appended CP dict (use copy.deepcopy)
so each test gets an independent, non-shared config and CP blob.
Average Current values. Handle cases where meter values bucket contains several EAIR with different contexts (Fixes #1709). Set energy_session initally to 0 to avoid spike. More tests.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests