Added detection between session and main meter energy values

[nlindn]

Added detection between session and main meter energy values as discussed in #366.

According to OCPP spec, if there is a transaction id the values are for that session and if there is no transaction id the values are from main meter. For example ABB Terra AC sends separate energy used for the session and the charger lifetime energy usage.

Now if there is a transaction id with the energy value, it is saved to entity "xxx.Session" (example: "Energy.Active.Import.Register.Session")

If there is no transaction id with the energy value, it is saved to entity "xxx" (example: ""Energy.Active.Import.Register)

This way the energy values between the sessions and main meters are kept separated so that they don't mix in the homeassistant history and graphs. Also, if the charger gives the energy used in the session it is not calculated duplicated.

[drc38]

Thanks, I take it ABB have not released a new firmware to conform to 3.9 of the errata then?
https://github.com/mobilityhouse/ocpp/blob/master/docs/v16/ocpp-1.6-errata-sheet.pdf

[nlindn]

Thanks, I take it ABB have not released a new firmware to conform to 3.9 of the errata then? https://github.com/mobilityhouse/ocpp/blob/master/docs/v16/ocpp-1.6-errata-sheet.pdf

Yeah, the spec 3.9 is not implemented yet in Terra AC. The Sample.Periodic response has the session energy started from 0 for every session.

receive message [2, "2503015", "MeterValues", {"connectorId": 1, "transactionId": 1695585075, "meterValue": [{"timestamp": "2023-09-24T21:40:03.000Z", "sampledValue": [{................................., {"value": "1868", "context": "Sample.Periodic", "format": "Raw", "measurand": "Energy.Active.Import.Register", "unit": "Wh"}, ..................................}] }] } ]

And the Sample.Clock sends the main meter values.

receive message [2, "2778188", "MeterValues", {"connectorId": 1, "meterValue": [{"timestamp": "2023-09-24T21:45:00.000Z", "sampledValue": [{"value": "927435", "context": "Sample.Clock", "format": "Raw", "measurand": "Energy.Active.Import.Register", "location": "Inlet"}]}]}]

If other chargers on the market are sending the main meter value not starting from 0 as suggested in the 3.9 of the errata. Then the default assumption in this pull request implementation is maybe not the best. Do we want the default assumption to be that the energy values does not start from zero for transactions and if 3.9 of the errata is not yet implemented, then do some workarounds to stop the mixing of values that is happening now?

[drc38]

If other chargers on the market are sending the main meter value not starting from 0 as suggested in the 3.9 of the errata. Then the default assumption in this pull request implementation is maybe not the best. Do we want the default assumption to be that the energy values does not start from zero for transactions and if 3.9 of the errata is not yet implemented, then do some workarounds to stop the mixing of values that is happening now?

That would be better, it seems to only be the ABB units with the issue. Also any reason to change from Energy.Session as it will break any reporting users have setup etc?

[nlindn]

That would be better, it seems to only be the ABB units with the issue. Also any reason to change from Energy.Session as it will break any reporting users have setup etc?

Changed the logic of the fix as discussed and kept the entitie names unchanged to avoid breaking setups.

[drc38]

@nlindn we shouldn't need to maintain a list of charger models in the integration that do not support errata 3.9. Can you rework to remove that requirement please.

[drc38]

Why did you drop the test case in the latest commits?

[nlindn]

@nlindn we shouldn't need to maintain a list of charger models in the integration that do not support errata 3.9. Can you rework to remove that requirement please.

From the meterStart value on startTransaction it can be detected if the errata 3.9 is fulfilled or not. Problem is that if homeassistant is restarted during a charging session the meterStart value is not available after the reboot for the still ongoing session. Any ideas for a good solution for getting the meterStart value for ongoing session if startTransaction is missed?

[drc38]

@nlindn we shouldn't need to maintain a list of charger models in the integration that do not support errata 3.9. Can you rework to remove that requirement please.

From the meterStart value on startTransaction it can be detected if the errata 3.9 is fulfilled or not. Problem is that if homeassistant is restarted during a charging session the meterStart value is not available after the reboot for the still ongoing session. Any ideas for a good solution for getting the meterStart value for ongoing session if startTransaction is missed?

It should be fairly rare for a HA restart to occur during a charge session. I wouldn't worry about it.

[nlindn]

Reworked the logic so it is automatically detected if the charger uses errata 3.9 or not from the meter start value. After reboots of HA, values for meter start and active transaction id are restored from HA which remembers the last values. Before, if HA was rebooted during a session, the ongoing transaction was seen as unknown transaction because the active transaction id was lost during the reboot. This is now fixed with the restore feature. Now the only requirement for the non errata 3.9 detection to work should be that when the integration is started for the first time the charge should be unplugged so the meterStart value is not missed for the first session otherwise the integration works as before until the next session starts.

[drc38]

Great work 🙏. Please add jsonschema==4.19.0 to requirements_dev.txt as I suspect that is a reason for the tests not running. Cheers

[nlindn]

Removed duplicated tests. Timeout of 30sec was still not enough so increased the timeout for test_cms_responses in test_charge_point.py to 60sec.

[codecov]

Codecov Report

Merging #919 (5da8b33) into main (01c6cee) will increase coverage by 0.45%.
Report is 1 commits behind head on main.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main     #919      +/-   ##
==========================================
+ Coverage   89.80%   90.26%   +0.45%     
==========================================
  Files          10       10              
  Lines        1305     1335      +30     
  Branches      181      187       +6     
==========================================
+ Hits         1172     1205      +33     
- Misses         73       75       +2     
+ Partials       60       55       -5     

Files	Coverage Δ
custom_components/ocpp/api.py	87.06% <90.00%> (+0.88%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[drc38]

Removed duplicated tests. Timeout of 30sec was still not enough so increased the timeout for test_cms_responses in test_charge_point.py to 60sec.

Nice one, just linting to fix and take a look at codecov report to see if you can increase test coverage.

[drc38]

@nlindn, testing the changes on my unit and it breaks the logic for a normal charger

Please review

[drc38]

@nlindn this should only be /1000 if unit == DEFAULT_ENERGY_UNIT

[drc38]

Same applies here, suggest moving your meter start logic outside into a separate if statement

[drc38]

adding assert float(cs.get_metric("test_cpid", "Energy.Active.Import.Register")) == float(1101.452) here will show error with current code

[nlindn]

@nlindn, testing the changes on my unit and it breaks the logic for a normal charger

Yeah, the logic has some bugs that passed the tests but reports values wrong on errata 3.9 chargers. Should I open a new pr for fixes or what is the best way to do it?

[drc38]

Open a new PR