cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

Improvement: Drop ohVfatMaskArray word (Good for new comer) #48

Closed bdorney closed 6 years ago

bdorney commented 6 years ago

Brief summary of issue

We can reduce complexity by removing the rpc request key ohVfatMaskArray in several functions in vfat3.h/cpp and calibration_routines.h/cpp and anywhere else it shows up, you can find occurrences via:

grep -R "ohVfatMaskArray" src/*.cpp
grep -R "ohVfatMaskArray" src/*.h

Types of issue

Note this would not necessarily be a breaking change for following branches:

Since if a key is set in an rpc request and is not used nothing bad happens. However for general maintenance the above branches would need to be modified.

Expected Behavior

Instead in each function where ohVfatMaskArray is needed, it could be dropped and replaced with getOHVFATMaskLocal(...):

https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/amc.cpp#L445-L456

And then the function depicted under Current Behavior below would become:

    uint32_t ohMask = request->get_word("ohMask");
    uint32_t dacSelect = request->get_word("dacSelect");

    struct localArgs la = {.rtxn = rtxn, .dbi = dbi, .response = response};
    for(int ohN=0; ohN<12; ++ohN){
        // If this Optohybrid is masked skip it
        if(!((ohMask >> ohN) & 0x1)){
            continue;
        }

        //Get VFAT Mask
        uint32_t vfatMask = getOHVFATMaskLocal(la, ohN);

        LOGGER->log_message(LogManager::INFO, stdsprintf("Programming VFAT3 ADC Monitoring on OH%i for Selection %i",ohN,dacSelect));
        configureVFAT3DacMonitorLocal(&la, ohN, vfatMask, dacSelect);
    } //End Loop over all Optohybrids

Thus reducing complexity.

Current Behavior

Currently the ohVfatMaskArray is sent as a key in the RPC message. e.g.:

https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/vfat3.cpp#L111-L125

This occurs in several RPC functions. It could be dropped entirely

Context (for feature requests)

One less "fiddly" bit a developer has to set; tool for automatic discovery exists, might as well use it.

Most importantly in the calibration suite it would reduce the number of rpc messages that need to be sent improving speed of some routines.

Also it's a simple fix a new comer could do.

jsturdy commented 6 years ago

Since if a key is set in an rpc request and is not used nothing bad happens. However for general maintenance the above branches would need to be modified.

To avoid a person feeling as though they "wasted" time working on something, I would caution against making any "maintenance" changes on the listed branches

bdorney commented 6 years ago

To avoid a person feeling as though they "wasted" time working on something, I would caution against making any "maintenance" changes on the listed branches

At some point I thought we where going to have a copy/paste into the appropriate HwManager class in cmsgemos. But yes I see the point, upstream changes don't need to be corrected in those listed branches.

bdorney commented 6 years ago

Closed by #50