Update of PR1812

[brownd1978]

This PR is based on Pasha's PR 1812, adding some fixes.

[FNALbuild]

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

• 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 6f93616: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 6f93616.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 6f93616 at d755001
build (prof)	✅	Log file. Build time: 04 min 25 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 1 files
clang-tidy	➡️	15 errors 23 warnings
whitespace check	✅	no whitespace errors found

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

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.

[oksuzian]

PR #1854 — "Update of PR1812" · Review

Status: Open · mergeable (clean) · 1 approval (@bonventre) · build ✅ · clang-tidy ⚠️ (15 errors / 23 warnings, non-blocking) · 2 files, +86/−143

This builds on PR #1812 (Pasha's StrawDigisFromArtdaqFragments decoder), removing dead config/commented code and adding data-corruption guards. It's small and CI-green, but a few issues are worth addressing before merge.

---

🔴 Potential issues

1. Removed false defaults make two FCL params mandatory (breaking)
In DAQ/src/StrawDigisFromArtdaqFragments_module.cc the third fhicl::Atom argument (the default) was dropped:

    fhicl::Atom<bool> allowOfflineFallbackWhenPanelMapMissing{
      fhicl::Name("allowOfflineFallbackWhenPanelMapMissing"),
      fhicl::Comment("If TrackerPanelMap lookup fails, decode using offline StrawId(dtc,link,straw)")};
    fhicl::Atom<bool> forceOfflineAddressing{
      fhicl::Name("forceOfflineAddressing"),
      fhicl::Comment("Ignore TrackerPanelMap/mnid and decode StrawId directly as (dtc,link,straw)")};

These are now required. The prolog was updated to set them, so the main path works — but any other/standalone FCL that instantiates this module without these keys will now throw at construction. Either restore the false defaults or confirm every config path includes the updated prolog.

2. Hit-packet bounds check only validates the start address

              int offset = (ihit*np_per_hit_+1)*packet_size;
              hit_data   = (...) (roc_data+offset);
              if (roc_data+offset >= last_address) { ... break; }

The guard ensures the start of the hit is in-bounds, but hit_data is then read for ≥16 bytes (and the waveform loop reads further). If roc_data+offset is just below last_address, you still over-read. Consider roc_data + offset + packet_size > last_address (and account for the ADC packets).

3. Second packet (h0) dereferenced before any bounds check

              h0 = (...) (roc_data+packet_size);
              ...
              if (h0->NumADCPackets == 0) { ... }

packetCount > 1 is checked, but on truncated/corrupt ROC data that claim can be false, so reading roc_data+packet_size can run past last_address. Add a bounds check before dereferencing.

4. last_address still derived from possibly-corrupt buf[0]

        int      nbytes       = buf[0];
        uint8_t* last_address = fdata+nbytes;

Ironically, print_fragment was deliberately switched away from buf[0] to Frag->dataSizeBytes() (line 202) because buf[0] is unreliable — yet the actual processing boundary still trusts buf[0] with no cross-check against frag->dataSizeBytes(). A nbytes <= frag->dataSizeBytes() clamp would harden all the downstream guards.

5. Stale cached nADCPackets_/nSamples_/np_per_hit_ across events & runs
These members are set once when < 0 and never re-evaluated, so a configuration change between runs would silently use stale values. The "trust but watch if it changes" comment acknowledges this, but nothing actually watches it.

---

🟡 Nits / cleanup

• Dead members after the cleanup: _last_run (set in ctor, beginRun is now empty), minnesota_map_, and channel_map_[36][6] appear unused now — remove them.
• Comment/code mismatches:
  • if (nhits > 255) vs comment "if nhits**>=**255 it is a corruption".
  • Message says "skip event" but break only exits the ROC loop for the current DTC, not the event.
  • Format-string typo: "n_fragment_collections):{}" has a stray ).
• Redundant guard: nSamples_ <= 3 can only occur when NumADCPackets == 0, which is already skipped earlier.
• Style: mix of if (debugMode_) and if (debugMode_ > 0).
• clang-tidy flags 15 errors / 23 warnings — worth a quick scan to confirm none are from this module.