Add ICARUS HW trigger information to CAFs

[rtriozzi]

This PR adds to the CAFs additional trigger information available within the ICARUS trigger hardware from Run3:

• trigger_cryo_source: the cryostat with the first trigger within the gate;
• trigger_logic_bits: the trigger logic for such first trigger (adder-only, majority-only, or both);
• beam_to_trigger_time gate_to_trigger_time: the trigger time from the beam gate opening for such first trigger (in nanoseconds).

Those variables are added to the CAFs via the dedicated FillTriggerICARUS function.
Some more information on the new diagnostics can be found in SBN DocDB 36624, SBN DocDB 36497.
This PR depends on the corresponding sbnanaobj PR #167 that adds the variables to the CAF structure.

---

• Have you added a label? (bug/enhancement/physics etc.)
• Have you assigned at least 1 reviewer?
• Is this PR related to an open issue / project?
• Does this PR affect CAF data format? If so, please assign a CAF maintainer as additional reviewer.
• Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)? If so, please link it in the description.
• Are you submitting this PR on behalf of someone else who made the code changes? If so, please mention them in the description.

[mvicenzi]

Changes looks good, but I have questions on their compatibility with SBND and also their backward compatibiliy with older files before approving. See the comments below.

[mvicenzi]

What guards FillTriggerICARUS from being called when building CAFs for SBND?
And if so, would it cause an error if the information is not available?

Maybe you should do as below?

f(fDet == kICARUS) FillTriggerICARUS(*extratrig_handle, srtrigger);

[PetrilloAtWork]

I don't believe SBND fills sbn::ExtraTriggerInfo (they could at least in part, and maybe they will in the future), in which case isValidTrigger is false and nothing is filled (!).

At this point, the code is written in such a way that if SBND fills the appropriate information in sbn::ExtraTriggerInfo, it will be correctly copied to the CAF object. On the other end, if we really want this function to also support SBND we should (a) listen to SBND comments and (b) name it accordingly.
For point (a), since there is a new SBN trigger efficiency group, I have added @terezakroupa as reviewer, and we can explain her in short what this is about too.

[mvicenzi]

What happens if this function runs on an older dataset (Run-2) that doesn't have this additional information from the trigger hardware? Are proper defaults already in place at the previous stage?

And also - possibly not as important - what about backward compatibility on older files? I don't recall the icaruscode version from which this additional trigger info is available. If you try to run the CAFMaker on older files, would that throw an error? Maybe that's not a concern?

[PetrilloAtWork]

For ICARUS:

Trigger tick count from the gate opening

• when the trigger decoder is run on data that does not have the tick count information, the time is assigned the NoTrigger value, exactly the same as if the information were present and there were no trigger;
• when art reads old sbn::ExtraTriggerInfo decoded data without beamToTrigger branch, that value is left to constructor default, which is NoTrigger.

Therefore, in both cases, the information will not be filled just like there were no trigger (that in the current version of this PR means default-initialised with big integers).

Trigger bit logic

• when the trigger decoder is run on data that does not have the trigger logic information, the mask is assigned the value 0, exactly the same as if the information were present and there were no trigger;
• when art reads old sbn::ExtraTriggerInfo decoded data without triggerLogicBits branch, that value is left to constructor default, which is 0.

Therefore, in both cases, the information will not be filled just like there were no trigger (that in the current version of this PR means default-initialised with big integers).

[PetrilloAtWork]

I have left some technical comment.

I second the discussion of how this code should and does deal with SBND data, and I have added a SBND trigger expert as reviewer to bootstrap it (it does not have to be a long discussion, but SBND should have a chance to voice any opinion).

[PetrilloAtWork]

I suggest a different coding approach:

1. determine which cryostat wins
2. fill with the information from that cryostat.

    int cryo = -1;
    if (addltrig_info_cryoE.hasTrigger() && (!addltrig_info_cryoW.hasTrigger() || (addltrig_info_cryoE.beamToTrigger <= addltrig_info_cryoW.beamToTrigger)))
      cryo = sbn::ExtraTriggerInfo::EastCryostat;
    else if (addltrig_info_cryoW.hasTrigger())
      cryo = sbn::ExtraTriggerInfo::WestCryostat;
    
    triggerInfo.trigger_cryo_source  = cryo;
    triggerInfo.trigger_logic_bits   = addltrig_info.cryostats[cryo].triggerLogicBits;
    triggerInfo.beam_to_trigger_time = addltrig_info.cryostats[cryo].beamToTrigger;

The advantage is that once the cryostat is determined there is no more code duplication.
The logic can be simplified given that it is guaranteed that NoTrigger, the special time value assigned to the trigger time when there is no trigger, is larger than any valid trigger time:

    int const cryo = addltrig_info.cryostats[sbn::ExtraTriggerInfo::EastCryostat].beamToTrigger < addltrig_info.cryostats[sbn::ExtraTriggerInfo::WestCryostat].beamToTrigger
      ? sbn::ExtraTriggerInfo::EastCryostat: sbn::ExtraTriggerInfo::WestCryostat;

    sbn::ExtraTriggerInfo::CryostatInfo const& cryoInfo = addltrig_info.cryostats[cryo];
    if (cryoInfo.hasTrigger()) {
      triggerInfo.trigger_cryo_source  = cryo;
      triggerInfo.trigger_logic_bits   = cryoInfo.triggerLogicBits;
      triggerInfo.beam_to_trigger_time = cryoInfo.beamToTrigger;
    }

[PetrilloAtWork]

For ICARUS:

Trigger tick count from the gate opening

• when the trigger decoder is run on data that does not have the tick count information, the time is assigned the NoTrigger value, exactly the same as if the information were present and there were no trigger;
• when art reads old sbn::ExtraTriggerInfo decoded data without beamToTrigger branch, that value is left to constructor default, which is NoTrigger.

Therefore, in both cases, the information will not be filled just like there were no trigger (that in the current version of this PR means default-initialised with big integers).

Trigger bit logic

• when the trigger decoder is run on data that does not have the trigger logic information, the mask is assigned the value 0, exactly the same as if the information were present and there were no trigger;
• when art reads old sbn::ExtraTriggerInfo decoded data without triggerLogicBits branch, that value is left to constructor default, which is 0.

Therefore, in both cases, the information will not be filled just like there were no trigger (that in the current version of this PR means default-initialised with big integers).

[PetrilloAtWork]

I don't believe SBND fills sbn::ExtraTriggerInfo (they could at least in part, and maybe they will in the future), in which case isValidTrigger is false and nothing is filled (!).

At this point, the code is written in such a way that if SBND fills the appropriate information in sbn::ExtraTriggerInfo, it will be correctly copied to the CAF object. On the other end, if we really want this function to also support SBND we should (a) listen to SBND comments and (b) name it accordingly.
For point (a), since there is a new SBN trigger efficiency group, I have added @terezakroupa as reviewer, and we can explain her in short what this is about too.

[rtriozzi]

Thanks @mvicenzi, thanks @PetrilloAtWork! As noted on sbnanaobj PR #167, gate_to_trigger_time (formerly beam_to_trigger_time) is now a float, and is converted to microseconds when filling the variable.

@terezakroupa let us know if you have further comments from the SBND side, thanks!

[PetrilloAtWork]

Looks good to me on my side.
The question to @terezakroupa is still pending.

[mvicenzi]

My concerns were addressed, thanks!

[terezakroupa]

Thanks for the ping! It looks and sounds like SBND won't be affected by this at this point. We do plan to add the trigger information into the CAFs though in the future, it's good to plan now.

Since the two systems are quite different, we won't be filling all the same variables, so having separate functions named specifically to the detectors make sense. I do like the suggestion above of adding a specific check that to whether the data is ICARUS or SBND and not just relying on the fact that our info is missing.