-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Home Assistant meter: add active battery control #26327
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
base: master
Are you sure you want to change the base?
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, and left some high level feedback:
- Consider making the strings 'normal', 'hold', and 'charge' configurable via template parameters so the setup can work with Home Assistant entities that use different option labels for the same battery modes.
- You currently restrict the mode entity to the
input_selectdomain; if some setups useselector another entity type for battery mode, you may want to broaden theservicefilter or make the domain configurable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the strings 'normal', 'hold', and 'charge' configurable via template parameters so the setup can work with Home Assistant entities that use different option labels for the same battery modes.
- You currently restrict the mode entity to the `input_select` domain; if some setups use `select` or another entity type for battery mode, you may want to broaden the `service` filter or make the domain configurable.
## Individual Comments
### Comment 1
<location> `templates/definition/meter/homeassistant.yaml:159-160` </location>
<code_context>
+ {{- if and (eq .usage "battery") .batterymodeswitch }}
+ batterymode:
+ source: switch
+ switch:
+ - case: 1 # normal
+ set:
+ source: homeassistant
</code_context>
<issue_to_address>
**issue (bug_risk):** Indentation of the `switch` sequence looks off and will likely produce invalid YAML.
`switch:` and `- case: 1` are currently aligned, so the list becomes a sibling of `switch` instead of its value, which will yield invalid or unexpected YAML. The list items should be indented under `switch:`, e.g.
```yaml
data:
batterymode:
source: switch
switch:
- case: 1 # normal
set:
...
- case: 2 # hold
set:
...
- case: 3 # charge
set:
...
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| switch: | ||
| - case: 1 # normal |
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.
issue (bug_risk): Indentation of the switch sequence looks off and will likely produce invalid YAML.
switch: and - case: 1 are currently aligned, so the list becomes a sibling of switch instead of its value, which will yield invalid or unexpected YAML. The list items should be indented under switch:, e.g.
data:
batterymode:
source: switch
switch:
- case: 1 # normal
set:
...
- case: 2 # hold
set:
...
- case: 3 # charge
set:
...Co-authored-by: andig <cpuidle@gmail.com>
Co-authored-by: andig <cpuidle@gmail.com>
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, and left some high level feedback:
- The YAML indentation under
batterymode.switchlooks off: the list items (- case: ...) should be indented underswitch:(e.g. two spaces before- case) to ensure the generated config is valid. - The batterymode mapping hardcodes the three modes
normal,hold, andcharge; consider either documenting clearly that these are the only supported values for the linkedinput_selector making the options configurable via parameters to avoid coupling to a specific naming convention.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The YAML indentation under `batterymode.switch` looks off: the list items (`- case: ...`) should be indented under `switch:` (e.g. two spaces before `- case`) to ensure the generated config is valid.
- The batterymode mapping hardcodes the three modes `normal`, `hold`, and `charge`; consider either documenting clearly that these are the only supported values for the linked `input_select` or making the options configurable via parameters to avoid coupling to a specific naming convention.
## Individual Comments
### Comment 1
<location> `templates/definition/meter/homeassistant.yaml:111-113` </location>
<code_context>
de: Entitäts-ID für L3 Spannungsmessung in Volt
- name: soc
+ usages: ["battery"]
description:
de: Batterieladestand
en: Battery State of Charge
</code_context>
<issue_to_address>
**issue (typo):** German description currently uses the English phrase, which might be unintended.
The `de` description currently reads `Battery Mode Switch Entity`, matching the English text. If that’s not intentional, please replace it with a German translation to stay consistent with the other localized descriptions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| description: | ||
| de: Batterieladestand | ||
| en: Battery State of Charge |
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.
issue (typo): German description currently uses the English phrase, which might be unintended.
The de description currently reads Battery Mode Switch Entity, matching the English text. If that’s not intentional, please replace it with a German translation to stay consistent with the other localized descriptions.
I´m having my battery implemented via a Home Assistant meter in evcc and unfortunately in this way the active battery control is not configurable. With this PR I´m trying to implement.
Please note I´m not a programmer, and this is basically my first try to adjust something like this. So kindly ask you to check, if this is even workable in that way.
cc also @scruysberghs and @sebastiansucker as you were working in the past on the homeassistant template. Maybe you also could take a look at it?