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

Layer1 unpacker and DQM modified for HCALFB #41028

Merged
merged 1 commit into from Mar 16, 2023

Conversation

hftsoi
Copy link
Contributor

@hftsoi hftsoi commented Mar 10, 2023

PR description:

This PR is to modify CaloLayer1 unpacker for proper readout from new fw version at CaloLayer1 about additional HCAL FB implementation. Previously the CaloLayer1 payload data contain only the first FB (FB0) from HCAL, now there is a new fw version that extends the payload data format to contain all other FB (FB1-5), so the unpacker is modified to unpack them. For flexibility there is a flag in the payload header to tell if the additional HCAL FB is implemented or not, therefore two new unpackers (one with UCTCTP7RawData_HCALFB.h for normal event unpacker, another with UCTCTP7RawData5BX_HCALFB.h for fat event unpacker) are added to be used when this flag is set, otherwise the usual unpackers (not for additional HCAL FB implementation) are used and no modification is made to them in this PR. DQM is also modified for these HCAL FB monitoring.

PR validation:

Offline DQM was run on past commissioning runs, expected DQM elements are added and properly filled. Several P5 tests (included in online DQM P5 tags already) were also done together with the new CaloLayer1 fw loaded during CRUZET, where HCAL sent feature bit patterns which could all be read out properly by CaloLayer1, and the output LLP bits were observed to be consistent with the logic. The P5 tests of modifications included in this PR were successful and this is to make it permanent in production system.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41028/34558

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @hftsoi (Ho-Fung Tsoi) for master.

It involves the following packages:

  • DQM/L1TMonitor (dqm)
  • EventFilter/L1TRawToDigi (l1)

@aloeliger, @epalencia, @emanueleusai, @cmsbuild, @syuvivida, @pmandrik, @micsucmed, @cecilecaillol, @rvenditti can you please review it and eventually sign? Thanks.
@dinyar, @missirol, @Martin-Grunewald, @thomreis, @eyigitba this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@epalencia
Copy link
Contributor

Please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-508486/31215/summary.html
COMMIT: 9968a15
CMSSW: CMSSW_13_1_X_2023-03-10-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41028/31215/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 7 lines to the logs
  • Reco comparison results: 18 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3530381
  • DQMHistoTests: Total failures: 135
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3530224
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 213 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@emanueleusai
Copy link
Member

type hcal

@cmsbuild cmsbuild added the hcal label Mar 13, 2023
@emanueleusai
Copy link
Member

type l1t

@cmsbuild cmsbuild added the l1t label Mar 13, 2023
@hftsoi
Copy link
Contributor Author

hftsoi commented Mar 13, 2023

For L1T/L1TStage2CaloLayer1/HCalDetail, the hcalOccSentFg0-5 are the 6 fine grain bits readout from HCAL, the plots now include both HBHE and HF to show the whole picture what HCAL is sending to Layer1, as compared to before that only HBHE is filled. So this is expected.

For L1T/L1TStage2CaloLayer1/MismatchDetail, I believe it is due to the test run in which the previous Layer1 firmware version was not updated yet so that the fine grain bits readout at Layer1 is not fully functioning, resulting in mismatches when comparing with HCAL readout. We also observed similar mismatches in past weeks during our online tests when the PR was deployed with previous FW version. The mismatches should go away when new Layer1 FW is deployed, which is the case very recently starting from run 364438 on Thursday. We also checked all runs beyond 364438 where both this PR and new Layer1 FW are in production, there have not been mismatches.

@emanueleusai
Copy link
Member

thank you very much for the explanation!

  • DQM plot differences understood

@emanueleusai
Copy link
Member

testing 13_0 backport at P5

}

// Current 6:1 LUT in fw
const uint64_t HCalFbLUT = 0xBBBABBBABBBABBBA;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know @vshang recently opened a PR that included this LUT as a config param to the caloparams:
https://github.com/cms-sw/cmssw/pull/41046/files#diff-4664492025070575b58209e109e6c1fa97512eae8c07c841724000157c2a4703R171-R173
This is hard coded directly. Is this resilient enought to change it can be left constant and hard coded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @aloeliger, as I understood every time new LUT is uploaded, a new calo param config file will be created with new file name (like the new caloParams_2022_v0_6_cfi.py @vshang created in his PR) instead of overwriting some existing config file in production, so not sure if there is easy way to load LUT dynamically without knowing the next file name until created. But anyway we anticipate this BBBA LUT to stay for years unless HCAL decides to change their LLP algorithm in short time, so hard coded in DQM for now.

