Conversation
There was a problem hiding this comment.
Pull request overview
Adds an OTS-oriented CRV DQM analyzer and supporting FHiCL/CMake updates to keep the FEB II workflow up to date.
Changes:
- Introduces a new
CrvOtsDqmart analyzer module that books ROOT histograms, optionally sends them viaHistoSender, and serves a small HTTP display. - Adds a new run configuration
RunCrvOtsDqm.fclto run the digi producer + new DQM analyzer. - Updates existing
RunCrvDQM.fclwith anoutputsblock and wiresCrvOtsDqminto the build viaArtModules/CMakeLists.txt.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| otsdaq-mu2e-crv/fcl/RunCrvOtsDqm.fcl | New job config to run FEBII digi production + OTS DQM analyzer and write a ROOT output file. |
| otsdaq-mu2e-crv/fcl/RunCrvDQM.fcl | Adds an out1 FileDumperOutput output module configuration. |
| otsdaq-mu2e-crv/ArtModules/CrvOtsDqm_module.cc | New CrvOtsDqm EDAnalyzer: histogram booking, digi processing, histogram sending, and HTTP display. |
| otsdaq-mu2e-crv/ArtModules/CMakeLists.txt | Builds/installs the new CrvOtsDqm plugin with required libraries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ROOT TFileService | ||
| art::ServiceHandle<art::TFileService> tfs_; | ||
|
|
||
| // Histograms | ||
| TH1F* h1_dummy_; // dummy | ||
| TH1F* h1_channels_; // global FEB channel hits | ||
| TH2F* h2_channels_; // FEB vs channel hits | ||
|
|
There was a problem hiding this comment.
Histogram member pointers (h1_dummy_, h1_channels_, h2_channels_) are declared but never initialized (e.g., to nullptr) in the constructor. If dummyHist_ is true, the non-dummy pointers remain indeterminate and later checks like else if (h1_channels_) in updateWebDisplay can read garbage and crash. Initialize all histogram pointers to nullptr in the constructor (or in-class), and only use them after they are assigned in beginJob().
| h1_channels_ = dir.make<TH1F>("h1_channels", "All FEB channels;Global FEB channel ID;Counts", 1984, -0.5, 1983.5); | ||
| h2_channels_ = dir.make<TH2F>("h2_channels", "All FEB channels;Channel;FEB", 64, -0.5, 63.5, 29, 0.5, 29.5); |
There was a problem hiding this comment.
The histogram binning/ranges for channel/FEB occupancy do not match the IDs you later fill. h1_channels_ is booked with 1984 bins (0–1983) and h2_channels_ has FEB axis 0.5–29.5, but analyze() computes globalChannelId = febChannel + 64*feb + (64*25)*(roc-1) and globalFebId = ((roc-1)*24)+feb, which can exceed those ranges for ROC>1 (and even for higher FEB numbers). This will push a lot of data into overflow bins and make the plots misleading; adjust the ID calculation or expand histogram axes to cover the full expected ROC/FEB ranges (or make per-ROC histograms).
| h1_channels_ = dir.make<TH1F>("h1_channels", "All FEB channels;Global FEB channel ID;Counts", 1984, -0.5, 1983.5); | |
| h2_channels_ = dir.make<TH2F>("h2_channels", "All FEB channels;Channel;FEB", 64, -0.5, 63.5, 29, 0.5, 29.5); | |
| // Use a larger range to accommodate globalChannelId = febChannel + 64*feb + (64*25)*(roc-1) | |
| // Here we allow up to 96 FEB IDs (e.g. 4 ROCs * 24 FEBs), i.e. 96*64 = 6144 channels. | |
| h1_channels_ = dir.make<TH1F>("h1_channels", "All FEB channels;Global FEB channel ID;Counts", 6144, -0.5, 6143.5); | |
| // Expand FEB axis to cover globalFebId = ((roc-1)*24)+feb for multiple ROCs (up to 96 FEB IDs). | |
| h2_channels_ = dir.make<TH2F>("h2_channels", "All FEB channels;Channel;FEB", 64, -0.5, 63.5, 96, 0.5, 96.5); |
| webCanvas_->Modified(); | ||
| webCanvas_->Update(); | ||
| gSystem->ProcessEvents(); | ||
| lastRefreshTime_ = now; | ||
| } |
There was a problem hiding this comment.
The HTTP server event loop is only pumped from updateWebDisplay() (via gSystem->ProcessEvents()), which is only called when events are being processed and the refresh period has elapsed. If the job is idle/no events arrive, the web server may become unresponsive (contrast with CrvDQM_module.cc which runs ProcessEvents in a keep-alive thread). Consider adding a small keep-alive thread/timer to call ProcessEvents periodically, or otherwise ensure the server can respond even when no events are flowing.
otsdaq-mu2e-crv/fcl/RunCrvDQM.fcl
Outdated
| outputs: { | ||
|
|
||
| out1: { | ||
| module_type: FileDumperOutput | ||
| wantProductFriendlyClassName: true # Print class names | ||
| onlyIfPresent: true # Only present products | ||
| } |
There was a problem hiding this comment.
The new output module out1 is defined under outputs, but it is not scheduled on any end path (end_paths only contains dqm_path). In art, output modules generally need to be included in an end path to run; otherwise this block has no effect. Add out1 to an end path (e.g., create an e1: [ out1 ] and include it in end_paths) or remove the outputs block if it’s not intended to execute.
|
|
||
| namespace ots | ||
| { | ||
| // clang-format off |
There was a problem hiding this comment.
The new .h clang format (found in otsdaq-mu2e) is quite nice, could be worth a try.
merging in recent changes to keep up to date