Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batch Collection measurement times drift over several hours #106

Open
mzieg opened this issue Feb 20, 2023 · 4 comments
Open

Batch Collection measurement times drift over several hours #106

mzieg opened this issue Feb 20, 2023 · 4 comments
Labels
5 - reimplement a self contained component could be made better Not Easy Requires architectural changes

Comments

@mzieg
Copy link
Member

mzieg commented Feb 20, 2023

The current BatchCollection code computes the "start of the next measurement" as an offset from the "start of the previous measurement:"

https://github.com/WasatchPhotonics/ENLIGHTEN/blob/main/enlighten/BatchCollection.py#L556
https://github.com/WasatchPhotonics/ENLIGHTEN/blob/main/enlighten/BatchCollection.py#L591

It's good that it tries to achieve a full "start-to-start" period. However, this approach has been found to drift over a period of hours.

It might be more accurate to instead base the "next start" time on current_batch_start_time + current_measurement_count x double(measurement_period_ms).

@mzieg
Copy link
Member Author

mzieg commented Feb 20, 2023

Further customer input suggests our current drift is on the order of 1 second / hour, which honestly isn't that bad.

@samiebee43
Copy link
Contributor

The amount of time it takes to schedule the next measurement after the dispatch of the previous one is roughly equal to the amount of time save_complete takes to execute. After an hour I would expect the drift to cause measurements to be late.

One possible solution is to have a a single start time and a variable that indicates how many measurements should have been made so far. At a frequency greater than the sample rate, perform the following check (pseudocode)

if int((time.now() - self.startTime) / deltaTime) > self.numberOfMeasurements:
    performMeasurement()
    self.numberOfMeasurements += 1

This way, the amount of time it takes for the code itself to execute is not excluded from our measurements.

@samiebee43
Copy link
Contributor

Instead of performing timing calculations, I'm approaching this by adjusting our use of QTimer

  • I've made some changes in b52425a which I expect eliminate some drifting. It does not handle absurd batch configurations as gracefully as main does. (For instance batch every 2000ms that consists of 3 x 1000ms measurements). Such configurations cause batches to fight over the variable current_measurement_count.

  • The revision does not take the first measurement immediately like main does.

  • Finally, timer_measurement should be changed to a recurring timer as well -- currently a sufficiently large batch would have drifting as well.

When these problems are resolved, the issue is ready to be closed.

@samiebee43
Copy link
Contributor

Absurd batch configurations, when the duration of a single batch is longer than the interval between batches, should be dealt with in one of the following ways.

image

  1. Set Batch Period minimum = Meas. Count x Meas. Period

image

image

  1. Let the true meas. count = min( meas. count , Batch Period / Meas. Period ) [Preferred]

image

Even if we validate the UI (or allow for differing underlying 'true' values) uncertainty in code execution time can create problems even for non-overlapping intervals. Take for instance a measurement that takes a really long time in the following diagram. In practice, this error happens when I set a measurement period of 10ms and a count that exactly fills the batch period.

image

We adjust the formula accordingly,

true meas. count = min( meas. count , (Batch Period - max_meas_duration) / Meas. Period )
where max_meas_duration is the longest duration that a single measurement is known to take, ~10 ms.

@samiebee43 samiebee43 mentioned this issue Mar 17, 2023
@samiebee43 samiebee43 added 5 - reimplement a self contained component could be made better Not Easy Requires architectural changes and removed enhancement labels May 15, 2023
@samiebee43 samiebee43 removed their assignment Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - reimplement a self contained component could be made better Not Easy Requires architectural changes
Projects
None yet
Development

No branches or pull requests

2 participants