cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

Improvement: multi link CTP7 modules should have an NOH key in the RPC request (Good for new comer) #49

Closed bdorney closed 6 years ago

bdorney commented 6 years ago

Brief summary of issue

Right now most of the multi link rpc modules have hardcoded the number of OH's to be 12 and rely on the ohMask to skip links (or to not consider additional links).

Due to the different link counts for the GE2/1 OH it may be that we have a different number of OH's on the CTP7. As a result we should not hard code the number of OH's but instead pass this as a parameter.

This would be a simple task for a new comer.

Types of issue

Should not be a breaking change, to do this you could simply do a check if the NOH key exists:

uint32_t NOH=12;
if (request->get_key_exists("NOH")){
   NOH = request->get_word("NOH");
}

If the key doesn't exist the number of optohybrids is defaulted to 12.

Note that while the number 12 doesn't need 32 bits to be expressed the rpcsvc API can only send/receive words in 32 bit representation. So to prevent a type mismatch you should use uint32_t. This is because the CTP7 itself executes each transaction between the Zynq processor and the FPGA on the AXI bus as a series of 32 bit words.

Expected Behavior

The RPC request for each of the following functions should have a key NOH which determines the number of optohybrids to be looped over in the OH loop:

https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/calibration_routines.cpp#L1517 https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/amc.cpp#L477 https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/vfat3.cpp#L102 https://github.com/cms-gem-daq-project/ctp7_modules/blob/a87d951df725e3477ae6f13fd8f1aeb7480c150d/src/vfat3.cpp#L287

For an example of the implementation see how the key NOH in the RPC request is obtained in:

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

Then this should be used to control the OH loop.

After making your changes you need to try to compile, you can do this by executing:

export LD_LIBRARY_PATH=/opt/xdaq/lib:$LD_LIBRARY_PATH
export LD_LIBRARY_PATH=/opt/wiscrpcsvc/lib:$LD_LIBRARY_PATH
export LD_LIBRARY_PATH=/opt/rwreg/lib:$LD_LIBRARY_PATH
export LD_LIBRARY_PATH=/opt/xhal/lib:$LD_LIBRARY_PATH
export PETA_STAGE=/data/bigdisk/sw/peta-stage/
source /data/bigdisk/sw/Xilinx/SDK/2016.2/settings64.sh
export XHAL_ROOT=$BUILD_HOME/xhal/
cd ctp7_modules/
source setup.sh
make clean && make

Then you'll need to request to test your changes on a given CTP7.

Current Behavior

Right now the number of OH's is hard coded in the multi link RPC modules, see example configureVFAT3DacMonitorMultiLink():

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

Context (for feature requests)

Due to different number of links for a GE2/1 OH we may have a different number of OH's on the backend, so hardcoding 12 is not a good idea.

jsturdy commented 6 years ago

Why not just have the module ask the FW how many links it supports? Or is this to attempt to control the looping at a finer grained level?

The code duplication argument pretty much goes out the window when discussing RPC philosophy (as each module is in effect a standalone operation, rather than part of an object), unless you have the module call another module on the card

bdorney commented 6 years ago

Why not just have the module ask the FW how many links it supports?

I didn't realise there was an AMC register that supports this. Yes this is an ideal solution.

The code duplication argument pretty much goes out the window when discussing RPC philosophy...unless you have the module call another module on the card

Yes we do, see for example the proposal in issue #48. Also functions in vfat.so and optohybrid.so call local functions in amc.so. Also functions in calibration_routines.so call local functions from the previous three mentioned shared libraries. This was done to decrease the number of RPC messages that are sent thus increasing speed of execution (e.g. there's no longer a reason for the RPC request to send the vfatmask, this can now be determined by "discovery" on the card, some refactoring is needed for all functions to make use of this).

bdorney commented 6 years ago

Why not just have the module ask the FW how many links it supports?

@jsturdy Do you have a copy of the CTP7 documentation? I tried the link that @andrewpeck sent but now it generates a 404 error and I didn't think to download it at the time.

bdorney commented 6 years ago

The register in question is:

eagle26 > doc GEM_AMC.GEM_SYSTEM.CONFIG.NUM_OF_OH
Name: GEM_AMC.GEM_SYSTEM.CONFIG.NUM_OF_OH
Description: Number of supported optohybrids
Address: 0x00900005
Permission: r
Mask: 0x0000001f
Module: False
Parent: GEM_AMC.GEM_SYSTEM.CONFIG
None
bdorney commented 6 years ago

Closed by #51