Skip processing MeterValues when HA reports unavailable sensor values#1806
Skip processing MeterValues when HA reports unavailable sensor values#1806nlindn wants to merge 2 commits intolbbrhzn:mainfrom
Conversation
nlindn
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds explicit short-circuit handling for Home Assistant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
custom_components/ocpp/ocppv16.py (1)
900-906: Duplicate logic (see comment on lines 875-881).This block duplicates the unavailability handling logic from lines 875-881. See the previous comment for refactoring suggestions.
🧹 Nitpick comments (1)
custom_components/ocpp/ocppv16.py (1)
875-881: Consider refactoring duplicate code and improving log message specificity.This block and the similar one at lines 900-906 contain nearly identical logic. Additionally, the log message states "Charger %s is unavailable" when it's more accurate to say the HA entity state is unavailable (the charger itself may still be operational).
Consider extracting the common logic into a helper function:
+def _handle_unavailable_metric(self, metric_name: str) -> bool: + """Return True if should skip MeterValues processing due to unavailable metric.""" + _LOGGER.debug( + "HA entity for %s on charger %s is unavailable — skipping MeterValues processing.", + metric_name, + self.settings.cpid, + ) + self.hass.async_create_task(self.update(self.settings.cpid)) + return TrueThen use it in both locations:
if value and value == STATE_UNAVAILABLE: - _LOGGER.debug( - "Charger %s is unavailable — skipping received MeterValues processing.", - self.settings.cpid, - ) - self.hass.async_create_task(self.update(self.settings.cpid)) - return call_result.MeterValues() + if self._handle_unavailable_metric("meter_start"): + return call_result.MeterValues()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/ocpp/chargepoint.py(1 hunks)custom_components/ocpp/ocppv16.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
custom_components/ocpp/ocppv16.py (1)
custom_components/ocpp/chargepoint.py (3)
value(84-86)value(89-91)update(605-637)
⏰ 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 (2)
custom_components/ocpp/chargepoint.py (1)
1028-1029: Behavior change: early return prevents fallback to other candidates.The early return when
STATE_UNAVAILABLEis encountered prevents checking remaining candidate entity IDs (e.g., falling back from connector-specific to generic sensor). During an HA restart (the primary use case), all entities are unavailable simultaneously, so this is appropriate. However, in edge cases where only the connector-specific sensor is unavailable, the generic sensor won't be used as a fallback.This trade-off is acceptable for the stated objective.
custom_components/ocpp/ocppv16.py (1)
10-10: LGTM!Import correctly added for the new unavailability checks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1806 +/- ##
==========================================
- Coverage 94.99% 94.90% -0.09%
==========================================
Files 12 12
Lines 2975 2985 +10
==========================================
+ Hits 2826 2833 +7
- Misses 149 152 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Stale pull request message |
|
Stale pull request message |
|
Bump, ready for review. |
Review comments above |
nlindn
temporarily deployed
to
continuous-integration
GitHub Actions
Inactive
2 participants
This change stops the MeterValues handler from processing readings when Home Assistant reports an entity as
STATE_UNAVAILABLE. This can happen during or right after an HA restart. Before this fix, the handler could receiveUnavailablefrom HA during the restore phase and mistakenly treat it as the value is not available at all. That caused incorrect restoration ofenergy_meter_startandtransaction_id, which in turn produced wrongenergy_sessionandenergy_active_importvalues.Changes
get_ha_metric(...)returnsSTATE_UNAVAILABLE, the incoming MeterValues message is skipped.Example
After an HA reboot, all
evchargerentities may temporarily showunavailablewhile the charger keeps sending OCPP MeterValues. Previously, this led toenergy_meter_startbeing possibly restored asNone, which produced incorrect energy calculations (energy_session,energy_active_import). With this change, the handler returns early and waits for HA to provide real values again, avoiding using responses before data is available in HA to be restored.Summary by CodeRabbit