Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file #81

dmgav · 2024-02-29T19:41:06Z

Saving the timing parameters arm_time, start_time and hw_time_offset_ns passed as part of the header from PandA Box to IOC in as root attributes of the HDF5 file.

Issue: #80

dmgav · 2024-02-29T19:48:09Z

This is initial working implementation of the code. Missing parameters are assigned default value "" (empty string). The code was initially tested on PandA with standard v3.0 firmware, which records NTP timestamp. The parameter hw_time_offset_ns is missing in the header and has default value of "". The timestamps received from PandA are saved as strings without modification (in the same format). The following attributes were saved to the HDF5 file:

$ h5dump panda_rbdata_20240229_143311.h5
HDF5 "panda_rbdata_20240229_143311.h5" {
GROUP "/" {
   ATTRIBUTE "arm_time" {
      DATATYPE  H5T_STRING {
         STRSIZE H5T_VARIABLE;
         STRPAD H5T_STR_NULLTERM;
         CSET H5T_CSET_UTF8;
         CTYPE H5T_C_S1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): "2024-02-29T19:33:11.212Z"
      }
   }
   ATTRIBUTE "hw_time_offset_ns" {
      DATATYPE  H5T_STRING {
         STRSIZE H5T_VARIABLE;
         STRPAD H5T_STR_NULLTERM;
         CSET H5T_CSET_UTF8;
         CTYPE H5T_C_S1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): ""
      }
   }
   ATTRIBUTE "start_time" {
      DATATYPE  H5T_STRING {
         STRSIZE H5T_VARIABLE;
         STRPAD H5T_STR_NULLTERM;
         CSET H5T_CSET_UTF8;
         CTYPE H5T_C_S1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): "2024-02-29T19:33:11.212Z"
      }
   }

dmgav · 2024-03-01T13:23:13Z

The result of testing with Clock Source set as sfp3_recovered_clock and Timestamp Source to sfp3:

