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

New FED modes handled in SiStrip unpacker #12157

Merged
merged 38 commits into from Nov 10, 2015

Conversation

forthommel
Copy link
Contributor

The FED FE packet codes has been reorganized to leave room to new modes (e.g. VR 10bit, ZS 8bit, ...). The SiStripRawToDigiUnpacker was hence modified to match these changes.
In an attempt to restore the previous unpacking behaviour a boolean has been introduced in the siStripDigis PSet to switch between the legacy modes and the new ones (siStripDigis.LegacyUnpacker = cms.bool(False)).
For more information on the requirements and new specifications, see https://its.cern.ch/jira/browse/CMSTRACK-127

amagnan and others added 30 commits April 20, 2015 15:39
…define the legacy behaviour and specify the run number when FEDs were upgraded
…s as it is not used afterwards ; added new FED modes in SiStripEnumsAndStrings
case READOUT_MODE_SCOPE: rawdigi_ = true; break;
case READOUT_MODE_ZERO_SUPPRESSED: rawdigi_ = false; break;
case READOUT_MODE_ZERO_SUPPRESSED_FAKE: rawdigi_ = false; break;
//case READOUT_MODE_ZERO_SUPPRESSED_CMOVERRIDE: rawdigi_ = false; break;
Copy link
Contributor

Choose a reason for hiding this comment

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

drop commented out code; or add some comments why it should stay here

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 9, 2015

@slava77
Copy link
Contributor

slava77 commented Nov 9, 2015

Now there are no changes in the fwLite based comparisons of reco products.

(something I missed to notice in the last comparisons) all comparisons of run1 and run2 pp processing now have differences in the DQM error plots:
these are from workflow 4.53 (Run2012B Photon PD)
wf4 53_any_apv

wf4 53_feds_badbuf

this one has 1 event with ~450 bad FEDs. It could be that the rest are following from this issue.

Are these changes in DQM plots expected?

@slava77
Copy link
Contributor

slava77 commented Nov 9, 2015

@forthommel please comment on significance of these changes in the DQM.
It seems like we are pretty close otherwise to be able to integrate this PR.

@venturia
Copy link
Contributor

venturia commented Nov 9, 2015

How sure are we that exactly the same events have been processed in the two cases? In the Tracker there are real occurences of very rare events for which all the FED channels have errors. The usual interpretation is that there is a glitch in the APV emulator and when the pipeline address declared by the APVe is compared to the one declared by the real APV's there is a mismatch and the full tracker is blamed. This change of the code is not expected to change this behaviour, I think, therefore it would be useful to check if the event with all the channels in error in one case has been processed also in the other case. If the DQM histograms are made available it would be possible to check the type of the error and confirm if it is of the expected type.

@slava77
Copy link
Contributor

slava77 commented Nov 9, 2015

@venturia
If different events entered, there would be a lot of changes in the comparisons of more or less all event products.
The kind of differences posted occur for all data workflows, and all of these workflows have no other differences in higher level objects (e.g., tracks are the same)

@mandrenguyen
Copy link
Contributor

@slava77 @davidlange6
The claim at the heavy ion run prep meeting was that this is purely related to a change of headers not being fully propagated to the DQM (correct me if that's wrong, @forthommel ). The point was raised that 755, including this PR, is needed for tests to be done on Wednesday. If this cannot be fixed in time, we would like to have an intermediate release cut, that can be patched very shortly afterwords.

@abaty
Copy link
Contributor

abaty commented Nov 9, 2015

When I run these tests, the number of track entries is the same for both a fresh release and a release with this PR included; the event sample being processed should be the same.

@icali
Copy link
Contributor

icali commented Nov 9, 2015

Could you point to us the code of the APV emulator? Do you know the sequence that is run there?
I would not be surprised if there is a mismatch because that part of the code was not updated. However, why should it be for only one event?

As Matt suggested, if we don't find a solution in the next few hours, cold it be create a release to be patched anyway later? It would be used only online and without sending any data at T0 (just for online testing).

@venturia
Copy link
Contributor

venturia commented Nov 9, 2015

I have understood that the difference has been observed in standard pp ZS data. The results should be identical. Are the DQM files of the two tests available anywhere?
If it is true that the difference is due only to one event, are we able to skim that event? Are the errors in the old processing or in the new processing?

                          Andrea

On Nov 9, 2015, at 20:22 , Ivan Amos Cali <notifications@github.commailto:notifications@github.com>
wrote:

Could you point to us the code of the APV emulator? Do you know the sequence that is run there?
I would not be surprised if there is a mismatch because that part of the code was not updated. However, why should it be for only one event?

As Matt suggested, if we don't find a solution in the next few hours, cold it be create a release to be patched anyway later? It would be used only online and without sending any data at T0 (just for online testing).


Reply to this email directly or view it on GitHubhttps://github.com//pull/12157#issuecomment-155164328.

@abaty
Copy link
Contributor

abaty commented Nov 9, 2015

@venturia

I've placed the DQM root files originating from workflow 4.53 for both the original and modified versions in the following directory:
/afs/cern.ch/user/a/abaty/public/PR_12157_DQM

@venturia
Copy link
Contributor

venturia commented Nov 9, 2015

Thanks!

there are two events out of 100 where all the FEDs have been found with "corrupted buffers". You can see that in the plot in SiStrip/Run summary/ReadoutView/FED/nFEDCorruptBuffers
Why these events are reconstructed this way with the new unpacker I don't know. Unless it is a shallow problem only at the DQM level, this means that no data in unpacked in those two events (when an error is detected in a FED channel the data from that channel are suppressed). Since this happened in 2% of the events it is better to investigate further.

If you want to skim those two events you can use this filter:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/DPGAnalysis/SiStripTools/python/fedbadmodulefilter_cfi.py
to select events with more than badModThr channels in error. It can be run after the unpacker

                                                 Andrea

On Nov 9, 2015, at 21:50 , abaty <notifications@github.commailto:notifications@github.com>
wrote:

@venturiahttps://github.com/venturia

I've placed the DQM root files originating from workflow 4.53 for both the original and modified versions in the following directory:
/afs/cern.ch/user/a/abaty/public/PR_12157_DQMhttp://cern.ch/user/a/abaty/public/PR_12157_DQM


Reply to this email directly or view it on GitHubhttps://github.com//pull/12157#issuecomment-155188902.

READOUT_MODE_ZERO_SUPPRESSED=0xA,
READOUT_MODE_ZERO_SUPPRESSED_LITE=0xC,
READOUT_MODE_ZERO_SUPPRESSED_FAKE=0xB,
Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be a mismatch here between the enum value 11 -> fake mode, 13=LITE8_BOTBOT_CMO, and the method DataFormats/SiStripCommon/interface/ConstantsForHardwareSystems.h line 143 where LITE8_BB_CMO is = 11 and 13 is not given to anything....

@amagnan
Copy link
Contributor

amagnan commented Nov 10, 2015

The CorruptBuffer check is defined in the DQM code as below:

bool FEDErrors::fillCorruptBuffer(const sistrip::FEDBuffer* aBuffer)

So it uses methods that are from the FEDBuffer class which is also used by the unpacker. Hence anything flagged by DQM should also result in not being unpacked.... I see the code has changes in the packetmodes for example, so maybe something has not been propagated properly to one of these buffer checks in link above ?

davidlange6 added a commit that referenced this pull request Nov 10, 2015
New FED modes handled in SiStrip unpacker
@davidlange6 davidlange6 merged commit f8c92bb into cms-sw:CMSSW_7_5_X Nov 10, 2015
@abaty
Copy link
Contributor

abaty commented Nov 10, 2015

@amangnan

Hi, thanks for your help. I had noticed this discrepancy in this ConstantsForHardwareSystems.h and SiStripFEDBufferComponents.h, but when I attempted to change ConstantsForHardwareSystems to match I saw no change in the behavior (although my changes might not have been correctly applied). We are currently digging into the CorruptBuffer check to find problems, and there does seem to be some issue with grabbing the correct packet mode.

forthommel added a commit to forthommel/cmssw that referenced this pull request Nov 11, 2015
slava77 pushed a commit to slava77/cmssw that referenced this pull request Nov 13, 2015
@forthommel forthommel deleted the CMSTRACK127_unpacker75 branch February 27, 2019 08:50
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