Skip to content

Save precise start times per TP #2408

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Save precise start times per TP #2408

wants to merge 12 commits into from

Conversation

MichaelHuth
Copy link
Collaborator

close #336

@MichaelHuth MichaelHuth self-assigned this Apr 25, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2408-mh_precise_tp_start_times branch from bc2d6e9 to 460bced Compare April 28, 2025 18:20
@MichaelHuth
Copy link
Collaborator Author

@t-b I have adapted the ITC fifopos threaded push and readout now including acq start time - fingers crossed

@MichaelHuth MichaelHuth force-pushed the feature/2408-mh_precise_tp_start_times branch 4 times, most recently from 0fd45cb to a9d8a54 Compare April 29, 2025 17:00
@t-b
Copy link
Collaborator

t-b commented Apr 29, 2025

The CI failure could be due to me doing something on the machine.

@MichaelHuth MichaelHuth force-pushed the feature/2408-mh_precise_tp_start_times branch from 996000c to ec33069 Compare April 30, 2025 16:29
@MichaelHuth MichaelHuth assigned t-b and MichaelHuth and unassigned MichaelHuth and t-b May 2, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2408-mh_precise_tp_start_times branch from ec33069 to 67038dc Compare May 2, 2025 11:11
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth May 2, 2025
@t-b
Copy link
Collaborator

t-b commented May 2, 2025 •
edited by MichaelHuth
Loading

I've played around with it and the issue that the timestamp plot has bunches is resolved.

Review:

532e9f9 (DAC: Save time of acquisition start per device, 2025-04-25)

  • Making HW_XX_StartAcq static and switchting to it should be its own commit

b8111bb (ITC: Save AcquisitionStartTime on acquisition restart in ITC Fifoloop, 2025-04-28)

  • Should be in the previous commit:
