Feature: Configuration of the battery capacity#2465
Feature: Configuration of the battery capacity#2465SW-Niko wants to merge 4 commits intohoylabs:developmentfrom
Conversation
add the nominal voltage and the capacity to config and webUI
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
👮 Files not reviewed due to content moderation or server errors (4)
WalkthroughAdds nominal battery metadata fields to configuration and runtime stats, propagates them through CAN provider parsing, live-view and MQTT publishing (Home Assistant), and exposes editable fields and localization in the web frontend. Changes
Sequence Diagram(s)sequenceDiagram
participant CAN as CAN Bus
participant Provider as Pytes Provider
participant Stats as Battery Stats
participant MQTT as MQTT / HA
participant Live as Web LiveView
CAN->>Provider: delivers message (e.g., 0x379 / 0x409)
Provider->>Stats: setNominalCapacity(capacity)
Provider->>Stats: update SOC using local capacity
Stats->>MQTT: publish nominalCapacity / nominalVoltage (if present)
Stats->>Live: include nominalCapacity / nominalVoltage in live-view JSON (if present)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
src/Configuration.cpp (1)
142-143: Serialize from the function argument instead of global state.Line [142]-Line [143] ignore
sourceand read fromconfig.Battery, which makes this serializer context-dependent and harder to reuse/test.♻️ Proposed fix
- target["nominal_voltage"] = config.Battery.NominalVoltage; - target["nominal_capacity"] = config.Battery.NominalCapacity; + target["nominal_voltage"] = source.NominalVoltage; + target["nominal_capacity"] = source.NominalCapacity;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Configuration.cpp` around lines 142 - 143, The serializer is reading from the global config.Battery instead of the function argument 'source', making it context-dependent; update the assignments so target["nominal_voltage"] and target["nominal_capacity"] use source.Battery.NominalVoltage and source.Battery.NominalCapacity (replace references to config.Battery with source.Battery) so the serializer operates on the provided source object rather than global state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Configuration.cpp`:
- Around line 577-578: Validate and sanitize the incoming nominal battery fields
instead of trusting the client: read source["nominal_voltage"] and
source["nominal_capacity"], ensure they are numeric and within allowed ranges
(e.g. nominal_voltage > 0 and within a sane upper limit, and nominal_capacity >
0 and within a sensible max), and then assign to target.NominalVoltage and
target.NominalCapacity only after validation; on out-of-range or non-numeric
values either clamp to the permitted min/max or return/raise a validation error
and log the rejection (include the field names nominal_voltage/nominal_capacity
and the offending value in the log) so invalid payloads cannot propagate into
Solar‑Surplus calculations.
---
Nitpick comments:
In `@src/Configuration.cpp`:
- Around line 142-143: The serializer is reading from the global config.Battery
instead of the function argument 'source', making it context-dependent; update
the assignments so target["nominal_voltage"] and target["nominal_capacity"] use
source.Battery.NominalVoltage and source.Battery.NominalCapacity (replace
references to config.Battery with source.Battery) so the serializer operates on
the provided source object rather than global state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74995799-3ff0-4d3c-a9fe-aa06dcd7ce0d
📒 Files selected for processing (6)
include/Configuration.hsrc/Configuration.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.jsonwebapp/src/types/BatteryConfig.tswebapp/src/views/BatteryAdminView.vue
Build ArtifactsFirmware built from this pull request's code:
Notice
|
read from source instead from config.Battery
|
What if the used Battery Provider already provides the capacity? Can we make use of it and hide the input field? e.g. Another topic is that merging this without merging the surplus feature as well does not make much sense. I know that this is a problem that i created because i don't have enough capacity to look into bigger PRs : / I am very happy about all the effort you put into this project!! Do you have any suggestions on how to improve the workflow? Having small/tiny PRs is amazing and i want to keep going like that. Or maybe feature-flags using |
Yes sure. The less configuration required, the fewer mistakes will be made. That's a good suggestion. 👍
I will update the PR, but will need some time to check all available provider.
To be honest ... I don't have much experience leading software projects. |
|
@AndreasBoehm i Like Both ideas. Branches and #defines. The following will be OT in this PR: I too startet working on local forks. I would like to contribute, but I don’t know how, as some small steps wouldn’t make sense without the overall goal. But all together might take some time and could be big. Like, a small step would be my „https powermeter voltage“ branch (https://github.com/spcqike/OpenDTU-OnBattery/tree/https_powermeter_voltage ), which lets you also query the voltages. Like some meters already do (Victron, SML) tjis is already working, but not perfect. I also created a #define firmware. (https://github.com/spcqike/OpenDTU-OnBattery/tree/reduce-OTA-size) as mentioned in #2459 So, I also would like to contribute smaller steps, even if they don’t make sense on themself, and I would like to use defines, also later on (like tasmota does) as I think it’s a great feature to customize / reduce firmware later on. |
- add nominal capacity and nominal voltage to stats - in general use information from the provider if available, up to now only pytes - remove redundant values from pytes - hide configuration values on the WebUI if they are not necessary
|
I've checked all battery providers.
All text have been changed to "Nominal charge" and "Nominal voltage." Note: This changed the MQTT topic from "Total Charge" to "Nominal charge." I haven't gone through the battery data sheets to see if we can read the data. |
|
Hello @vaterlangen, Could you please take a look at this PR? I suspect the Zendure Battery is also affected. If so, then the provider would need to be adjusted to the changed general stats, and the duplicate data in the Zenture Provider would need to be removed. Would you prefer to adjust this yourself later, or should I change it now? |
|
@SW-Niko I have also changes pending in #2451 where I (due to different dataset available via local and official API) shifted to std::optional for the values within stats. Wouldn't that also help here, in case no battery capacity has been reported? Currently I have other stuff pending, so no much time for looking at this.... |
4 participants
Enables configuration of:
This information is required for the Solar-Surplus calculation.
You can find more information here: Link