Skip to content

Conversation

@michaelmackenzie
Copy link
Contributor

No description provided.

@FNALbuild
Copy link
Collaborator

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

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

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at c41285f.

Test Result Details
test with Command did not list any other PRs to include
merge Merged c41285f at a04e36a
build (prof) Log file. Build time: 04 min 32 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. Return Code 1.
FIXME, TODO TODO (0) FIXME (0) in 1 files
clang-tidy 🔶 2 errors 11 warnings
whitespace check no whitespace errors found

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

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.

@brownd1978
Copy link
Collaborator

Out of curiosity, how are EWM produced Online? The only producer I know of is the MC one.

@michaelmackenzie
Copy link
Contributor Author

I'm not positive if we are Online yet, I don't think I've seen an Online producer for this either. We could add a producer to make this object from the CFO fragment or DTC subevent header I believe. For now, the best place might be the EventHeaderFromCFOFragment module.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request enhances the PrefetchDAQData module by adding support for prefetching EventWindowMarker (EWM) and ProtonBunchTime (PBT) data products, and migrates the module from using legacy fhicl::ParameterSet to the validated FHiCL configuration pattern using art::EDProducer::Table<Config>.

Changes:

  • Migrated constructor from fhicl::ParameterSet to validated configuration using Config struct and Parameters typedef
  • Added EWM and PBT data products to the prefetch mechanism with corresponding configuration parameters and member variables
  • Added includes for EventWindowMarker and ProtonBunchTime data product headers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +30 to +43
fhicl::Atom<int> debugLevel {Name("debugLevel" ), Comment("debugLevel" ), 0};
fhicl::Atom<int> fetchCaloDigis {Name("fetchCaloDigis" ), Comment("fetchCaloDigis" )};
fhicl::Atom<int> fetchStrawDigis {Name("fetchStrawDigis" ), Comment("fetchStrawDigis" )};
fhicl::Atom<int> fetchCaloFragments {Name("fetchCaloFragments" ), Comment("fetchCaloFragments" )};
fhicl::Atom<int> fetchTrkFragments {Name("fetchTrkFragments" ), Comment("fetchTrkFragments" )};
fhicl::Atom<int> fetchAllFragments {Name("fetchAllFragments" ), Comment("fetchAllFragments" )};
fhicl::Atom<int> fetchEWM {Name("fetchEWM" ), Comment("fetchEWM" )};
fhicl::Atom<int> fetchPBT {Name("fetchPBT" ), Comment("fetchPBT" )};
fhicl::Atom<std::string> caloDigiCollectionTag {Name("caloDigiCollectionTag" ), Comment("caloDigiCollectionTag" )};
fhicl::Atom<std::string> strawDigiCollectionTag{Name("strawDigiCollectionTag"), Comment("strawDigiCollectionTag")};
fhicl::Atom<std::string> caloFragmentTag {Name("caloFragmentTag" ), Comment("caloFragmentTag" )};
fhicl::Atom<std::string> trkFragmentTag {Name("trkFragmentTag" ), Comment("trkFragmentTag" )};
fhicl::Atom<std::string> pbtTag {Name("pbtTag" ), Comment("pbtTag" )};
fhicl::Atom<std::string> ewmTag {Name("ewmTag" ), Comment("ewmTag" )};
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The Comment fields for all configuration parameters simply repeat the parameter names without providing meaningful documentation. These should describe what each parameter does, expected values, or constraints. For example, debugLevel could be 'Debug output verbosity level (0=off)', fetchCaloDigis could be 'Enable prefetching of calorimeter digis (0=off, 1=on)'.

Suggested change
fhicl::Atom<int> debugLevel {Name("debugLevel" ), Comment("debugLevel" ), 0};
fhicl::Atom<int> fetchCaloDigis {Name("fetchCaloDigis" ), Comment("fetchCaloDigis" )};
fhicl::Atom<int> fetchStrawDigis {Name("fetchStrawDigis" ), Comment("fetchStrawDigis" )};
fhicl::Atom<int> fetchCaloFragments {Name("fetchCaloFragments" ), Comment("fetchCaloFragments" )};
fhicl::Atom<int> fetchTrkFragments {Name("fetchTrkFragments" ), Comment("fetchTrkFragments" )};
fhicl::Atom<int> fetchAllFragments {Name("fetchAllFragments" ), Comment("fetchAllFragments" )};
fhicl::Atom<int> fetchEWM {Name("fetchEWM" ), Comment("fetchEWM" )};
fhicl::Atom<int> fetchPBT {Name("fetchPBT" ), Comment("fetchPBT" )};
fhicl::Atom<std::string> caloDigiCollectionTag {Name("caloDigiCollectionTag" ), Comment("caloDigiCollectionTag" )};
fhicl::Atom<std::string> strawDigiCollectionTag{Name("strawDigiCollectionTag"), Comment("strawDigiCollectionTag")};
fhicl::Atom<std::string> caloFragmentTag {Name("caloFragmentTag" ), Comment("caloFragmentTag" )};
fhicl::Atom<std::string> trkFragmentTag {Name("trkFragmentTag" ), Comment("trkFragmentTag" )};
fhicl::Atom<std::string> pbtTag {Name("pbtTag" ), Comment("pbtTag" )};
fhicl::Atom<std::string> ewmTag {Name("ewmTag" ), Comment("ewmTag" )};
fhicl::Atom<int> debugLevel {Name("debugLevel" ), Comment("Debug output verbosity level (0 = off; higher values produce more logging)" ), 0};
fhicl::Atom<int> fetchCaloDigis {Name("fetchCaloDigis" ), Comment("Enable prefetching of calorimeter digi collections (0 = disabled, 1 = enabled)" )};
fhicl::Atom<int> fetchStrawDigis {Name("fetchStrawDigis" ), Comment("Enable prefetching of straw tracker digi collections (0 = disabled, 1 = enabled)" )};
fhicl::Atom<int> fetchCaloFragments {Name("fetchCaloFragments" ), Comment("Enable prefetching of calorimeter DAQ fragments (0 = disabled, 1 = enabled)" )};
fhicl::Atom<int> fetchTrkFragments {Name("fetchTrkFragments" ), Comment("Enable prefetching of tracker DAQ fragments (0 = disabled, 1 = enabled)" )};
fhicl::Atom<int> fetchAllFragments {Name("fetchAllFragments" ), Comment("If nonzero, prefetch all available DAQ fragments regardless of subdetector-specific fetch flags" )};
fhicl::Atom<int> fetchEWM {Name("fetchEWM" ), Comment("Enable prefetching of EventWindowMarker objects (0 = disabled, 1 = enabled)" )};
fhicl::Atom<int> fetchPBT {Name("fetchPBT" ), Comment("Enable prefetching of ProtonBunchTime objects (0 = disabled, 1 = enabled)" )};
fhicl::Atom<std::string> caloDigiCollectionTag {Name("caloDigiCollectionTag" ), Comment("Input tag for the calorimeter digi collection to be prefetched" )};
fhicl::Atom<std::string> strawDigiCollectionTag{Name("strawDigiCollectionTag"), Comment("Input tag for the straw tracker digi collection to be prefetched" )};
fhicl::Atom<std::string> caloFragmentTag {Name("caloFragmentTag" ), Comment("Input tag identifying the calorimeter DAQ fragment collection to be prefetched" )};
fhicl::Atom<std::string> trkFragmentTag {Name("trkFragmentTag" ), Comment("Input tag identifying the tracker DAQ fragment collection to be prefetched" )};
fhicl::Atom<std::string> pbtTag {Name("pbtTag" ), Comment("Input tag for the ProtonBunchTime product to be prefetched" )};
fhicl::Atom<std::string> ewmTag {Name("ewmTag" ), Comment("Input tag for the EventWindowMarker product to be prefetched" )};

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +37
fhicl::Atom<int> fetchCaloDigis {Name("fetchCaloDigis" ), Comment("fetchCaloDigis" )};
fhicl::Atom<int> fetchStrawDigis {Name("fetchStrawDigis" ), Comment("fetchStrawDigis" )};
fhicl::Atom<int> fetchCaloFragments {Name("fetchCaloFragments" ), Comment("fetchCaloFragments" )};
fhicl::Atom<int> fetchTrkFragments {Name("fetchTrkFragments" ), Comment("fetchTrkFragments" )};
fhicl::Atom<int> fetchAllFragments {Name("fetchAllFragments" ), Comment("fetchAllFragments" )};
fhicl::Atom<int> fetchEWM {Name("fetchEWM" ), Comment("fetchEWM" )};
fhicl::Atom<int> fetchPBT {Name("fetchPBT" ), Comment("fetchPBT" )};
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The fetch flags use int type when bool would be more semantically appropriate for on/off flags. Using bool would make the API clearer and prevent invalid values beyond 0/1. This is a breaking change that would require updating configuration files.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you make a suggestion where these flags and the ones they initialize are bool types instead of ints?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@copilot Can you make a suggestion where these flags and the ones they initialize are bool types instead of ints?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@copilot Can you make a suggestion where these flags and the ones they initialize are bool types instead of ints?

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

auto handle = evt.getValidHandle<ProtonBunchTime>(_pbtTag);
_pbt = handle.product();
}
//-----------------------------------------------------------------------------
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

The comment delimiter line is misplaced. It should appear before line 160 to separate the data fetching section from the data access section, not in the middle of the data fetching logic.

Copilot uses AI. Check for mistakes.
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