@@ -645,7 +650,7 @@ Function HW_StartAcq(variable hardwareType, variable deviceID, [variable trigger

     device = HW_GetMainDeviceName(hardwareType, deviceID, flags = flags)
     SVAR lastAcqStartTime = $GetLastAcquisitionStartTime(device)
-    lastAcqStartTime = GetIso8601TimeStamp(numFracSecondsDigits = 5)
+    lastAcqStartTime = HW_GetAcquisitionStartTimestamp()

  • If HW_GetAcquisitionStartTimestamp accepts something like secondsSinceIgorEpoch as optional parametr you
    can avoid code duplication in HW_ITC_ReadFifoPos

  • Please first instroduce HW_ITC_ReadFifoPos and then later add support for lastAcqStartTime. The current
    diff is too distracting.

e056da4 (Time: Add utility functions to convert time in secs UTC <-> Local, 2025-04-25)

Nice!

4a31f90 (SCOPE: Add code comment to TP loop in SCOPE_UpdateOscilloscopeData, 2025-04-25)

Good.

72d9e28 (SCOPE: Use precise timestamp per TP for tpInput data, 2025-04-25)

Very nicely solved.

72e02da (TP: Remove TPStorage field %TimeInSeconds, 2025-04-29)

  • Function documentation for PUB_TPResult needs updating

f3369e6 (Debug: A variable was missed in a rename refactor in a debug code section, 2025-04-29)

Nice find. Please cite the commit introducing the compile error. Good thinking to add that define to the
compilation test.

2b0dfa1 (Fix: LeftOverSweepTime - treat fifopos of NaN as finished sweep, 2025-04-30)

With a test, nice one!

5933a76 (TS: Use finite timeout only when threadqueue has run empty, 2025-04-30)
67038dc (Fix: TS_GetNewestFromThreadQueue working only for finite values, 2025-04-30)

Nicely found and solved.

@t-b t-b assigned MichaelHuth and unassigned t-b May 2, 2025
@MichaelHuth MichaelHuth force-pushed the feature/2408-mh_precise_tp_start_times branch from 67038dc to 5cdc407 Compare May 2, 2025 14:23
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth May 2, 2025
@t-b t-b force-pushed the feature/2408-mh_precise_tp_start_times branch 2 times, most recently from db67cf7 to 25c1e2e Compare May 6, 2025 17:33
@t-b
Copy link
Collaborator

t-b commented May 6, 2025

@MichaelHuth I've fixed the test failure. The issue was that the saved stimsets were not in good shape. I don't know how you save them, but it looks like there is a special workflow only I know.

I did:

  • undo your stimset addition
  • Loadstimsets()
  • Add your new stimset again
  • savestimsets()
  • RewriteAnalysisFunctions_IGNORE()
  • Commit it

Code LGTM.

t-b
t-b previously approved these changes May 6, 2025
View reviewed changes
@t-b t-b assigned timjarsky and unassigned t-b May 6, 2025
@MichaelHuth
Copy link
Collaborator Author

I think I did:

  • Loadstimsets()
  • Add stimset by saving another existing with a different name, I think I chose the AFT13 mid sweep stimset as template
  • RewriteAnalysisFunctions_IGNORE()
  • savestimsets()

MichaelHuth and others added 12 commits May 7, 2025 19:15
@MichaelHuth @t-b
HW: Make specialized StartAcq functions static
40ade5c
@MichaelHuth @t-b
HW: Factor out readback of ITC fifoPos to HW_ITC_ReadFifoPos
591b396 
- no functional change
@MichaelHuth @t-b
DAC: Save time of acquisition start per device
e5218cc 
The time is saved as global string as ISO8601 timestamp.
@MichaelHuth @t-b
ITC: Save AcquisitionStartTime on acquisition restart in ITC Fifoloop
148f7c6 
The ITC Fifoloop thread in TFH_FifoLoop also restarts the acquisition
on demand. This resets the fifoPosition and the acquisition start time
needs to be updated.
The acquisition start time is transferred through the thread queue as
variable, such that TS_GetNewestFromThreadQueueMult can be used for
readout in the new function HW_ITC_ReadFifoPos.

IT IS CRITICAL HERE THAT the updated fifopos and start time value is
pushed to the thread queue in the same dfr entity and not by
TS_ThreadGroupPutVariable that creates a thread queue entity per variable.

There the update of the acquisition start time global is handled
transparently for the callers.
@MichaelHuth @t-b
SCOPE: Add code comment to TP loop in SCOPE_UpdateOscilloscopeData
642a139
@MichaelHuth @t-b
SCOPE: Use precise timestamp per TP for tpInput data
df6d9de 
In our TP data we used until now only simply DateTime of the moment the
data was sent into analysis for the timestamp. This resulted in multiple
TPs (all from the same new FIFO data) having nearly the same timestamp.

Now the timestamp of each TP based on the acquisition start and TP
position in the acquisition is determined.

Note regarding the calculation:
(tpCounter + i) * tpInput.samplingIntervalDAC * tpInput.tpLengthPointsDAC * MILLI_TO_ONE

tpCounter originates from NI devices in TP acquisition mode.
There the acquisition with a single TP repeats itself in hardware.
The TPs are counted based on the FIFO data and each TP is sent to
SCOPE_UpdateOscilloscopeData separately. Thus, i is always zero and
tpCounter the number of the TP since acquisition start.

For all other hardware devices and acquisition modes tpCounter is zero and
i counts the absolute TP number based on the position of the data in the FIFO.
@MichaelHuth @t-b
TP: Remove TPStorage field %TimeInSeconds
f1e2802 
The field was redundant to %TimeStamp and even less precise.
Since the TimeStamp field was updated in a recent commit to
include the precise acquisition start time of the TP, the
field %TimeInSeconds is obsolete and is removed.

The TPStorage version was increased.
The WaveUpgrade removes the %TimeInSeconds.

The x-axis of the TP resistance fit uses now %TimeStamp, which makes
the fit results much more reliable with fewer data points.

The element was also removed from ZMQ publishing of the TP data.
@t-b
Debug: A variable was missed in a rename refactor in a debug code sec…
883ba02 
…tion

Fix: Renamed the variable in TP_TSAnalysis debug section
lengthTPInPoints -> lengthTPInPointsADC

Add TP_ANALYSIS_DEBUGGING define to list in CompilationTester
@t-b
Tests/ChangeAnalysisFunctions_IGNORE: Fix it for test case AFT6b
5587a21 
This test case checks that we ignore other set analysis functions if a
generic one is set.

This test fails after calling ChangeAnalysisFunctions_IGNORE via
RewriteAnalysisFunctions_IGNORE().

The reason is that ST_SetStimsetParameter uses
WB_SetAnalysisFunctionGeneric to set the generic analysis function. And
that helper clears other functions set for the per event cases (which were
used for V1/V2 analysis functions).

So we now have to set the generic one before all others.

Bug introduced in 7c207c5 (Tests: Pefer ST_SetStimsetParameter over
direct WPT access, 2022-08-09).
@MichaelHuth @t-b
Fix: LeftOverSweepTime - treat fifopos of NaN as finished sweep
d94ccf6 
Previously a fifopos of NaN triggered an ASSERTion. However if the sweep already
finished when LeftOverSweepTime was called, then the fifopos was NaN.

Fix: Correctly return 0 for this case.
@MichaelHuth @t-b
TS: Use finite timeout only when threadqueue has run empty
984495a 
Before the timeout constant was used always, such that ThreadGroupGetDFR
waited at least once with the timeout for the case the thread queue was read empty.
This wait is not necessary as the thread queue can be checked for emptiness also
with a zero timeout.

TS_GetNewestFromThreadQueue and TS_GetNewestFromThreadQueueMult use now a timeout of
zero for reading the thread queue empty and for the then first check of emptyness,
where ThreadGroupGetDFR returns a null DFR. Only further read attempts,
if timeout_retries are set, use the TS_GET_REPEAT_TIMEOUT_IN_MS.
@MichaelHuth @t-b
Fix: TS_GetNewestFromThreadQueue working only for finite values
693d185 
For TS_GetNewestFromThreadQueue the thread could only return finite values
for the requested variable.
Instead of using the finite/non-finite state of the read value itself for
the decision of returning the value a flag variable was introduced that
is set when any value was read.

This allows also to send a NaN value from a thread.
(Note: It is best to use a custom timeout_default of e.g. -1 then)

TS_GetNewestFromThreadQueueMult already uses the logic with a flag variable
and does not need a fix.
@t-b t-b force-pushed the feature/2408-mh_precise_tp_start_times branch from 25c1e2e to 693d185 Compare May 7, 2025 17:19
@t-b
Copy link
Collaborator

t-b commented May 7, 2025

@MichaelHuth Thanks. That approach is the good one and that should also have worked. But it was broken. See 5587a21 (Tests/ChangeAnalysisFunctions_IGNORE: Fix it for test case AFT6b, 2025-05-07). The "funny" part is that the commit message introducing the bug says "Tests: Pefer ST_SetStimsetParameter over direct WPT access It is shorter and safer.".

@t-b t-b self-requested a review May 7, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-b t-b t-b approved these changes

@timjarsky timjarsky Awaiting requested review from timjarsky timjarsky is a code owner

Assignees

@timjarsky timjarsky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TP results plotting looks different for TP during DAQ and TP during ITI
3 participants
@MichaelHuth @t-b @timjarsky