Feature sbit rate multilink

mexanick commented 5 years ago

Implementing #101

Description

All the (not masked) optohybrids will be engaged in the calibration. In summary, up to 12 optohybrids are supported. The caller code should be modified to provide the ohMask instead of ohN and maskOh(vfatMask), the vfatMask will be determined automatically at runtime as suggested in the initial issue. Returning data types will be similar to the single link version but expanded to hold the data from 12 OH. In case some of the OHs are masked, the data at the positions reserved to them will be random garbage. Indexing when data assigned:

int idx = ohN*(dacMax-dacMin+1)/dacStep + (dacVal-dacMin)/dacStep;
outDataDacVal[idx] = dacVal;
outDataTrigRateOverall[idx] = readRawAddress(ohTrigRateAddr[ohN][24], la->response);
for(int vfat=0; vfat<24; ++vfat){
    if ( !( (notmask >> vfat) & 0x1)) continue;
    idx = ohN*24*(dacMax-dacMin+1)/dacStep + vfat*(dacMax-dacMin+1)/dacStep+(dacVal-dacMin)/dacStep;
    outDataTrigRatePerVFAT[idx] = readRawAddress(ohTrigRateAddr[ohN][vfat], la->response);
}

Indexing in the caller code:

    if(isParallel){
        uint32_t outDataTrigRatePerVFAT[12*24*(dacMax-dacMin+1)/dacStep];
        uint32_t outDataDacValPerOH[12*(dacMax-dacMin+1)/dacStep];
        uint32_t outDataTrigRatePerOH[12*(dacMax-dacMin+1)/dacStep];
        sbitRateScanParallelLocal(&la, outDataDacValPerOH, outDataTrigRatePerVFAT, outDataTrigRatePerOH, ch, dacMin, dacMax, dacStep, scanReg, ohMask);

        response->set_word_array("outDataVFATRate", outDataTrigRatePerVFAT, 12*24*(dacMax-dacMin+1)/dacStep);
        response->set_word_array("outDataDacValue", outDataDacValPerOH, 12*(dacMax-dacMin+1)/dacStep);
        response->set_word_array("outDataCTP7Rate", outDataTrigRatePerOH, 12*(dacMax-dacMin+1)/dacStep);
    }

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

Required to faster and more efficient detector calibration and commissioning

How Has This Been Tested?

Not yet tested. Requires matching changes in caller code and analyzing routines.

Screenshots (if appropriate):

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

bdorney commented 5 years ago

@mexanick looks like this will need a rebase.

mexanick commented 5 years ago

I'm not 100% sure whether I made the rebase in the cleanest possible way, but it is done...

bdorney commented 5 years ago

I've validated through commit fb5cc7dc32f3c0f48e3ce08f45a502977715027f and this should be considered no longer work in progress but ready for review.

jsturdy commented 5 years ago

I'm not 100% sure whether I made the rebase in the cleanest possible way, but it is done...

I suspect not, because as I try to look at the diff, I see many changes that were included in #105 I suppose next SW dev meeting I'll have to outline the procedure I've been employing for getting up-to-date in these cases

jsturdy commented 5 years ago

Questions for clarity:

