Add validation for invalid offline ID during digi creation

[giro94]

Reverted some unwanted changes introduced in #1887

And added offline id check for hits appearing in channels that are supposed to be empty / invalid

[FNALbuild]

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

• CaloConditions
• 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 29fb921: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 29fb921.

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

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

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]

1. [S1] CaloHitsFromDTCEvents consumes the same map, unguarded — invalid ids now index out of bounds there.

DAQ/src/CaloHitsFromDTCEvents_module.cc:317-326 (and the twin block at 380-389):

mu2e::CaloSiPMId offlineId = calodaqconds.offlineId(rawId);
uint16_t SiPMID = offlineId.id();
float eDep = thisHitPeak * peakADC2MeV_[SiPMID];
float time = ... + timeCalib_[SiPMID];
if(!isCaphri) ++nhits[offlineId.crystal().disk()];
With the map now legally returning CaloSiPMId(_invalid) (= 9999) for spare/disconnected channels — from both the fcl path (this PR) and the DB path (since #1887) — a hit on such a channel does peakADC2MeV_[9999] / timeCalib_[9999] on
std::array<float, _nCrystalChannel> (out-of-bounds read, UB) and ++nhits[...] on a 2-element array indexed via crystal().disk() of a nonsense crystal id (potential out-of-bounds write). This module has no useOfflineID_ toggle — it always uses
the offline map. The PR's own premise is that such hits occur in real data, so this is the same bug being fixed in CaloDigisFromDTCEvents, left live in its sibling.

Fix: add the same skip-and-count guard there (the new InvalidChannel enum is already available), or factor the check into CaloDAQUtilities. Worth noting while there: even valid PIN-diode ids (>= _nCrystalChannel) overrun these
_nCrystalChannel-sized calibration arrays — pre-existing, but it argues for a range guard rather than an invalid-only guard.

**2. [S2 — pre-existing, 5 lines] fromFcl lacks the completeness check its fromDb twin has.**

CalDAQMapMaker::fromDb ends with an explicit post-loop check (with the comment "check that all roid were filled since some table entries are set to invalid…") that throws CALDAQMAPMAKER_MISSING if any valid offline channel was never mapped.
fromFcl now has the same invalid-tolerance but no such check: a malformed map file that marks a connected channel's row invalid still passes (nRead == _nRawChannel holds), silently leaving that offline2Raw entry default-constructed to _invalid
— and the offline→raw consumers (ArtBinaryPacketsFromDigis, CaloDigisToFragments) get a garbage raw id. This gap existed pre-#1887 too, but since this PR restores the tolerance, mirroring the fromDb check here would close it for good.

**3. [S3] Minor**
- The new guard runs before the useDTCROCID_ override, so with useOfflineID_ && useDTCROCID_ both set, hits are dropped based on offline-map validity even though the stored id is DTC/ROC-derived. Probably a nonsensical config combination anyway
— a one-line comment (or moving the check after the override) would make the precedence explicit.
- The diagnostic print is std::cout gated at diagLevel_ > 1, consistent with the module's existing style (message facility would be the strict-standards choice, but that's a module-wide matter, not this PR's).

[giro94]

@FNALbuild run build test

[FNALbuild]

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

[giro94]

@oksuzian I'm unsure about S2 (to me that block shouldn't be there) so I'll let @bechenard answer.
This PR can go through anyway.

[FNALbuild]

☀️ The build tests passed at 53b5613.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 53b5613 at f96d9fc
build (prof)	✅	Log file. Build time: 08 min 52 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 4 files
clang-tidy	➡️	5 errors 114 warnings
whitespace check	✅	no whitespace errors found

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

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.

[bechenard]

The changes are ok (I made the modification last time to avoid an out-of-bound access but you changed the logic, so this is good now).

Regarding S2: Claude flagged similar issues for CalSimParamMaker (this was basically a copy of the CalDAQMap code). Have a look at it, there might be a few things to pick up. But this is optional