$ h5dump panda_rbdata_20240305_152712.h5
HDF5 "panda_rbdata_20240305_152712.h5" {
GROUP "/" {
   ATTRIBUTE "arm_time" {
      DATATYPE  H5T_STRING {
         STRSIZE H5T_VARIABLE;
         STRPAD H5T_STR_NULLTERM;
         CSET H5T_CSET_UTF8;
         CTYPE H5T_C_S1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): "2024-03-05T20:27:12.607841574Z"
      }
   }
   ATTRIBUTE "hw_time_offset_ns" {
      DATATYPE  H5T_STD_I64LE
      DATASPACE  SCALAR
      DATA {
      (0): 4002155797
      }
   }
   ATTRIBUTE "start_time" {
      DATATYPE  H5T_STRING {
         STRSIZE H5T_VARIABLE;
         STRPAD H5T_STR_NULLTERM;
         CSET H5T_CSET_UTF8;
         CTYPE H5T_C_S1;
      }
      DATASPACE  SCALAR
      DATA {
      (0): "2024-03-05T20:27:08.605729480Z"
      }
   }

dmgav · 2024-03-01T15:19:10Z

@coretl Do you think we need to convert hw_time_offset_ns to the same time format? Or we can leave it as integer?

coretl · 2024-03-04T12:48:08Z

@coretl Do you think we need to convert hw_time_offset_ns to the same time format? Or we can leave it as integer?

It is an int64 with units of nanoseconds so probably best to store it in the HDF file as such. This is only used for debugging really...

dmgav · 2024-03-04T13:41:57Z

Yes, I agree with keeping hw_time_offset_ns as int64 (it is saved as a string). But my understanding is that it is still going to be used for time computation (start_time + hw_time_offset_ns). But this depends on how the firmware is implemented and it is not important for the IOC unless IOC is responsible for computing the absolute time.

coretl · 2024-03-05T10:24:09Z

At the moment the as yet unmerged docs from PandABlocks/PandABlocks-server#45 read:

name	description
arm_time	System timestamp when ARM command was sent
start_time	Timestamp of when PCAP became both armed and enabled
hw_time_offset_ns	Offset in ns from hardware timestamp to captured system time, only present when hardware time source is used.

This means that if hw_time_offset_ns is present, start_time will be the hardware time, and start_time + hw_time_offset_ns will give you the system timestamp. This means that start_time should always be used for timestamp computation, whether its source is hardware or software, and hw_time_offset_ns is only used for debugging to work out how close those clocks are.

I will suggest some different words for PandABlocks/PandABlocks-server#45 to try and make that clearer, please comment there if you can think of some improvements for the docs too.

Yes, I agree with keeping hw_time_offset_ns as int64 (it is saved as a string).

Will you put it as an int64 attribute in the HDF file then? I'm not sure what datatypes HDF attributes support...

dmgav · 2024-03-05T20:36:57Z

I changed the type of hw_time_offset_ns to H5T_STD_I64LE. It is set to zero if there is no hardware timestamps (hw_time_offset_ns is not in the header) and to -1 if the string value in the header can not be interpreted as int64. I guess the latter case should never happen unless there is a serious bug in Panda firmware.

tests/conftest.py

evalott100

Just one type annotation

src/pandablocks/responses.py

evalott100 · 2024-03-11T09:42:19Z

CI won't work until #78 is merged.

Tests pass locally.

coretl · 2024-03-11T09:44:09Z

docs/user/tutorials/commandline-hdf.rst

+    import h5py
+    import pandas as pd
+
+    with h5py.File('data/panda_rbdata_20240305_152712.h5', 'r') as f:
+        arm_time = f.attrs['arm_time']
+        start_time = f.attrs['start_time']
+        hw_time_offset_ns = f.attrs['hw_time_offset_ns']
+
+        print(f'Arm time: {arm_time!r}')
+        print(f'Start time: {start_time!r}')
+        print(f'Hardware time offset: {hw_time_offset_ns!r} ns')
+
+        if start_time:
+            # Compute and print the start time that includes the offset
+            ts_start = pd.Timestamp(start_time)
+            if hw_time_offset_ns > 0:
+                ts_start += pd.Timedelta(nanoseconds=hw_time_offset_ns)
+            print(f'Start TS including the offset: {ts_start}')


Please could you put snippets of Python into the examples/ directory and include their contents here so they can be easily tested?

E.g.:
https://github.com/PandABlocks/PandABlocks-client/blob/master/docs/user/how-to/library-hdf.rst

Changes to the documentation were implemented. The example code code was moved to examples/load_abs_timestamps.py.

src/pandablocks/responses.py

src/pandablocks/hdf.py

src/pandablocks/connections.py

dmgav · 2024-03-11T18:23:33Z

I tested the package with Panda IOC and Panda again to make sure it works as expected.

dmgav · 2024-03-11T18:28:28Z

I had an impression that hw_time_offset_ns is always positive or zero, but additional testing showed that it can be negative indeed.

coretl

Only one clarification to the text, start_time will be hardware if available, falling back to software if not. Suggested some changes to make this clear

docs/user/tutorials/commandline-hdf.rst

examples/load_abs_timestamps.py

Co-authored-by: Tom C (DLS) <[email protected]>

…an be converted to 'int64' Co-authored-by: Tom C (DLS) <[email protected]>

Co-authored-by: Tom C (DLS) <[email protected]>

dmgav · 2024-03-14T15:32:55Z

It looks like the PR is ready to be merged.

codecov · 2024-03-15T08:31:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.90%. Comparing base (e6f8b67) to head (8d95812).

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   96.86%   96.90%   +0.03%     
==========================================
  Files          12       12              
  Lines        1181     1194      +13     
==========================================
+ Hits         1144     1157      +13     
  Misses         37       37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dmgav force-pushed the abs-ts branch 3 times, most recently from 8a0919e to f663798 Compare March 10, 2024 19:25

dmgav marked this pull request as ready for review March 10, 2024 19:38

evalott100 reviewed Mar 11, 2024

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

evalott100 approved these changes Mar 11, 2024

View reviewed changes

src/pandablocks/responses.py Outdated Show resolved Hide resolved

coretl requested changes Mar 11, 2024

View reviewed changes

coretl reviewed Mar 12, 2024

View reviewed changes

docs/user/tutorials/commandline-hdf.rst Outdated Show resolved Hide resolved

docs/user/tutorials/commandline-hdf.rst Outdated Show resolved Hide resolved

examples/load_abs_timestamps.py Outdated Show resolved Hide resolved

dmgav and others added 12 commits March 14, 2024 10:38

Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file

ed21b75

Save 'hw_time_offset_ns' as int64 to HDF5 header

5e976d2

Update unit tests

0080d51

Unit test for abs. timing parameters

9d21fd0

Fix 'ruff' formatting

18c0d3e

Documentation on abs.timestamps

7707653

Change default types to 'None'

f9a98d9

Co-authored-by: Tom C (DLS) <[email protected]>

Save abs. timing attributes to HDF5 only if they are not 'None'

e692da2

Co-authored-by: Tom C (DLS) <[email protected]>

Process 'hw_time_offset_ns' only if it is present in the header and c…

b2bc480

…an be converted to 'int64' Co-authored-by: Tom C (DLS) <[email protected]>

Use default value of 'None' for 'arm_time' and 'start_time' parameters

4be32cf

Co-authored-by: Tom C (DLS) <[email protected]>

Fixed unit tests

6a680b2

Removed conflicting file

81ab6e9

dmgav and others added 4 commits March 14, 2024 10:48

Fixed the description of start_time parameter

54c0952

Co-authored-by: Tom C (DLS) <[email protected]>

Fixed description of the parameter in print statement

be76a97

Co-authored-by: Tom C (DLS) <[email protected]>

Removed artifacts left from rebase

8940694

Fix documentation formatting

c76ab94

dmgav force-pushed the abs-ts branch from 99446e6 to c76ab94 Compare March 14, 2024 15:12

Fixed formatting

8d95812

coretl approved these changes Mar 15, 2024

View reviewed changes

evalott100 merged commit 9c01c18 into PandABlocks:master Mar 15, 2024
14 checks passed

abbiemery mentioned this pull request Apr 3, 2024

Write absolute timestamps to HDF file #80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file #81

Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file #81

dmgav commented Feb 29, 2024 •

edited

Loading

dmgav commented Feb 29, 2024 •

edited

Loading

dmgav commented Mar 1, 2024 •

edited

Loading

dmgav commented Mar 1, 2024

coretl commented Mar 4, 2024

dmgav commented Mar 4, 2024

coretl commented Mar 5, 2024

dmgav commented Mar 5, 2024

evalott100 left a comment

evalott100 commented Mar 11, 2024 •

edited

Loading

coretl Mar 11, 2024

dmgav Mar 11, 2024

dmgav commented Mar 11, 2024 •

edited

Loading

dmgav commented Mar 11, 2024

coretl left a comment

dmgav commented Mar 14, 2024

codecov bot commented Mar 15, 2024

Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file #81

Save 'arm_time', 'start_time' and 'hw_time_offset_ns' to file #81

Conversation

dmgav commented Feb 29, 2024 • edited Loading

dmgav commented Feb 29, 2024 • edited Loading

dmgav commented Mar 1, 2024 • edited Loading

dmgav commented Mar 1, 2024

coretl commented Mar 4, 2024

dmgav commented Mar 4, 2024

coretl commented Mar 5, 2024

dmgav commented Mar 5, 2024

evalott100 left a comment

Choose a reason for hiding this comment

evalott100 commented Mar 11, 2024 • edited Loading

coretl Mar 11, 2024

Choose a reason for hiding this comment

dmgav Mar 11, 2024

Choose a reason for hiding this comment

dmgav commented Mar 11, 2024 • edited Loading

dmgav commented Mar 11, 2024

coretl left a comment

Choose a reason for hiding this comment

dmgav commented Mar 14, 2024

codecov bot commented Mar 15, 2024

Codecov Report

dmgav commented Feb 29, 2024 •

edited

Loading

dmgav commented Feb 29, 2024 •

edited

Loading

dmgav commented Mar 1, 2024 •

edited

Loading

evalott100 commented Mar 11, 2024 •

edited

Loading

dmgav commented Mar 11, 2024 •

edited

Loading