What do "parallel" and "series" mean in these modules?
- multiple threads?
- simultaneous setup and sequential readout of multiple ?
- historical?
When should a non-"multi-link" method ever be used (can not "single-link" be effected by using a single-bit mask in a "multi-link" function"?)
- Simplifies the module code (maintainability)
- Transfers the onus of correct setup onto the calling code

bdorney commented 5 years ago

Questions for clarity:

What do "parallel" and "series" mean in these modules?

multiple threads?

simultaneous setup and sequential readout of multiple ?

historical?

This was historical. Before @andrewpeck implemented per VFAT sbit counters in the OH FW this was being done only in the CTP7 FW which as a single trigger rate counter. So the VFATs had to be placed into run mode one by one and done in series. Whereas with the "new" (now very old) per VFAT counters this is done in parallel.

When should a non-"multi-link" method ever be used (can not "single-link" be effected by using a single-bit mask in a "multi-link" function"?)

I'm not sure I understand the question/word choice you're using. Do you mean "can I use a multilink method with ohMask 0x1 just as easily as 0x3"? The answer is "yes you can."

The only time a "single" link function should be developed is if some resource cannot be used for multiple OH's simultaneously, the example of this is the VFAT_DAQ_MONITOR or the SBIT_MONITOR blocks in the FW which can only listen to one OH at a time.

Does this answer your questions?

jsturdy commented 5 years ago

Questions for clarity:

What do "parallel" and "series" mean in these modules?

multiple threads?

simultaneous setup and sequential readout of multiple ?

historical?

This was historical. Before @andrewpeck implemented per VFAT sbit counters in the OH FW this was being done only in the CTP7 FW which as a single trigger rate counter. So the VFATs had to be placed into run mode one by one and done in series. Whereas with the "new" (now very old) per VFAT counters this is done in parallel.

OK, so the series functions should be marked as obsolete and removed at some point?

When should a non-"multi-link" method ever be used (can not "single-link" be effected by using a single-bit mask in a "multi-link" function"?)

I'm not sure I understand the question/word choice you're using. Do you mean "can I use a multilink method with ohMask 0x1 just as easily as 0x3"? The answer is "yes you can."

The only time a "single" link function should be developed is if some resource cannot be used for multiple OH's simultaneously, the example of this is the VFAT_DAQ_MONITOR or the SBIT_MONITOR blocks in the FW which can only listen to one OH at a time.

Does this answer your questions?

I think almost, my final point would be that given that the partial answer is "yes you can", and that the more complete answer is "only when sharing a resource is not possible", I would still be in favour of eliminating "multi-link" and "single-link" nomenclature from the functions, as this can easily be incorporated into the design, as is done for the current "multi-link" things. For truly shared "must be done one-at-a-time" things, one can simply move the loop to the top level of the function, e.g.,

for oh in range(nOH):
  if ((ohMask>>oh) & 0x1):
    setup for link X
    run link X
    retrieve data from link X
    disable link X

In this way, the function describes what is being done, but doesn't burden anyone with details that are probably not salient (series, parallel, multi-link). If the calling code wants to operate on all links, it is done (whether as a "multi-link"/parallel solution, or as a series solution), and if operation on only a single link is desired, the same function call is used, with a different parameter

bdorney commented 5 years ago

OK, so the series functions should be marked as obsolete and removed at some point?

Yes.

bdorney commented 5 years ago

I think almost, my final point would be that given that the partial answer is "yes you can", and that the more complete answer is "only when sharing a resource is not possible", I would still be in favour of eliminating "multi-link" and "single-link" nomenclature from the functions, as this can easily be incorporated into the design, as is done for the current "multi-link" things.

In this way, the function describes what is being done, but doesn't burden anyone with details that are probably not salient (series, parallel, multi-link). If the calling code wants to operate on all links, it is done (whether as a "multi-link"/parallel solution, or as a series solution), and if operation on only a single link is desired, the same function call is used, with a different parameter

Sorry but I don't understand the problem/issue you're trying to solve. Or what the concern is. Can you give a more concrete or simpler explanation? From the documentation it's clear what each function does. And if it is not we should make a PR to improve this.

jsturdy commented 5 years ago

Closes #101

jsturdy commented 5 years ago

I think almost, my final point would be that given that the partial answer is "yes you can", and that the more complete answer is "only when sharing a resource is not possible", I would still be in favour of eliminating "multi-link" and "single-link" nomenclature from the functions, as this can easily be incorporated into the design, as is done for the current "multi-link" things.

In this way, the function describes what is being done, but doesn't burden anyone with details that are probably not salient (series, parallel, multi-link). If the calling code wants to operate on all links, it is done (whether as a "multi-link"/parallel solution, or as a series solution), and if operation on only a single link is desired, the same function call is used, with a different parameter

Sorry but I don't understand the problem/issue you're trying to solve. Or what the concern is. Can you give a more concrete or simpler explanation? From the documentation it's clear what each function does. And if it is not we should make a PR to improve this.

The problem I'm specifically trying to solve is the proliferation of "ugly/duplicate" interfaces:

<functionality>MultiLink (especially when separately there exists <functionality>), e.g., getOHVFATMask and getOHVFATMaskMultiLink, both of these could be replaced by a completely generic getOHVFATMask that would handle both single and multi link operation
<functionality>Parallel (I understand that this is not really a problem and will be removed)
<functionality>Series (I understand there aren't currently any of these)

cms-gem-daq-project / ctp7_modules