Add binary_sensor platform to Vistapool#172234
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Vistapool binary_sensor platform and translations, plus test coverage to validate entity creation/state behavior across different module configurations.
Changes:
- Introduce
homeassistant/components/vistapool/binary_sensor.pywith module-gated and diagnostic binary sensors. - Add
binary_sensorentity translations tostrings.jsonand register the new platform in__init__.py. - Add a new test module covering default, hydrolysis/electrolysis branching, all-modules, acid-tank, and multi-pool scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/vistapool/test_binary_sensor.py | Adds tests validating binary sensor setup, gating, states, and registry defaults. |
| homeassistant/components/vistapool/strings.json | Adds translation strings for the new binary sensor entities. |
| homeassistant/components/vistapool/quality_scale.yaml | Updates exemption comments to reflect broader (non sensor-only) integration scope. |
| homeassistant/components/vistapool/const.py | Adds PATH_HASIO constant used for IO module diagnostics. |
| homeassistant/components/vistapool/binary_sensor.py | Implements binary sensor entities and setup logic (including hydrolysis/electrolysis and acid tank). |
| homeassistant/components/vistapool/init.py | Registers the binary_sensor platform in PLATFORMS. |
| if coordinator.get_value(PATH_HASHIDRO): | ||
| is_electrolysis = coordinator.get_value("hidro.is_electrolysis") | ||
| entities.append( | ||
| VistapoolBinarySensor( | ||
| coordinator, | ||
| VistapoolBinarySensorEntityDescription( | ||
| key="electrolysis_low" if is_electrolysis else "hydrolysis_low", | ||
| translation_key=( | ||
| "electrolysis_low" if is_electrolysis else "hydrolysis_low" | ||
| ), | ||
| device_class=BinarySensorDeviceClass.PROBLEM, | ||
| value_path="hidro.low", | ||
| ), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Holding off here for consistency: the merged sensor.py constructs electrolysis/hydrolysis inline in async_setup_entry the same way (https://github.com/home-assistant/core/blob/dev/homeassistant/components/vistapool/sensor.py search for is_electrolysis). Lifting only the binary_sensor version into module-level constants would split the convention across two files. Happy to do both in a follow-up PR if maintainers prefer that pattern integration-wide.
Without this, the entity is created on controllers where hasCL=1 but hasHidro=0 and reports unknown permanently because the hidro.* subtree isn't present at all.
Treating missing tank readings as off was misleading — it looked like "all tanks OK" when the integration actually had no data to evaluate. Now the sensor ignores None values and reports unknown only when every checked tank path is unset.
The previous keys carried suffixes (_status, _alarm, _installed) that didn't appear in the entity_id (which is derived from the translated entity name) or in the strings.json names. The unique_id, which uses description.key, ended up with cruft that didn't match either. Renaming the keys to match the slugified entity name keeps key, translation_key, unique_id suffix, and entity_id slug in sync.
| """Test binary sensors come up with the expected states for the default fixture.""" | ||
| mock_vistapool_client.fetch_pool_data.return_value = mock_pool_data | ||
| mock_config_entry.add_to_hass(hass) | ||
|
|
||
| assert await hass.config_entries.async_setup(mock_config_entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| # Always-on entities. | ||
| assert hass.states.get("binary_sensor.my_pool_filtration").state == STATE_ON | ||
| assert hass.states.get("binary_sensor.my_pool_backwash").state == STATE_OFF | ||
| assert hass.states.get("binary_sensor.my_pool_heating").state == STATE_OFF | ||
|
|
||
| # Module-gated; the fixture sets hasHidro=1, hasPH=1, hasRX=1. | ||
| assert hass.states.get("binary_sensor.my_pool_hidro_flow").state == STATE_OFF | ||
| assert ( | ||
| hass.states.get("binary_sensor.my_pool_hidro_cover_reduction").state | ||
| == STATE_OFF | ||
| ) | ||
| assert hass.states.get("binary_sensor.my_pool_ph_pump").state == STATE_OFF | ||
| assert hass.states.get("binary_sensor.my_pool_ph_acid_pump").state == STATE_OFF | ||
| assert hass.states.get("binary_sensor.my_pool_ph_base_pump").state == STATE_OFF | ||
| assert hass.states.get("binary_sensor.my_pool_redox_pump").state == STATE_OFF | ||
|
|
||
| # is_electrolysis=True in the fixture, so electrolysis_low is exposed. | ||
| assert hass.states.get("binary_sensor.my_pool_electrolysis_low").state == STATE_OFF | ||
| assert hass.states.get("binary_sensor.my_pool_hydrolysis_low") is None | ||
|
|
||
| # Acid tank: any-of CD/CL/PH/RX — present because hasPH and hasRX. | ||
| assert hass.states.get("binary_sensor.my_pool_acid_tank").state == STATE_OFF | ||
|
|
||
| # Module-gated and absent in the fixture (hasCD=0, hasCL=0). | ||
| assert hass.states.get("binary_sensor.my_pool_hidro_fl2") is None | ||
| assert hass.states.get("binary_sensor.my_pool_chlorine_pump") is None | ||
|
|
||
| # Diagnostic module-installed entities are disabled by default. | ||
| for entity_id in ( | ||
| "binary_sensor.my_pool_hidro_module", | ||
| "binary_sensor.my_pool_io_module", | ||
| ): | ||
| entry = entity_registry.async_get(entity_id) | ||
| assert entry is not None | ||
| assert entry.disabled_by is er.RegistryEntryDisabler.INTEGRATION |
There was a problem hiding this comment.
Instead I would recommend to enable all entities by default and use snapshot_platform
There was a problem hiding this comment.
The default-modules test now runs under entity_registry_enabled_by_default and pins the registry via snapshot_platform, so the disabled_by flag on every diagnostic entity, plus every key/translation_key/unique_id/device_class, is now captured in the .ambr snapshot rather than spread across per-entity assertions.
Kept the targeted tests for genuinely dynamic behavior (electrolysis vs. hydrolysis branch, multi-gate hidro_fl2, dosing_tank low/unknown, all-modules-enabled positive case, multi-pool fan-out) since those exercise code paths the default-fixture snapshot can't reach.
| """Test every module-gated binary sensor comes up when all `has*` flags are set.""" | ||
| mock_vistapool_client.fetch_pool_data.return_value = { | ||
| "main": { | ||
| "hasCD": 1, | ||
| "hasCL": 1, | ||
| "hasIO": 1, | ||
| "hasPH": 1, | ||
| "hasRX": 1, | ||
| "hasHidro": 1, | ||
| "version": 1, | ||
| }, | ||
| "modules": { | ||
| "ph": {"tank": 0, "pump_high_on": 0, "pump_low_on": 0, "al3": 0}, | ||
| "rx": {"tank": 0, "pump_status": 0}, | ||
| "cl": {"tank": 0, "pump_status": 0}, | ||
| "cd": {"tank": 0}, | ||
| }, | ||
| "hidro": {"is_electrolysis": True, "fl1": 0, "fl2": 0, "cover": 0, "low": 0}, | ||
| "filtration": {"status": 0}, | ||
| "backwash": {"status": 0}, | ||
| "relays": {"filtration": {"heating": {"status": 0}}}, | ||
| } |
There was a problem hiding this comment.
Let's have this combined with that snapshot test, we can then test that entities are not created when some things are false
There was a problem hiding this comment.
default: pool_data.json (hasCD=hasCL=hasUV=hasIO=0, hasPH=hasRX=hasHidro=1); snapshot pins that chlorine_pump, hidro_fl2, conductivity_module, etc. are not created (or are disabled-by-default for the diagnostic ones).
all_modules_enabled: new pool_data_all_modules.json with every has* flag set; snapshot pins that every module-gated entity is created, including the multi-gate hidro_fl2.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
The entity ORs the tank-low flags of every installed dosing module (pH / Rx / Cl / CD), not only the pH/acid tank, so "Acid tank" was misleading. Renamed translation_key, strings.json key, unique_id suffix, and class to dosing_tank / VistapoolDosingTankBinarySensor; behavior is unchanged.
The entity reads an alarm flag (modules.ph.al3) with device_class PROBLEM, but ph_pump alongside RUNNING-class ph_acid_pump and ph_base_pump was ambiguous. Renamed key, translation_key, strings.json key, and the test assertions to ph_pump_alarm / "pH pump alarm".
…e test Per joostlek's review on home-assistant#172234, replace the per- entity assertions on the default fixture with snapshot_platform run under entity_registry_enabled_by_default so the snapshot also pins the disabled-by-default flag on every diagnostic entity. Dynamic-behavior tests (hydrolysis branch, dosing tank low/unknown, fl2 multi-gate, multi-pool, and the all-modules-enabled positive case for the multi- gate) remain explicit. The .ambr snapshot needs to be generated locally with uv run pytest tests/components/vistapool/test_binary_sensor.py --snapshot-update and committed in a follow-up commit before this PR can pass CI.
HA's home-assistant-async-load-fixtures pylint rule rejects the sync load_json_object_fixture from async test bodies.
The code-PR review on home-assistant/core#172234 renamed two entities: - acid_tank -> dosing_tank (the aggregator covers every dosing module, not only the acid tank) - ph_pump -> ph_pump_alarm (the entity is a PROBLEM-class alarm, not the pH pump's running state) Updates the binary_sensors list on the docs page to match.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11702fded8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _coerce_to_bool(value: Any) -> bool | None: | ||
| """Coerce a coordinator value to bool. | ||
|
|
||
| The Vistapool API returns some numeric fields as strings (for example | ||
| `"0"` / `"1"`), so a plain `bool(value)` would treat the string `"0"` as | ||
| truthy. Strings are normalized via `int` first; anything unparsable is | ||
| treated as missing data and reported as unknown. | ||
| """ | ||
| if value is None: | ||
| return None | ||
| if isinstance(value, str): | ||
| try: | ||
| value = int(value) | ||
| except ValueError: | ||
| return None | ||
| return bool(value) |
There was a problem hiding this comment.
Why don't we just do if value in (True, "1") in is_on?
There was a problem hiding this comment.
Happy to simplify. Two questions before I push:
Do you also want me to drop the None → unknown semantic in is_on and VistapoolDosingTankBinarySensor.is_on? value in (True, "1") always returns a bool, so the entity reads "Off" (i.e. "OK" for PROBLEM class) when data is genuinely missing instead of "Unknown". Currently a Copilot review earlier in this batch pushed me toward returning None for missing data happy to revert that if you'd rather have the simpler behavior.
Same defensive concern was raised on the setup-time gates (exists_path truthiness, is_electrolysis, dosing-module any(...)). Want me to apply the same simplification there (drop _coerce_to_bool entirely, trust the API returns ints), or just inline the in (True, "1") check for is_on and keep the helper for the gates?
Thanks as always for your review and helpful hints 👍
There was a problem hiding this comment.
Okay so
- If data is genuinely missing, we should indeed return None. If data was never expected there, we should not have created the entity to begin with
- I think you can just do this login in is_on. I think ideally this kind of data is pre-processed in the library and the library acts as a nice abstraction between the mess of the API. This way the consumers of the library don't have to deal with this.
There was a problem hiding this comment.
Makes sense, so
- Simplify is_on to keep the None → unknown semantic but drop the helper: if value is None: return None; return value in (True, "1").
- Same shape for VistapoolDosingTankBinarySensor.is_on (filter None over the four tank paths, return None if all missing, otherwise any(v in (True, "1") for v in present)).
- Remove _coerce_to_bool entirely and let the setup-time gates use plain truthiness those have an exists_path/entity-creation contract that already handles "data was never expected here".
- Open a follow-up issue/PR on aioaquarite to normalize the numeric-as-string values once at the library boundary, so the integration stays a thin wrapper.
Drops the _coerce_to_bool helper. is_on now keeps the None -> unknown semantic for missing data but uses a one-liner equality check (value in (True, "1")) for the runtime state, matching joostlek's suggestion. The setup-time gates (exists_path, PATH_HASHIDRO, is_electrolysis, dosing-module any()) go back to plain truthiness, matching the pattern used in sensor.py. The numeric-string-to-bool defensiveness moves to the aioaquarite library boundary as a follow-up, so the integration stays a thin wrapper instead of duplicating the normalization. The test_binary_sensors_string_has_flags case was pinning the in-integration defensiveness on the gates and is removed; the runtime string case is still covered by test_binary_sensors_string_values.
# Conflicts: # homeassistant/components/vistapool/__init__.py # homeassistant/components/vistapool/quality_scale.yaml # homeassistant/components/vistapool/strings.json
…utation Same fix as the parallel work on vistapool-number and vistapool-select. The merge from upstream/dev pulled in the button platform's tests, which mutate _LED_DATA via the optimistic update. Wrap each use site in deepcopy() so each test starts from a fresh dict and pytest-xdist ordering doesn't cause flakes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Proposed change
Adds the
binary_sensorplatform to the Vistapool integration. This is the follow-up PR after the initial sensor-only integration merged as #168051, per the maintainers' guidance to limit each new-platform PR to a single platform.Entities exposed (per pool, with appropriate
device_class):RUNNING): filtration, backwash, heating, pH acid pump, pH base pump, chlorine pump, redox pump, hidro cover reductionPROBLEM): pH pump, hidro flow, hidro FL2, electrolysis low / hydrolysis low (dynamic key based onhidro.is_electrolysis, mirroring the existing pattern insensor.py), acid tank (true if any installed dosing tank reports low; returnsunknownwhen no tank data has arrived yet)CONNECTIVITY, diagnostic, disabled by default): conductivity, chlorine, redox, pH, IO, hidroThe
present("Connected") entity from the upstream library / HACS source is intentionally not ported — Home Assistant already models per-entity availability via the coordinator.Implementation notes
Aligned with the patterns already accepted on the sensor PR:
VistapoolBinarySensorEntityDescriptionextendsBinarySensorEntityDescription, mirroringVistapoolSensorEntityDescriptioninsensor.py.exists_pathfield on the description (a single path or a tuple of paths; all must be truthy) instead of separateifblocks.hidro_fl2requires bothhasHidroandhasCL.entry.runtime_data.coordinators.values()for multi-pool support.AddConfigEntryEntitiesCallback,PARALLEL_UPDATES = 1,translation_key-only naming, unique IDs viabuild_unique_id(description.key).description.key,translation_key, thestrings.jsonkey, the unique_id suffix, and the entity_id slug all line up.PATH_HASIOadded toconst.pyfor the new IO module diagnostic.quality_scale.yamlexemption comments foraction-setup,docs-actions, andaction-exceptionswere refreshed to drop the stale "sensor-only platform" wording — no rule statuses changed. No manifest or requirements changes.Tests
tests/components/vistapool/test_binary_sensor.pycovers:hidro_module,io_module) registered as disabled-by-default.is_electrolysis=False) createshydrolysis_lowinstead ofelectrolysis_low.has*flags enabled: every module-gated entity created.onwhen a tank value is non-zero, andunknownwhen no tank data is available.hasCL=1, hasHidro=0:hidro_fl2is not created, butchlorine_pumpstill is.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: