HGCal emulator and unpacker #41461
HGCal emulator and unpacker #41461
Conversation
- S-link emulator from test beam data - channel survival probability in emulator - transient object storing channels PoI and H/T, E/B/O, and S bits info after emulation - storing a vector of PoIs for each eRx - splitting probabilities maps into a subparameter block - optional FEDHeader/Trailer words - optional emulator transient information storage (channels PoI and H/T, E/B/O, and S bits info)
- factorizing data formats - updating class version - adding first version of electronics id class - from Laurent: also added const-qualified getters for the HGCalElectronicsId object
- from Laurent: stripped unpacker from header-only implementation
- more flexibility in Slink header/trailer definition - fixed 64b endianness, thanks to joint debugging with yulunmiao - safer dump of packed words through a LogDebug - also added a trivial emulator mode (first attempt to fill the digis with incorrect mapping) - added configuration flags to the test config - also store unpacked HGCalElectronicsId-indexed digis - added capability to steer individual channels in "trivial" mode emulator - added steering of characterisation mode, and number of enabled channels per eRx - made emulator info metadata storage an optional flag
Emulator: - increased modularity of channels/eRx, eRx/ECON-D in simulation - distinguished ECON-D header CRC (8bit) from full payload CRC (32bit) - optionally store S-link header/trailer in event payload - fixed channels list order in eRx header - merged trivial/empty emulators - make the ECON-D idle word addition optional - module tree reader is now only unpacking the proper number of eRx to be emulated - allow multiple capture blocks to be emulated in one S-link payload, even with a random number of them - a random number of ECON-D payloads can also be emulated in one go - simplified eRx payload composer with all channels information into a single 32bit words collection - simplified the API for emulator metadata Unpacker: - number of eRxs per ECON-D is now an unpacker parameter - using STL vectors instead of C arrays in unpacker algorithm (CMS coding rule 7.10) - safer composition of 32bit word from FEDRawData - dropped the 'get' prefix from getter methods (CMS coding rule 2.11) Miscellaneous: - made most of HGCROC channel dataframe get'ters const-qualified
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41461/35324
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41461/35325
|
|
A new Pull Request was created by @forthommel (Laurent Forthomme) for master. It involves the following packages:
@civanch, @clacaputo, @cmsbuild, @AdrianoDee, @srimanob, @mdhildreth, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d7657/32265/summary.html Comparison SummarySummary:
|
|
Thanks a lot, that was fast! |
|
+1 |
|
@AdrianoDee, @srimanob, anything you need from us for your |
|
all that can be done to speed up the integration of this PR is appreciated - we'll rely on it for the upcoming test beams in august/september. Thanks |
|
+Upgrade If I read the PR description correctly, no change is expected from this PR. |
cmsbuild
added
fully-signed
upgrade-approved
and removed
pending-signatures
upgrade-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
@cmsbuild please test Just to refresh the test before merging. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5d7657/32797/summary.html Comparison SummarySummary:
|
|
Good to see this is still running as fine as ~1 month ago :) |
|
+1
|
|
Many many thanks to everyone involved! |
| for (size_t i = 0; i < fed_data.size(); i += 4) | ||
| data_32bit.emplace_back(((ptr + i) ? ((*(ptr + i) & 0xff) << 0) : 0) + | ||
| ((ptr + i + 1) ? ((*(ptr + i + 1) & 0xff) << 8) : 0) + | ||
| ((ptr + i + 2) ? ((*(ptr + i + 2) & 0xff) << 16) : 0) + | ||
| ((ptr + i + 3) ? ((*(ptr + i + 3) & 0xff) << 24) : 0)); |
There was a problem hiding this comment.
@forthommel the el8_amd64_gcc12 builds are complaining in the IB about these lines, see e.g. https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/el8_amd64_gcc12/CMSSW_13_2_X_2023-05-27-1100/EventFilter/HGCalRawToDigi
Indeed: what are you checking here? The pointer always exists, it may point to some non existent field at most.
Could something as follows correspond to what you had in mind, maybe?
size_t fed_data_size = fed_data.size();
for (size_t i = 0; i < fed_data_size; i += 4)
data_32bit.emplace_back(((*(ptr + i) & 0xff) << 0) +
((i + 1 < fed_data_size )? ((*(ptr + i + 1) & 0xff) << 8) : 0) +
((i + 2 < fed_data_size) ? ((*(ptr + i + 2) & 0xff) << 16) : 0) +
((i + 3 < fed_data_size) ? ((*(ptr + i + 3) & 0xff) << 24) : 0));There was a problem hiding this comment.
Hi @perrotta !
Thanks for the extra x-check. Sure, this would indeed be a better practice. I can prepare a PR candidate to follow this up.
As a matter of fact I was actually surprised such a simple words merging operation was not already part of the common FWCore/Utilities tools!
PR description:
This PR introduces an unpacker module+algorithm for HGCal. It provides the missing link between the S-link frames cast into FED raw data and the digis collection already introduced since #38538, and allows future developments for the validation of the whole transmission chain.
As a way to test this unpacking, a full frame emulator is also provided along this PR. It allows the emulation of S-link, capture block, and ECON-D level payloads to be fed to the unpacker to simulate various boundary conditions. It can recast either realistic frames collected in lab/test beam, or fully "trivial" channels content containing the minimal information possible. Along with this emulation, a few metadata objects are introduced for future in-depth validations, when the error handling of the unpacker will be improved.
Various tests, defined along with the DPG (see e.g. this presentation from 12.04) are introduced as "unit tests", and allow these boundary conditions checks to be performed.
A few
const-qualification of the main data formats were introduced, possibly for the thread safety of some of these tests.Beside the long development history leading to this PR, it only contains a limited (although >1) number of commits to reflect the various contributions from the experts involved (@pfs, @yulunmiao).
PR validation:
This PR is relatively orthogonal to all parts of the validation suite, and only a limited, but successful
runTheMatrix.pyrun was performed. As mentioned above, a few unit tests are introduced along with these two components. These can be steered through the standardscram b runtests.If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for: N/A