Fix background stats refresh
spiir-O4-review-v5
ran into a critical bug in science mode on ER-15: It wasn't uploading any triggers.
Reading the new livetime_1w
field, @manoj.kovalam found that we weren't refreshing backgrounds, so we never exceeded our required 1 million data points for upload.
The issue was that I'm stoopid. In !182 (merged) I asked Luke to subtract two uint64s !182 (comment 700212). Thinking of the case of an underflow, I said we need a MIN(0, unsigned_a - unsigned_b)
. That's obviously crazy, even if it doesn't underflow, it will always be 0.
Instead we should subtract and cast to a signed value of the same size. The GstClockTime
type already has types and #define
s for this problem:
https://gstreamer.freedesktop.org/documentation/gstreamer/gstclock.html?gi-language=c#GstClockTimeDiff,
https://gstreamer.freedesktop.org/documentation/gstreamer/gstclock.html?gi-language=c#GST_CLOCK_DIFF
I've used that in this patch. For review, it's now included along with other fixes in spiir-O4-review-v5.3
.
Tests
Our usual tests didn't catch the issue for two reasons:
- Running on the injection stream, our MDC test hit the 1 Million data points in its first background dump. (Our non-injection run on ER-15 hit about 700k.)
- @manoj.kovalam is going to launch a new run on ER-15, uploading to test.
- Plotting the IFAR of that MDC run may have indicated an issue.
- Our usual offline injection tests don't refresh backgrounds, so although I could see that the new columns were working, I didn't verify that they updated.
- I've now done a quick offline run with short intervals and printed the
nevent_1w
andfar
columns. - This could be converted into an automated test with our new columns.
- We should always write specific test cases for new code changes.
- I've now done a quick offline run with short intervals and printed the
Both v5 and v5.3 load the first set of backgrounds (backgrounds & load times vary slightly due to calcfap run time and atomic add):
v5:
v5.3:
v5.3 loads new backgrounds (based on nevent and livetime), but v5 never does:
v5.3 first refresh:
Last zerolags v5 vs v5.3:
Internal gitlab review should also have caught it. The issue was that it was a reviewer comment asking for changes, and that needs to be double checked too. I think in future a call to discuss when suggesting changes to an MR is the right way to go. We should consider test cases of any updated code in review.