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

PixelFed25 error propagation to tracking #20151

Merged
merged 12 commits into from Oct 2, 2017
Merged
2 changes: 1 addition & 1 deletion CondFormats/SiPixelObjects/src/SiPixelFrameConverter.cc
Expand Up @@ -38,7 +38,7 @@ PixelROC const * SiPixelFrameConverter::toRoc(int link, int roc) const {
stm << "Map shows no fed="<<theFedId
<<", link="<< link
<<", roc="<< roc;
LogDebug("SiPixelFrameConverter") << stm.str();
edm::LogWarning("SiPixelFrameConverter") << stm.str();

Choose a reason for hiding this comment

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

@veszpv how often is this warning issued actually?
I am just concerned that this might fill up logs at Tier0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was changed to debug as a hack, I am putting the warning back now. Should never be issued after the PR. If so, it is a good reason to investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen this show up in my test on run 300517 (LS ~600) on 4K events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to generate ~418 warning per event for uncabled FED channels.

Copy link

Choose a reason for hiding this comment

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

@veszpv, @slava77 thanks!

}
return rocp;
}
Expand Down
17 changes: 17 additions & 0 deletions DataFormats/SiPixelDetId/interface/PixelFEDChannel.h
@@ -0,0 +1,17 @@
#ifndef SiPixelDetId_PixelFEDChannel_H
#define SiPixelDetId_PixelFEDChannel_H

#include "DataFormats/Common/interface/DetSetVectorNew.h"

struct PixelFEDChannel {
unsigned int fed, link, roc_first, roc_last;
};

inline bool operator<( const PixelFEDChannel& one, const PixelFEDChannel& other) {
if (one.fed == other.fed) return one.link<other.link;
return one.fed < other.fed;
}

typedef edmNew::DetSetVector<PixelFEDChannel> PixelFEDChannelCollection;

#endif
22 changes: 22 additions & 0 deletions DataFormats/SiPixelDetId/src/classes.h
Expand Up @@ -2,3 +2,25 @@
#include <boost/cstdint.hpp>
#include "DataFormats/SiPixelDetId/interface/PXFDetId.h"

#include "DataFormats/SiPixelDetId/interface/PixelFEDChannel.h"
#include "DataFormats/Common/interface/EDCollection.h"
#include "DataFormats/Common/interface/DetSetVectorNew.h"
#include "DataFormats/Common/interface/Wrapper.h"

namespace DataFormats_PixelFEDChannel {
struct dictionary {
std::vector<PixelFEDChannel> vFC_;
edm::EDCollection<PixelFEDChannel> edcFC_;
edmNew::DetSet<PixelFEDChannel> dsFC_;
std::vector<edmNew::DetSet<PixelFEDChannel> > vdsFC_;
edmNew::DetSetVector<PixelFEDChannel> dsvFC_;
PixelFEDChannelCollection FCc_;

edm::Wrapper<std::vector<PixelFEDChannel> > vFCw_;
edm::Wrapper<edm::EDCollection<PixelFEDChannel> > edcFCw_;
edm::Wrapper<edmNew::DetSet<PixelFEDChannel> > dsFCw_;
edm::Wrapper<std::vector<edmNew::DetSet<PixelFEDChannel> > > vdsFCw_;
edm::Wrapper<edmNew::DetSetVector<PixelFEDChannel> > dsvFCw_;
edm::Wrapper<PixelFEDChannelCollection> FCcw_;
};
}
13 changes: 13 additions & 0 deletions DataFormats/SiPixelDetId/src/classes_def.xml
Expand Up @@ -7,4 +7,17 @@
<version ClassVersion="11" checksum="1218239916"/>
<version ClassVersion="10" checksum="64579558"/>
</class>
<class name="PixelFEDChannel" ClassVersion="3">
<version ClassVersion="3" checksum="2591256255"/>
</class>
<class name="std::vector<PixelFEDChannel>"/>
<class name="edm::EDCollection<PixelFEDChannel>"/>
<class name="edmNew::DetSet<PixelFEDChannel>"/>
<class name="std::vector<edmNew::DetSet<PixelFEDChannel> >"/>
<class name="edmNew::DetSetVector<PixelFEDChannel>"/>
<class name="edm::Wrapper<std::vector<PixelFEDChannel> >"/>
<class name="edm::Wrapper<edm::EDCollection<PixelFEDChannel> >"/>
<class name="edm::Wrapper<edmNew::DetSet<PixelFEDChannel> >"/>
<class name="edm::Wrapper<std::vector<edmNew::DetSet<PixelFEDChannel> > >"/>
<class name="edm::Wrapper<edmNew::DetSetVector<PixelFEDChannel> >"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

are all of these dictionaries really necessary, considering that only edmNew::DetSetVector is persisted (and only std::vector<PixelFEDChannel> is apparently also required )?

  • is EDCollection needed?
  • is vector<edmNew::DetSet needed?
  • is edmNew::DetSet needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DetSetVector is the one actually used for the bad channel propagation. So I believe EDCollection is the only line above that is not being used (explicitly or implicitly) for the time being; however, if there is no argument against keeping it, it would be a good to do so in case some user does not want to put the overhead of the DetId mapping in the event that otherwise comes with PixelFEDChannelCollection. (PixelFEDChannel is a generic class, not necessarily for what we use it here.)

</lcgdict>
2 changes: 1 addition & 1 deletion DataFormats/SiPixelRawData/src/SiPixelRawDataError.cc
Expand Up @@ -56,7 +56,7 @@ fedId_ = fedId;
void SiPixelRawDataError::setMessage() {
switch (errorType_) {
case(25) : {
errorMessage_ = "Error: ROC=25";
errorMessage_ = "Error: Disabled FED channel (ROC=25)";
break;
}
case(26) : {
Expand Down
2 changes: 2 additions & 0 deletions EventFilter/SiPixelRawToDigi/interface/ErrorChecker.h
Expand Up @@ -14,6 +14,7 @@
class FEDRawData;

class SiPixelFrameConverter;
class SiPixelFedCabling;

class ErrorChecker {

Expand All @@ -37,6 +38,7 @@ class ErrorChecker {
bool checkTrailer(bool& errorsInEvent, int fedId, int nWords, const Word64* trailer, Errors& errors);

bool checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameConverter* converter,
const SiPixelFedCabling* theCablingTree,
Word32& errorWord, Errors& errors);


Expand Down
2 changes: 2 additions & 0 deletions EventFilter/SiPixelRawToDigi/interface/PixelDataFormatter.h
Expand Up @@ -107,6 +107,8 @@ class PixelDataFormatter {
int maxROCIndex;
bool phase1;

friend class SiPixelRawToDigi;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the friend could be avoided with a function doing the bit operations of SiPixelRawToDigi.cc line 258

(itPixelError->getWord32() >> formatter.LINK_shift) & formatter.LINK_mask

Copy link
Contributor

Choose a reason for hiding this comment

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

now that the PixelDataFormatter::linkId method is available, the friend declaration is not needed.
Please remove.
Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is added only to get access to LINK_shift and LINK_mask.
Please add accessors to these or better yet define a linkId(const SiPixelRawDataError& error) const to encapsulate it fully in a class/method that actually knows what's inside

Copy link
Contributor

Choose a reason for hiding this comment

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

.. actually, looking at the additions in SiPixelRawToDigi.cc,
it seems to be better to have all of the code lines there that convert SiPixelRawDataError to a PixelFEDChannel to be added as a new method here, e.g PixelFEDChannel error25Channel(const SiPixelRawDataError&) const .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PixelFEDChannel is a generic class that can be used to list pixel channels in any code, should not even need to know what FED errors are.
Error code 25 is a firmware choice; in general, I would not find it advisable to use it in any function/class names.
Surely, this PR hard-codes this error in the unpacker, but at least it is done explicitly, localized in a place that is the most obvious choice (where we are filling special collections derived from the generic FED error collection) for any pixel experts.
Note, there is not a single bit-field decoding function in PixelDataFormatter. Defining one would mean that it should be used exclusively everywhere. Points outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is not a single bit-field decoding function in PixelDataFormatter

Every single function in PixelDataFormatter does bit-field operations either on the argument passed to it directly or on something derived from it.
And prior to this PR this was the only place where such manipulations were done.
It would be better to keep it that way and not expose bitmasks and shifts to outside functions.
[I'm just restating the points of previous comments]


int checkError(const Word32& data) const;

int digi2word( cms_uint32_t detId, const PixelDigi& digi,
Expand Down
71 changes: 52 additions & 19 deletions EventFilter/SiPixelRawToDigi/plugins/SiPixelRawToDigi.cc
Expand Up @@ -4,6 +4,7 @@
// enabled by: process.siPixelDigis.UseQualityInfo = True (BY DEFAULT NOT USED)
// 20-10-2010 Andrew York (Tennessee)
// Jan 2016 Tamas Almos Vami (Tav) (Wigner RCP) -- Cabling Map label option
// Jul 2017 Viktor Veszpremi -- added PixelFEDChannel

#include "SiPixelRawToDigi.h"

Expand Down Expand Up @@ -31,6 +32,7 @@

#include "CondFormats/SiPixelObjects/interface/SiPixelQuality.h"

#include "DataFormats/SiPixelDetId/interface/PixelFEDChannel.h"
#include "EventFilter/SiPixelRawToDigi/interface/PixelUnpackingRegions.h"
#include "FWCore/Framework/interface/ConsumesCollector.h"

Expand Down Expand Up @@ -67,6 +69,7 @@ SiPixelRawToDigi::SiPixelRawToDigi( const edm::ParameterSet& conf )
produces< edm::DetSetVector<SiPixelRawDataError> >();
produces<DetIdCollection>();
produces<DetIdCollection>("UserErrorModules");
produces<edmNew::DetSetVector<PixelFEDChannel> >();
}

// regions
Expand Down Expand Up @@ -191,6 +194,7 @@ void SiPixelRawToDigi::produce( edm::Event& ev,
auto errorcollection = std::make_unique<edm::DetSetVector<SiPixelRawDataError>>();
auto tkerror_detidcollection = std::make_unique<DetIdCollection>();
auto usererror_detidcollection = std::make_unique<DetIdCollection>();
auto disabled_channelcollection = std::make_unique<edmNew::DetSetVector<PixelFEDChannel> >();

//PixelDataFormatter formatter(cabling_.get()); // phase 0 only
PixelDataFormatter formatter(cabling_.get(), usePhase1); // for phase 1 & 0
Expand Down Expand Up @@ -241,30 +245,58 @@ void SiPixelRawToDigi::produce( edm::Event& ev,
// in the configurable error list in the job option cfi.
// Code needs to be here, because there can be a set of errors for each
// entry in the for loop over PixelDataFormatter::Errors
if(!tkerrorlist.empty() || !usererrorlist.empty()){
DetId errorDetId(errordetid);
edm::DetSet<SiPixelRawDataError>::const_iterator itPixelError=errorDetSet.begin();
for(; itPixelError!=errorDetSet.end(); ++itPixelError){
// fill list of detIds to be turned off by tracking
if(!tkerrorlist.empty()) {
std::vector<int>::iterator it_find = find(tkerrorlist.begin(), tkerrorlist.end(), itPixelError->getType());
if(it_find != tkerrorlist.end()){
edm::DetSet<SiPixelRawDataError>::const_iterator itPixelError=errorDetSet.begin();
std::vector<PixelFEDChannel> disabledChannelsDetSet;

for(; itPixelError!=errorDetSet.end(); ++itPixelError){
Copy link
Contributor

Choose a reason for hiding this comment

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

for (auto const& aPixelError : errorDetSet)

// For the time being, we extend the error handling functionality with ErrorType 25
// In the future, we should sort out how the usage of tkerrorlist can be generalized
if (itPixelError->getType()==25) {
assert(itPixelError->getFedId()==fedId);
const sipixelobjects::PixelFEDCabling* fed = cabling_->fed(fedId);
if (fed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be fed != nullptr (surprising that clang-tidy did not flag this)

cms_uint32_t linkId = (itPixelError->getWord32() >> formatter.LINK_shift) & formatter.LINK_mask;
Copy link
Contributor

Choose a reason for hiding this comment

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

this [starting from if (itPixelError->getType()==25] looks like a hack for a work that is designed to be done by the formatter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, this is the right place to populate the disabled_channelcollection staying consistent with the earlier code (and avoid a potential double-counting of FED 25 errrors)

Copy link
Contributor

Choose a reason for hiding this comment

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

it's fine to populate the disable collection here.
The point of the comment was that SiPixelRawToDigi should not do the word decoding and it instead is a functionality to be done by the formatter.

const sipixelobjects::PixelFEDLink* link = fed->link(linkId);
if (link) {
// The "offline" 0..15 numbering is fixed by definition, also, the FrameConversion depends on it
// in contrast, the ROC-in-channel numbering is determined by hardware --> better to use the "offline" scheme
PixelFEDChannel ch = {fed->id(), linkId, 25, 0};
for (unsigned int iRoc=1; iRoc<=link->numberOfROCs(); iRoc++) {
const sipixelobjects::PixelROC * roc = link->roc(iRoc);
if (roc->idInDetUnit()<ch.roc_first) ch.roc_first=roc->idInDetUnit();
if (roc->idInDetUnit()>ch.roc_last) ch.roc_last=roc->idInDetUnit();
}
disabledChannelsDetSet.push_back(ch);
}
}
} else {
// fill list of detIds to be turned off by tracking
if(!tkerrorlist.empty()) {
std::vector<int>::iterator it_find = find(tkerrorlist.begin(), tkerrorlist.end(), itPixelError->getType());
if(it_find != tkerrorlist.end()){
tkerror_detidcollection->push_back(errordetid);
}
}
}
// fill list of detIds with errors to be studied
if(!usererrorlist.empty()) {
std::vector<int>::iterator it_find = find(usererrorlist.begin(), usererrorlist.end(), itPixelError->getType());
if(it_find != usererrorlist.end()){
usererror_detidcollection->push_back(errordetid);
}
}

// fill list of detIds with errors to be studied
if(!usererrorlist.empty()) {
std::vector<int>::iterator it_find = find(usererrorlist.begin(), usererrorlist.end(), itPixelError->getType());
if(it_find != usererrorlist.end()){
usererror_detidcollection->push_back(errordetid);
}
}

} // loop on DetSet of errors

if (disabledChannelsDetSet.size()>0) {
disabled_channelcollection->insert(errordetid, disabledChannelsDetSet.data(), disabledChannelsDetSet.size());
}
}
}
}
}

} // if error assigned to a real DetId
} // loop on errors in event for this FED
} // if errors to be included in the event
} // loop on FED data to be unpacked

if(includeErrors) {
edm::DetSet<SiPixelRawDataError>& errorDetSet = errorcollection->find_or_insert(dummydetid);
Expand All @@ -289,5 +321,6 @@ void SiPixelRawToDigi::produce( edm::Event& ev,
ev.put(std::move(errorcollection));
ev.put(std::move(tkerror_detidcollection));
ev.put(std::move(usererror_detidcollection), "UserErrorModules");
ev.put(std::move(disabled_channelcollection));
}
}
1 change: 1 addition & 0 deletions EventFilter/SiPixelRawToDigi/plugins/SiPixelRawToDigi.h
Expand Up @@ -22,6 +22,7 @@ class SiPixelFedCabling;
class SiPixelQuality;
class TH1D;
class PixelUnpackingRegions;
class PixelDisabledFEDChannel;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed here? there is no other mentioning of PixelDisabledFEDChannel in this file


class SiPixelRawToDigi : public edm::stream::EDProducer<> {
public:
Expand Down
33 changes: 18 additions & 15 deletions EventFilter/SiPixelRawToDigi/src/ErrorChecker.cc
Expand Up @@ -25,6 +25,7 @@ namespace {
constexpr int PXID_bits = 8;
constexpr int ADC_bits = 8;
constexpr int OMIT_ERR_bits = 1;
constexpr int AUTO_MASK_bits = 1;

constexpr int CRC_shift = 2;
constexpr int ADC_shift = 0;
Expand All @@ -33,6 +34,7 @@ namespace {
constexpr int ROC_shift = DCOL_shift + DCOL_bits;
constexpr int LINK_shift = ROC_shift + ROC_bits;
constexpr int OMIT_ERR_shift = 20;
constexpr int AUTO_MASK_shift = 20;

constexpr cms_uint32_t dummyDetId = 0xffffffff;

Expand Down Expand Up @@ -110,13 +112,16 @@ bool ErrorChecker::checkTrailer(bool& errorsInEvent, int fedId, int nWords, cons
return fedTrailer.moreTrailers();
}

bool ErrorChecker::checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameConverter* converter, Word32& errorWord, Errors& errors)
bool ErrorChecker::checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameConverter* converter,
const SiPixelFedCabling* theCablingTree, Word32& errorWord, Errors& errors)
{
int errorType = (errorWord >> ROC_shift) & ERROR_mask;
if likely(errorType<25) return true;

switch (errorType) {
case(25) : {
CablingPathToDetUnit cablingPath = { unsigned(fedId), (errorWord >> LINK_shift) & LINK_mask, 1 };
if (!theCablingTree->findItem(cablingPath)) return false;
LogDebug("")<<" invalid ROC=25 found (errorType=25)";
errorsInEvent = true;
break;
Expand Down Expand Up @@ -145,6 +150,15 @@ bool ErrorChecker::checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameCo
}
case(30) : {
LogDebug("")<<" TBM error trailer (errorType=30)";
int StateMatch_bits = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

please make these const and unsigned ints in a follow up pr.

int StateMatch_shift = 8;
uint32_t StateMatch_mask = ~(~uint32_t(0) << StateMatch_bits);
int StateMatch = (errorWord >> StateMatch_shift) & StateMatch_mask;
if( StateMatch!=1 && StateMatch!=8 ) {
LogDebug("")<<" FED error 30 with unexpected State Bits (errorType=30)";
return false;
}
if( StateMatch==1 ) errorType = 40; // 1=Overflow -> 40, 8=number of ROCs -> 30
errorsInEvent = true;
break;
}
Expand All @@ -157,15 +171,6 @@ bool ErrorChecker::checkROC(bool& errorsInEvent, int fedId, const SiPixelFrameCo
};

if(includeErrors) {
// check to see if overflow error for type 30, change type to 40 if so
if(errorType==30) {
int StateMach_bits = 4;
int StateMach_shift = 8;
uint32_t StateMach_mask = ~(~uint32_t(0) << StateMach_bits);
int StateMach = (errorWord >> StateMach_shift) & StateMach_mask;
if( StateMach==4 || StateMach==9 ) errorType = 40;
}

// store error
SiPixelRawDataError error(errorWord, errorType, fedId);
cms_uint32_t detId;
Expand Down Expand Up @@ -232,16 +237,15 @@ cms_uint32_t ErrorChecker::errorDetId(const SiPixelFrameConverter* converter,

switch (errorType) {
case 25 : case 30 : case 31 : case 36 : case 40 : {
// set dummy values for cabling just to get detId from link if in Barrel
// set dummy values for cabling just to get detId from link
cabling.dcol = 0;
cabling.pxid = 2;
cabling.roc = 1;
cabling.link = (word >> LINK_shift) & LINK_mask;

DetectorIndex detIdx;
int status = converter->toDetector(cabling, detIdx);
if (status) break;
if(DetId(detIdx.rawId).subdetId() == static_cast<int>(PixelSubdetector::PixelBarrel)) return detIdx.rawId;
if (!status) return detIdx.rawId;
break;
}
case 29 : {
Expand Down Expand Up @@ -274,8 +278,7 @@ cms_uint32_t ErrorChecker::errorDetId(const SiPixelFrameConverter* converter,
cabling.link = chanNmbr;
DetectorIndex detIdx;
int status = converter->toDetector(cabling, detIdx);
if (status) break;
if(DetId(detIdx.rawId).subdetId() == static_cast<int>(PixelSubdetector::PixelBarrel)) return detIdx.rawId;
if (!status) return detIdx.rawId;
break;
}
case 37 : case 38: {
Expand Down
2 changes: 1 addition & 1 deletion EventFilter/SiPixelRawToDigi/src/PixelDataFormatter.cc
Expand Up @@ -183,7 +183,7 @@ void PixelDataFormatter::interpretRawData(bool& errorsInEvent, int fedId, const

if ( (nlink!=link) | (nroc!=roc) ) { // new roc
link = nlink; roc=nroc;
skipROC = likely(roc<maxROCIndex) ? false : !errorcheck.checkROC(errorsInEvent, fedId, &converter, ww, errors);
skipROC = likely(roc<maxROCIndex) ? false : !errorcheck.checkROC(errorsInEvent, fedId, &converter, theCablingTree, ww, errors);
if (skipROC) continue;
rocp = converter.toRoc(link,roc);
if unlikely(!rocp) {
Expand Down
Expand Up @@ -5,6 +5,7 @@
outputCommands = cms.untracked.vstring(
'keep DetIdedmEDCollection_siStripDigis_*_*',
'keep DetIdedmEDCollection_siPixelDigis_*_*',
'keep PixelFEDChanneledmNewDetSetVector_siPixelDigis_*_*',
'keep *_siPixelClusters_*_*',
'keep *_siStripClusters_*_*',
'keep *_clusterSummaryProducer_*_*')
Expand All @@ -14,6 +15,7 @@
outputCommands = cms.untracked.vstring(
'keep DetIdedmEDCollection_siStripDigis_*_*',
'keep DetIdedmEDCollection_siPixelDigis_*_*',
'keep PixelFEDChanneledmNewDetSetVector_siPixelDigis_*_*',
'keep *_siPixelClusters_*_*',
'keep *_siStripClusters_*_*',
'keep ClusterSummary_clusterSummaryProducer_*_*')
Expand Down