Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added two new trigger candidate types for ICEBERG #29

Closed
wants to merge 3 commits into from

Conversation

bieryAtFnal
Copy link
Contributor

I have tried to follow the style of existing TC type names.

I would appreciate it if experts could provide feedback.

@bieryAtFnal bieryAtFnal added the miscellaneous deliverable A change that is/will be part of a release but is not substantial enough to be a daq-deliverable label Jun 12, 2024
@MRiganSUSX
Copy link
Contributor

Hi Kurt,
I have no issues with the naming. I do have a comment and a question though:

Question: Do we want to increase the s_trigger_candidate_version just because we added TC types? In my understanding, this was meant to reflect when we changed the actual parameters of the TC object (added, removed, changed data type and therefore size).

Comment:
Unless there is a reason not to, please add the new types to pybind enums as well:
https://github.com/DUNE-DAQ/trgdataformats/blob/production/v4/pybindsrc/trigger_candidate.cpp#L68-L110

@bieryAtFnal
Copy link
Contributor Author

Thanks @MRiganSUSX , both of those are good points.

I've added the Python entries and backed out the version change.

Copy link
Contributor

@MRiganSUSX MRiganSUSX left a comment

Choose a reason for hiding this comment

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

LGTM.

As mentioned, there will be some new types added for CRT group as well. We may need to coordinate merging.

@bieryAtFnal
Copy link
Contributor Author

Closing this PR since its changes have been included in #30

@bieryAtFnal bieryAtFnal deleted the kbiery/iceberg_tc_types branch June 18, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miscellaneous deliverable A change that is/will be part of a release but is not substantial enough to be a daq-deliverable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants