MTD: initial implementation of BTL electronics/cabling mapping#50918
MTD: initial implementation of BTL electronics/cabling mapping#50918martinamalberti wants to merge 9 commits into
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50918/49300
|
|
A new Pull Request was created by @martinamalberti for master. It involves the following packages:
The following packages do not have a category, yet: CondFormats/MTDObjects @Dr15Jones, @bsunanda, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
if accepted, the new package should be added to https://github.com/cms-sw/cms-bot/blob/master/categories_map.py |
|
+1 Size: This PR adds an extra 40KB to repository Comparison SummarySummary:
|
|
@cms-sw/alca-l2 @cms-sw/db-l2 the package added by this PR normally should belong to your domains |
perrotta
left a comment
There was a problem hiding this comment.
Not yet a review, just a couple of trivial findings spotted after a quick glance
| #ifndef DATAFORMATS_BTLELECTRONICSMAPPING_H | ||
| #define DATAFORMATS_BTLELECTRONICSMAPPING_H 1 |
There was a problem hiding this comment.
| #ifndef DATAFORMATS_BTLELECTRONICSMAPPING_H | |
| #define DATAFORMATS_BTLELECTRONICSMAPPING_H 1 | |
| #ifndef CONDFORMATS_MTDOBJECTS_BTLELECTRONICSMAPPING_H | |
| #define CONDFORMATS_MTDOBJECTS_BTLELECTRONICSMAPPING_H 1 |
| @@ -1,5 +1,6 @@ | |||
| #include "FWCore/MessageLogger/interface/MessageLogger.h" | |||
| #include "Geometry/MTDCommonData/interface/BTLElectronicsMapping.h" | |||
| //#include "Geometry/MTDCommonData/interface/BTLElectronicsMapping.h" | |||
There was a problem hiding this comment.
| //#include "Geometry/MTDCommonData/interface/BTLElectronicsMapping.h" |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50918/49325
|
|
Pull request #50918 was updated. @Dr15Jones, @bsunanda, @civanch, @cmsbuild, @kpedro88, @makortel, @mdhildreth can you please check and sign again. |
The new package is requested in cms-sw/cmssw#50918
perrotta
left a comment
There was a problem hiding this comment.
Once you are done with the updates, it wouldn't be bad to squeeze all commits into one
| // TEMPORARY MAPPING: within a group of 6 trays ( = supertray): tray 0 --> first block of 12 links, tray 1--> second block of 12 links, etc. | ||
| // For the first block (FF_N5), channel ids of the optical tx are reversed. | ||
|
|
||
| /* struct Tx { |
There was a problem hiding this comment.
Just to understand your plans: when do you intend to uncomment these lines of code?
There was a problem hiding this comment.
What about adding a unit test here, similar to the ones that you removed from CondFormats/MTDObjects/test/TestBTLElectronicsMapping.cc ?
|
Pull request #50918 was updated. @Alejandro1400, @Dr15Jones, @JanChyczynski, @arunhep, @atpathak, @bsunanda, @civanch, @cmsbuild, @francescobrivio, @kpedro88, @makortel, @mdhildreth, @perrotta can you please check and sign again. |
There was a problem hiding this comment.
There was a problem hiding this comment.
No ROOT dictionaries are requested in this package though?
There was a problem hiding this comment.
No ROOT dictionaries are requested in this package though?
Ah ok...
I had not clear the rationale then: it looked to me the same as https://github.com/cms-sw/cmssw/pull/50916/changes#diff-7ba615a6382d50ce403a4051572e48f79a25feadcefaceab675efdb90a317743R1
Ok, I let you advise about it.
There was a problem hiding this comment.
it looked to me the same as https://github.com/cms-sw/cmssw/pull/50916/changes#diff-7ba615a6382d50ce403a4051572e48f79a25feadcefaceab675efdb90a317743R1
To me that link lead to DataFormats/DetId/BuildFile.xml (which requests dictionaries). Just to make sure, did you mean that or some other package?
There was a problem hiding this comment.
I exactly meant DataFormats/DetId/BuildFile.xml (which looked to me a pretty almost identical BuildFile than the one in this PR)
There was a problem hiding this comment.
Thanks for the clarification. The difference is that DataFormats/DetId/src has classes_def.xml for the dictionaries, whereas this added CondFormats/MTDObjects does not seem to have.
There was a problem hiding this comment.
Thank you @perrotta @makortel for your comments. This was an initial porting from the original Geometry/MTDCommonData/interface/BTLElectronicsMapping.h with simple functions based on static arrays. We are now considering to evolve it in a way more aligned with the general structure of conditions packages. We put this PR on hold for now.
|
hold |
|
Pull request has been put on hold by @martinamalberti |
PR description:
This PR introduces a new CondFormat package for MTD under
CondFormat/MTDObjects. It adds an initial version of the BTL cabling mapping inBTLElectronicsMapping.h, providing the link identifiers required for the BTL RAW data payload [1], including e-link, HS-link, and S-link ID. The S-link identifiers (which will eventually correspond to FED IDs) are currently assigned temporarily in the [0–11] range and will be updated once the official FED IDs are defined. This work is a prerequisite to start the development of the BTL packer/unpacker. A testing script has also been added inCondFormat/MTDObjects/testto verify the mapping and validate the behaviour of all implemented functions.[1] slide 5 of https://indico.cern.ch/event/1484764/contributions/6257318/attachments/2983853/5254667/MTD_DAQ_PCD_CMS_WEEK.pdf
PR validation:
The code compiles successfully and was tested with
CondFormats/MTDObjects/test/testBTLElectronicsMapping.py,to validate the mapping and verify that the implemented functions work as expected.