ImperialCollegeLondon / FINESSE

A graphical user interface for controlling and monitoring an interferometer device
BSD 3-Clause "New" or "Revised" License
3 stars 1 forks source link

Sync timestamps in data file #535

Closed jamesturner246 closed 7 months ago

jamesturner246 commented 8 months ago

Description

This change ensures that timestamps for temperature readings in the data file are synchronised using the time source (host or NTP server). The temperature monitor pubsub send call is updated to retrieve time from the time source before publishing.

Fixes #522.

Type of change

Key checklist

Further checks

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 44.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 80.66%. Comparing base (e95042d) to head (5c0127f). Report is 54 commits behind head on main.

:exclamation: Current head 5c0127f differs from pull request most recent head 78c90b5. Consider uploading reports for the commit 78c90b5 to get more accurate results

Files Patch % Lines
finesse/hardware/__init__.py 14.28% 12 Missing :warning:
finesse/gui/temperature_plot.py 0.00% 1 Missing :warning:
finesse/hardware/data_file_writer.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #535 +/- ## ========================================== - Coverage 80.73% 80.66% -0.08% ========================================== Files 68 69 +1 Lines 3338 3361 +23 ========================================== + Hits 2695 2711 +16 - Misses 643 650 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexdewar commented 7 months ago

Sorry to jump in here unbidden :wink:... but just wanted to add that I agree with @dalonsoa. On reflection, I'm not sure it makes sense to have a separate HostTime device type, because you always need some time source, regardless of whether there's a time device connected. Maybe it would be better to have time devices return the time offset instead? Then you could drop the HostTime class and just have the offset default to 0.

On a related note, there is another issue with how things are being done currently: the NTP server is being queried whenever the time is requested (around every 2 secs). Normally you only resynchronise with an NTP server occasionally (e.g. every 10 mins) and assume that your clock hasn't drifted too much in the meantime.

I think we should probs close this PR and address these issues with the time devices first. I'm not sure what your plans are for the rest of the sprint @jamesturner246, but I'm at a bit of a loose end this week so happy to take this on.

jamesturner246 commented 7 months ago

I've removed the HostTime class, and finesse time now defaults to local time.

I've also changed NTPTime so that it queries periodically rather than every time it's needed.

Tests have been updated.

Looks like there's some conflicts. I'll sort them out when tests pass.

alexdewar commented 7 months ago

@dalonsoa We had this discussion with Jon a while back. He wants everything to be in UTC (as it is is for their current software, I believe).

There is one place where we record the timezone: in the metadata header for the CSV files. I figured having localtime was more informative in that context (e.g. if you're trying to remember when you recorded a dataset). The timestamps in the actual CSV part of the file are still UTC though.

dalonsoa commented 7 months ago

Works well on Windows, now.