Skip to content

Stm daq develop 2026#1808

Open
Etho-b02 wants to merge 29 commits intoMu2e:mainfrom
Etho-b02:stm-daq-develop-2026
Open

Stm daq develop 2026#1808
Etho-b02 wants to merge 29 commits intoMu2e:mainfrom
Etho-b02:stm-daq-develop-2026

Conversation

@Etho-b02
Copy link
Copy Markdown

An update version of the offline unpacking module which reads in container fragments. Container fragment represents corresponding detector data. Also includes an event and job summary of what the module reads.

@FNALbuild
Copy link
Copy Markdown
Collaborator

Hi @Etho-b02,
You have proposed changes to files in these packages:

  • STMReco
  • STMMC
  • DAQ
  • RecoDataProducts

which require these tests: build.

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

📝 The author of this pull request is not a member of the Mu2e github organisation.

About FNALbuild. Code review on Mu2e/Offline.

@AndrewEdmonds11 AndrewEdmonds11 self-requested a review April 24, 2026 19:29
Copy link
Copy Markdown
Contributor

@AndrewEdmonds11 AndrewEdmonds11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening the PR, Bryan. This looks good. Just a few in-line comments we should work through, Let me know if you have any questions. Thanks

Comment on lines +140 to +143
std::unique_ptr<mu2e::STMWaveformDigiCollection> raw_waveform_digis(new mu2e::STMWaveformDigiCollection);
std::unique_ptr<mu2e::STMWaveformDigiCollection> zs_waveform_digis(new mu2e::STMWaveformDigiCollection);
std::unique_ptr<mu2e::STMPHDigiCollection> ph_digis(new mu2e::STMPHDigiCollection);
std::unique_ptr<mu2e::STMWaveformDigiCollection> raw_header_waveform_digis(new mu2e::STMWaveformDigiCollection);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this module is just for writing the binary file for debugging, we don't need to create the Offline data products. You can remove these lines and others in this module

Comment thread DAQ/src/STMDigisFromFragments_module.cc Outdated
ContainerFragID = frag.fragmentID();
if (_verbosityLevel >= 3){std::cout << "\nFrag_id : " << ContainerFragID << "\n";}

if(ContainerFragID == 103) {isHPGe = true;} else if (ContainerFragID == 203){isLaBr = true ;}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want to hardcode the numbers 103 and 203. Is it possible to define them in the STMFragment.hh file?

