updating producer to reflect decoder changes by matthewstortini · Pull Request #1717 · Mu2e/Offline

matthewstortini · 2026-02-09T20:31:10Z

Changing MTP (mobile timing paddles) to MSD (mobile synchronization detector), and making small producer changes to reflect updates in corresponding decoder. Relies on artdaq-core-mu2e PR#103 Mu2e/artdaq-core-mu2e#103.

FNALbuild · 2026-02-09T20:31:38Z

Hi @matthewstortini,
You have proposed changes to files in these packages:

RecoDataProducts
DAQ

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for fb1772c: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild · 2026-02-09T20:41:35Z

☔ The build is failing at fb1772c.

scons: *** [build/al9-prof-e29-p091/Offline/tmp/DAQ/src/MSDHitsFromDTCEvents_module.os] Error 1

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `fb1772c` at `a500268`
build (prof)	❌	Log file.
ceSimReco	〰️	Log file.
g4test_03MT	〰️	Log file.
transportOnly	〰️	Log file.
POT	〰️	Log file.
g4study	〰️	Log file.
cosmicSimReco	〰️	Log file.
cosmicOffSpill	〰️	Log file.
ceSteps	〰️	Log file.
ceDigi	〰️	Log file.
muDauSteps	〰️	Log file.
ceMix	〰️	Log file.
rootOverlaps	〰️	Log file.
g4surfaceCheck	〰️	Log file.
trigger	❗	Log file.
check_cmake	〰️	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 4 files
clang-tidy	➡️	3 errors 4 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at fb1772c after being merged into the base branch at a500268.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

michaelmackenzie

This looks nice, thank you for this update. I've just added two questions, but no required changes.

This looks ready to merge once Mu2e/artdaq-core-mu2e#103 is merged and a new muse env is created.

michaelmackenzie · 2026-02-09T22:08:35Z

DAQ/src/MSDHitsFromDTCEvents_module.cc

