Adding HCal FB LUT to caloParams #41046
Conversation
… code to allow writing of LUT values to config file for uploading to DB.
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41046/34606
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-41046/34608
|
|
A new Pull Request was created by @vshang for master. It involves the following packages:
@epalencia, @cmsbuild, @saumyaphor4252, @aloeliger, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@vshang thanks for the PR. Is this present already at P5? Are we just updating offline software to match? I recall some of the discussion from the offline software meeting, is this present in HCAL, or just at L1 side? Do we need to tag in anyone from HCAL, or ALCA and O2O? |
|
The LUTs themselves seem pretty hard-coded. Is this seen as the solution for CaloL1 going forward? |
|
Hi @vshang ! cmssw/CondCore/L1TPlugins/src/UpgradeRecords1.cc Lines 8 to 12 in a443569 which are widely used in many GTs (including the online GTs), so introducing the change would break backward compatibility. The only other option, as far as I know, is to create a new object/Rcd to add the new member std::vector<unsigned long long> u64params_. Or maybe other experts have other solutions in mind (tagging @mmusich for help)?
|
from a quick look, I don't see other options that formally comply with the "no schema evolution" policy. |
Yes, the HCAL FB LUT has already been added to the DB and the new firmware and SWATCH code has been kept in production at P5. There is no change to the logic of the LLP trigger already present in both the L1T emulator and firmware as the caloParams file being added contains the default logic (i.e. entries are 0xBBBABBBABBBABBBA). I'm not sure about tagging HCAL or ALCA, but some of the changes might affect what O2O is doing.
Before this change, the LUTs were hard coded into the CaloL1 firmware, so this change was proposed to introduce flexibility in being able to change the LUTs through the DB instead by modifying the caloParams file in a new CMSSW release. This solution is what we want to use from the CaloL1 side moving forward. @francescobrivio Thanks for the heads up. Can you clarify what you mean by "break the schema evolution of CondFormats which is forbidden"? I simply want to add a parameter to allow the use of 64b vectors instead of just 32b. Is this not possible? |
Correct, adding new members to CondFormats is not possible as it breaks already existing tags that were made in the past. That is actually the reason why the rest of them are not called something specific but are called Can you explain to me why what you are doing couldnt be done with |
These PRs should be completely independent as the changes in this PR do not depend on any of the changes for the unpacker/DQM, and vice versa.
We want to store 64b hexadecimal values in the LUT in the caloParams file, and I was under the impression that the 'double' type was not sufficient for this purpose in C++. Is this not correct? |
|
It depends on how high you go with your LUT values. Anyway, if you really need a |
|
A |
Ideally we would have the flexibility to use all possible 64b values for the LUT, so I think an unsigned long log is required. Can the changes to CondFormats/L1TObjects/interface/CaloParams.h be backported to whatever the GTs are running on? Othwerise, the simplest change would be to store the 64b LUT values as two 32b entries instead. However, this would also require modifying the SWATCH code and testing the new CaloL1 config key at P5 to ensure the HCal FB LUT values can be properly loaded into firmware. |
This would mean backporting to each release back to at least the releases in 2017. (1) I'm pretty sure you dont wanna do that, seems like a huge pain, (2) and it wouldn't matter, it's not the software that matters, but the fact that the DB has entries with this format. And no, we cannot remove those from the DB. |
|
In that case, what we can do is instead split the HCal FB LUT in the caloParams file into two parameters, each containing 32b words corresponding to the upper/lower half of the 64b entries for the LUT. This way there would be no need make any changes to CondFormats. The config key for uploading to the DB will remain the same, containing the LUT with the full 64b words as each entry, so there would be no need to change anything with respect to SWATCH or DB. Does this solution sound ok? |
sounds good for |
|
Ok, should I leave this PR open and update with another commit, or do you want to close this PR and open a new one once the changes are ready? |
|
I think it makes sense to close this one, and open another, since there is a bit of a shift in the approach |
|
I will close this PR and open a new one once the changes are ready then. |
|
-1
|
PR description:
This PR is to add a parameter to the latest caloParams file (caloParams_2022_v0_6_cfi.py) containing the HCal FB LUT for the LLP trigger as well as modifications to CaloL1 software to handle converting the HCal FB LUT from the caloParams file to the CaloL1 Algo settings key to upload to the DB.
PR validation:
Changes were tested locally by using the modified caloParams file to produce a CaloL1 Algo settings key containing the correct HCal FB LUT. This Algo settings key has been tested and validated during several P5 tests together with the new CaloLayer1 firmware and SWATCH software to upload the HCal FB LUT from DB to firmware.