-
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
Totem Timing detectors: updated mapping for future rereco of dedicated run data #24683
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24683/6618 |
A new Pull Request was created by @nminafra (Nicola Minafra) for master. It involves the following packages: CondFormats/CTPPSReadoutObjects @perrotta, @tocheng, @cmsbuild, @franzoni, @slava77, @ggovi, @pohsun, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
@@ -343,7 +343,7 @@ void RawToDigiConverter::run(const VFATFrameCollection &coll, const TotemDAQMapp | |||
|
|||
const TotemDAQMapping::TotemTimingPlaneChannelPair SWpair = mapping.getTimingChannel( totemSampicFrame.getHardwareId() ); | |||
// for FW Version > 0 plane and channel are encoded in the dataframe | |||
if ( totemSampicFrame.getFWVersion() == 0 ) // Mapping not present in HW, read from SW | |||
if ( totemSampicFrame.getFWVersion() < 0x30 ) // Mapping not present in HW, read from SW for FW versions < 3.0 |
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.
is there a history document for this?
It may help to have it linked inline to make the version number related logic less magic.
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.
Not at the moment, we'll try to provide it.
Here a small mapping related summary:
FW 0x00 was the first test, without any mapping information encoded in the data format.
FW 0x2n (in particular 0x23 that was used for the data taking) are firmware versions with mapping information: the mapping in the FPGA overruled the software for on-line debugging*. Now, it is better to use the mapping in the software for easier history and debugging.
In case of future version (>0x30) the mapping will be read from the FPGA for the same reasons as above.
*: as a reminder, we had one shot. One week of data taking without any way to double check the mapping before (with data).
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.
Do I understand correctly, that no data actually existed for production processing with getFWVersion() == 0
and also that no data exists yet with version >= 0x30
?
In this case everything was so far processed in the else // Mapping read from HW, checked by SW
block below.
It's worth taking a look at the data with these detectors present to have a sanity check.
Please suggest an input file.
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.
We used FW == 0 in some test runs without beam or anyway without any physical importance.
FW >= 0x30 is only for future compatibility
The 2 FW versions used in production were:
0x23 for the 90m alignment run: 318529 for example, or anyway the whole fill 6837
0x24 for the 90m dedicated run: fills 6877, 6879, 6881, 6882, 6884, 6885, 6890, 6891, 6892
a good run is for example 319270
NOTE: FW 0x24 doesn't change the data format, it was a small bug fix
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.
a good run is for example 319270
do you happen to have a pointer to a /RAW file from this run?
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.
Actually I'm having problems with DAS, but you can use this run instead that I had in one of my previous tests:
/store/data/Run2018B/TOTEM3/RAW/v1/000/319/176/00000/009A26F8-6E7F-E811-ADB1-FA163EF96F9B.root
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.
I don't find this file on disk (or anything from TOTEM3 dataset).
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.
@slava77 Honestly, I wouldn't know what to answer. the files are listed on DAS and we used them to test the mapping a few weeks ago and now I'm also not able to access them.
https://cmsweb.cern.ch/das/request?view=list&limit=150&instance=prod%2Fglobal&input=file+dataset%3D%2FTOTEM3%2FRun2018B-v1%2FRAW+run%3D319176
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.
I guess the relevant data workflows are not tested. As is, this PR will likely end up in 10_4_X. BTW, what is the status of making this mapping to the database? |
@slava77 Thanks for pointing this out. The re reco for this run is not planned yet (to my understanding); therefore we'll see in the future if we need a backport. Regarding the database, at the moment there is no planning for future data taking with those detectors (the front-end was physically removed from the tunnel...) therefore there is no plan to move this particular mapping to the database. In case, this priority can be rediscussed with the coordinators. |
+1 |
+1
It seems like this commissioning data is good enough to check that the code logic is OK |
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
@nminafra |
@slava77 the readout of the totem timing was physically removed from the tunnel during TS2. Of course, the pps timing (diamonds, not touched by this pr) is still there, but roman pots will anyways not be inserted during heavy ion runs. |
@fabiocos |
+1 it does not look to require backport for data taking |
This PR updates the mapping for Totem timing detectors for dedicated 90m run.
In more detail: the internal cabling of some detectors was not corresponding to the tables. We used the tomography with strip detectors to reverse engineer the correct correspondence.
NOTES:
in a previous PR (#23198) was mentioned "not all channels are connected, therefore it is not possible to double check all channels."
Now all channels have been checked and corrected.
Thanks
@jan-kaspar @forthommel