+            if (decoder.GetHitTOT(&packet, hit, hitTOT)) {
+              msdHit.setTOT(hitTOT);
+            }
+            if (msdHit.hasTime() || msdHit.hasTOT()) {


Is it a valid case to have a TOT but no time information?

Some versions will have time, but not TOT. But yeah we should never end up with a TOT, but no time. I guess just checking the time would be better.

michaelmackenzie · 2026-02-09T22:11:43Z

DAQ/src/MSDHitsFromDTCEvents_module.cc

+                std::cout << "Hit " << hit << " in packet: time = " << msdHit.time()
+                          << " ns, TOT = " << msdHit.tot() << " ns" << std::endl;
+              }
+            } else if (_debugLevel > 1) {


Should this have a printout without a requirement that the debug is on to log events with an MSD packet but the time and TOT aren't able to be retrieved?

matthewstortini · 2026-02-09T22:32:59Z

This looks nice, thank you for this update. I've just added two questions, but no required changes.

This looks ready to merge once Mu2e/artdaq-core-mu2e#103 is merged and a new muse env is created.

Yes, I will address the PR comments tomorrow (about to catch a flight). But when this PR has everyone's approval, I will go ahead and merge the adcm PR, and make a new adcm tag. At that point hopefully Dave/Yuri can make a new envset using said adcm tag, and trigger the build bot for this PR with the envset PR. It would be best to get approvals first so we can do this all quickly so as not to disrupt any online folks building with the heads of online and Offline. Does that sound good @brownd1978 and @oksuzian?

oksuzian · 2026-02-10T16:08:21Z

Here's my review of PR #1717, focusing on critical issues. This PR renames MTP (Mobile Timing Paddles) to MSD (Mobile Synchronization Detector) and rewrites the producer module to use the new MobileSyncDataDecoder.

Issues Found

🔴 1. Bug: `index` counter only increments inside debug block (will report wrong fragment number)

  size_t index = 0;
  for (const auto& frag : fragments) {
    mu2e::DTCEventFragment eventFragment(frag);
    auto dtcSubEvents =
        eventFragment.getSubsystemData(DTCLib::DTC_Subsystem::DTC_Subsystem_MobileSync);
    if (_debugLevel > 2) {
      std::cout << "  Fragment " << index << " has " << dtcSubEvents.size()
                << " MSD DTC subevents, total sub events = "
                << eventFragment.getData().GetSubEvents().size() << std::endl;
      ++index;  // ← BUG: only increments when debug is on!
    }

The ++index is inside the if (_debugLevel > 2) block, so it only increments when debugging is enabled. When debug is off, index stays 0 forever. The same bug exists for index_subevt (line 103–107). Move the increment outside the if block.

🔴 2. Hit validity check uses `||` but should use `hasTime()` only (as acknowledged in existing review discussion)

            if (msdHit.hasTime() || msdHit.hasTOT()) {

As @michaelmackenzie already flagged and the author agreed: a hit with TOT but no time is not valid. The condition should be msdHit.hasTime() (not ||). Currently, a hit with only TOT (and time = -1.0) would be stored in the collection — this is incorrect physics behavior and could corrupt downstream analyses.

🔴 3. `MSDHit::hasTime()` / `hasTOT()` use `>= 0` check but `hitTime`/`hitTOT` are initialized to `0` — zero is accepted as valid

  bool hasTime() const { return _time >= 0; }
  bool hasTOT() const { return _tot >= 0; }

In the producer, hitTime and hitTOT are initialized to 0:

            double hitTime = 0;
            double hitTOT = 0;

If decoder.GetHitTime() returns false (failure), hitTime remains 0, and setTime() is never called so _time stays -1.0 — this is fine. However, if the decoder API modifies hitTime to 0.0 on failure (a common pattern), then calling setTime(0.0) would make hasTime() return true for an invalid hit. The correctness depends on the decoder contract. More critically, a legitimate hit at exactly time 0.0 ns would be considered valid, which is correct — but the sentinel value -1.0 means a real hit time of -1.0 is impossible to represent. For floating-point sentinels, consider using NaN or a separate boolean flag.

🔴 4. `clockFrequency` parameter removed — breaking change for existing FHiCL configurations

The old MTPHitsFromDTCEvents module had a clockFrequency fcl parameter:

    fhicl::Atom<float> clockFrequency    {fhicl::Name("clockFrequency"), fhicl::Comment("clock frequency in MHz")};

The new MSDHitsFromDTCEvents removes it entirely. This is presumably intentional (the decoder now handles time conversion internally), but any existing FHiCL scripts referencing MTPHitsFromDTCEvents will need to be updated. If there are FHiCL files in this repo that reference the old module, they are not updated in this PR.

🟡 5. `channelID` field dropped from `MSDHit` — data model regression

The old MTPHit stored _channelID (paddle identifier). The new MSDHit has no channel/detector ID at all. If there are multiple MSD paddles/channels, there is no way for downstream code to distinguish which detector element produced the hit. Confirm this is intentional with the detector group.

🟡 6. Old `classes_def.xml` was missing `MTPHitCollection` and `Wrapper` — but confirm this didn't break persistency

The old classes_def.xml only declared mu2e::MTPHit but not MTPHitCollection or art::Wrapper<MTPHitCollection>:

  <class name="mu2e::MTPHit"/>

The new version correctly adds all three. Good fix, but this means the old module may have had a ROOT dictionary issue for MTP data persistency.

Summary

Severity	Issue	Location
🔴 Critical	`index`/`index_subevt` only increment inside debug blocks	`MSDHitsFromDTCEvents_module.cc:97,107`
🔴 Critical	`hasTime() \|\| hasTOT()` should be `hasTime()` only	`MSDHitsFromDTCEvents_module.cc:124`
🔴 Critical	Floating-point sentinel `-1.0` is fragile for validity checks	`MSDHit.hh:22-23`
🔴 Critical	Existing FHiCL configs referencing old module not updated	Missing files in PR
🟡 Medium	`channelID` dropped — may lose detector element identification	`MSDHit.hh`
🟡 Medium	Old `classes_def.xml` was missing collection/wrapper entries	`classes_def.xml` (fixed)

matthewstortini · 2026-02-10T18:05:21Z

Bug: index counter only increments inside debug block (will report wrong fragment number)

Hit validity check uses || but should use hasTime() only (as acknowledged in existing review discussion)

MSDHit::hasTime() / hasTOT() use >= 0 check but hitTime/hitTOT are initialized to 0

clockFrequency parameter removed — breaking change for existing FHiCL configurations

channelID field dropped from MSDHit — data model regression

Old classes_def.xml was missing MTPHitCollection and Wrapper — but confirm this didn't break persistency

This is fine -- we only need this increment for debugging.
Fixed in latest commit. These boolean functions are no longer used in producer, but I'm keeping them there for use in analysis modules.
This is fine. The hitTime / hitTOT are only used if the decoder functions that update them return true - otherwise msdHit will keep it's default values of -1 for each member.
This is now in the decoder.
Not being tracked .. can add in future if we do end up tracking.
This is fine.

matthewstortini · 2026-02-14T04:55:47Z

@FNALbuild run build test with #1726

FNALbuild · 2026-02-14T04:56:13Z

⌛ The following tests have been triggered for def696c: build (Build queue - API unavailable)

FNALbuild · 2026-02-14T05:17:41Z

☀️ The build tests passed at def696c.

Test	Result	Details
test with	✅	Included #1726 @ `43b328f` by merge
merge	✅	Merged `def696c` at `7a15006`
build (prof)	✅	Log file. Build time: 08 min 43 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 4 files
clang-tidy	➡️	2 errors 7 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at def696c after being merged into the base branch at 7a15006.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

FNALbuild · 2026-02-14T05:17:44Z

📝 The HEAD of main has changed to db6161d. Tests are now out of date.

matthewstortini · 2026-02-15T18:58:30Z

@FNALbuild run build test with #1726

FNALbuild · 2026-02-15T18:58:54Z

⌛ The following tests have been triggered for def696c: build (Build queue - API unavailable)

FNALbuild · 2026-02-15T19:11:02Z

☀️ The build tests passed at def696c.

Test	Result	Details
test with	✅	Included #1726 @ `43b328f` by merge
merge	✅	Merged `def696c` at `6d7cc02`
build (prof)	✅	Log file. Build time: 04 min 10 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 4 files
clang-tidy	➡️	2 errors 7 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at def696c after being merged into the base branch at 6d7cc02.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

matthewstortini added 2 commits February 9, 2026 14:25

changing mtp to msd and updating producer to reflect decoder changes

3ac11ac

producer and reco product for MSD hits

fb1772c

FNALbuild added build pending build running and removed build pending labels Feb 9, 2026

removing print statement

53852f3

FNALbuild added build pending and removed build running labels Feb 9, 2026

This was referenced Feb 9, 2026

new decoder for mobile synchronization detector Mu2e/artdaq-core-mu2e#103

Merged

updating analysis module given changes in reco product name Mu2e/otsdaq-mu2e-sync#6

Merged

michaelmackenzie approved these changes Feb 9, 2026

View reviewed changes

GetHitTime is the ultimate check if hit is valid or not

def696c

matthewstortini mentioned this pull request Feb 14, 2026

p092 envset for adcm 9.1.0 #1726

Merged

FNALbuild added build running and removed build pending labels Feb 14, 2026

FNALbuild added build pending and removed build running labels Feb 14, 2026

FNALbuild added build running and removed build pending labels Feb 15, 2026

FNALbuild added build finished and removed build running labels Feb 15, 2026

oksuzian merged commit 0a3502c into Mu2e:main Feb 15, 2026
14 checks passed

Conversation

matthewstortini commented Feb 9, 2026

Uh oh!

FNALbuild commented Feb 9, 2026

Uh oh!

FNALbuild commented Feb 9, 2026

Uh oh!

michaelmackenzie left a comment

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

matthewstortini Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

oksuzian commented Feb 10, 2026

Issues Found

🔴 1. Bug: index counter only increments inside debug block (will report wrong fragment number)

🔴 2. Hit validity check uses || but should use hasTime() only (as acknowledged in existing review discussion)

🔴 3. MSDHit::hasTime() / hasTOT() use >= 0 check but hitTime/hitTOT are initialized to 0 — zero is accepted as valid

🔴 4. clockFrequency parameter removed — breaking change for existing FHiCL configurations

🟡 5. channelID field dropped from MSDHit — data model regression

🟡 6. Old classes_def.xml was missing MTPHitCollection and Wrapper — but confirm this didn't break persistency

Summary

Uh oh!

matthewstortini commented Feb 10, 2026

Uh oh!

matthewstortini commented Feb 14, 2026

Uh oh!

FNALbuild commented Feb 14, 2026

Uh oh!

FNALbuild commented Feb 14, 2026

Uh oh!

FNALbuild commented Feb 14, 2026

Uh oh!

matthewstortini commented Feb 15, 2026

Uh oh!

FNALbuild commented Feb 15, 2026

Uh oh!

FNALbuild commented Feb 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matthewstortini commented Feb 9, 2026 •

edited

Loading

🔴 1. Bug: `index` counter only increments inside debug block (will report wrong fragment number)

🔴 2. Hit validity check uses `||` but should use `hasTime()` only (as acknowledged in existing review discussion)

🔴 3. `MSDHit::hasTime()` / `hasTOT()` use `>= 0` check but `hitTime`/`hitTOT` are initialized to `0` — zero is accepted as valid

🔴 4. `clockFrequency` parameter removed — breaking change for existing FHiCL configurations

🟡 5. `channelID` field dropped from `MSDHit` — data model regression

🟡 6. Old `classes_def.xml` was missing `MTPHitCollection` and `Wrapper` — but confirm this didn't break persistency