updated CRV KPP geometry and G-blocks (extracted geometry v3)

[ehrlich-uva]

• updated KPP geometry for CRV and G-blocks (extracted geometry v3)
• added temporary option to replace ROC2 with ROC4 in CRV digi generator

[FNALbuild]

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

• Mu2eG4
• 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 20bb564: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 20bb564.

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

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

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 #1815 Review — Updated CRV KPP geometry and G-blocks (extracted geometry v3)

PR Summary

Author: @ehrlich-uva
Branch: ehrlich-uva:CrvCalibration → Mu2e:main
State: Open · Draft: no · Mergeable: ✅ clean · 1 commit · 4 files changed (+223, −7)
Labels: build finished
Reviews: none yet · Inline comments: 0 · Issue comments: 2 (FNALbuild only)

Description (verbatim):

• Updated KPP geometry for CRV and G-blocks (extracted geometry v3)
• Added temporary option to replace ROC2 with ROC4 in CRV digi generator

CI status: All FNALbuild tests ✅ passed at 20bb564 (build prof, ceSimReco, g4test_03MT, transportOnly, POT, g4study, cosmicSimReco, cosmicOffSpill, ceSteps, ceDigi, muDauSteps, ceMix, rootOverlaps, g4surfaceCheck, trigger, check_cmake, whitespace).
Soft flags: TODO (1) FIXME (0) in 1 file, clang-tidy: 2 errors 53 warnings.

Risk assessment: Low–Medium. Geometry-file additions are additive (new v03 files, only the geom_common_extracted.txt include is bumped). The DAQ module change is small but introduces a runtime ROC-ID arithmetic change that has subtle correctness implications (see below).

---

Core Changes

1. Geometry version bump (additive, low risk)

Mu2eG4/geom/geom_common_extracted.txt switches the included file from v02 to v03:

-#include "Offline/Mu2eG4/geom/geom_common_extracted_v02.txt"
+#include "Offline/Mu2eG4/geom/geom_common_extracted_v03.txt"

Two new files describe the v03 KPP geometry:

• Mu2eG4/geom/geom_common_extracted_v03.txt — top-level geometry for extracted detector cosmics; updates the G-block layout (hatchblock x/y positions, halfThicknesses, offsets) and pulls in the new CRV counter file.
• Mu2eG4/geom/crv_counters_extracted_v03.txt — defines 3 sectors (EX, T1, T2), 4 layers, scintillator dimensions, gaps, FEB layout, and SiPM placement for the extracted-detector KPP setup.

The previous v02 files are left in place (no deletions), so existing fcl that hard-codes v02 paths is unaffected.

2. DAQ change — CrvDigisFromArtdaqFragmentsFEBII (the substantive logic change)

-fhicl::Atom<int>  diagLevel{...};
-fhicl::Atom<bool> produceZS{..., true};
-fhicl::Atom<bool> produceNZS{..., false};
+fhicl::Atom<int>     diagLevel{...};
+fhicl::Atom<uint8_t> firstCrvDtcID{fhicl::Name("firstCrvDtcID"), fhicl::Comment("First CRV DTC ID"), 0};
+fhicl::Atom<bool>    produceZS{..., true};
+fhicl::Atom<bool>    produceNZS{..., false};
+fhicl::Atom<bool>    useROC4asROC2{fhicl::Name("useROC4asROC2"), fhicl::Comment("use ROC4 as ROC2"), false};
...
-uint16_t rocID = dtcID*CRVId::nROCPerDTC + linkID + 1;  //ROC IDs are between 1 and 18
+uint16_t rocID = (dtcID-_firstCrvDtcID)*CRVId::nROCPerDTC + linkID + 1;  //ROC IDs are between 1 and 18
+if(_useROC4asROC2 && rocID==4) rocID=2;

The same two-line edit is applied in both the ZS and NZS code paths (lines ~218 and ~291).

---

Issues found

🔴 Issue 1 — Unsigned-underflow risk in the new ROC-ID computation

The change is:

uint16_t dtcID  = header->GetID();          // uint16_t
// _firstCrvDtcID is uint8_t (promoted to int in the subtraction)
uint16_t rocID = (dtcID - _firstCrvDtcID) * CRVId::nROCPerDTC + linkID + 1;

If a CRV fragment ever arrives with dtcID < _firstCrvDtcID (e.g., because firstCrvDtcID was misconfigured in fcl, or non-CRV DTC fragments slip through the dispatch), the subtraction produces a large unsigned value, multiplied by nROCPerDTC, and the resulting rocID will be far outside the [1,18] range stated in the comment. That value is then fed into the channel map lookup (_channelMap_h) downstream and may translate into a silently wrong (or wrapped) channel rather than an error.

Suggested fix: add a guard, e.g.

if (dtcID < _firstCrvDtcID) {
  throw cet::exception("CrvDigisFromArtdaqFragmentsFEBII")
        << "DTC ID " << dtcID << " is below firstCrvDtcID=" << unsigned(_firstCrvDtcID);
}
uint16_t rocID = (dtcID - _firstCrvDtcID) * CRVId::nROCPerDTC + linkID + 1;

Both call sites (ZS path ~L221 and NZS path ~L294) need the guard since the logic was duplicated.

🟡 Issue 2 — fhicl::Atom<uint8_t> is unusual

Most fhicl atoms in this codebase use int/unsigned/size_t. uint8_t is unsigned char on this platform, and some serializers/printers treat it as a character rather than a number (so a config dump can render firstCrvDtcID: '' for value 0 or print a control character for non-zero values, and parsing firstCrvDtcID: 65 could be ambiguous on some fhicl versions). The build passed, but consider using fhicl::Atom<unsigned> (or uint16_t) for safety and readability — and to match the type of dtcID directly, eliminating the integer-promotion subtlety in Issue 1.

🟡 Issue 3 — Hardcoded ROC4→ROC2 remap is duplicated and labeled "temporary"

if(_useROC4asROC2 && rocID==4) rocID=2;

• The PR description explicitly calls this temporary. Please add a // TODO: (or // FIXME:) comment in the source and an issue/ticket reference so this doesn't quietly persist.
• Logic is duplicated in both ZS and NZS branches; consider extracting a small helper (e.g., decodeRocId(dtcID, linkID)) so both paths stay in sync if/when this hack is removed or extended.

🟢 Issue 4 — Soft-flagged TODO in new geometry file

Mu2eG4/geom/crv_counters_extracted_v03.txt introduces:

int crs.nSupportStructures = 0;   //TODO: this is only a place holder so far
vector<string> crs.supportStructureNames = {};

This is the single TODO surfaced by FNALbuild. Worth confirming that downstream consumers tolerate nSupportStructures = 0 (the older v02 may have set this differently).

🟢 Issue 5 — Default for useROC4asROC2 is correct; default for firstCrvDtcID = 0 preserves prior behavior

Backwards compatibility looks intentional and correct: with the defaults (firstCrvDtcID=0, useROC4asROC2=false), the math reduces exactly to the original rocID = dtcID*nROCPerDTC + linkID + 1. ✅

---

Merge Readiness

• ✅ All FNALbuild tests pass; no whitespace, cmake, or build errors.
• ✅ Mergeable / clean against main.
• ⚠️ No human review yet (0 reviews, 0 inline comments). Given that this touches the DAQ → channel-map path, a CRV-DAQ owner sign-off is advisable before merge.
• ⚠️ Recommend addressing Issue 1 (underflow guard) before merge — it's a small, defensive change that prevents a hard-to-diagnose silent miscalibration.
• ⚠️ Suggest addressing Issue 2 (drop uint8_t for fhicl atom) at the same time since the fix touches the same lines.

---

Possible Improvements

1. Factor the ROC-ID decoding into a private helper (computeRocId(dtcID, linkID)) to avoid duplicating the new logic and the future cleanup.
2. Log a one-time mf::LogInfo at module startup printing _firstCrvDtcID and _useROC4asROC2 so operators can see the active calibration mode in the log.
3. Add a brief comment near the ROC4→ROC2 hack referencing the doc/db note or ticket explaining the hardware reason (which ROC card is physically swapped), so future maintainers don't have to git-blame to understand it.
4. Consider keeping crv_counters_extracted_v02.txt only as long as needed and add a deprecation note at the top of v02; otherwise downstream fcl will keep both pinned indefinitely.

---

Want me to

1. Draft a suggested patch for the underflow guard + uint8_t → unsigned change in CrvDigisFromArtdaqFragmentsFEBII_module.cc?
2. Diff crv_counters_extracted_v02.txt against the new v03 file to enumerate exactly which physical parameters changed (sector layout, first-counter coordinates, gaps, FEB layout)?
3. Diff geom_common_extracted_v02.txt against geom_common_extracted_v03.txt to summarize the G-block geometry changes?
4. Search for fcl files that set CrvDigisFromArtdaqFragmentsFEBII.* to see whether any production configs need to be updated for the new firstCrvDtcID/useROC4asROC2 parameters?
5. Open the suggested fixes as a PR against ehrlich-uva:CrvCalibration?

[FNALbuild]

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