-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
add layer-1 monitoring for new slot-7 cards [13_0_7] #42021
Conversation
A new Pull Request was created by @hftsoi (Ho-Fung Tsoi) for CMSSW_13_0_X. It involves the following packages:
@aloeliger, @epalencia, @nothingface0, @emanueleusai, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here
|
hi @syuvivida this is the 13_0_7 version on top of #41383 with additional minor checks added, could you please integrate this into online DQM production, we will do a quick test on it as soon as this PR is there. Thanks! |
please test |
type hcal |
working on it |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9db053/33255/summary.html Comparison SummarySummary:
|
Hi @hftsoi we have run the test in our playback systems with this PR, you can have a look at the output root file at EOS in this path: |
thanks @micsucmed it looks good, could you please deploy it? |
@hftsoi, great, we will deploy it tomorrow after the RC meeting |
+1
|
//dqm::reco::MonitorElement *hcalOccFg4Discrepancy_; | ||
//dqm::reco::MonitorElement *hcalOccFg5Discrepancy_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are no longer needed, could you remove them?
//const bool Hfg4Agreement = (abs(ieta) < 29) ? (layer1fg4 == uHTRfg4) : true; | ||
//const bool Hfg5Agreement = (abs(ieta) < 29) ? (layer1fg5 == uHTRfg5) : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These as well
//if (not Hfg4Agreement) { | ||
// eventMonitors.hcalOccFg4Discrepancy_->Fill(ieta, iphi); | ||
//} | ||
//if (not Hfg5Agreement) { | ||
// eventMonitors.hcalOccFg5Discrepancy_->Fill(ieta, iphi); | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These as well
//eventMonitors.hcalOccFg4Discrepancy_ = | ||
// bookHcalOccupancy("hcalOccFg4Discrepancy", "HCal Fine Grain 4 Discrepancy between uHTR and Layer1"); | ||
//eventMonitors.hcalOccFg5Discrepancy_ = | ||
// bookHcalOccupancy("hcalOccFg5Discrepancy", "HCal Fine Grain 5 Discrepancy between uHTR and Layer1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are commented at the moment to mute monitoring of channels where HCAL is sending unphysical data to layer1, and will be uncommented once they fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me then
Re-open this PR, P5 test went fine and we want these changes to stay permanent. A PR is opened to master (#42048), and this will be a backport. |
backport of #42048 |
please test |
Hi, I don't think the failed test has anything to do with this PR (similar failing has been seen in #38361), could you please check? Thank you! |
-1 Failed Tests: RelVals RelVals-INPUT RelVals
RelVals-INPUT |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9db053/33534/summary.html Comparison SummarySummary:
|
+l1
|
@emanueleusai you signed this already before the latest minor updates that made this backport PR identical to its already merged master version, also signed by @cms-sw/dqm-l2 |
+1 |
merge |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_2_X is complete. This pull request will be automatically merged. |
PR description:
This PR modifies Calo-Layer1 unpacker to adapt the additions of a new CTP7 card in slot-7 in each of the three layer-1 crates (FEDs 1354, 1356, 1358), where each card sends the same payload header and trailer as all other existing calo cards, but with a fixed payload data size of 6 32-bit words, regardless of normal or FAT events being sent. New monitoring elements are added to layer-1 DQM for the 3x6x32 bits. The modification is done in such a way that it works before and after the card addition.
Note that the monitoring elements for HCAL FB4-5 are commented out, we will put them back once HCAL fixes them (FB4-5 are reserved bits and not used for LLP, but they are sending unphysical data there which layer-1 could not read out, causing discrepancies seen when comparing them).
PR validation:
Validated by running offline DQM on past commissioning runs, it works as expected for current production firmware. This adds minor firmware status checks on top of #41383 which has been tested online with new firmware.