Change to produce a fragment per DTC

[michaelmackenzie]

I don't see very much difference in the timing, but I think it's more correct to collect data into a fragment per DTC. I tested this on 20k MDC2020 and 20k MDC2025 events and found no issues in calo or tracker digis.

[FNALbuild]

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

• DAQ

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[Copilot]

Pull request overview

This PR updates the DAQ digi-to-fragment producers to build and emit one DTCEVT fragment per DTC, rather than packing multiple DTC subevents into a single DTCLib::DTC_Event and then post-splitting.

Changes:

• Refactors both Straw and Calo fragment builders to produce std::map<uint8_t, DTCLib::DTC_Event> (one DTC_Event per DTC).
• Updates produce() in both modules to loop over per-DTC events and emit one artdaq::Fragment per DTC.
• Sets output fragment ID to the DTC ID for both tracker and calo fragment producers.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
DAQ/src/StrawDigisToFragments_module.cc	Build per-DTC DTC_Events and emit one DTCEVT fragment per DTC (tracker).
DAQ/src/CaloDigisToFragments_module.cc	Build per-DTC DTC_Events and emit one DTCEVT fragment per DTC (calorimeter).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[FNALbuild]

☀️ The build tests passed at 8761315.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 8761315 at a31b07e
build (prof)	✅	Log file. Build time: 04 min 06 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 2 files
clang-tidy	➡️	16 errors 133 warnings
whitespace check	✅	no whitespace errors found

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

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]

I'll review this pull request by examining its details and changes.Here's my review of PR #1781. The core change refactors both CaloDigisToFragments and StrawDigisToFragments to produce one artdaq fragment per DTC instead of a single combined fragment (or per-subevent fragment). Overall the approach is sound and simplifies the code, but I found several issues:

---

1. Inconsistent indentation in StrawDigisToFragments::buildDtcEventsFromDigis signature

void mu2e::StrawDigisToFragments::buildDtcEventsFromDigis(
    art::Event const& event, mu2e::StrawDigiCollection const& strawDigis,
  mu2e::StrawDigiADCWaveformCollection const& strawADCs,
  std::map<uint8_t, DTCLib::DTC_Event>& dtcEvents) {

The first continuation line uses 4-space indent while the next two use 2-space indent. This should be consistent (either all 4-space aligned or all matching the opening paren).

---

2. Fragment ID collision risk between Calo and Tracker modules

Both modules now set the fragment ID to dtcID:

fragPtr->setFragmentID(static_cast<artdaq::Fragment::fragment_id_t>(dtcID));

fragPtr->setFragmentID(static_cast<artdaq::Fragment::fragment_id_t>(dtcID));

If calo DTC 0 and tracker DTC 0 both produce fragments in the same event, they will share the same (sequenceID, fragmentID) pair. Downstream consumers that key on fragment ID may conflate them. Consider encoding the subsystem into the fragment ID (e.g., dtcID | (subsystem << 8)) or using a running counter to guarantee uniqueness within an event.

---

3. std::map operator[] silently creates empty DTC_Event entries for DTCs with no digis

In buildDtcEventsFromDigis, the loop iterates over byDtcAndLink:

  for (auto& [dtcID, linkMap] : byDtcAndLink) {
    auto& dtcEvent = dtcEvents[dtcID];
    dtcEvent.SetEventWindowTag(DTCLib::DTC_EventWindowTag(static_cast<uint64_t>(event.event())));

This is fine because byDtcAndLink only has entries for DTCs that have actual digis. However, inside the inner link loop (lines 249–250), linkMap[linkID] is called for every link 0–5, default-inserting empty TrackerBlockData/CaloBlockData entries even for links with no data. While these produce empty blocks (which is intentional for the protocol), be aware that if the protocol ever changes, these phantom entries could cause subtle issues.

---

4. Asymmetric size-mismatch handling between Calo and Tracker

In CaloDigisToFragments::produce, there's robust size-mismatch handling with skipFragmentOnSizeMismatch_ and diagnostic messages at diagLevel_ > 0:

    bool sizeMismatch = packed.size() != eventBytes;
    if (sizeMismatch) {
      if (skipFragmentOnSizeMismatch_) {
        ...
        continue;
      }
      ...
    }

In StrawDigisToFragments::produce, the size-mismatch check only logs at diagLevel_ > 1 and never skips the fragment:

    if (diagLevel_ > 1 && packed.size() != eventBytes) {
      std::cout << "[StrawDigisToFragments::produce] WARNING: DTC " << static_cast<int>(dtcID)
                << " packed size " << packed.size()
                << " differs from event header size " << eventBytes << std::endl;
    }

This inconsistency means corrupted tracker fragments could be silently emitted. Consider adding the same skipFragmentOnSizeMismatch option to the tracker module, or at minimum logging at diagLevel_ > 0 to match the calo module.

---

5. Potential uint16_t overflow in transferByteCount for tracker

      block.transferByteCount = static_cast<uint16_t>((numPackets + 1) * 16);

If a ROC link has many hits (e.g., numPackets > 4095), (numPackets + 1) * 16 exceeds 65535 and silently wraps. This is a pre-existing issue but worth noting given that data is now grouped per-DTC (potentially more hits per DTC event). A bounds check or uint32_t intermediate would be safer.

---

6. Test FCL: CaloFragmentsToDigis input tag may need updating

    CaloFragmentsToDigis : {
      module_type   : "CaloDigisFromDTCEvents"
      dataType : 0
      diagLevel : 0
      useOfflineID : true
      useDTCROCID : false
    }

The CaloDigisFromDTCEvents module presumably reads artdaq::Fragments from the event. By default it likely expects CaloDigisToFragments as the input tag. If the default doesn't match, this test will fail silently (producing zero digis) and the comparator may pass vacuously. Verify that the default input tag for CaloDigisFromDTCEvents is actually "CaloDigisToFragments", or explicitly set it in the FCL.

---

Summary

#	Severity	Issue
1	Low	Inconsistent indentation in function signature
2	Medium	Fragment ID collision risk between calo and tracker DTCs with same ID
3	Low	operator[] default-inserts empty map entries (benign but worth documenting)
4	Medium	Asymmetric size-mismatch handling — tracker silently emits potentially corrupt fragments
5	Low	Potential uint16_t overflow in transferByteCount (pre-existing)
6	Low	Test FCL may need explicit input tag for CaloDigisFromDTCEvents

The most actionable items are #2 (fragment ID uniqueness) and #4 (matching the size-mismatch safety between the two modules).

[michaelmackenzie]

Responding to the AI comments:

1. Fixed the indentation
2. I added a DTC ID offset, but in general the subsystem ID is used to select fragments so I don't think should be an issue
3. This is expected
4. I removed this check in calo, as it was from early debugging
5. This is the data type used in the fragment blocks. If we have more than 4k hits / ROC / event we may be in more trouble
6. Fragments are retrieved with a getMany call, no tags are used

I also updated the test fcl to reflect these changes

[oksuzian]

@FNALbuild build test

[michaelmackenzie]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 1c924fc.

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

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

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.