Feat/kaco nh3 hybrid template - #33044
Conversation
…id, Battery)\n\n- Modbus-RTU (RS485) connection support with configurable baud rate\n- Grid, PV (with curtailment), and Battery usage modes\n- SunSpec compliant (Models 701, 702, 703, 704, 714, 802)\n- Dynamic power calculation from battery voltage (P = U × 30A)\n- Full battery control (normal/hold/charge modes)\n- Curtailment via Model 704 with Model 123 fallback\n\nVerified against KACO Blueplanet Hybrid NH3 SunSpec PICS template
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
requirements.descriptiontext hardcodes a 9600 baud Modbus-RTU connection, but the template exposes a configurablebaudrateparam (9600/19200/38400); consider updating the description so it aligns with the actual configuration options. - The
modBusDeviceIdparameter name is inconsistent with the other parameter naming (mixed casing vs. lowercase with no camelCase); consider normalizing this to match existing conventions for easier template reuse and discoverability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `requirements.description` text hardcodes a 9600 baud Modbus-RTU connection, but the template exposes a configurable `baudrate` param (9600/19200/38400); consider updating the description so it aligns with the actual configuration options.
- The `modBusDeviceId` parameter name is inconsistent with the other parameter naming (mixed casing vs. lowercase with no camelCase); consider normalizing this to match existing conventions for easier template reuse and discoverability.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/kaco-nh3-hybrid.yaml" line_range="118" />
<code_context>
+ power:
+ source: sunspec
+ {{- include "modbus" . | indent 2 }}
+ value: {{ if eq .battery "2" }}714:Prt.2.DCW{{ else }}714:Prt.1.DCW{{ end }} # Model 714 (DC Module - Battery Power)
+ # Prt.1 = Battery 1, Prt.2 = Battery 2 via Model 714 (Multi-Port DC Module)
+ soc:
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the battery parameter type with the comparison to avoid a non-matching condition.
`battery` is defined as a numeric choice (`[1, 2]`), so YAML will parse it as integers, but the template compares it to a string (`"2"`). That condition will never be true, and `Prt.1.DCW` will always be used. Please either make the choices strings (`["1", "2"]`) or compare against an integer (`if eq .battery 2`) so the correct port is selected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Fix hardcoded baud rate in requirements.description to mention configurable options (9600, 19200, 38400) - Normalize parameter naming: modBusDeviceId → modbusdeviceid (lowercase, no camelCase) - Fix battery parameter type mismatch: change choice from [1, 2] to ["1", "2"] for string comparison - Battery selector now correctly uses string comparison (if eq .battery "2") matching the parameter type
…SunSpec Model 802 - Add capacity source: 802:WHRtg (Battery nominal capacity in Wh) - Add minsoc source: 802:SoCMin (Minimum SOC limit % from inverter) - Add maxsoc source: 802:SoCMax (Maximum SOC limit % from inverter) - Remove manual parameter definitions - all values now read from inverter - Parameters automatically sync with inverter settings (no manual configuration needed) - If SoC limits change on inverter, EVCC reads new values automatically
| config: | ||
| source: sunspec | ||
| {{- include "modbus" . | indent 6 }} | ||
| value: 704:WMaxLimPctEna # Model 704 at address 50639 |
There was a problem hiding this comment.
drop the comments, they don't provide additional value
There was a problem hiding this comment.
okey. i put them in so i could remember where it was coming from. but okey. i can leave them out.
| # Formula: Power (W) = Battery Voltage (V) × Max Current (30 A) | ||
| # Battery voltage varies with SoC (e.g., 300-330V for 6-series Dyness stack) | ||
| # This ensures accurate power limits throughout the charge cycle | ||
| maxchargepower: |
There was a problem hiding this comment.
pls keep it simple- too much magic
There was a problem hiding this comment.
hmm. i would like this limitation as the charging/discharging power is highly dependent on the battery voltage (for high voltage batteries). if it was a low voltage battery then it's more simpel.
There was a problem hiding this comment.
We're not doing this for any other (HV) battery sofar. Lets keep it consistent and simple.
Co-authored-by: andig <cpuidle@gmail.com>
|
Code review Found 2 high-confidence issues in the new template.
The custom modbusdeviceid param (default 3) is never referenced in the render section below. Since the template does not define a param literally named id, evcc auto-injects the standard id param (see ModbusParams in evcc/util/templates/template_modbus.go Lines 28 to 31 in eeb2f59 evcc/util/templates/defaults.yaml Lines 844 to 849 in eeb2f59
evcc's SunSpec point grammar only supports model:point or model:block:point, colon-delimited (see ParsePoint at Lines 16 to 42 in eeb2f59 evcc/templates/definition/meter/sunspec-hybrid-curtailable.yaml Lines 20 to 27 in eeb2f59 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the
curtail.setswitch for the 100% case, you have a nestedsetblock undersource: const(const → set → sunspec), which is a different structure than the default branch and may not match the expected schema for a switch case; consider aligning it with the default branch pattern (setdirectly setting the Sunspec value) to avoid runtime misconfiguration. - The
maxchargepowerandmaxdischargepowergoscripts duplicate the same logic for reading 802:V and computingint(voltage * 30); if the template system allows, consider extracting this into a shared helper or a single computed field to avoid divergence if the formula needs updating later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `curtail.set` switch for the 100% case, you have a nested `set` block under `source: const` (const → set → sunspec), which is a different structure than the default branch and may not match the expected schema for a switch case; consider aligning it with the default branch pattern (`set` directly setting the Sunspec value) to avoid runtime misconfiguration.
- The `maxchargepower` and `maxdischargepower` `go` scripts duplicate the same logic for reading 802:V and computing `int(voltage * 30)`; if the template system allows, consider extracting this into a shared helper or a single computed field to avoid divergence if the formula needs updating later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ve comments - Simplify curtailment switch default case to align with case 100 pattern - Remove inline comments that don't add significant value - Keep core comments for critical logic - Improves code consistency and reduces verbosity - All fixes address Sourcery AI review feedback
Add KACO NH3 Hybrid Inverter Template (Modbus-RTU)
Description
Add complete support for KACO Blueplanet Hybrid NH3 inverters with full hybrid functionality and automatic inverter synchronization.
Features
Verified Against
Key Improvements
Notes