read the number of OHs from the AMC register and perform checks on the NOH rpc key

AndrewLevin commented 5 years ago

Currently, the number of OHs is hardcoded as 12 in some places and it is read from the RPC request in other places. This pull request makes use of the NUM_OF_OH AMC register to avoid the hardcoding and perform a check on the number of OHs from in the RPC request.

Description

The first 4 of the following functions (in amc.cpp, in calibration_routines.cpp, and vfat3.cpp) were previously using hardcoded number of OHs. The next 6 of the following functions (in daq_monitor.cpp) already used the number of OHs from the RPC request. I have changed all of these functions to: use the number of OHs from the RPC request if that exists and is less than or equal to the value of the NUM_OF_OH register, and otherwise to use the NUM_OF_OH register.

1) getOHVFATMaskMultiLink in amc.cpp 2) dacScanMultiLink in calibration_routines.cpp 3) configureVFAT3DacMonitorMultiLink in vfat3.cpp 4) readVFAT3ADCMultiLink in vfat3.cpp 5) getmonTRIGGERmain in daq_monitor.cpp 6) getmonTRIGGEROHmain in daq_monitor.cpp 7) getmonDAQOHmain in daq_monitor.cpp 8) getmonOHmain in daq_monitor.cpp 9) getmonOHSCAmain in daq_monitor.cpp 10) getmonOHSysmon in daq_monitor.cpp

Types of changes

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

Motivation and Context

There may be a different number of links on an OH, as explained in https://github.com/cms-gem-daq-project/ctp7_modules/issues/49. Instead of hardcoding the number or passing the number as a parameter to the rpc call, the number can be easily read from the NUM_OF_OH AMC register. In some cases the user may want to fewer than the total number of OHs, so passing the number as a parameter should be supported in that case.

How Has This Been Tested?

I have compiled the changes and tested them on eagle64 as described in http://cmsonline.cern.ch/cms-elog/1066648

Screenshots (if appropriate):

Checklist:

[x] My code follows the code style of this project.
[ ] 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.

AndrewLevin commented 5 years ago

@bdorney, @sturdy, @mexanick, all tests were successful, so the pull request is ready for your review

AndrewLevin commented 5 years ago

@bdorney, so if the NOH key exists, I should use it, and if the key does not exist, then I should use the number in the NUM_OF_OH register?

bdorney commented 5 years ago

@dorney, so if the NOH key exists, I should use it, and if the key does not exist, then I should use the number in the NUM_OF_OH register?

Yes, I would also make a check that the value of the NOH key does not exceed NUM_OF_OH if it does there should be a WARN (or WARNING?) level message printed to the CTP7 log and then an automatic reset to NUM_OF_OH (this should be included in the log)

e.g. "NOH requested > NUM_OF_OH, reseting NOH to NUM_OF_OH"

AndrewLevin commented 5 years ago

@bdorney, I made the requested change, rebuilt the .so files, tested them on eagle64, and found the test is successful

bdorney commented 5 years ago

@bdorney, I made the requested change, rebuilt the .so files, tested them on eagle64, and found the test is successful

You're missing the requested changes to daq_monitor functions. e.g.

Additionally monitoring functions (now in daq_monitor.cpp) could benefit from a similar treatment. Please add.

AndrewLevin commented 5 years ago

@bdorney, in daq_monitor.cc, NOH appears both as a function argument (e.g. in getmonTRIGGERmainLocal) and as a request key (e.g. in getmonOHmain). Please clarify how you would like me to treat the two cases. In both cases, should I reset NOH to NUM_OF_OH if the NOH request is larger?

bdorney commented 5 years ago

@bdorney, in daq_monitor.cc, NOH appears both as a function argument (e.g. in getmonTRIGGERmainLocal) and as a request key (e.g. in getmonOHmain). Please clarify how you would like me to treat the two cases. In both cases, should I reset NOH to NUM_OF_OH if the NOH request is larger?

I think there's no need to change the local function, since it's taken as an argument and if it's called from another function on the card register access is O(50us).

The non-local versions modify as you have done before. Does that clarify things?

bdorney commented 5 years ago

The real goal is that we want to steer away from hard coding the number of OH's per AMC since this is expected to change in the case of GE2/1; but may of these functions will use pieces of the FW that will be common to all GEM subsystems (e.g. SCA block, Sysmon block, VFAT address space, etc...).

This will reduce overall complexity and increase functionality. The address space of the FPGA may change, but as long as the node names in remain the same in the xml this can be 100% handled by simply having a different xml and different contents of lmdb on the CTP7.

AndrewLevin commented 5 years ago

@bdorney, I have now implemented the same logic for the NOH variables in daq_monitor that are read from the rpc request, please have a look

bdorney commented 5 years ago

Please update the PR description to reflect the changes made (e.g. functions modified in daq_monitor are not mentioned in the description).

AndrewLevin commented 5 years ago

@bdorney, it is updated now

cms-gem-daq-project / ctp7_modules