STM unpacking latest changes and updates

[Etho-b02]

Changes to the Offline in Unpacking flow

• STMDigisFromFragment was changed to make module more readable and structure more clear
• STMPrint now includes switches that can fit how you want to view an art file for the STM side
• STMWaveform includes raw pointer index
• STMWaveform also includes the option to change sample_number to time (ns) via switch in fcl
• New data products in STMFragemntSummary to capture how many events were skipped due to RawHeaderFlags

[FNALbuild]

Hi @Etho-b02,
You have proposed changes to files in these packages:

• DAQ
• RecoDataProducts
• STMReco

which require these tests: build.

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

📝 The author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

[AndrewEdmonds11]

All looks good to me. Thanks, Bryan!

[oksuzian]

@FNALbuild run build test

[oksuzian]

PR Review Summary — #1931

Reviewed at head 685c2c19. First pass.

Decision

• 🔴 request changes — one committed fcl cannot start (finding 1), plus three S1 items. Everything below is checkable from the diff; nothing here is a style objection.

Scope understood

• Rewrite of STMDigisFromFragments (+1183/−753): per-detector state objects replacing parallel scalars, new raw-header validation (length, anchors, bad/missing, prescale), and a reinterpretation of the PH payload.
• New print switches in STMPrintFragments; four new counters on the STMFragmentSummary data product; plotting/fcl updates.

---

Findings

1. 🔴 [S0] plotSTMWaveformDigis.fcl names an analyzer that does not exist — the job cannot start.

   • Evidence: STMReco/fcl/plotSTMWaveformDigis.fcl:68

     anaPath : [ plotRawWaveformDigisHPGe, plotRawWaveformDigisTimeHPGe,
                 plotRawWaveformDigisEvTimeHPGe, plotZSWaveformDigisHPGe, ... ]
     

     The analyzers block (lines 23–65) defines plotRawWaveformDigisHPGe,
     plotRawWaveformDigisTimeHPGe, plotZSWaveformDigisHPGe,
     plotRawWaveformDigisLaBr, plotZSWaveformDigisLaBr. There is no
     plotRawWaveformDigisEvTimeHPGe, and neither included prolog defines one.
   • Impact: art rejects an unknown module label on a path at configuration time.
     mu2e -c Offline/STMReco/fcl/plotSTMWaveformDigis.fcl fails before the first
     event, so the file as committed is unusable.
   • Suggested fix: add the missing analyzer (presumably the event_time twin of
     plotRawWaveformDigisTimeHPGe, i.e. xAxis : "event_time"), or drop the label
     from anaPath.

2. 🟠 [S1] The raw payload length is taken from the header and used unchecked — a corrupt RAW_LEN reads past the end of the fragment.

   • Evidence: payloadWords() in the overlay returns rawLength() for a raw
     fragment — a header field (stm::RawHeader::RAW_LEN), not a size derived from
     the fragment. DAQ/src/STMDigisFromFragments_module.cc then walks and copies
     that many words:

     auto payloadPtr   = stm_frag.payloadBegin();   // data_ + RawHeader::WORDS
     auto payloadWords = stm_frag.payloadWords();   // == rawLength(), from the header
     for (size_t k = 0; k < payloadWords; ++k) { if (payloadPtr[k] != 0) ... }
     ...
     stm_waveform.set_data(payloadWords, payloadPtr);

     The new guard if (stm_frag.dataWords() < stm::RawHeader::WORDS) only
     establishes that the 22-word header fits; it says nothing about whether
     rawLength() words follow it.
   • Impact: a raw header whose RAW_LEN exceeds the real payload produces an
     out-of-bounds read and a STMWaveformDigi filled with whatever follows the
     fragment — silently, since the all-zeros scan will usually find a non-zero byte
     and classify it "good".
   • This is in scope even though the same pattern is on main: the PR's purpose
     here is to add exactly this validation, and its own ZS path already does the
     check properly —

     if (currentZSLength > static_cast<size_t>(dataEndPtr - adc)) { malformedZS = true; break; }

     The raw path needs the equivalent.
   • Suggested fix: before using payloadWords, require
     stm::RawHeader::WORDS + payloadWords <= stm_frag.dataWords(); on failure count
     it with the other invalid-header cases and continue.