Comment on lines +363 to +416
void CaloLayer1Unpacker::makeHFTPGs_HCALFB(uint32_t lPhi,
UCTCTP7RawData_HCALFB& ctp7Data_HCALFB,
HcalTrigPrimDigiCollection* hcalTPGs) {
UCTCTP7RawData_HCALFB::CaloType cType = UCTCTP7RawData_HCALFB::HF;
for (uint32_t side = 0; side <= 1; side++) {
bool negativeEta = false;
if (side == 0)
negativeEta = true;
for (uint32_t iEta = 30; iEta <= 40; iEta++) {
for (uint32_t iPhi = 0; iPhi < 2; iPhi++) {
if (iPhi == 1 && iEta == 40)
iEta = 41;
int cPhi = 1 + lPhi * 4 + iPhi * 2; // Calorimeter phi index: 1, 3, 5, ... 71
if (iEta == 41)
cPhi -= 2; // Last two HF are 3, 7, 11, ...
cPhi = (cPhi + 69) % 72 + 1; // cPhi -= 2 mod 72
int cEta = iEta;
if (negativeEta)
cEta = -iEta;
// This code is fragile! Note that towerDatum is packed as is done in HcalTriggerPrimitiveSample
// Bottom 8-bits are ET
// Then feature bit
// Then minBias ADC count bit
// The remaining bits are undefined presently
// We use next three bits for link details, which we did not have room in EcalTriggerPrimitiveSample case
// We use next three bits to set the tower masking, link masking and link status information as done for Ecal
// To decode these custom six bits use ((EcalTriggerPrimitiveSample::raw() >> 9) & 0x77)
uint32_t towerDatum = ctp7Data_HCALFB.getET(cType, negativeEta, iEta, iPhi);
towerDatum |= ctp7Data_HCALFB.getFB(cType, negativeEta, iEta, iPhi) << 8;
if (ctp7Data_HCALFB.isLinkMisaligned(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x0400;
if (ctp7Data_HCALFB.isLinkInError(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x0800;
if (ctp7Data_HCALFB.isLinkDown(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x1000;
if (ctp7Data_HCALFB.isTowerMasked(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x2000;
if (ctp7Data_HCALFB.isLinkMasked(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x4000;
if (ctp7Data_HCALFB.isLinkMisaligned(cType, negativeEta, iEta, iPhi) ||
ctp7Data_HCALFB.isLinkInError(cType, negativeEta, iEta, iPhi) ||
ctp7Data_HCALFB.isLinkDown(cType, negativeEta, iEta, iPhi))
towerDatum |= 0x8000;
HcalTriggerPrimitiveSample sample(towerDatum);
HcalTrigTowerDetId id(cEta, cPhi);
id.setVersion(1); // To not process these 1x1 HF TPGs with RCT
HcalTriggerPrimitiveDigi tpg(id);
tpg.setSize(1);
tpg.setSample(0, sample);
hcalTPGs->push_back(tpg);
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are multiple instances of code that looks very similar to this. Can any of this be abstracted to avoid repeated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are made explicit and separate so that all 4 unpackers (normal vs 5BX and HCALFB impl vs not) are completely independent to each other and for easier debugging with FW in future if we want to test/modify one of them at a time.

@aloeliger
Copy link
Contributor

@hftsoi Right now @vshang has #41046 open with changes relating to this LUT. Are these PRs interdependent in any way at all? Will the DQM/unpacker break without the DB changes Victor is proposing?

@hftsoi
Copy link
Contributor Author

hftsoi commented Mar 14, 2023

@hftsoi Right now @vshang has #41046 open with changes relating to this LUT. Are these PRs interdependent in any way at all? Will the DQM/unpacker break without the DB changes Victor is proposing?

The PRs are independent to each other, since in this PR the LUT is hard coded in DQM instead of loading from DB etc. The recent P5 tests were also performed before any DB changes.

@aloeliger
Copy link
Contributor

Okay then, I think I am satisfied. @epalencia is there anything else here you would like to see or check?

@epalencia
Copy link
Contributor

+l1

@emanueleusai
Copy link
Member

+1

  • P5 tests successful
  • DQM differences understood

@cmsbuild
Copy link
Contributor

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)

@perrotta
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants