SBND Timing Reconstruction Refactor by VCLanNguyen · Pull Request #915 · SBNSoftware/sbndcode

VCLanNguyen · 2026-02-14T00:13:22Z

Description

Timing reconstruction is refactored in this PR, affecting the decode/reconstruction workflow of CRT/PMT/XA.
After the refactoring, the @FrameShiftModule@ runs first in the decode chain, outputs timing products to be used at PMT/XA decoder and CRTStrip reconstruction.
Relevant PMT reconstruction modules at Reco1/Reco2 are updated.
The timing correction applied at CAFMakeer is undone. - The timing correction applied at CAFMakeer is undone. This should ressolve this issue: Converge timing correction in CAFMaker for SBND and ICARUS sbncode#567
Details and validation plots can before in the linked docdb.

Link(s) to docdb describing changes (optional)

https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=46654

Relevant PR links (optional)

This PR needs the XA decoder PR in sbndcode to go in first:
#847

This PR needs to be merged together with this group of PRs:

sbndcode: SBND Timing Reconstruction Refactor #915
sbncode: SBND Timing Reconstruction Refactor sbncode#637
sbnobj: SBND Timing Reconstruction Refactor sbnobj#170
sbnanaobj:SBND Timing Reconstruction Refactor sbnanaobj#188

Checklist

Added at least 1 label from available labels.
Assigned at least 1 reviewer under Reviewers,
Assigned all contributers including yourself under Assignees
Linked any relevant issues under Developement
Does this PR affect CAF data format? If so, please assign a CAF maintainer (PetrilloAtWork or JosiePaton) as additional reviewer.
Does this affect the standard workflow?
Is this PR a patch for the ongoing production? If so, separate PR must also be made for production/v10_06_00 branch!

… extended fragments combination

… not fulfill the requirements

…v10_11_01_hlay_functions Functionalise

…ected jittering of the extended fragments.

…indexing. Improve jittering application.

…/ptb

… timing corrections at an earlier stage of reconstruction

aantonakis · 2026-04-14T16:10:22Z

crt veto looks good

lynnt20

approved for PMT side!

aliciavr

All good on XA's side!

VCLanNguyen · 2026-04-21T18:04:38Z

@JosiePaton @PetrilloAtWork can you guys review for CAF?

PetrilloAtWork

Some changes suggested here and there as usual, plus:

something that looks like a bug (which must be fixed, hence the rejection)
a questionable implementation of the difference of timestamps

A reiterated comment/recommendation: please format all the relevant comments on the usage of the functions and data member to Doxygen format.

PetrilloAtWork · 2026-04-27T21:21:55Z

+      art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;
+      e.getByLabel(fFrameShiftModuleLabel, frameShiftHandle);

-      if(DAQHeaderHandle.isValid())
-        {
-          artdaq::RawEvent rawHeaderEvent = artdaq::RawEvent(*DAQHeaderHandle);
-          raw_ts = rawHeaderEvent.timestamp() - fRawTSCorrection;
-        }
+      if(!frameShiftHandle.isValid())
+        throw std::runtime_error("Frame Shift Info object is invalid, check data quality");


Since the handle is only used to access its data, a more compact expression is:

Suggested change

art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;

e.getByLabel(fFrameShiftModuleLabel, frameShiftHandle);

if(DAQHeaderHandle.isValid())

{

artdaq::RawEvent rawHeaderEvent = artdaq::RawEvent(*DAQHeaderHandle);

raw_ts = rawHeaderEvent.timestamp() - fRawTSCorrection;

}

if(!frameShiftHandle.isValid())

throw std::runtime_error("Frame Shift Info object is invalid, check data quality");

auto const & frameShift

= e.getProduct<sbnd::timing::FrameShiftInfo> (fFrameShiftModuleLabel);

that also throws an exception in case of failure; note that you lose control on the exception message though. Since art::Event::getByLabel() return the status of the operation, another alternative is:

Suggested change

art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;

e.getByLabel(fFrameShiftModuleLabel, frameShiftHandle);

if(DAQHeaderHandle.isValid())

{

artdaq::RawEvent rawHeaderEvent = artdaq::RawEvent(*DAQHeaderHandle);

raw_ts = rawHeaderEvent.timestamp() - fRawTSCorrection;

}

if(!frameShiftHandle.isValid())

throw std::runtime_error("Frame Shift Info object is invalid, check data quality");

art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;

if (!e.getByLabel(fFrameShiftModuleLabel, frameShiftHandle))

throw std::runtime_error("Frame Shift Info object is invalid, check data quality");

PetrilloAtWork · 2026-04-27T21:28:57Z

+	{
+	  std::set<uint32_t> unix_set = UnixSet(FEBDataVec);
+          ref_time_s = unix_set.size() ? *unix_set.rbegin(): 0;
+	}


Please fix the indentation (I suppose tabulate characters are mixed in).

PetrilloAtWork · 2026-04-27T21:34:59Z

+
+      if(unix_diff < -1 || unix_diff > 1)
+        {
+          throw std::runtime_error(Form("Unix timestamps differ by more than 1 (%li)", unix_diff));


Without pulling in ROOT's Form():

Suggested change

throw std::runtime_error(Form("Unix timestamps differ by more than 1 (%li)", unix_diff));

throw std::runtime_error("Unix timestamps differ by more than 1 (" + std::to_string(unix_diff) + ")");

std::to_string() is defined in the C++ standard header string.

PetrilloAtWork · 2026-04-27T21:36:06Z

+      if(unix_diff == 1)
+	t0_etrig -= 1e9;
+      else if(unix_diff == -1)
+	t0_etrig += 1e9;
+
+      if(t0_etrig < fETrigMin || t0_etrig > fETrigMax)
+	return stripHits;


Fix the code indentation:

Suggested change

if(unix_diff == 1)

t0_etrig -= 1e9;

else if(unix_diff == -1)

t0_etrig += 1e9;

if(t0_etrig < fETrigMin || t0_etrig > fETrigMax)

return stripHits;

if(unix_diff == 1)

t0_etrig -= 1e9;

else if(unix_diff == -1)

t0_etrig += 1e9;

if(t0_etrig < fETrigMin || t0_etrig > fETrigMax)

return stripHits;

PetrilloAtWork · 2026-04-27T21:38:43Z

  art::ServiceHandle<SBND::CRTChannelMapService> fCRTChannelMapService;

  std::string           fFEBDataModuleLabel;
+  std::string           fFrameShiftModuleLabel;


Note that the appropriate type is art::InputTag, not std::string.
Historically the latter was used because fhicl::ParameterSet::get<art::InputTag>() was broken; that time is long gone.
Not necessarily recommending the change here, unless you change all the tag data members.

PetrilloAtWork · 2026-04-28T05:31:37Z

+private:
+
+  // FCL Parameters
+  std::string fDAQHeaderModuleLabel;   // Instance label for DAQ header product


Turn the comments into Doxygen format, so that they are rendered in the documentation. For example:

Suggested change

std::string fDAQHeaderModuleLabel; // Instance label for DAQ header product

std::string fDAQHeaderModuleLabel; ///< Instance label for DAQ header product

PetrilloAtWork · 2026-04-28T05:45:03Z

+    art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;
+    e.getByLabel(fFrameShiftLabel, frameShiftHandle);

-    if (!timingRefHandle.isValid()){
-        throw cet::exception("WaveformAlignment") << "No raw::TimingReferenceInfo found w/ tag " << fTimingRefLabel << ". Check data quality!";
+    if (!frameShiftHandle.isValid()){
+        throw cet::exception("WaveformAlignment") << "No sbnd::timing::FrameShiftInfo found w/ tag " << fFrameShiftLabel << ". Check data quality!";
    }
    else{
-        raw::TimingReferenceInfo const& pmt_timing(*timingRefHandle);
-
-        _pmt_timing_type = pmt_timing.timingType;
-        _pmt_timing_ch = pmt_timing.timingChannel;
-
-        if (fDebugTimeRef){
-            std::cout << "Timing Reference For Decoding PMT" << std::endl;
-            std::cout << "   Type = " << _pmt_timing_type << " (SPECTDC = 0; PTB HLT = 1; CAEN-only = 3)." << std::endl;
-            std::cout << "   Channel = " << _pmt_timing_ch << " (TDC ETRIG = 4; PTB BNB Beam+Light = 2)." << std::endl;
-        }
+        _frameDefault = frameShiftHandle->FrameDefault();
+        _pmt_timing_type = frameShiftHandle->TimingTypeDefault();
    }


I suggest:

Suggested change

art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;

e.getByLabel(fFrameShiftLabel, frameShiftHandle);

if (!timingRefHandle.isValid()){

throw cet::exception("WaveformAlignment") << "No raw::TimingReferenceInfo found w/ tag " << fTimingRefLabel << ". Check data quality!";

if (!frameShiftHandle.isValid()){

throw cet::exception("WaveformAlignment") << "No sbnd::timing::FrameShiftInfo found w/ tag " << fFrameShiftLabel << ". Check data quality!";

}

else{

raw::TimingReferenceInfo const& pmt_timing(*timingRefHandle);

_pmt_timing_type = pmt_timing.timingType;

_pmt_timing_ch = pmt_timing.timingChannel;

if (fDebugTimeRef){

std::cout << "Timing Reference For Decoding PMT" << std::endl;

std::cout << " Type = " << _pmt_timing_type << " (SPECTDC = 0; PTB HLT = 1; CAEN-only = 3)." << std::endl;

std::cout << " Channel = " << _pmt_timing_ch << " (TDC ETRIG = 4; PTB BNB Beam+Light = 2)." << std::endl;

}

_frameDefault = frameShiftHandle->FrameDefault();

_pmt_timing_type = frameShiftHandle->TimingTypeDefault();

}

auto const& frameShift = e.getProduct<sbnd::timing::FrameShiftInfo>(fFrameShiftLabel);

_frameDefault = frameShift.FrameDefault();

_pmt_timing_type = frameShift.TimingTypeDefault();

or, if control on the exception message is desired,

Suggested change

art::Handle<sbnd::timing::FrameShiftInfo> frameShiftHandle;

e.getByLabel(fFrameShiftLabel, frameShiftHandle);

if (!timingRefHandle.isValid()){

throw cet::exception("WaveformAlignment") << "No raw::TimingReferenceInfo found w/ tag " << fTimingRefLabel << ". Check data quality!";

if (!frameShiftHandle.isValid()){

throw cet::exception("WaveformAlignment") << "No sbnd::timing::FrameShiftInfo found w/ tag " << fFrameShiftLabel << ". Check data quality!";

}

else{

raw::TimingReferenceInfo const& pmt_timing(*timingRefHandle);

_pmt_timing_type = pmt_timing.timingType;

_pmt_timing_ch = pmt_timing.timingChannel;

if (fDebugTimeRef){

std::cout << "Timing Reference For Decoding PMT" << std::endl;

std::cout << " Type = " << _pmt_timing_type << " (SPECTDC = 0; PTB HLT = 1; CAEN-only = 3)." << std::endl;

std::cout << " Channel = " << _pmt_timing_ch << " (TDC ETRIG = 4; PTB BNB Beam+Light = 2)." << std::endl;

}

_frameDefault = frameShiftHandle->FrameDefault();

_pmt_timing_type = frameShiftHandle->TimingTypeDefault();

}

try {

auto const& frameShift = e.getProduct<sbnd::timing::FrameShiftInfo>(fFrameShiftLabel);

_frameDefault = frameShift.FrameDefault();

_pmt_timing_type = frameShift.TimingTypeDefault();

}

catch(art::Exception const&) {

throw cet::exception("WaveformAlignment") << "No sbnd::timing::FrameShiftInfo found w/ tag " << fFrameShiftLabel << ". Check data quality!";

}

PetrilloAtWork · 2026-04-28T05:47:31Z

-art_dictionary(DICTIONARY_LIBRARIES 
-lardataobj::RawData)
+art_make_library(
+                SOURCE  TimingUtils.cc


This is redundant. Any reason to specify it?

Suggested change

SOURCE TimingUtils.cc

PetrilloAtWork · 2026-04-28T05:53:47Z

+    //Subtract two timestamps in UTC format
+    //ts1 and ts2 are in nanoseconds in uint64_t format
+    //Return the difference in nanoseconds as double
+    double SubtractUTCTimestmap(const uint64_t& ts1, const uint64_t& ts2);


Same comment as somewhere earlier:

Suggested change

double SubtractUTCTimestmap(const uint64_t& ts1, const uint64_t& ts2);

double SubtractUTCTimestmap(uint64_t ts1, uint64_t ts2);

PetrilloAtWork · 2026-04-28T05:56:37Z

+
+    //Subtract two timestamps in UTC format
+    //ts1 and ts2 are in nanoseconds in uint64_t format
+    //Return the difference in nanoseconds as double


It may be useful to explicitly say which sign that difference has:

Suggested change

//Return the difference in nanoseconds as double

//Return the difference `ts1 - ts2` in nanoseconds as double

Also, please convert this comment into Doxygen format.

aliciavr and others added 30 commits October 9, 2025 04:05

Setup to start extended fragments feature

151b2c9

Shift timing function

1e492fd

Waveforms decoding function

3f93ceb

Add dump waveforms function

b416a56

Combine waveforms and correctly differentiates nominal from extended

0d9f644

Combine all extended fragments succesfully

c1a42c0

Add new comments for code documenting. New variable for debugging the…

e165606

… extended fragments combination

Update SBN Document for reference

60036c5

Add version number and update number of debug waveforms to 0

2f7ff45

First version of timing check implemented (some extended fragments do…

15f34cd

… not fulfill the requirements

Add jittering debug option and combine fragments option

3f44697

refactor frameshift module to run at decode stage

f527fb8

add timing channel

7e7c3c4

add initialisation as max uint64_t

8d8d8a1

Use namespace and enumeration

daa63dc

Begin functionalisation

6739521

Make constexpr for regularly used cast

f7dead4

Make function for formatting timestamp

34e6ffe

Functionalise frame TDC/PTB definitions

500d130

Reduce duplication in final logic

a21b73f

Consistent indentation

4697328

Clean up fcl parameters and document

8102b2e

Merge pull request #883 from SBNSoftware/feature/frameshift_refactor_…

ce18cbd

…v10_11_01_hlay_functions Functionalise

Update combination condition with timing features integrating the exp…

43f7614

…ected jittering of the extended fragments.

adjust fcl parameters

6a24518

Add warning for some unexpected types of fragments and fix fragments …

177f2b4

…indexing. Improve jittering application.

Move constexpr upstream

2d93ca4

Use FrameShiftInfo object in CRT data reconstruction

857c9ed

Add catch for NoShift and allow cutting on ETrig frame

3ca8e1b

add frameshift functionality instead of direclty referencing spec tdc…

d849488

…/ptb

VCLanNguyen added crt Cosmic Ray Tagger breaking change pds Photon Detection System caf Common analysis framework data features for data processing labels Mar 4, 2026

update xa decode fcl

6b65e1b

VCLanNguyen changed the title ~~Feature/frame shift refactor v10 14 02~~ Timing Reconstruction Refactor Mar 4, 2026

This was referenced Mar 4, 2026

SBND Timing Reconstruction Refactor SBNSoftware/sbncode#637

Open

SBND Timing Reconstruction Refactor SBNSoftware/sbnobj#170

Open

SBND Timing Reconstruction Refactor SBNSoftware/sbnanaobj#188

Open

VCLanNguyen changed the title ~~Timing Reconstruction Refactor~~ SBND Timing Reconstruction Refactor Mar 4, 2026

VCLanNguyen marked this pull request as ready for review March 4, 2026 01:49

VCLanNguyen assigned aliciavr, asanchezcastillo, henrylay97 and lynnt20 Mar 4, 2026

VCLanNguyen requested review from aliciavr, asanchezcastillo, henrylay97 and lynnt20 March 4, 2026 01:51

aantonakis and others added 3 commits April 2, 2026 11:15

Removed timing corrections from the CRT veto since Lan has introduced…

8ec8a0e

… timing corrections at an earlier stage of reconstruction

Add dataTier to PMT decoder output configuration

6bc5699

Change dataTier from 'decoded-raw' to 'reconstructed'

93faf62

VCLanNguyen requested a review from PetrilloAtWork April 14, 2026 14:56

lynnt20 approved these changes Apr 14, 2026

View reviewed changes

aliciavr approved these changes Apr 14, 2026

View reviewed changes

VCLanNguyen requested a review from JosiePaton April 20, 2026 19:37

PetrilloAtWork requested changes Apr 28, 2026

View reviewed changes

	throw std::runtime_error(Form("Unix timestamps differ by more than 1 (%li)", unix_diff));
	throw std::runtime_error("Unix timestamps differ by more than 1 (" + std::to_string(unix_diff) + ")");

	std::string fDAQHeaderModuleLabel; // Instance label for DAQ header product
	std::string fDAQHeaderModuleLabel; ///< Instance label for DAQ header product

	double SubtractUTCTimestmap(const uint64_t& ts1, const uint64_t& ts2);
	double SubtractUTCTimestmap(uint64_t ts1, uint64_t ts2);

	//Return the difference in nanoseconds as double
	//Return the difference `ts1 - ts2` in nanoseconds as double

Conversation

VCLanNguyen commented Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link(s) to docdb describing changes (optional)

Relevant PR links (optional)

Checklist

Uh oh!

aantonakis commented Apr 14, 2026

Uh oh!

lynnt20 left a comment

Choose a reason for hiding this comment

Uh oh!

aliciavr left a comment

Choose a reason for hiding this comment

Uh oh!

VCLanNguyen commented Apr 21, 2026

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

VCLanNguyen commented Feb 14, 2026 •

edited

Loading