Bcolumicount#4214
Merged
pinkenburg merged 9 commits intoMar 11, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new bcolumicount package providing a BcoInfo interface and BcoInfov1 concrete type, two SubsysReco modules (BcoLumiReco for reconstruction and BcoLumiCheck for monitoring), ROOT LinkDef entries, and full Autotools build support including dictionary generation and test scaffolding. Changes
Sequence Diagram(s)sequenceDiagram
participant PRDF as PRDF Event
participant BcoLumiReco as BcoLumiReco
participant DST as DST Node
participant BcoInfo as BcoInfo
participant Sync as SyncObject
participant Buff as Rolling Buffers
PRDF->>BcoLumiReco: process_event(topNode)
BcoLumiReco->>DST: retrieve Gl1Packet (Packet 14001) and SyncObject
DST-->>BcoLumiReco: Gl1Packet, SyncObject
BcoLumiReco->>PRDF: extract BCO, evtno
PRDF-->>BcoLumiReco: BCO value, evtno
BcoLumiReco->>Buff: push_bco(BCO), push_evtno(evtno)
Buff->>Buff: rotate [future ← current ← previous]
BcoLumiReco->>DST: retrieve/create BcoInfo node
DST-->>BcoLumiReco: BcoInfo object
BcoLumiReco->>BcoInfo: set_previous/current/future BCO and evtno
BcoInfo-->>DST: updated node stored
BcoLumiReco-->>PRDF: return EVENT_OK (or abort on missing DST/first-event)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2351db00-9963-481b-ac9c-3b90a0cb2413
📒 Files selected for processing (13)
offline/packages/bcolumicount/BcoInfo.ccoffline/packages/bcolumicount/BcoInfo.hoffline/packages/bcolumicount/BcoInfoLinkDef.hoffline/packages/bcolumicount/BcoInfov1.ccoffline/packages/bcolumicount/BcoInfov1.hoffline/packages/bcolumicount/BcoInfov1LinkDef.hoffline/packages/bcolumicount/BcoLumiCheck.ccoffline/packages/bcolumicount/BcoLumiCheck.hoffline/packages/bcolumicount/BcoLumiReco.ccoffline/packages/bcolumicount/BcoLumiReco.hoffline/packages/bcolumicount/Makefile.amoffline/packages/bcolumicount/autogen.shoffline/packages/bcolumicount/configure.ac
Comment on lines
+9
to
+24
| void BcoInfov1::identify(std::ostream& out) const | ||
| { | ||
| out << "identify yourself: I am an BcoInfov1 Object\n"; | ||
| out << std::hex; | ||
| out << "previous event: " << get_previous_evtno() << std::hex | ||
| << " bco: 0x" << get_previous_bco() << "\n" | ||
| << std::dec | ||
| << "current event: " << get_current_evtno() << std::hex | ||
| << " bco: 0x" << get_current_bco() << "\n" | ||
| << std::dec | ||
| << "future event: " << get_future_evtno() << std::hex | ||
| << " bco: 0x" << get_future_bco() << std::dec | ||
| << std::endl; | ||
|
|
||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent output format for previous_evtno.
Line 12 sets std::hex, so get_previous_evtno() on line 13 will be printed in hexadecimal, while current_evtno (line 16) and future_evtno (line 19) are printed in decimal. This appears unintentional given the consistent decimal formatting for the other event numbers.
🔧 Suggested fix
void BcoInfov1::identify(std::ostream& out) const
{
out << "identify yourself: I am an BcoInfov1 Object\n";
- out << std::hex;
- out << "previous event: " << get_previous_evtno() << std::hex
+ out << "previous event: " << get_previous_evtno() << std::hex
<< " bco: 0x" << get_previous_bco() << "\n"
Comment on lines
+54
to
+59
| SyncObject *syncobject = findNode::getClass<SyncObject>(topNode, syncdefs::SYNCNODENAME); | ||
| Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001); | ||
| if (gl1packet) | ||
| { | ||
| std::cout << "Event No: " << syncobject->EventNumber() << std::hex | ||
| << " gl1: bco 0x" << gl1packet->lValue(0, "BCO") << std::dec << std::endl; |
Contributor
There was a problem hiding this comment.
Guard the SyncObject dereference.
Line 58 calls syncobject->EventNumber() whenever a GL1 node is present, but Line 54 never verifies that the sync node exists. On inputs missing syncdefs::SYNCNODENAME, this will crash the event loop.
Proposed fix
- if (gl1packet)
+ if (gl1packet && syncobject)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SyncObject *syncobject = findNode::getClass<SyncObject>(topNode, syncdefs::SYNCNODENAME); | |
| Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001); | |
| if (gl1packet) | |
| { | |
| std::cout << "Event No: " << syncobject->EventNumber() << std::hex | |
| << " gl1: bco 0x" << gl1packet->lValue(0, "BCO") << std::dec << std::endl; | |
| SyncObject *syncobject = findNode::getClass<SyncObject>(topNode, syncdefs::SYNCNODENAME); | |
| Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001); | |
| if (gl1packet && syncobject) | |
| { | |
| std::cout << "Event No: " << syncobject->EventNumber() << std::hex | |
| << " gl1: bco 0x" << gl1packet->lValue(0, "BCO") << std::dec << std::endl; |
Comment on lines
+55
to
+56
| Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001); | ||
| if (gl1packet) |
Contributor
There was a problem hiding this comment.
Add the legacy GL1 node fallback.
This only probes the version-specific 14001 node. On files that still publish GL1 as GL1Packet, the checker will silently stop validating BCO alignment.
Proposed fix
Gl1Packet *gl1packet = findNode::getClass<Gl1Packet>(topNode, 14001);
+ if (!gl1packet)
+ {
+ gl1packet = findNode::getClass<Gl1Packet>(topNode, "GL1Packet");
+ }Based on learnings, GL1Packet-like lookups should try the version-specific key first and then fall back to the legacy/general key to stay compatible across data versions.
Contributor
Author
There was a problem hiding this comment.
That's just wrong. Internally the int is converted to a string which then does the lookup, checking here in addition with a string is a waste of time - it will give the same result
.
Contributor
There was a problem hiding this comment.
@pinkenburg, thank you for the correction — I apologize for the noise. You're right that passing the integer 14001 already results in the same string-based lookup internally, making a separate "GL1Packet" fallback completely redundant.
I'll update the long-term memory to reflect this so the same incorrect suggestion isn't made again.
✏️ Learnings added
Learnt from: pinkenburg
Repo: sPHENIX-Collaboration/coresoftware PR: 4214
File: offline/packages/bcolumicount/BcoLumiCheck.cc:55-56
Timestamp: 2026-03-10T19:51:40.934Z
Learning: In the sPHENIX coresoftware framework, `findNode::getClass<Gl1Packet>(topNode, 14001)` internally converts the integer argument to a string and performs the node lookup. Calling it a second time with the string `"GL1Packet"` is redundant and gives the same result. No dual-key fallback (int + string) is needed for GL1Packet node lookups when using the integer overload.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: emclaughlin2
Repo: sPHENIX-Collaboration/coresoftware PR: 4098
File: offline/QA/Calorimeters/CaloValid.cc:270-270
Timestamp: 2026-01-08T18:39:48.023Z
Learning: In C++ code that parses or traverses data nodes, implement a dual-key fallback when looking up a GL1Packet-like node: first try the version-specific key (e.g., "14001"), then fall back to the legacy/general key (e.g., "GL1Packet"). This ensures compatibility with multiple data versions where the node naming may have changed over time. Apply this pattern to files under the offline/ Calorimeters area (and similar modules) to maintain data-structure resilience across versions.
🧠 Learnings used
Learnt from: emclaughlin2
Repo: sPHENIX-Collaboration/coresoftware PR: 4098
File: offline/QA/Calorimeters/CaloValid.cc:270-270
Timestamp: 2026-01-08T18:39:48.023Z
Learning: In C++ code that parses or traverses data nodes, implement a dual-key fallback when looking up a GL1Packet-like node: first try the version-specific key (e.g., "14001"), then fall back to the legacy/general key (e.g., "GL1Packet"). This ensures compatibility with multiple data versions where the node naming may have changed over time. Apply this pattern to files under the offline/ Calorimeters area (and similar modules) to maintain data-structure resilience across versions.
Comment on lines
+23
to
+24
| # | ||
| #include <iostream> |
Contributor
There was a problem hiding this comment.
Stray preprocessor character.
Line 23 has a lone # which is invalid preprocessor syntax. This may cause compilation warnings or errors depending on compiler strictness.
🔧 Suggested fix
`#include` <Event/packet.h> // for Packet
-#
+
`#include` <iostream>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # | |
| #include <iostream> | |
| `#include` <Event/packet.h> // for Packet | |
| `#include` <iostream> |
Comment on lines
+73
to
+81
| Packet *packet = evt->getPacket(14001); | ||
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << std::hex << "packet ival: 0x" << packet->lValue(0, "BCO") | ||
| << " uint64_t: 0x" << gtm_bco << std::dec << std::endl; | ||
| } | ||
| push_bco(gtm_bco); | ||
| delete packet; |
Contributor
There was a problem hiding this comment.
Null pointer dereference: packet not checked before use.
evt->getPacket(14001) can return nullptr if packet 14001 is not present in the event. Line 74 dereferences packet unconditionally, which will cause a crash.
🔧 Suggested fix
Packet *packet = evt->getPacket(14001);
+ if (!packet)
+ {
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
uint64_t gtm_bco = packet->lValue(0, "BCO");
if (Verbosity() > 1)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Packet *packet = evt->getPacket(14001); | |
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << std::hex << "packet ival: 0x" << packet->lValue(0, "BCO") | |
| << " uint64_t: 0x" << gtm_bco << std::dec << std::endl; | |
| } | |
| push_bco(gtm_bco); | |
| delete packet; | |
| Packet *packet = evt->getPacket(14001); | |
| if (!packet) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| uint64_t gtm_bco = packet->lValue(0, "BCO"); | |
| if (Verbosity() > 1) | |
| { | |
| std::cout << std::hex << "packet ival: 0x" << packet->lValue(0, "BCO") | |
| << " uint64_t: 0x" << gtm_bco << std::dec << std::endl; | |
| } | |
| push_bco(gtm_bco); | |
| delete packet; |
Comment on lines
+26
to
+28
| ROOTDICTS = \ | ||
| BcoInfo_Dict.cc \ | ||
| BcoInfov1_Dict.cc |
Contributor
There was a problem hiding this comment.
Distribute the LinkDef headers.
The dictionary rule at Lines 75-76 depends on %LinkDef.h, but those files are not in any distributed file set here. make dist / tarball builds will miss them and fail when rootcint regenerates the dictionaries.
Proposed fix
ROOTDICTS = \
BcoInfo_Dict.cc \
BcoInfov1_Dict.cc
+
+EXTRA_DIST = \
+ BcoInfoLinkDef.h \
+ BcoInfov1LinkDef.hAlso applies to: 34-38, 75-76
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
offline/packages/bcolumicount/BcoLumiReco.cc (2)
23-24:⚠️ Potential issue | 🟠 MajorStray preprocessor character.
Line 23 has a lone
#which is invalid preprocessor syntax and will cause compilation warnings or errors.`#include` <Event/packet.h> // for Packet -# + `#include` <iostream>
79-87:⚠️ Potential issue | 🔴 CriticalNull pointer dereference:
packetnot checked before use.
evt->getPacket(14001)returnsnullptrwhen the packet is absent. Line 80 dereferences unconditionally, causing a crash.🔧 Suggested fix
Packet *packet = evt->getPacket(14001); + if (!packet) + { + return Fun4AllReturnCodes::ABORTEVENT; + } uint64_t gtm_bco = packet->lValue(0, "BCO");
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 370fc325-e8ee-40b9-b019-20bedebebbdd
📒 Files selected for processing (3)
offline/packages/bcolumicount/BcoInfov1.ccoffline/packages/bcolumicount/BcoLumiReco.ccoffline/packages/bcolumicount/BcoLumiReco.h
✅ Files skipped from review due to trivial changes (1)
- offline/packages/bcolumicount/BcoInfov1.cc
Comment on lines
+98
to
+114
| if (!m_synccopy) | ||
| { | ||
| m_synccopy = dynamic_cast<SyncObject *>(syncobject->CloneMe()); // clone for second event | ||
| m_tmpsync = dynamic_cast<SyncObject *>(m_synccopy->CloneMe()); // just to create this object | ||
| return Fun4AllReturnCodes::ABORTEVENT; // and abort | ||
| } | ||
| BcoInfo *bcoinfo = findNode::getClass<BcoInfo>(topNode, "BCOINFO"); | ||
|
|
||
| if (Verbosity() > 0) | ||
| { | ||
| std::cout << "current event is: " << syncobject->EventNumber() << "\n"; | ||
| std::cout << "saving as event: " << m_synccopy->EventNumber() << "\n"; | ||
| } | ||
| // here we store the current sync object and overwrite its content with the cached copy | ||
| *m_tmpsync = *syncobject; // save current version in tmp | ||
| *syncobject = *m_synccopy; // copy previously cached version | ||
| *m_synccopy = *m_tmpsync; // cache current version |
Contributor
There was a problem hiding this comment.
Null pointer dereference: syncobject not validated before use.
Line 89 shows syncobject can be null (conditional push_evtno call). However, lines 100, 108, and 112-114 dereference syncobject unconditionally. If the sync node is absent, this will crash.
🔧 Suggested fix
if (ifirst) // abort first event since it does not have a previous bco
{
ifirst = false;
return Fun4AllReturnCodes::ABORTEVENT;
}
+ if (!syncobject)
+ {
+ std::cout << PHWHERE << " SyncObject is missing" << std::endl;
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
if (!m_synccopy)
{
Build & test reportReport for commit b4ff7243ed7d749c37e58568334d1d8761a9e23a:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit dbd254b4c4f0416e42b29df4782209ade3a83c23:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ecbd7812f80b31abc540f36c81a2eceef4dd93a8:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR adds the bcolumicount package - so far it saves the previous,current and future bco in a DST (in the BcoInfo object) and tricks the synchronization to line it up with the current event in another file. The first event is dropped since there is no previous bco for that one in our data
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / Context
Add a small package to record and persist Beam Crossing Order (BCO) information and align BCOs with the current DST event. Downstream reconstruction and luminosity monitoring need a stable mapping of previous/current/future BCO and event-number triplets so algorithms can reference the correct BCO for each event.
Key changes
Potential risk areas
Possible future improvements
Note: This summary was prepared by an AI assistant — please verify synchronization semantics, first-event drop behavior, and any schema/compatibility implications against the code and tests, as AI-generated summaries can be mistaken.