3. 🟠 [S1] PH digis are silently truncated to the header's count, and a zero count is misreported as a zero-filled fragment.

   • Evidence: DAQ/src/STMDigisFromFragments_module.cc

     size_t const nPairsInFragment = payloadWords / 2;
     size_t const nPairsToRead = extractedPHInfo ? std::min<size_t>(nPairsInFragment, phCount)
                                                 : nPairsInFragment;
     for (size_t k = 0; k < nPairsToRead; ++k) {
         if (payloadPtr[k*2+1] != 0) { allPHAreZeros = false; break; }
     }
     if (allPHAreZeros) { ++eventMetrics.ph.zero; ... continue; }

   • Impact: when the header's PH_NUM disagrees with what the payload actually
     carries, the excess pairs are dropped with no counter and no message. In the
     limit phCount == 0, nPairsToRead is 0, the zero-check loop never executes,
     allPHAreZeros stays at its initial true, and a fragment full of real pulse
     heights is discarded and tallied as zero-filled — the diagnostics then say the
     data was empty rather than that the header was inconsistent.
   • Note the inconsistency with the ZS path in the same file, where the identical
     class of header/payload disagreement is fatal (ZS LENGTH MISMATCH,
     ZS REGION COUNT MISMATCH). One of the two treatments should change.
   • Suggested fix: compare nPairsInFragment against phCount explicitly, count and
     report the mismatch, and decide deliberately whether to throw (as ZS does) or
     read the fragment's own count. Initialize allPHAreZeros from whether the loop
     ran, not to true.

4. 🟠 [S1] The new STMPrintFragments switch cannot be set — the fcl key and the Config name differ.

   • Evidence: DAQ/src/STMPrintFragments_module.cc declares
     fhicl::Atom<bool> printInnerFrags {fhicl::Name("printInnerFrags"), ...} with no
     default, so it is a required parameter. DAQ/test/inspectSTMFile.fcl:51 sets
     printInnerFragments: false.
   • Impact: as committed nothing fails, only because stmPrint is on no path
     (end_paths is commented out — see finding 7). The moment anyone puts it on a
     path, art reports an unknown key printInnerFragments and a missing required
     printInnerFrags. So the switch advertised in the PR description is not
     reachable from the one fcl that configures it.
   • Suggested fix: rename the fcl key to printInnerFrags, and put stmPrint back
     on a path so the three new switches are exercised at least once.

5. 🟡 [S2] The PH payload is reinterpreted from "one word per pulse height" to "(time, height) pairs" — a data-format change the PR body does not mention.

   • Evidence: before, every payload word became a digi with time hard-coded to 0:

     for (size_t i_PH = 0; i_PH < digiWords ; ++i_PH){
       int16_t PH = digiPtr[i_PH];
       mu2e::STMPHDigi PH_digi(0, PH);

     after, words are consumed in pairs and the time field is filled from the payload.
   • Impact: the number of STMPHDigi per fragment halves and time() becomes
     meaningful. Anyone comparing against previously produced files will see both.
   • I believe the new reading is the correct one and worth stating plainly in the PR
     body rather than leaving to be discovered: the raw header carries PH_NUM
     (stm::RawHeader::PH_NUM), STMPHDigi has always had a _time member that the
     old code never populated, and the companion histogram change in
     PlotSTMPHSpectrum_module.cc moves the spectrum from (1000, 0, 1e4) to
     (10000, -10000, 0) — consistent with the old plot having been polluted by
     positive time words sitting in an otherwise negative-going ADC spectrum.
   • Suggested fix: say so in the PR description, and show one before/after PH
     spectrum from the same input file as validation.

6. 🟡 [S2] Raw-header and parent state are reset only when a raw fragment arrives, never after being consumed.

   • Evidence: headerState = RawHeaderState{} and the parentState reset both live
     at the top of the isRaw() branch. The old code cleared its ZS expectations
     inside the ZS branch immediately after copying them out ("After copying
     variables, reset detector specific variables").
   • Impact: for one raw followed by several ZS fragments this is an improvement —
     every ZS in the set now gets the parent art::Ptr, where before only the first
     did. But if a ZS fragment can ever arrive without a preceding raw fragment in the
     same event, it now inherits the previous set's expectedZSLength /
     expectedZSRegions and is checked against them — which throws
     STM_UNPACKING and kills the job — and would attach a parent art::Ptr pointing
     at an unrelated raw waveform.
   • I could not determine from the code whether the DAQ can emit ZS without raw, so
     this is a question rather than an observed defect: can it? If yes, the state
     needs a "consumed" reset or a per-set sequence check; if no, a comment saying the
     set is guaranteed complete would stop the next reader worrying about it.

7. 🟡 [S2] Both test fcl files are committed in personal working state.

   • Evidence: DAQ/test/inspectSTMFile.fcl:58,63 comment out the output path and
     end_paths, so the job now produces products and discards all of them;
     outputs.stmOutput is left defined but unreachable. Line 74 writes to
     data/dig.stm.art, and STMReco/fcl/plotSTMWaveformDigis.fcl:17 writes
     data/stmWaveformDigis.root.
   • Impact: the data/ prefix requires that directory to already exist in the
     working directory — RootOutput and TFileService both fail otherwise, so these
     files only run from one person's sandbox layout. And inspectSTMFile.fcl no
     longer demonstrates the thing it is named for.
   • Suggested fix: drop the data/ prefixes and restore end_paths : [e1].

8. ⚪ [S3] Collected minor items — grouped deliberately, none of these gate anything.

   • uint16_t outerFragID; is now declared uninitialized (it was {0}); it is
     assigned before use, but the standard asks for initialization at declaration.
   • Dead state and commented-out code: //size_t unknownFragsThisEvent{0};,
     // headerState.rawHeaderIsValid = stm_frag.hasValidAnchors(); leaving
     RawHeaderState::rawHeaderIsValid written nowhere and read nowhere; the two
     commented-out accessor/member blocks in STMFragmentSummary.hh; //uint16_t lastLen = 0; in STMPrintFragments. containsZSInfo and containsPHInfo are
     always assigned true together at the same point, so they carry no information
     the RawHeaderState object's freshness does not already carry.
   • PlotSTMWaveformDigis_module.cc tests _xAxis == "waveform_time [nsec]", but
     STMUtils::getBinning only accepts sample_number, waveform_time,
     event_time and throws otherwise — so that branch is unreachable and the
     waveform_time histograms get no x-axis title.
   • PlotSTMPHSpectrum gives the 2D plot an x-range of (-1000, 1000) while the
     filled quantity is binBlock = eventCount/100, which is never negative.
   • The file was reindented from 2 to 4 spaces in the same commits as the logic
     rewrite, which is most of why the diff is 1900 lines. Separating a pure
     reformat from behaviour changes makes both reviewable; worth doing next time
     rather than redoing now.

---

Verified, no action needed

• 🟢 The six save* fcl parameters were renamed consistently. saveRawWaveform_HPGe
  and its five siblings appear nowhere in Offline outside the module and
  inspectSTMFile.fcl (both updated here), and STMDigisFromFragments is not
  referenced anywhere in Production. No orphaned keys.
• 🟢 Every new overlay accessor the module now calls — phCount(), badData(),
  missing(), hasValidAnchors(), rawPrescaled(), rawPrescaleValue(),
  zsPrescaled(), zsPrescaleValue(), stm::RawHeader::WORDS — is present in
  artdaq_core_mu2e v9_06_00, which is what main pins (envset p106, bumped
  2026-08-09). No companion release is needed. This module is the first consumer of
  them in Offline, which is why the build matters here.
• 🟢 Removing plotPHWaveformDigisHPGe/plotPHWaveformDigisLaBr is a real fix, not
  just cleanup: they pointed PlotSTMWaveformDigis at makeSTMDigis:phHPGe, which is
  an STMPHDigiCollection, not the STMWaveformDigiCollection that module consumes.
• 🟢 Adding four members to STMFragmentSummary needs no ClassVersion bump —
  0 of the 295 entries in RecoDataProducts/src/classes_def.xml carry one, so
  auto-versioning is the convention throughout this package.

Validation check

• Build/tests run by reviewer: none. All findings above are from reading the diff and
  the current main/overlay sources; no code was compiled or executed.
• CI at the reviewed head: no build result. mu2e/buildtest has been pending
  since the PR was opened (2026-08-12T22:43Z) and jenkins/ghprb is the only green
  context. FNALbuild notes the author is not a member of the Mu2e organisation, so
  the build needs a member to trigger it — I have requested one.
• Config contract check: fail — findings 1 and 4.
• Cross-repo consistency: pass — external overlay verified against the pinned
  adcm v9_06_00; no Production or mu2e-trig-config impact.

Residual risk

• Nothing here has been compiled. Findings 1 and 4 are configuration errors that a
  build will not catch — only running the fcl does.
• Finding 6 depends on a DAQ format guarantee I could not establish from the code;
  if ZS-without-raw is possible, the consequence is a thrown exception mid-job.
• The PH reinterpretation (finding 5) has no before/after evidence attached to the
  PR, so its correctness rests on the header layout rather than on a demonstration.

Author follow-ups

1. Fix anaPath in plotSTMWaveformDigis.fcl — the file cannot run as committed.
2. Bound the raw payload against the real fragment size before reading rawLength()
   words, mirroring what the ZS path already does.
3. Decide and make consistent how a header/payload count disagreement is handled:
   ZS throws, PH silently truncates.
4. Rename printInnerFragments → printInnerFrags and put stmPrint back on a path.
5. Answer whether a ZS fragment can arrive without a preceding raw fragment in the
   same event.
6. State the PH (time, height) pair change in the PR description and attach a
   before/after spectrum.
7. Restore end_paths and drop the data/ output prefixes from both fcl files.

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 685c2c1.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 685c2c1 at 38f6943
build (prof)	✅	Log file. Build time: 04 min 27 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 5 files
clang-tidy	➡️	8 errors 72 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at 685c2c1 after being merged into the base branch at 38f6943.

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.