Add option to disable GivTCP power data#3359
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a configuration option to allow users to disable GivTCP REST API power data reading and use configured sensor entities instead. It addresses issue #3353 where predbat.load_power was incorrectly double the actual load power during battery charging when using GivTCP REST API data.
Changes:
- Adds new
givtcp_rest_powerconfiguration option (per-inverter boolean, defaults to False) - When True, uses sensor entities (load_power, pv_power, grid_power, battery_power) instead of REST API power values
- Documents the new option in apps-yaml.md
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/predbat/inverter.py | Adds conditional check to skip REST API power data reading when givtcp_rest_power is True, falling back to sensor entity reading instead |
| docs/apps-yaml.md | Documents the new givtcp_rest_power configuration option with usage instructions |
| self.soc_percent = calc_percent_limit(self.soc_kw, self.soc_max) | ||
|
|
||
| if self.rest_data and ("Power" in self.rest_data): | ||
| if self.rest_data and ("Power" in self.rest_data) and not self.base.get_arg("givtcp_rest_power", default=False, index=self.id): |
There was a problem hiding this comment.
The configuration name givtcp_rest_power is confusing because when set to True, it actually DISABLES reading power from the REST API, rather than enabling it. This is counterintuitive.
Consider renaming to something clearer that indicates what True does, such as:
givtcp_rest_power_disable(True disables REST power reading)use_sensor_power(True uses sensor entities instead of REST)disable_rest_power(explicit about disabling)
This would make the intent much clearer in configuration files and reduce user confusion.
| if self.rest_data and ("Power" in self.rest_data) and not self.base.get_arg("givtcp_rest_power", default=False, index=self.id): | |
| # Prefer new, clearer config name `disable_rest_power`; fall back to legacy `givtcp_rest_power` for compatibility | |
| disable_rest_power = self.base.get_arg("disable_rest_power", default=None, index=self.id) | |
| if disable_rest_power is None: | |
| # Legacy name: `givtcp_rest_power` historically meant "disable REST power when True" | |
| disable_rest_power = self.base.get_arg("givtcp_rest_power", default=False, index=self.id) | |
| if self.rest_data and ("Power" in self.rest_data) and not disable_rest_power: |
| self.soc_percent = calc_percent_limit(self.soc_kw, self.soc_max) | ||
|
|
||
| if self.rest_data and ("Power" in self.rest_data): | ||
| if self.rest_data and ("Power" in self.rest_data) and not self.base.get_arg("givtcp_rest_power", default=False, index=self.id): |
There was a problem hiding this comment.
The new configuration option givtcp_rest_power is not defined in the APPS_SCHEMA dictionary in config.py. While this matches the pattern used by other optional flags like battery_power_invert, it means the configuration won't be validated and won't appear in the web interface configuration editor.
If this is intended to be a supported configuration option that users should use (especially given it addresses a bug), it should be added to APPS_SCHEMA with type "boolean_list" and entries "num_inverters" to match the pattern of other per-inverter boolean settings like those at line 2042 in config.py.
| if self.rest_data and ("Power" in self.rest_data) and not self.base.get_arg("givtcp_rest_power", default=False, index=self.id): | |
| if self.rest_data and ("Power" in self.rest_data): |
| - **givtcp_rest_power** - Optional, defaults to False. When set to True for a given inverter, Predbat will use the configured sensor entities (load_power, pv_power, grid_power, battery_power) instead of reading power values from the GivTCP REST API. | ||
| This can be useful if you want to use alternative sensors or if the REST API power readings are unreliable. Set this to False (or omit) to use REST API power readings (default behavior). |
There was a problem hiding this comment.
The documentation says "When set to True... Predbat will use the configured sensor entities... instead of reading power values from the GivTCP REST API." However, this wording is unclear about the behavior when set to False or omitted.
Consider rewording for clarity:
- "When True: Uses configured sensor entities (load_power, pv_power, grid_power, battery_power) for power readings instead of GivTCP REST API"
- "When False or omitted (default): Uses GivTCP REST API for power readings"
Also, the second sentence "Set this to False (or omit) to use REST API power readings (default behavior)" is redundant since that's already stated as the default in the first sentence.
| - **givtcp_rest_power** - Optional, defaults to False. When set to True for a given inverter, Predbat will use the configured sensor entities (load_power, pv_power, grid_power, battery_power) instead of reading power values from the GivTCP REST API. | |
| This can be useful if you want to use alternative sensors or if the REST API power readings are unreliable. Set this to False (or omit) to use REST API power readings (default behavior). | |
| - **givtcp_rest_power** - Optional, defaults to False. When True, Predbat uses the configured sensor entities (load_power, pv_power, grid_power, battery_power) for power readings instead of the GivTCP REST API. When False or omitted (default), Predbat uses power readings from the GivTCP REST API. | |
| This can be useful if you want to use alternative sensors or if the REST API power readings are unreliable. |
| self.soc_percent = calc_percent_limit(self.soc_kw, self.soc_max) | ||
|
|
||
| if self.rest_data and ("Power" in self.rest_data): | ||
| if self.rest_data and ("Power" in self.rest_data) and not self.base.get_arg("givtcp_rest_power", default=False, index=self.id): |
There was a problem hiding this comment.
There is no test coverage for the new givtcp_rest_power configuration option. Given that this feature addresses a significant bug (issue #3353 where load_power was double the actual value), it should have test coverage to ensure:
- When
givtcp_rest_poweris False (default), power values are read from REST API - When
givtcp_rest_poweris True, power values are read from sensor entities - The transition between modes works correctly
Existing tests in apps/predbat/tests/test_inverter.py already test REST vs non-REST power reading separately (e.g., around lines 551-570 and 856-882), so you could add a test that validates the new configuration option controls which path is taken.
1 participant
#3353