Skip to content

Conversation

@mfuchs1984
Copy link
Collaborator

Fix #24991

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `templates/definition/meter/goodwe-hybrid.yaml:233-235` </location>
<code_context>
                 type: writemultiple
                 encoding: uint16
   capacity: {{ .capacity }} # kWh
+  maxchargepower: {{ .maxchargepower }} # W
+  minsoc: {{ .minsoc }} # %
+  maxsoc: {{ .maxsoc }} # %
   {{- end }}
</code_context>

<issue_to_address>
**suggestion:** Consider including 'maxdischargepower' in the rendered output for completeness.

Since 'maxchargepower' is present, adding 'maxdischargepower' would improve consistency and ensure all relevant parameters are available for downstream use.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andig andig added the devices Specific device support label Nov 3, 2025
@andig andig marked this pull request as draft November 3, 2025 18:47
@mfuchs1984
Copy link
Collaborator Author

Could you help me understand what's wrong with the UI job?

@andig
Copy link
Member

andig commented Nov 4, 2025

Looks like invalid white space or line feed?

@mfuchs1984 mfuchs1984 marked this pull request as ready for review November 4, 2025 08:20
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@mfuchs1984
Copy link
Collaborator Author

I know there were some discussions on the defaults. Should we keep them or not? What happens when they are not configured, will evcc enforce setting them?

@mfuchs1984 mfuchs1984 marked this pull request as draft November 4, 2025 08:27
@andig
Copy link
Member

andig commented Nov 4, 2025

Should we keep them or not?

Or not. That's why we have them ;)

will evcc enforce setting them?

As test will show- yes.

@andig andig marked this pull request as ready for review November 5, 2025 08:12
@andig andig merged commit 3338383 into evcc-io:master Nov 5, 2025
7 checks passed
@mfuchs1984
Copy link
Collaborator Author

@naltatis Looks like the settings don't show up in the UI configuration. When looking at e.g. Tesla Powerwall, they show up in the advanced settings section. In the Powerwall template, they are marked with advanced: true. Shouldn't they also show up when they are not advanced, something missing in the UI? Or should they always be advanced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devices Specific device support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants