converts CrvDigis to artFragments

[ehrlich-uva]

Added a module that converts CrvDigis to artFragments and an additional modules that compares two CrvDigiCollections.
Small changes for the CRV photon generator (removal of cutoffs for yield variations, not needed anymore) were already in my workspace. I will keep them in this PR. The PR also contains a merge from changes in Offline that happened in the meantime.

[FNALbuild]

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

• DAQ
• CRVResponse

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☔ The build tests failed for 943b1fb.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 943b1fb at 891d8a5
build (prof)	✅	Log file. Build time: 04 min 24 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. Return Code 1.
FIXME, TODO	➡️	TODO (1) FIXME (1) in 4 files
clang-tidy	➡️	6 errors 153 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 943b1fb after being merged into the base branch at 891d8a5.

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]

@FNALbuild run build test

[FNALbuild]

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

[FNALbuild]

☀️ The build tests passed at 99f99a7.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 99f99a7 at 891d8a5
build (prof)	✅	Log file. Build time: 04 min 09 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 (1) FIXME (1) in 4 files
clang-tidy	➡️	6 errors 153 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 99f99a7 after being merged into the base branch at 891d8a5.

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]

Review: PR #1802 — converts CrvDigis to artFragments

Build tests pass on the latest commit (99f99a7). Below are the issues I found after reading the diff. They are grouped by severity.

🔴 Bugs / correctness

1. Wrong exception category in CrvDigiCollectionsComparator

  if (failOnMismatch_) {
    throw cet::exception("CALODIGI_COMPARE") << summary.str();

Category is CALODIGI_COMPARE — this is the CRV module. Looks like copy/paste from a calorimeter analog. Should be something like "CRV_DIGI_COMPARE".

2. Dead branch introduced in MakeCrvPhotons::GetAverageNumberOfCerenkovPhotons

    if(beta<=i->first) //first beta in the Cerenkov map is alway greater than 0.0 and has number of photons of 0.
    {
      if(i->first-prevBeta==0) return i->second;  //this shouldn't happen
      double numberPhotons=prevNumberPhotons+(i->second-prevNumberPhotons)/(i->first-prevBeta)*(beta-prevBeta);

i->first - prevBeta == 0 is unreachable: std::map keys are unique so two consecutive first values can never be equal. Also, floating-point equality to zero is fragile. Either remove the guard or use a proper < epsilon test. Typo alway → always.

3. hitPacket.hitInfo.hitTime = digi.GetStartTDC()*2 truncation

  hitPacket.hitInfo.hitTime     = digi.GetStartTDC()*2; //online: period of 6.25ns, offline: period of 12.5ns.

GetStartTDC() returns uint16_t, so *2 can overflow past 65535 before it is stored into the (typically 16-bit) hitTime field. Cast to a wider type first, clamp, or mask explicitly, and either verify startTDC <= 32767 or document the invariant. If hitTime is bitfield-narrower than 16 bits, the truncation is even worse.

4. Missing EventWindowTag2 — only 32 of 48 EWT bits encoded

      block.crvROCstatus.EventWindowTag0=event.event() & 0xffff;
      block.crvROCstatus.EventWindowTag1=(event.event()>>16) & 0xffff;

The CRV ROC status packet has three 16‑bit EWT words. EventWindowTag2 is left uninitialized. If the default ctor zeros it, fine for small event numbers, but this should be explicit:

block.crvROCstatus.EventWindowTag2 = (event.event()>>32) & 0xffff;

5. fragment timestamp / EWT is the event number, not a real EventWindowTag

    fragPtr->setSequenceID(static_cast<artdaq::Fragment::sequence_id_t>(event.event()));
    fragPtr->setFragmentID(static_cast<artdaq::Fragment::fragment_id_t>(dtcID + _fragmentIdOffset));
    fragPtr->setTimestamp(static_cast<artdaq::Fragment::timestamp_t>(event.event()));

and similarly

    dtcEvent.SetEventWindowTag(DTCLib::DTC_EventWindowTag(static_cast<uint64_t>(event.event())));

Using event.event() as both the EWT and the artdaq timestamp is OK for purely synthetic MC, but for anything closer to real data this should come from the event‑window info (e.g. EventWindowMarker data product) rather than the art event number. Worth at least a FIXME/note.

6. Typo in log message

        std::cout << "[CrvDigisToFragments::buildDrcEventsFromDigis] WARNING: ...

buildDrcEventsFromDigis → buildDtcEventsFromDigis.

7. Explicit FIXME left in merged code

    fhicl::Atom<int>           crvDtcIdStart{Name("CrvDtcIdStart"), Comment("First CRV DTC ID"), 0};  //FIXME: Which DTC IDs do the CRV use?

The CRV DTC ID range needs to be nailed down before this is used for real. Also _crvDtcIdStart is declared as int but later used in uint8_t arithmetic and as fragment IDs — negative or >255 values would silently wrap. Consider fhicl::Atom<uint8_t> with validation.

🟡 Design / maintainability

8. CrvDigisToFragments generates empty datablocks for every unused ROC

    for(uint8_t linkID = 0; linkID < CRVId::nROCPerDTC; ++linkID)
    {
      auto& block = linkMap[linkID];  // inserts empty entries in the map
      ...
      if(_ROCs.find(ROC)==_ROCs.end()) //not a used ROC
      {
        byteCount=0;
        packetCount=0;
        transferByteCount=kBytesPerPacket;

Every DTC that has any digi gets nROCPerDTC data blocks emitted, even for links that aren't configured. If the intent is to emit exactly one block per used ROC, gate the loop on _ROCs.find(ROC) != _ROCs.end(). If the protocol really does want empty blocks for unused links, add a comment saying so — otherwise this is wasted bytes and adds noise.

9. if(byteCount%2!=0) continue;

A silent continue on an odd byte count breaks the per-DTC packet layout (subsequent links still get written but this one's hits are dropped). Given that every CRVHitRawFEBII is a fixed even size and CRVROCStatusPacketFEBII likewise, this branch should be unreachable — so it should throw (or at minimum emit a mf::LogError) rather than silently drop digis.

10. Uses std::cout throughout instead of the message facility

Both new modules use std::cout << .... All surrounding code uses mf::LogInfo / mf::LogWarning. Please switch for consistency (and so the messages honor services.message configuration).

11. endJob() summary in the comparator is gated behind diagLevel_>0

  if (diagLevel_ > 0) {
    std::cout << "\n ----- [CrvDigiCollectionsComparator] Summary ----- " << std::endl;

A comparator that could be used as a regression gate should always print the matched/mismatched count (and preferably exit non-zero if mismatchedEvents_ > 0 && failOnMismatch_==false).

12. _totalDigis += crvDigis.size(); counts skipped digis

NZS and ROC==0 digis are skipped during encoding but still counted in _totalDigis. Either rename the counter (_totalInputDigis) and add a _totalEncodedDigis, or skip before adding.

13. _fragmentIdOffset is a hard-coded const int member

  const int _fragmentIdOffset = 0; // IDs start from here

If never configurable, make it constexpr static. If it's intended to be configurable (which is likely once the CRV DTC range is chosen), add a fhicl Atom.

🟢 Minor / style

• CrvDigisFromArtdaqFragments CMake now links CRVConditions, ProditionsService — but those are only needed for the new CrvDigisToFragments module, right? Double-check that the existing CrvDigisFromArtdaqFragments(FEBII) source actually pulls in those libraries (otherwise you're adding unneeded link-time dependencies).
• photonYieldVariationCutoffLow/High removal: the three files drop the cutoffs with no explanation in the PR description. Worth a sentence in the PR body why the cutoff knobs are no longer needed (moved into the photon‑yield prodition?).
• Old fcl files DAQ/test/generateCrvDigiFromFragment.fcl and DAQ/test/generateCrvGlobalRunDataFromFragments.fcl are deleted. Grep the repo/downstream scripts (gen_Reco, production configs, Mu2eII DocDB) to make sure nothing references them by name.
• compareCrvDigiCollections_extracted.fcl writes crvRecoExtractedComparison.art, while generateCrvDigisFromFragments_extracted.fcl writes crvRecoExtracted.art. Fine, just flagging the naming is inconsistent with the old crvDigis.art.
• Hard‑coded dated calibration files status_extracted_20260221.txt / calib_extracted_20260221.txt — confirm these exist in CRVConditions/data/ on this branch (the diff does not include them).
• The comparator's describe() limit logic head = max/2, tail = max - head is correct but under-documented; add a comment that the chosen samples are the first head and last tail.
• static_assert(sizeof(DTCLib::DTC_EventHeader)==24) / sizeof(DTC_SubEventHeader)==48 is great — consider adding similar static_asserts for CRVHitRawFEBII / CRVROCStatusPacketFEBII sizes, since the memcpys rely on exact layout.
• hdrPkt.SetByte(5, ...) workaround comment says "this function has a bug" in ConvertToDataPacket. Please open an upstream issue/PR in artdaq-core-mu2e and reference it in the comment so this temporary hack can be removed.

Summary

Must-fix before merge: #1 (wrong exception tag), #3 (TDC overflow), #4 (missing EWT2), #7 (CRV DTC ID FIXME), #6 (typo).
Should-fix: #2, #5, #8, #9, #10, #11, #13.
Nice-to-have: remaining minor items.

The overall direction (round-trip test via CrvDigisToFragments → CrvDigisFromArtdaqFragmentsFEBII → CrvDigiCollectionsComparator) is a really nice way to validate the decoder. Once the TDC / EWT encoding is hardened it will double as a regression gate.

[ehrlich-uva]

Responses to AI

1. Fixed.
2. Wrong, it guards against a division by zero in the next line. So this branch is reachable, if the first map entry has key 0, which results in i->first to be 0 and prevBeta to be zero. So this line is correct. As noted in the code, it shouldn't happen since the lookup maps should not start at beta=0.
3. Fixed. I set the hit time to 0xffff in these cases and print out a warning.
4. Wrong, the CRV ROC status packet has no EventWindowTag2.
5. Fixed timestamp.
6. Fixed typo.
7. I will keep the FIXME in the code.
8. I added a comment stating that unused links still send out data blocks.
9. I changed the warning to a throw.
10. I will keep the couts since the other DigiToFragment modules also use couts.
11. I changed it so it is always printed out. Otherwise the module is pointless.
12. Fixed.
13. Fixed.

[oksuzian]

Follow-up review of PR #1802 (head 98451bd)

Build tests now pass and the author has addressed most of the items from the previous round. Summary of status plus a few remaining / new observations after the fix commits.

✅ Addressed

#	Item	Fix
1	Wrong exception tag CALODIGI_COMPARE	now CRVDIGI_COMPARE
3	TDC×2 overflow	now clamped to 0xffff with a warning
5	setTimestamp used event number	now event.time().value()
6	Typo buildDrcEventsFromDigis	fixed
8	Unused-ROC data blocks	comment added explaining intent
9	Silent continue on odd byte count	now throw
11	Comparator summary gated on diagLevel>0	always printed
12	Counter includes skipped digis	split into _totalDigis / _totalDigisEncoded
13	Hard-coded _fragmentIdOffset	now a fhicl::Atom

↩️ Author pushed back — acceptable

• Add diagnostics #2 if(i->first==0) return i->second; guards the only reachable division-by-zero case (first iteration, where prevBeta is still 0). Logic is correct but a brief comment clarifying why would help future readers ("prevBeta is 0 on first iteration, so this only fires when the map starts at beta=0").
• merge with my branch trackerPatRec #4 EventWindowTag2 — fair, CRVROCStatusPacketFEBII only has EventWindowTag0/1. Ignore.
• Tracker pat rec #10 std::cout kept for consistency with other *DigisToFragments modules — fine.

🟡 Still open / newly visible in 98451bd

A. cet::exception with trailing std::endl

throw cet::exception("CRVDIGI_TO_FRAGMENT") << "Byte count is not a multiple of 2" << std::endl;

cet::exception is not an ostream and std::endl shouldn't be streamed into it. Drop it (use "\n" or nothing). Apply the same to any other cet::exception additions.

B. _fragmentIdOffset still int, casts to 16-bit fragment ID

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

dtcID is uint8_t and _fragmentIdOffset is int; negative or >65535 values will silently wrap into fragment_id_t. Either use fhicl::Atom<uint16_t> with a range check, or add a ctor-time validation such as:

if(_fragmentIdOffset < 0 || _fragmentIdOffset + 0xFF > 0xFFFF)
  throw cet::exception("CRVDIGI_TO_FRAGMENT") << "fragmentIdOffset out of range";

C. _crvDtcIdStart still int but used as uint8_t

fhicl::Atom<int> crvDtcIdStart{Name("CrvDtcIdStart"), Comment("First CRV DTC ID"), 0};

uint8_t dtcID  = _crvDtcIdStart + (ROC-1)/CRVId::nROCPerDTC;

Same silent-truncation concern. Validate at construction.

D. hdrPkt.SetByte(5, ...) workaround

auto hdrPkt = hdr.ConvertToDataPacket();  //this function has a bug. it does not transfer the subsystem ID to the DTC_DataPacket.
hdrPkt.SetByte(5, static_cast<uint8_t>(((packetCount & 0x0700) >> 8) | (subsystem << 5)));  //this is a temporary bug fix.

Please open an upstream issue in artdaq-core-mu2e and reference its URL in this comment so this hack is easy to remove later.

E. Comparator endJob() cosmetic indentation

void CrvDigiCollectionsComparator::endJob() {
    std::cout << "\n ----- [CrvDigiCollectionsComparator] Summary ----- " << std::endl;
    std::cout << "Total events: " << totalEvents_ << std::endl;
    ...
}

Body still indented by 4 spaces from when it was inside an if; re-indent to 2 spaces to match the rest of the file.

F. Layout assumptions — consider static_asserts

The memcpys in buildDtcEventsFromDigis assume exact layouts of CRVHitRawFEBII and CRVROCStatusPacketFEBII. You already do this for DTC_EventHeader/DTC_SubEventHeader; add the same for the two CRV packet structs so any upstream size change fails loudly at compile time.

G. _totalDigisEncoded not printed when diagLevel==0

Both summaries in endJob() are still inside if(_diagLevel > 0). For a round-trip / regression module, an always-on one-line summary like the comparator's is useful. Low priority.

H. Duplicated "Cannot handle … samples" skip silently drops digis

These digis will be absent from the round-trip collection and will cause a size-mismatch in CrvDigiCollectionsComparator that is not easy to distinguish from a real encoding bug. Consider either (a) throwing here when the comparator is used in a regression fcl, or (b) making the comparator aware of "skippable" digis (NZS + wrong-size waveform) so the round-trip is exact.

I. Unrelated changes piggy-backed on this PR

The diff now also touches CMakeLists.txt (version bump 11.01.00 → 13.07.00), CalPatRec/* (proton time cluster collection, debug prints), DAQ/CMakeLists.txt (add art::Framework_Principal), and Sources/src/PBISequence_source.cc (firstSubRun/firstEvent options). None of these are mentioned in the PR description and none are related to "converts CrvDigis to artFragments." They look like an accidental rebase/merge artifact — please confirm they belong here or strip them out.

J. Documentation

• New fcl files reference CRVConditions/data/status_extracted_20260221.txt and calib_extracted_20260221.txt — verify these exist on the branch; the diff does not contain them.
• Update the PR description: the cutoff removals (photonYieldVariationCutoffLow/High) and the CalPatRec / PBISequence changes (if intended) should be documented there.

Recommendation

Blocking before merge: A, B, C, I (especially the unrelated PBISequence / CalPatRec / version-bump changes — they should either be explained or reverted).
Should fix: D, F, H.
Nice-to-have: E, G, J, comment on #2.

Overall the round-trip (CrvDigisToFragments → CrvDigisFromArtdaqFragmentsFEBII → CrvDigiCollectionsComparator) is a very good addition and the core encoding logic looks solid.

[ehrlich-uva]

Responses to AI:
A: Fixed.
B: Fixed. No range needed, since they will always be very small numbers.
C: Fixed.
D: No PR yet. Maintainer of the code got notified.
E: Fixed.
F: Not necessary. If the class size changes, the number of bytes in memcpy will also change.
G: The DigiToFragment module should only print out the summary, if necessary. Nothing by default.
H: Not necessary at the moment. Dropped digis should cause a failure.
I: That's due to merging changes that happened in the mean time. I will keep it the way it is.
J: The condition files exist. I will update the PR description.