-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add Xtherma heatpump (experimental) #26788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `templates/definition/charger/xtherma.yaml:12-14` </location>
<code_context>
+params:
+ - name: modbus
+ choice: ["tcpip"]
+ - name: tempsource
+ type: choice
+ choice: ["warmwater", "buffer"]
+render: |
+ type: heatpump
</code_context>
<issue_to_address>
**issue:** The `tempsource` parameter is defined but not used in the rendered config.
Since changing `tempsource` currently has no effect, either wire it into the template logic (e.g., conditionally select different `temp`/`limittemp` registers or scaling based on its value) or remove it until it’s actually used to avoid confusing users.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
55Jojo2024
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had a chance to properly test this yet, the sun isnt shining, but these are my findings so far.
| type: input | ||
| encoding: int32 | ||
| scale: 10 | ||
| limittemp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be templimit instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you guessing or did you actually test this? See https://github.com/evcc-io/evcc/blob/master/charger/measurement/heating.go#L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @55Jojo2024 did you test this?
Co-authored-by: Indianer <[email protected]>
Co-authored-by: Indianer <[email protected]>
Co-authored-by: Indianer <[email protected]>
Co-authored-by: Indianer <[email protected]>
Fix #26409