Comment thread DAQ/src/STMPrintFragments_module.cc Outdated
struct Config
{
fhicl::Atom<art::InputTag> stmTag {fhicl::Name("stmTag"), fhicl::Comment("stmTag for new file")};
// // fhicl::Atom<int> diagLevel{fhicl::Name("diagLevel"), fhicl::Comment("diagnostic Level")};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove commented out lines like this to clean up the code

Comment on lines +46 to +48
double _peak_fitTime1; // fit time of first rising edge (ns) // TODO: remove this parameter
double _peak_fitTime2; // fit time of second rising edge (ns) // TODO: remove this parameter
double _peak_sep; // separation time (ns) // TODO: remove this parameter
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these parameters now, or are they being used somewhere else in the code?

Comment thread RecoDataProducts/inc/STMWaveformDigi.hh Outdated
@@ -35,16 +35,17 @@ namespace mu2e {
double peak_fitTime2 () const { return _peak_fitTime2; }
double peak_sep () const { return _peak_sep; }
const std::vector<int16_t>& adcs () const { return _adcs; }

void set_data ( size_t n_data, int16_t const* data ) { _adcs.resize(n_data); std::copy(data, data+n_data, _adcs.begin()); } // TODO: remove this method
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove that // TODO comment because I think we want to keep this function

_stmWaveformDigisToken(consumes<STMWaveformDigiCollection>(config().stmWaveformDigisTag())),
_subtractPedestal(config().subtractPedestal()),
_xAxis(config().xAxis()),
_verbosityLevel(config().verbosityLevel()),
_channel(STMUtils::getChannel(config().stmWaveformDigisTag()))
_channel(STMChannel::findByName("HPGe")) // FIXME: don't hardcode this probably don't want to do what we had before and try to infer it from the art::InputTag like this "STMUtils::getChannel(config().stmWaveformDigisTag()))"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this FIXME on our to-do list to fix ASAP. Maybe we can even get it in before this PR completes review...

@oksuzian
Copy link
Copy Markdown
Collaborator

Bugs / Blockers

  1. Instance-name mismatch between producer and downstream fcls — multiple files will fail at runtime. STMDigisFromFragments only ever calls produces<> with HPGe/LaBr suffixes (rawHPGe, zsHPGe, phHPGe, rawLaBr,
    zsLaBr, phLaBr). But the downstream fcls reference unsuffixed instance names:
  • STMReco/fcl/mwd.fcl — stmWaveformDigisTag : "makeSTMDigis:zs" for both phHPGe and phLaBr producers.
  • STMReco/fcl/plotSTMPHSpectrum.fcl — stmPHDigisTag : "makeSTMDigis:ph".
  • STMReco/fcl/makeSTMHits.fcl — stmPHDigisTag : "makeSTMDigis:ph" (and the other instance "phHPGe" references a module label phHPGe, which doesn't exist in this fcl either).
  • STMReco/fcl/plotSTMWaveformDigis.fcl — "makeSTMDigis:raw", "makeSTMDigis:zs", "makeSTMDigis:ph".

These will throw "no product found" at runtime. The reason CI may be green is that each fcl has one of HPGe/LaBr commented out of trigger_paths (see #15), so end-to-end runs aren't actually exercising these
tags.

  1. MakeSTMHits constructor hardcodes STMChannel::LaBr for both instances. STMReco/src/MakeSTMHits_module.cc:
    ,_channel(STMChannel::LaBr)
    replaces the previous STMUtils::getChannel(config().stmMWDDigisTag()). Same root cause as Andrew's FIXME on PlotSTMWaveformDigis_module.cc:68, but unmarked here. Currently masked because makeSTMHits.fcl
    comments out the HPGe path — re-enabling it will silently apply LaBr energy calibrations to HPGe data.

  2. STMPrintFragments::analyze prints the outer container block_count times instead of iterating into inner frags.
    for (size_t ii = 0; ii < contf.block_count(); ++ii){
    const auto dataBegin = frag.dataBegin(); // outer frag, ii unused
    const auto dataEnd = frag.dataEnd();
    ...
    }
    The loop variable ii is never used; frag.dataBegin() always returns the outer container's bytes. Should be auto inner = contf.at(ii); auto dataBegin = inner->dataBegin();.

  3. _unreadInnerFrags is gated by verbosity — counters are zero at default verbosity. STMDigisFromFragments_module.cc:
    else {
    if (_verbosityLevel >= 3) {
    ++_unreadInnerFrags; // both job and event counters live inside the if
    ++unread_InnerFrags;
    }
    }
    The endJob "Unread frags" line will always read 0 unless verbosity is ≥ 3. Move the increments outside the if.

  4. xAxis condition flipped — looks like an intended pre-existing bug fix; please call it out in the PR body. STMMovingWindowDeconvolution_module.cc:~166 changed if (_xAxis != "") to if (_xAxis == "") around the
    throw. The new condition matches the message ("No xAxis scale defined despite requesting verbosity level >= 5") and is correct, but it's a behavior change that future spelunkers will want to see noted.

Risks / Robustness

  1. Unknown container fragment IDs are silently dropped. Beyond Andrew's hardcoding comment, the bigger issue is the silent path: when ContainerFragID is neither 103 nor 203, both isHPGe and isLaBr stay false,
    the nested if(isHPGe) ... else if(isLaBr) ... skips every branch, and no log line records that an inner frag was discarded. Add a default log at verbosity ≥ 1.

  2. ZSfromRaw != totalLen cross-check has lifetime issues. readRawZSinfo is set true after a Raw block and never reset, so it then checks every subsequent ZS block in the same container against the same
    ZSfromRaw. If only one Raw–ZS pair per container is expected, document it; if multiples are possible, reset readRawZSinfo/ZSfromRaw after each ZS check.

  3. STMBinaryDigisFromFragments::produce() allocates four unique_ptr<...Collection>s that are never event.put()'d. Andrew flagged removing them in spirit; concretely they're per-event allocations of
    STMWaveformDigiCollection × 3 + STMPHDigiCollection × 1 that immediately get destroyed.

  4. STMPrintFragments declares BTrk_difAlgebra as a build dep but never uses BTrk. DAQ/CMakeLists.txt:138. Looks copy-pasted from a CRV module — drop it.

Code quality

  1. Five debug std::couts not gated by verbosity.
  • STMMovingWindowDeconvolution_module.cc: std::cout<<"BG"<<_xAxis<<std::endl; (author-initials marker), "event id = ", "waveformDigisHandle size = ", "waveform id =" (last three printed every waveform of every
    event).
  • PlotSTMWaveformDigis_module.cc: std::cout<<"size = "<size()<<std::endl;
  1. STMPrintFragments header comment is wrong: "STMPrintFragments_plugin: Add CRV data products to the event". Copy-paste from a CRV module.

  2. _totalRaw is ambiguous — it includes the Empty + Zero counts. A reader of the job summary doesn't know whether to subtract. Either rename to _totalRawSeen or add a _totalGoodRaw line.

  3. Top-level non-container fragments silently no-op. The else { /* fallback non-container case */ ... } at the outer loop never logs anything — same family as Adjust parameters for new production #6.

Nits

  1. Commented-out FragmentWatcher block in inspectSTMFile.fcl has three layers of # stacked (# frag: / blank / # { / # module_type ...). Either delete or reformat as a single comment.

  2. Test fcls ship with one detector path commented out and the choice is inconsistent across files. mwd.fcl disables LaBr; makeSTMHits.fcl disables HPGe. The PR does not say why.

  3. STMBinaryDigisFromFragments and STMDigisFromFragments include the entire ROOT graphics stack (TROOT.h, TApplication.h, TCanvas.h, TF1.h, TString.h, TLine.h, TGraph.h) but don't use any ROOT graphics. Drop.

  4. , , included in both new modules, never used. Lint cleanup.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants