feat(pumps): expose VSP power and pump diagnostics#147
Conversation
|
Real-world validation note from a live OmniLogic system (firmware 5.1.85):
Additional observation: the current diagnostics request () returned a different computed power from on this system (1345W), while telemetry power was 541W. So telemetry appears to align better with the UI power number for this equipment. |
|
Follow-up with clean real-world validation data from a live OmniLogic system (firmware 5.1.85):
Additional observation: On this system, power derived from diagnostics bytes (PowerMSB/PowerLSB from GetUIFilterDiagnosticInfo) did not match telemetry power (diagnostics computed to 1345 W while telemetry showed 541 W). So telemetry power appears to align better with the UI power value for this equipment. |
|
Added follow-up refactor commit 8bc1e91 to reduce duplication while preserving the pump-power feature:\n\n- Consolidated repeated diagnostics CLI output into a shared helper\n- Removed duplicated overload boilerplate on pump diagnostics wrapper\n- Parameterized near-identical diagnostics XML tests\n\nThis was done specifically to address Sonar duplication on new code. If Sonar does not auto-run, please re-run analysis on the updated head. |
| return resp | ||
| return FilterDiagnostics.load_xml(resp) | ||
|
|
||
| async def async_get_pump_diagnostics(self, pool_id: int, equipment_id: int, raw: bool = False) -> FilterDiagnostics | str: |
There was a problem hiding this comment.
If all this method does is call the async_get_filter_diagnostics method, then lets not add it, and instead just use the async_get_filter_diagnostics method directly.
| @click.argument("bow_id", type=int) | ||
| @click.argument("equip_id", type=int) | ||
| @click.pass_context | ||
| def get_pump_diagnostics(ctx: click.Context, bow_id: int, equip_id: int) -> None: |
There was a problem hiding this comment.
Same feedback here, seems like we could just rename the get_filter_diagnostics click command to get_filter_pump_diagnostics. The debug CLI has no guarantee of stability, so we can just rename it.
- Parse pump power from telemetry (@power) and expose Pump.power - Add async_get_pump_diagnostics API alias for VSP feature pumps - Add debug get-pump-diagnostics CLI command - Decode diagnostics power/revision/error into human-readable fields - Add tests for pump diagnostics request and power parsing Closes cryptk#99
- Remove async_get_pump_diagnostics() alias per reviewer feedback: just use async_get_filter_diagnostics() directly since it works for both filters and pumps - Rename CLI command from get-filter-diagnostics to get-filter-pump-diagnostics (debug CLI has no stability guarantee) - Remove separate get-pump-diagnostics CLI command - Simplify test to directly test async_get_filter_diagnostics - Update README to reflect single unified command
tonyfruzza
force-pushed
the
feat/pump-power-diagnostics
branch
from
ede0559 to
255f5d4
Compare
The test was converted from a parametrized test but still referenced the old 'equipment_id' parameter variable. Replace with the literal value '2' that matches the test's direct call.
|
Just as a heads up, I'll be out of the country for the next ~2 weeks, so a review on this and your other PR will have to wait until I'm back. |
|
2 participants
Summary
Implements issue #99 by adding pump-focused diagnostics support and exposing power usage for VSP feature pumps.
Closes #99
What changed
powerparsing toTelemetryPump(@power) with default0Pump.powerproperty so power is available when using the library directlyOmniLogicAPI.async_get_pump_diagnostics()as a discoverable alias for pump diagnosticsomnilogic debug get-pump-diagnostics <bow_id> <equip_id>debug get-filter-diagnosticsdebug get-pump-diagnosticsWhen not using
--raw, diagnostics now prints:Power: <watts> WDrive Rev: <revision>Display Rev: <revision>Error: No errors detected(or status code)Why this solves #99
Feature pumps that are VSPs can now be read similarly to filters:
Pump.powerThis enables flows like Node-RED / dashboards to show values similar to the OmniLogic app (e.g.
541 W).Testing
Added tests in
tests/test_api.pyfor:541 W,Display Rev 10.1.7,Drive Rev 1.0A, no-error summary)I could not run pytest in this environment because
pytestis not installed on this host.