-
Notifications
You must be signed in to change notification settings - Fork 13
[cleanup] Common blocks related to supOH #162
Comments
Since we are going on safe automatic determination of the hardware masks (e.g. vfat mask, oh mask), I suggest to run binary xor between the provided ohMask and automatically determined one. In this case we are on the safe side w.r.t. number of OH, but also w.r.t. some link with "good" number being not ready. |
correction: should be no xor, but simply |
Do we have an automatic determination of the connected OHs currently (or in the pipeline)? |
OK, so then at least for this case where the uint32_t supOH = readReg(&la, "GEM_AMC.GEM_SYSTEM.CONFIG.NUM_OF_OH");
if (ohMask > (0x1<<supOH))
LOG4CPLUS_WARN(logger, "Requested OptoHybrids (0x"<< std::hex << std::setfill('0') << std::setw(8) << ohMask << std::dec << ") > NUM_OF_OH AMC register value ("<< supOH << "), request will be reset to register max");
} is the appropriate one and will implement it.
|
Indeed, this solution is appropriate. The only caveat is that the calling code must not assume it will receive the number of OHs it has asked for and/or that the RPC method will never be called with a This issue also raises the more general question of HW constants determination and the question of parameter range checks. The HW constants can be split into two main groups:
Also, what about the parameter range checks? As discussed here, the RPC modules could not check any parameter and assume that they are all in range. If all the call go though the Slightly outside the scope of the |
Brief summary of issue
As part of the porting (#160), I've come across several regularly repeated blocks of code I'm trying to clean up.
The first is some check against the FW supported number of OptoHybrid links, e.g.,
I tried initially to just simplify this with the following
I believe this captures the intention of the original code, however it can fail in the actual implementation, e.g., if the
ohMask
is sparse, but still toggles on a link that is not supported, or, worse, if theohMask
is sparse, not toggling any unsupported OHs, but as a byproduct, limits the range of the loop. When I realized this, I started trying to think of a better solution.This is safe:
Though what we really want to check is whether any bit in the mask is asking for an OptoHybrid that is outside the valid range, and loop only up to those OptoHybrids which are requested/supported.
In the end, I'm not even really sure how this
NOH
key was intended to be used, so I'd like some feedback before changing things completely.Additionally, we've had several discussions in the past about taking values from the FW itself (@lpetre-ulb) so this might be the right issue to address our ideal path forward on that.
Types of issue
The text was updated successfully, but these errors were encountered: