-
Notifications
You must be signed in to change notification settings - Fork 13
GE2/1 compatilibility #132
GE2/1 compatilibility #132
Conversation
Is this targeting "working now with setups that may be using legacy", if so, needs to target |
|
||
/*! \brief Mapping from elink index to the 3 registers addresses in the GBT. | ||
*/ | ||
constexpr std::array<std::array<uint16_t, 3>, 10> ELINK_TO_REGISTERS { |
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.
Unrelated to this PR; but now would be a good time to add a GE2/1 heading to the guide similar to this one:
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.
unrelated to this PR, but related to the comment, now would be a good time to convert the guide to a paginated format, a la gitbook
include/utils.h
Outdated
timeout+vfatErrs.timeout, | ||
axi_strobe+vfatErrs.axi_strobe, | ||
sum+vfatErrs.sum, | ||
nTransactions+vfatErrs.nTransactions); |
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.
this is in the legacy branch already but one comment here is that I overlooked the possibility that overflow could occur in this addition here. That is not presently handled and it might be better to change to:
return slowCtrlErrCntVFAT(
overflowTest(crc,vfatErrs.crc),
overflowTest(packet,vfatErrs.packet),
...
...
overflowTest(nTransactions,vfatErrs.nTransactions));
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.
This comment is still relevant I think.
uint32_t ohTrigRateAddr[12][25]; //idx 0->23 VFAT counters; idx 24 overall rate | ||
for (int ohN = 0; ohN < 12; ++ohN) { | ||
uint32_t ohTrigRateAddr[amc::OH_PER_AMC][oh::VFATS_PER_OH + 1]; //idx 0->23 VFAT counters; idx 24 overall rate | ||
for (unsigned int ohN = 0; ohN < amc::OH_PER_AMC; ++ohN) { |
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.
Some caution should be used here when designing a loop over the number of OH's; and it's been previously overlooked (now is probably a good time to address this). Consider the case where you're running on a 4OH FW version; or a GLIB with a quad/octal SFP FMC added, the number of OH's is not 12. Attempting to loop over amc::OH_PER_AMC
may actually cause an error. For example consider lines 651 & 652 below:
sprintf(regBuf,"GEM_AMC.OH.OH%i.FPGA.TRIG.CNT.VFAT%i_SBITS",ohN,vfat);
ohTrigRateAddr[ohN][vfat] = getAddress(la, regBuf); ohTrigRateAddr[ohN][vfat] = getAddress(la, regBuf);
The getAddress
call here would generate an error.
When looping over the number of OH's for this reason we may want to only loop over the number of OH's in the actual FW. This of course requires a reg read to get this value; but perhaps it's also retrievable from the xml...?
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.
Alternatively amc::OH_PER_AMC
could be a constant set at compile time; and be platform (e.g. uTCA shelf/test stand) dependent.
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.
My first comment above should be considered a "global comment" applied everywhere.
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'd consider this as a separate issue as it is not really closely related to this PR. Also, IMHO a rule of thumb would be to always have a proper address table for 12 OHs in LMDB on all test stands.
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.
IMHO a rule of thumb would be to always have a proper address table for 12 OHs in LMDB on all test stands.
How feasible is this for AMC's not the CTP7? I don't know enough about how we are doing the GLIB to know this myself.
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.
OH_PER_AMC
(number of supported OHs) is a property of the FW, so @mexanick's point is feasible.
One could couple the LMDB fixed to 12, with any arrays fixed to 12 (or better yet, use dynamic containers), and in an implementation with fewer supported OHs, you have a check in whatever function so that you'll know what comes out is the "right size"
However, I agree that this is a separate issue, unless this code is targeting develop
rather than the current release tree, as develop
will be picking up the templating and this is much better suited to be addressed at that time.
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.
Current target branch is develop
src/calibration_routines.cpp
Outdated
@@ -641,12 +642,12 @@ void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t | |||
} //End loop over optohybrids | |||
|
|||
//Get the SBIT Rate Monitor Address | |||
uint32_t ohTrigRateAddr[12][25]; //idx 0->23 VFAT counters; idx 24 overall rate | |||
for (int ohN = 0; ohN < 12; ++ohN) { | |||
uint32_t ohTrigRateAddr[amc::OH_PER_AMC][oh::VFATS_PER_OH + 1]; //idx 0->23 VFAT counters; idx 24 overall rate |
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.
Trailing comment here needs to be updated.
@@ -630,7 +631,7 @@ void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t | |||
{ | |||
//Then mask all other channels except for channel ch | |||
uint32_t notmask = ~vfatmask[ohN] & 0xFFFFFF; |
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.
Correct me if I'm wrong but doesn't this also need an update, e.g.:
uint32_t notmask = ~vfatmask[ohN] & 0xFFFFFF
The 0xFFFFFF
is now specific to ge11
and ge21
so this would need a constant no?
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.
No, it is OK here, we never touch the chips above the oh::VFATS_PER_OH
So having it here as a maximum value constant is fine
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.
No, it is OK here, we never touch the chips above the oh::VFATS_PER_OH
So having it here as a maximum value constant is fine
I guess my point here, and also with this comment is to ensure a standardization through the repo where all constants/loop variables/etc... of this nature are specified in a single location.
Recall the issues we were debugging in the CSC office in 904 earlier this week? We were using one namespace protected constant to get the VFAT mask but in another location we were looping over VFATs beyond that. I think enforcing namespace protected constants in all cases like this would avoid possible issues in the future.
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 fully agree that such constant should specified in a single location.
However, for this precise constant, what about removing it? I think it is only used in loops between 0 and oh::VFATS_PER_OH
so it would be safe and we would get rid of one constant in both GE1/1 and GE2/1 code.
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.
If you get rid of this constant how do you determine notmask
? Maybe I misunderstood the proposal. Using only ~vfatmask[ohN]
will flip all bits no, e.g. bits [25, 32]. This could be problematic no?
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.
As long as notmask
is only used in for
-loops it wouldn't be problematic. If it is used in other contexts then I fully support the constant in hw_constants.h
.
I think the comment also applies to ohMask
(line 610) :
ctp7_modules/src/calibration_routines.cpp
Lines 606 to 615 in d384cbb
void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t *outDataTrigRatePerVFAT, uint32_t *outDataTrigRateOverall, uint32_t ch, uint32_t dacMin, uint32_t dacMax, uint32_t dacStep, std::string scanReg, uint32_t ohMask=0xFFF) | |
{ | |
char regBuf[200]; | |
// Check that OH mask does not exceeds 0xFFF | |
if (ohMask > 0xFFF) { | |
LOGGER->log_message(LogManager::ERROR, "sbitRateScan supports only up to 12 optohybrids per CTP7"); | |
sprintf(regBuf,"sbitRateScan supports only up to 12 optohybrids per CTP7"); | |
la->response->set_string("error",regBuf); | |
return; | |
} |
@@ -670,10 +671,10 @@ void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t | |||
LOGGER->log_message(LogManager::INFO, stdsprintf("Setting %s to %i for all optohybrids in 0x%x",scanReg.c_str(),dacVal,ohMask)); | |||
|
|||
//Set the scan register value | |||
for (int ohN = 0; ohN < 12; ++ohN) { | |||
for (unsigned int ohN = 0; ohN < amc::OH_PER_AMC; ++ohN) { | |||
if ((ohMask >> ohN) & 0x1) { | |||
uint32_t notmask = ~vfatmask[ohN] & 0xFFFFFF; |
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.
Same comment as here basically apply this everywhere.
|
||
//bits [10:0] is the address of the cluster | ||
//bits [14:12] is the cluster size | ||
//bits 15 and 11 are not used | ||
uint32_t thisCluster = readRawAddress(addrSbitCluster[cluster], la->response); | ||
int clusterSize = (thisCluster >> 12) & 0x7; | ||
unsigned int clusterSize = (thisCluster >> 12) & 0x7; | ||
uint32_t sbitAddress = (thisCluster & 0x7ff); | ||
bool isValid = (sbitAddress < 1536); //Possible values are [0,(24*64)-1] | ||
int vfatObserved = 7-int(sbitAddress/192)+int((sbitAddress%192)/64)*8; |
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 the sbit clustering and addressing system in GE1/1 the same as GE2/1? e.g. will these formulas to unpack the cluster word work for GE2/1?
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.
For sure there are no longer 1536, see line 869, possible cluster addresses; so this would also need to be protected as a constant, e.g.
ge11::MAX_SBIT_ADDR_PER_OH
ge21::MAX_SBIT_ADDR_PER_OH
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 see we added the namespace protected constants which is good; but I think my first comment is still outstanding, e.g.:
Is the sbit clustering and addressing system in GE1/1 the same as GE2/1? e.g. will these formulas to unpack the cluster word work for GE2/1?
Could we check with @andrewpeck that this is the case; if so then no issue. Otherwise we will obviously need to handle this.
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.
Hi Brian, the cluster formula is the same at the moment (which means there is a wasted bit).. eventually we could change it to regain the bit for other purposes but compatibility seems to be a better target right now rather than optimizing bandwidth.. we also have no identified use for the extra bits right now
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.
some additional differences between GE1/1 and GE2/1 have been overlooked (e.g. sbit clustering); need additional gemType
specific constants. Target branch unclear.
It’s possible I missed other blocks in the code due to not appearing here. Especially in the calibration toolkit. |
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.
Very few comments in addition to @bdorney's that I fully agree with.
@bdorney @lpetre-ulb your comments has been addressed except first one from @bdorney which I believe deserves a separate issue |
if (syncErrCnt > 0x0) { //Case: nonzero sync errors, mask this vfat | ||
mask = mask + (0x1 << vfatN); | ||
if (syncErrCnt == 0x0) { //Case: zero sync errors, unmask this vfat | ||
mask = mask - (0x1 << vfatN); |
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.
To increase reliability of the output of this function we could; in the case that syncErrCnt == 0x0
add some slow control transaction on this VFAT. This could be done with repeatedRegRead
(with a small number of reads to keep this function "fast") or it could be done with a one-off read of say of the hardware version register or something similar.
However I'm fine with letting this go and not being addressed in this PR.
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.
Also, nice solution.
After another pass on this PR I think the ctp7_modules/include/amc/sca_enums.h Lines 211 to 267 in 2c5d65f
Also I would suggest a look at all I think we should address this given the board design has been completed in this PR. Apologies for having missed this earlier. |
src/gbt.cpp
Outdated
@@ -154,7 +174,7 @@ void writeGBTPhase(const RPCMsg *request, RPCMsg *response) | |||
|
|||
bool writeGBTPhaseLocal(localArgs *la, const uint32_t ohN, const uint32_t vfatN, const uint8_t phase) | |||
{ | |||
LOGGER->log_message(LogManager::INFO, stdsprintf("Writing the VFAT #%u phase of OH #%u.", vfatN, ohN)); | |||
//LOGGER->log_message(LogManager::INFO, stdsprintf("Writing %u to the VFAT #%u phase of OH #%u.", phase, vfatN, ohN)); |
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 this was moved to line 194? If so could we delete this commented line?
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.
Also, do we want to keep the INFO
log level for this message since it the phase scans produce a lot of them?
What about the general semantic of log levels? How can we change the rpcsvc
log level on the fly or at least when rpcsvc
is started (e.g. one may want to trace all read and write calls during a debugging session)? There is no API to change it and it is fixed at compile time... I guess this is how out of the scope of this PR though.
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.
Yeah this probably needs it's own issue. Not something for this PR.
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 think the log level should be set by sysmgr, but I'm not 100% sure.
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.
Currently it is set in rpcsvc
and if the level given to log_message
is below that value, the message is not even sent to the syslog
daemon. I don't see how the sysmgr
could help in this case... I'm going to open an issue.
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.
Some comments still outstanding and a look at the sca_enums.h
is needed I think.
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.
Some changes are still required, particularly in the SCA enums that I also completely overlooked during my first review. :-(
I think all the aliases should go into a GEM_VARIANT
protected namespace since the mapping to ADC channels and GPIOs are differents. Once (or before) the ADC readout functions will be merged in the main branch some work will be also be required to make it compatible with GE2/1.
6aa27e7
to
d17a5fc
Compare
I agree that SCA enums has to be changed, but I don't have any info on this w.r.t. GE2/1. Can anyone point me to where I can get it? |
@mexanick, you should be able to find all the information in the GE2/1 OH specification and/or on the GE2/1 OH schematic : http://padley.rice.edu/cms/OH_GE21/ohdesign.html |
Should these changes still go to develop or you prefer them to go under legacy/1.1.X branch? |
…rovements w.r.t warnings and gbt logging
…nly those where the sync error counter is 0
d17a5fc
to
7fe844c
Compare
Changed the target and performed a rebase |
Approving these changes per discussion in meeting; but if @mexanick you could open a new issue on the repo to address the SCA enum issues we discussed for GE1/1 vs. GE2/1 issues. |
@@ -54,15 +54,15 @@ struct vfat3DACAndSize{ | |||
} | |||
}; | |||
|
|||
/*! \fn std::unordered_map<uint32_t, uint32_t> setSingleChanMask(int ohN, int vfatN, unsigned int ch, localArgs *la) | |||
/*! \fn std::unordered_map<uint32_t, uint32_t> setSingleChanMask(unsigned int ohN, int vfatN, unsigned int ch, localArgs *la) |
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.
any reason ohN
is changed to unsigned but not vfatN
? (here and elsewhere)
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.
no good reason - has to be standardized indeed. However a further refactoring should be done under a separate PR (also including other comments like that)
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.
in that case i would suggest to either do all the changes for this PR, or none of the changes for this PR, e.g., revert the changes that have been introduced that are only partial...
@@ -22,6 +22,12 @@ namespace amc { | |||
constexpr uint32_t OH_PER_AMC = 12; ///< The number of OptoHybrids per AMC. | |||
} | |||
|
|||
/*! \brief GE2/1 specific namespace. | |||
*/ |
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.
should standardize a type for these constants probably...
|
||
/*! \brief Mapping from elink index to the 3 registers addresses in the GBT. | ||
*/ | ||
constexpr std::array<std::array<uint16_t, 3>, 10> ELINK_TO_REGISTERS { |
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.
unrelated to this PR, but related to the comment, now would be a good time to convert the guide to a paginated format, a la gitbook
uint32_t mask = 0x0; | ||
for (int vfatN=0; vfatN<24; ++vfatN) { //Loop over all vfats | ||
uint32_t mask = 0xffffff; //Start with all vfats masked for max VFAT/GEB amount | ||
for (unsigned int vfatN=0; vfatN<oh::VFATS_PER_OH; ++vfatN) { //Loop over all vfats |
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.
(here and elsewhere) why the affinity for unsigned int
rather than uintX_t
(as appropriate) or simply unsigned
?
Merging as is, further refactoring foreseen |
Addressing #131
Description
The changes should be transparent and does not require changes in the other software for the GE1/1 usage
Types of changes
Motivation and Context
The hardware constants has to be defined in one place and corresponding compile-time switch has to be used
How Has This Been Tested?
Tested with the GE2/1 integration stand setup
Screenshots (if appropriate):
Checklist: