chore(instrument): log warning when background daemon cant run at requested rate - #338
Conversation
|
PR author is not in the allowed authors list. |
|
Claude finished @maxleblang's task in 1m 42s —— View job Review
OverviewSmall, well-scoped addition: One inline nit posted: the new |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| """Warn once if the averaged work time shows the requested interval isn't achievable. | ||
|
|
||
| This doesn't give timing guarantees about the background daemon rate, | ||
| just warn when work_time > requested interval. |
There was a problem hiding this comment.
Multi-paragraph docstring — repo convention is one short line max (INSTRO-248 cleanup, see AGENTS.md). Fold the caveat into the summary line or drop it.
| self._background_config.interval = seconds | ||
| self._reset_daemon_timing() | ||
|
|
||
| def _reset_daemon_timing(self): |
There was a problem hiding this comment.
We should also call this method whenever the background daemon is redefined by the user
| return | ||
|
|
||
| average_work_time_s = sum(self._daemon_work_times) / len(self._daemon_work_times) | ||
| if average_work_time_s <= self._background_config.interval: |
There was a problem hiding this comment.
This check will always fail for InstroDAQ when it has the default 0 sec background interval for hardware-paced reads. We should short-circuit for interval <=0
| "Background daemon for instrument '%s' cannot achieve the requested interval of %.6f s; " | ||
| "average work time over %d cycles is %.6f s, which is the fastest achievable interval.", |
There was a problem hiding this comment.
Technically not true that it is the fastest achievable interval, can we change the wording to be more accurate? Instead of saying it is the fastest achievable interval, we can provide guidance on how to fix? "Set background daemon interval to a value greater than the work time" or something like that
| if self._interval_warning_issued or len(self._daemon_work_times) != self._daemon_work_times.maxlen: | ||
| return | ||
|
|
||
| average_work_time_s = sum(self._daemon_work_times) / len(self._daemon_work_times) |
There was a problem hiding this comment.
Is it possible that len(self._daemon_work_times) == 0 due to a reset between the previous check? We might want to grab a static copy of self._daemon_work_times in a local variable just in case
There was a problem hiding this comment.
I don't understand the scenario you're talking about. We should never be able to reach the division if self._daemon_work_times isn't 10 (maxlen in this case). What would grabbing a static topic buy us?
There was a problem hiding this comment.
It's technically possible that self._reset_daemon_timing() gets called in between the check on line 394 and computation of average_work_time_s on line 397., in which case self._daemon_work_times becomes empty with length 0. I'm not seeing any guarantees of protection against that.
There was a problem hiding this comment.
I understand now. I'll have reset just create a new deque pointer instead of clearing the shared one.
2 participants
Summary
Based on the average work time of 10 cycles of the background daemon, warn the user if their requested rate isn't achievable (avg_work_time > requested_rate). We only warn once per instrument run.
Type of change
fix)feat)feat!/fix!)refactor)docs)chore)Verification
Entire test suite passes
Tests
Checklist
feat(driver): add support for Keysight E36300)Notes for reviewers
Because of the way we pace the loop by calling
wait()and the GIL, the daemon loop often runs at a slightly slower rate than the requested interval (loop_time = work_time + sw_latency). This is a pitfall of a sw timed loop in Python that we can't really get around. I just wanted to bring attention to the fact that this is only checking if the work time of the daemon is higher than the requested rate, not that the daemon is delivering on the requested loop interval (loop_time > requested_interval?)