Add configurable reporting interval - #15
Conversation
Agent-Logs-Url: https://github.com/poolski/pytap/sessions/c67ff8ad-19f3-4cf4-8d07-983b2bc09efb Co-authored-by: poolski <1942093+poolski@users.noreply.github.com>
… step Agent-Logs-Url: https://github.com/poolski/pytap/sessions/dc53ebe2-17ef-480f-bcf1-7ab9f5dec747 Co-authored-by: poolski <1942093+poolski@users.noreply.github.com>
azebro
left a comment
There was a problem hiding this comment.
Review: Configurable Reporting Interval
Thanks for this PR — the performance problem you describe is real and worth solving. The buffering + averaging approach is sound in principle. A few things need to change before this can merge:
1. Missing "Live reporting" toggle (default ON)
The current push-per-event behavior should remain the default. Users who want throttling should explicitly opt in. Please add a boolean checkbox (e.g. live_reporting, default True) that preserves the existing behavior. The write_interval field should only appear (or matter) when live_reporting is unchecked.
This avoids silently changing behavior for existing users and keeps the sub-second latency that is a headline feature of PyTap.
2. Time-weighted average, not arithmetic mean
The current implementation uses sum(values) / len(values). If readings arrive unevenly (e.g. 3 readings in the first second, then a 4-second gap before the next), the simple mean over-represents the burst period.
Please use a time-weighted average: weight each reading by the duration until the next reading (or until flush for the last one). This properly represents the physical quantity over the interval.
3. Config entry version bump
Adding CONF_WRITE_INTERVAL (and the proposed live_reporting) to entry data requires a migration from v4 → v5 so existing entries get correct defaults. The current .get() fallback works at runtime but a proper migration is cleaner and follows the pattern already established in __init__.py.
4. Minor issues
- Flush-on-disconnect: the disconnect path discards the buffer and pushes raw
self.datainstead of averaged values, creating a brief inconsistency in the HA state history. DEFAULT_WRITE_INTERVAL = 5: if the default mode becomes "live", this value only matters when the user explicitly enables interval mode. Consider whether 5s is the right default for that case (it is quite short — the PR description mentions the problem occurring with 13 panels, where 15–30s might be more typical).
Summary
The core averaging and buffering logic is solid and well-tested. The main gap is UX: live reporting should be the default with an explicit opt-in for interval mode, and the averaging should be time-weighted. Happy to help iterate on this.
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable “write/reporting interval” to reduce Home Assistant CPU/IO load by batching incoming TAP telemetry and pushing averaged node metrics to HA at a controlled cadence, rather than emitting an update for every optimizer event.
Changes:
- Add
write_intervalconfig (default 5s) and expose it via a new Options Flow “Reporting Settings” screen. - Buffer per-node numeric readings between writes and publish per-barcode averages to Home Assistant on each interval.
- Add unit tests covering coordinator initialization, throttling state, buffer population, and averaged snapshot behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
custom_components/pytap/const.py |
Introduces CONF_WRITE_INTERVAL and DEFAULT_WRITE_INTERVAL. |
custom_components/pytap/config_flow.py |
Adds “change_reporting” options step to configure the write interval. |
custom_components/pytap/coordinator.py |
Implements buffering + averaged snapshot publishing and write-interval throttling. |
custom_components/pytap/translations/en.json |
Adds UI strings for the new reporting settings step. |
custom_components/pytap/strings.json |
Mirrors UI strings for the new reporting settings step. |
tests/test_config_flow.py |
Adds options-flow tests for the new reporting settings menu/step. |
tests/test_write_interval.py |
Adds tests for write interval init, throttling bookkeeping, buffering, and averaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Push to HA at most once per write interval, sending | ||
| # per-barcode averages over the buffered readings rather | ||
| # than the most-recent raw snapshot. | ||
| now = time.monotonic() | ||
| if self._ha_update_pending and ( | ||
| now - self._last_ha_update >= self._write_interval | ||
| ): |
| if self._ha_update_pending and ( | ||
| now - self._last_ha_update >= self._write_interval | ||
| ): |
| if self._ha_update_pending: | ||
| self._reading_buffers.clear() | ||
| self.hass.loop.call_soon_threadsafe( | ||
| self.async_set_updated_data, | ||
| dict(self.data), | ||
| ) | ||
| self._ha_update_pending = False |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Context
Because the TAP emits a packet every time one of the optimizers sends an update, there is no way to know the volume of events on the wire ahead of time. During periods of low activity (i.e. at night), there's not a lot of activity on the wire.
In the morning/throughout the day, however, I was finding that the sheer volume of events from my 13 panels (which isn't all that much) was causing a huge amount of CPU and IO overhead on my Home Assistant machine, to the point where the frontend was disconnecting and timing out.
My Home Assistant instance is no slouch:
CPU: 12-core Intel(R) Core(TM) i7-10710U,
RAM: 32GB RAM
Storage: 512GB NVMe SSD
HAOS is running on bare metal.
The sheer volume of events that PyTAP was processing from the wire was causing issues, which was a bit unexpected.
Proposed solution
After having a bit of a poke through the code, I added a reporting interval setting to the coordinator (and wired that through the setup screens). I'd imagine few people want down-to-the-second resolution for their panel/TS4 telemetry, so my thinking is that an average of the values across
reporting_intervalis a reasonable compromise.The updated implementation
reporting_intervalThis approach also makes things like ApexCharts much more responsive. By default, ApexCharts loads all data points for a given series, so frequent state updates result in even simple charts with one series being sluggish.
By spacing the state updates out a bit more and having the option to customise the reporting interval (5-300s seems plenty big a range) people can tune their PyTAP setup to their particular needs.
UI Changes