Fix: Calculate real-time Session Energy for phased MeterValues#1920
Fix: Calculate real-time Session Energy for phased MeterValues#1920ric866 wants to merge 13 commits intolbbrhzn:mainfrom
Conversation
Refactor metric unit handling to compute final_value/final_unit once instead of repeating assignments for power and energy conversions. Add logic to amend session energy when the charger doesn't report it directly: on Energy.Active.Import.Register updates (DEFAULT_MEASURAND) and when not _charger_reports_session_energy, detect an active transaction, use meter_start and session_energy metrics to initialize baseline if missing, and compute session energy as (current_total - start_total) with unit parity checks and rounding.
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors per-phase metric assignment to compute a consolidated final value/unit, fixes a meter-start metric lookup bug, and adds session-energy derivation (or meter_start initialization) during phase aggregation when the charger does not report session energy directly. Changes
Sequence Diagram(s)sequenceDiagram
participant Charger as Charger
participant Processor as ChargePoint.process_phases
participant Metrics as MetricsStore
participant Session as CSess
Charger->>Processor: send MeasurandValue(s) (EAIR / phases)
Processor->>Metrics: lookup per-connector metrics (meter_start, session_energy, etc.)
Processor->>Processor: compute per-phase final_value / final_unit
alt csess has transaction and meter_start exists
Processor->>Session: calculate session_energy = final_value - meter_start
Processor->>Metrics: write updated `session_energy` and main register
else meter_start is None
Processor->>Metrics: set meter_start = final_value
Processor->>Metrics: set session_energy = 0.0
end
Processor->>Metrics: persist consolidated metric (target_cid, metric)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
- Coverage 94.99% 94.95% -0.04%
==========================================
Files 12 12
Lines 2975 2992 +17
==========================================
+ Hits 2826 2841 +15
- Misses 149 151 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…hub.com/ric866/hassio-ocpp into fix-session-energy-multiphased-chargers
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
While looking through the process_phases neutral voltage overwrite that my SyncEV gets itself into, I noticed why Energy Session doesn't update during the charge (values are currently only processed into Session values if they are Phase : None). This is my attempt at a fix inside the phase processing function. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/ocpp/chargepoint.py`:
- Line 836: The lookup for the stored metric uses the wrong key type: change the
_metrics.get call to use the Enum's string value so the stored metric is found;
specifically, when retrieving ms_metric use csess.meter_start.value (same
pattern as csess.session_energy.value) so
_ConnectorAwareMetrics.get((target_cid, csess.meter_start.value)) returns the
stored metric and the session energy calculation runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b917d8e-0254-4645-8319-b304e9d0e615
📒 Files selected for processing (2)
custom_components/ocpp/chargepoint.pytests/test_charge_point_core.py
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_charge_point_core.py (1)
367-393: Add one test for the inactive-transaction guard path.These additions cover the two positive branches well, but the new guard (
active transaction required) is not exercised yet. A no-transaction test would lock in thatsession_energyis not derived when transaction state is missing/false.✅ Suggested test addition
+def test_process_phases_does_not_derive_session_energy_without_active_transaction(hass): + cp = _mk_cp(hass) + target_cid = 1 + + cp._metrics[(target_cid, csess.meter_start.value)].value = 100.0 + cp._metrics[(target_cid, csess.meter_start.value)].unit = HA_ENERGY_UNIT + cp._metrics[(target_cid, csess.session_energy.value)].value = 0.0 + cp._metrics[(target_cid, csess.session_energy.value)].unit = HA_ENERGY_UNIT + cp._metrics[(target_cid, csess.transaction_id.value)].value = None # inactive + + bucket = [ + _mv("Energy.Active.Import.Register", 105.0, phase="L1", unit=HA_ENERGY_UNIT) + ] + cp.process_phases(bucket, connector_id=target_cid) + + assert cp._metrics[(target_cid, csess.session_energy.value)].value == 0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_charge_point_core.py` around lines 367 - 393, Add a new unit test that exercises the "active transaction required" guard by creating a ChargePoint via _mk_cp, ensuring the transaction flag/metric (csess.transaction_id) is absent or falsy, setting meter_start to None and session_energy to None, then calling cp.process_phases with a single L1 Energy.Active.Import.Register reading (use _mv and HA_ENERGY_UNIT) and asserting that meter_start and session_energy remain None (or that session_energy is not derived). Reference cp.process_phases, cp._metrics, csess.transaction_id, csess.meter_start, csess.session_energy, _mk_cp, _mv and HA_ENERGY_UNIT so the new test mirrors the existing tests but with no active transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_charge_point_core.py`:
- Around line 367-393: Add a new unit test that exercises the "active
transaction required" guard by creating a ChargePoint via _mk_cp, ensuring the
transaction flag/metric (csess.transaction_id) is absent or falsy, setting
meter_start to None and session_energy to None, then calling cp.process_phases
with a single L1 Energy.Active.Import.Register reading (use _mv and
HA_ENERGY_UNIT) and asserting that meter_start and session_energy remain None
(or that session_energy is not derived). Reference cp.process_phases,
cp._metrics, csess.transaction_id, csess.meter_start, csess.session_energy,
_mk_cp, _mv and HA_ENERGY_UNIT so the new test mirrors the existing tests but
with no active transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64356d42-2635-4b21-bb5c-5f0455b1fd7d
📒 Files selected for processing (1)
tests/test_charge_point_core.py
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
force-pushed
the
fix-session-energy-multiphased-chargers
branch
from
88daaaa to
6dad97e
Compare
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
force-pushed
the
fix-session-energy-multiphased-chargers
branch
from
6dad97e to
22bc955
Compare
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
force-pushed
the
fix-session-energy-multiphased-chargers
branch
from
22bc955 to
8e942c9
Compare
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
How does a 3 phase charger provide the "Energy.Active.Import.Register", does it report it for each phase or in totality? If this is only required for single phase chargers that include phase info, prior to the current logic that only processes phase None could you simply update the measurand phase to None and avoid adding all the extra logic? |
Hi @drc38, I did a bit of digging into this... and I prefer your solution (simple is better for me)... But... It might be a problem for chargers taking liberties with the protocol, now, or in future, (even though as far as I can tell, it's only lazy single phase chargers that currently tag phase onto "Energy.Active.Import.Register"). Looking at the OCPP1.6 specification (ocpp-1.6 edition 2), phase is optional on seemingly all measurands, but the "Energy.Active.Import.Register" is defined in Section 7.31, as the "Numerical value read from the 'active electrical energy' (Wh or kWh) register of the (most authoritative) electrical meter measuring energy imported (from the grid supply)". Suggesting but not stating it should not be tagged with a phase, though 3 phase chargers are capable of measuring individually. I've raised PR #1921, but it fails one of the tests, owing to the testing sending all three phases. If that test was derived from a real world charger / scenario, IMHO we should definitely go with this extra logic to make sure all cases / weirdness is correctly handled. Kind regards Ric |
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/ocpp/chargepoint.py`:
- Around line 825-855: In process_measurands, the dictionary lookup for the
meter-start metric uses csess.meter_start (the enum) instead of
csess.meter_start.value, causing a new Metric key to be created and breaking
baseline initialization; change the lookup that sets ms_metric from using
(target_cid, csess.meter_start) to (target_cid, csess.meter_start.value) so it
matches other usages (e.g., csess.session_energy.value) and correctly retrieves
the existing metric for baseline math.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3de367c-37e1-4594-b8ca-f48f5db37998
📒 Files selected for processing (2)
custom_components/ocpp/chargepoint.pytests/test_charge_point_core.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_charge_point_core.py
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
force-pushed
the
fix-session-energy-multiphased-chargers
branch
from
c8927e5 to
829249c
Compare
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@custom_components/ocpp/chargepoint.py`:
- Around line 823-856: process_measurands() currently normalizes measurand,
unit, value and context into local vars but still appends the original raw
sampled_value to unprocessed, which causes phase-tagged
Energy.Active.Import.Register (EAIR) entries to be dropped or aggregated with
wrong units in process_phases(); instead, construct and append a normalized
MeasurandValue(measurand, value, phase, unit, context, location) (with
DEFAULT_MEASURAND, converted numeric value and resolved unit when applicable)
into unprocessed so downstream process_phases() and the session-energy logic
(which checks DEFAULT_MEASURAND and _charger_reports_session_energy) receive the
normalized entry and update ms_metric/se_metric correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6ad0722-c1ea-475e-9482-3eedfefb8ff2
📒 Files selected for processing (1)
custom_components/ocpp/chargepoint.py
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
ric866
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
PR lbbrhzn#1920 has got into a mess. This is the cleaned up version.
2 participants
This PR fixes a bug where the Session Energy sensor fails to update during an active charging session for chargers that send phase-specific energy readings (e.g.,
Energy.Active.Import.Register.L1).For these chargers, the Session Energy sensor currently remains at
0for the duration of the charge and only populates with the final value when theStopTransactionpayload is received.Root Cause :
In
on_meter_values, the real-time session energy math (Current - Start) is currently trapped inside theif phase is None:block.If a charger sends a phase-tagged main energy register, it bypasses this block and gets passed to
process_phases()via theunprocessedlist. Whileprocess_phases()correctly aggregates the phases and updates the main lifetime energy sensor, it lacked the logic to simultaneously update the active Session Energy sensor.Fix :
Added the standard session energy calculation directly to the end of
process_phases().final_valuevariable.final_value - meter_start) and updates the metric so the HA dashboard graph updates continuously during the charge.Testing :
L1energy registers now correctly increment the Session Energy sensor in real-time upon receiving periodicMeterValues._charger_reports_session_energy).Summary by CodeRabbit
Bug Fixes
Tests