Skip to content

HD full module readout corrected #50644

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.

Sign up for GitHub

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

Open
rchudasa wants to merge 6 commits into
base: master
Choose a base branch
from
Open

HD full module readout corrected #50644

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9351489
routing of eRX for HD unpacker
rchudasa Mar 27, 2026
91a8b28
delta calculated outside the channel ID loop
rchudasa Apr 1, 2026
c97a23a
updating muxMode and delta data type and warning for MultiPlex if it …
rchudasa Apr 14, 2026
8501e33
apply code format
rchudasa Apr 14, 2026
32a156e
mux changed to const int32_t directly
rchudasa Apr 14, 2026
bf65c8d
type casting the erxIdx to int in delta calculation
rchudasa Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CondFormats/HGCalObjects/interface/HGCalConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
// @short configuration for ECON eRX (one half of HGROC)
struct HGCalROCConfig {
uint32_t charMode; // characterization mode; determines data fields in ROC dataframe
uint32_t muxMode; // multiplexing mode; determines routing of eRx between the ROCs and ECON-Ds
COND_SERIALIZABLE;
};

Expand Down
9 changes: 7 additions & 2 deletions EventFilter/HGCalRawToDigi/src/HGCalUnpacker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,12 @@ uint16_t HGCalUnpacker::parseFEDData(unsigned fedId,
<< erxHeader << ", cmSum = " << std::dec << cmSum;
iword += 2;

const auto mux = fedConfig.econds[globalECONDIdx].rocs[erxIdx].muxMode;
Copy link
Copy Markdown
Contributor

@pfs pfs Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

why not making the static_cast directly here, or simple declaring const int32_t mux, that would avoid the repetition in the line below

uint32_t delta = (mux - erxIdx) * 37;
Copy link
Copy Markdown

@Alejandro1400 Alejandro1400 Apr 7, 2026

Choose a reason for hiding this comment

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

The logic looks correct, but since the shift can be ±37, (mux - erxIdx) can be negative? When computed with uint32_t, which I understand cannot represent negative numbers, this wraps to a large positive value and may produce incorrect indices.

A simple fix is to use signed arithmetic with int for the delta since both max and erxIdx are defined as uint32_t:

const int delta = (static_cast<int>(mux) - static_cast<int>(erxIdx)) * 37;

Copy link
Copy Markdown
Contributor Author

@rchudasa rchudasa Apr 14, 2026

Choose a reason for hiding this comment

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

updated the delta to be 0 if mux is -1 or else calculate the delta value by shifting the indices.

// parse erx body (channel data)
uint32_t iBit = 0;
for (uint32_t channelIdx = 0; channelIdx < HGCalMappingCellIndexer::maxChPerErx_; channelIdx++) {
uint32_t denseIdx = moduleIndexer.getIndexForModuleData(fedId, globalECONDIdx, erxIdx, channelIdx);
uint32_t denseIdx = moduleIndexer.getIndexForModuleData(fedId, globalECONDIdx, erxIdx, channelIdx) + delta;

// check if the channel has data
if (((erxHeader >> channelIdx) & 1) == 0) {
Expand Down Expand Up @@ -396,9 +398,12 @@ uint16_t HGCalUnpacker::parseFEDData(unsigned fedId,
<< erxHeader << ", cmSum = " << std::dec << cmSum;
iword += 2;

const auto mux = fedConfig.econds[globalECONDIdx].rocs[erxIdx].muxMode;
Copy link
Copy Markdown
Contributor

@pfs pfs Apr 14, 2026

Choose a reason for hiding this comment

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

same comment as above, maybe use const int32_t directly?

Copy link
Copy Markdown
Contributor Author

@rchudasa rchudasa Apr 14, 2026

Choose a reason for hiding this comment

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

updated it.

uint32_t delta = (mux - erxIdx) * 37;
Copy link
Copy Markdown

@Alejandro1400 Alejandro1400 Apr 7, 2026

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

@rchudasa rchudasa Apr 14, 2026

Choose a reason for hiding this comment

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

updated the delta to be 0 if mux is -1 or else calculate the delta value by shifting the indices.


// parse erx body (channel data)
for (uint32_t channelIdx = 0; channelIdx < HGCalMappingCellIndexer::maxChPerErx_; channelIdx++) {
uint32_t denseIdx = moduleIndexer.getIndexForModuleData(fedId, globalECONDIdx, erxIdx, channelIdx);
uint32_t denseIdx = moduleIndexer.getIndexForModuleData(fedId, globalECONDIdx, erxIdx, channelIdx) + delta;

// check if the channel has data
if (((erxHeader >> channelIdx) & 1) == 0) {
Expand Down
3 changes: 2 additions & 1 deletion RecoLocalCalo/HGCalRecAlgos/plugins/HGCalConfigurationESProducer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class HGCalConfigurationESProducer : public edm::ESProducer, public edm::EventSe
uint32_t nfeds = moduleMap.numFEDs();
uint32_t ntot_mods = 0, ntot_rocs = 0;
const std::vector<std::string> fedkeys = {"mismatchPassthroughMode", "cbHeaderMarker", "slinkHeaderMarker"};
const std::vector<std::string> modkeys = {"headerMarker", "CalibrationSC"};
const std::vector<std::string> modkeys = {"headerMarker", "CalibrationSC", "MultiPlex"};
if (nfeds != fed_config_data.size())
edm::LogWarning("HGCalConfigurationESProducer")
<< "Total number of FEDs found in JSON file " << fedjsonurl << " (" << fed_config_data.size()
Expand Down Expand Up @@ -157,6 +157,7 @@ class HGCalConfigurationESProducer : public edm::ESProducer, public edm::EventSe
ntot_rocs++;
HGCalROCConfig roc;
roc.charMode = getint(mod_config_data[modkey]["CalibrationSC"][iroc], charMode_);
roc.muxMode = getint(mod_config_data[modkey]["MultiPlex"][iroc], -1);
Copy link
Copy Markdown

@Alejandro1400 Alejandro1400 Apr 7, 2026

Choose a reason for hiding this comment

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

I see a similar potential issue here.

getint(..., -1) is assigned to muxMode, which is a uint32_t. If the value is missing, -1 will wrap to a large positive value, which could silently break the remapping logic. Could you clarify what getint should return and whether -1 is intended as an invalid case or for indexing purposes?

Maybe consider using a signed type for muxMode, or explicitly validating this case here and throwing an exception.

Copy link
Copy Markdown
Contributor Author

@rchudasa rchudasa Apr 14, 2026

Choose a reason for hiding this comment

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

updated the muxMode to signed integer and throw a warning if MultiPlex key does not exist in config file and set the muxMode to -1.

mod.rocs[iroc] = roc; // add to ECON-D's vector<HGCalROCConfig> of eRx half-ROCs
}
fed.econds[imod] = mod; // add to FED's vector<HGCalECONDConfig> of ECON-D modules
Expand Down