Conversation
7cbb736 to
954b320
Compare
954b320 to
be2a709
Compare
|
Hello! This is an automated message regarding your pull request. We've noticed that this PR has been inactive for a while, with no updates or activity in the last 30 days. If no further updates or responses are provided within the next 7 days, this pull request will be closed as stale. If you're still actively working on this PR, please leave a comment to let us know. We're happy to keep it open and review your changes. Otherwise, feel free to reopen the PR if you have more work to contribute in the future. Thank you for your understanding and contributions! |
|
not stale |
|
Hello! This is an automated message regarding your pull request. We've noticed that this PR has been inactive for a while, with no updates or activity in the last 30 days. If no further updates or responses are provided within the next 7 days, this pull request will be closed as stale. If you're still actively working on this PR, please leave a comment to let us know. We're happy to keep it open and review your changes. Otherwise, feel free to reopen the PR if you have more work to contribute in the future. Thank you for your understanding and contributions! |
|
no, please don't close this.... |
kr0ner
left a comment
kr0ner
left a comment
There was a problem hiding this comment.
instead of duplicating the content, we should create a wxx_base.yaml (analog to the txx_base.yaml), that carries the common parts for WPL_XXX and WPE-XXX and adjust the inheritance
address kr0ner#363 The WPM family is not limited to the WPL models. The WPE-I speaks the same protocol but has no integrated DHW, and its Manager does not serve the BETRIEBS_STATUS word (verified on a WPE-I 08 HK 230 Premium, where the panel showed Heizkreispumpe 2 running while all twelve binaries stayed off). Split wpl_base.yaml the same way txx_base.yaml and thz_base.yaml are layered: - wxx_base.yaml carries everything common to WPL and WPE - wpl_base.yaml includes it and adds the WPL specific parts, namely the Manager BETRIEBS_STATUS binaries and the four DHW energy counters No behavior change for wpl13, wpl17 and wpl23: they keep including wpl_base.yaml and end up with exactly the same entities as before.
be2a709 to
4c1a851
Compare
adrai
changed the title
|
Thanks, that layering is much better than what I had. Reworked along your suggestion, modelled on Those two blocks are WPL specific because the WPE-I has no integrated DHW, and its Manager does not serve the BETRIEBS_STATUS word at all: on my machine the panel showed Heizkreispumpe 2 running while all twelve binaries stayed off. No behaviour change for the existing models. I diffed the fully resolved |
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain with model-specific blocks and unverified sensor mappings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces wxx_base.yaml as a shared configuration layer for WPL/WPE variants and updates the documented hierarchy.
Changes:
- Extracts shared configuration into
yaml/wxx_base.yaml. - Updates
yaml/wpl_base.yamlto consume the shared base. - Revises the README hierarchy documentation.
File summaries
| File | Summary |
|---|---|
yaml/wxx_base.yaml |
Adds shared WXX configuration; refrigerant sensors with unverified mappings should remain model-specific. |
yaml/wpl_base.yaml |
Consumes the shared base, but DHW energy and Manager status blocks still require relocation. |
README.md |
Updates the configuration hierarchy diagram. |
Review details
Suppressed comments (1)
yaml/wxx_base.yaml:44
- These refrigerant-circuit sensors are being promoted into the WXX layer, but the PR description explicitly says the WPE-I readings for
VERDAMPFERTEMP,RUECKLAUFTEMP,VORLAUFTEMP,VERDICHTER_EINTRITTSTEMP, andHEISSGAS_TEMPare still known to map incorrectly. A WPE manifest consuming this shared base would expose those incorrect values; keep unresolved model-specific sensors out ofwxx_base.yamluntil their mappings are verified.
VERDAMPFERTEMP: !include { file: templates/wp_temperature.yaml, vars: { property: "VERDAMPFERTEMP" , target: "Heizmodul" }}
RUECKLAUFISTTEMP: !include { file: templates/wp_temperature.yaml, vars: { property: "RUECKLAUFISTTEMP" , update_interval: $interval_medium , target: "MFG" }}
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Related: #363
Reworked after review: instead of duplicating blocks into the three WPL manifests, this introduces a shared base layer, modelled on the existing
txx_base.yaml/thz_base.yamllayering.wxx_base.yaml(new) carries everything common to the WPL and WPE modelswpl_base.yamlnow includes it and keeps only the WPL specific parts: the ManagerBETRIEBS_STATUSbinaries with their callback, and the four DHW energy counterswpl13.yaml/wpl17.yaml/wpl23.yamlare untouched, they still includewpl_base.yamlWhy those two blocks are WPL specific, both verified on a WPE-I 08 HK 230 Premium:
0x8000"not available" marker.BETRIEBS_STATUSword always reads 0 on this machine. The panel showed Heizkreispumpe 2 running while all twelve binaries stayed off. The WPE-I publishes its status in two other words, see Add support for WPE-I (brine/water, no DHW) #367.No behaviour change for the existing models. I diffed the fully resolved
esphome configoutput for wpl13, wpl17 and wpl23 before and after this change: identical in all three cases (264, 476 and 266 entries respectively). The README hierarchy diagram is updated to match.Tested with ESPHome 2026.6.5: config validated for all nine model manifests, and wpl17, wpl23, thz504 and thz304 compiled.