[bugfix] SCA readout update #157
Conversation
| @@ -212,7 +212,7 @@ class SCASettings { | |||
| enum EADCChannel { //ADCChannel settings | |||
There was a problem hiding this comment.
Not technically part of this PR, but is there a specific reason why you preferred enum's in struct's instead of scope enumerations in this file? Using strongly-typed aliases would help preventing bugs and would allow using functions overloads (e.g. replace all sca*Command with a single overloaded scaCommand).
Also not part of this PR, the GE1/1 aliases should be updated to reflect the latest OptoHybrid changes .
There was a problem hiding this comment.
Yes, I had used the struct { enum} because there is code in cmsgemos using the same, and it was foreseen that these headers are part of the files that can be included there.
I fully expect that very shortly these will be replaced with the strongly typed enum class syntax, as we're moving to a (more) modern compiler :-)
| @@ -148,14 +148,23 @@ uint32_t getNumNonzeroBits(uint32_t value) | |||
| return numNonzeroBits; | |||
| } | |||
|
|
|||
| uint32_t getMask(localArgs * la, const std::string & regName) | |||
| bool regExists(localArgs * la, const std::string & regName, lmdb::val * db_res) | |||
There was a problem hiding this comment.
It is good to have this elementary functionality shared. As as note, the DAC scans will also have to be adapted at some point in order to avoid using low-level LMDB access throughout the code.
| * bits [26:24]: link ID | ||
| * bits [31:29]: constant 0's | ||
| * bit [28] : data present | ||
| * bits [27:24]: link ID |
There was a problem hiding this comment.
I was wondering how many OHs will be supported by the foreseen ATCA board? I imagine that it will be more than 16. As a bugfix for the legacy modules, the change it certainly more than enough, but the format should probably be improved when moving to the templated RPC modules.
There was a problem hiding this comment.
indeed, this format should be adapted for an expected future system before it gets into the production stream (templated RPCs)
| // FIXME: DECIDE WHETHER TO HAVE HERE // if (regExists(la, "GEM_AMC.SLOW_CONTROL.SCA.ADC_MONITORING.MONITORING_OFF")) { | ||
| // FIXME: DECIDE WHETHER TO HAVE HERE // writeReg(la,"GEM_AMC.SLOW_CONTROL.SCA.ADC_MONITORING.MONITORING_OFF", 0xffffffff); | ||
| // FIXME: DECIDE WHETHER TO HAVE HERE // } | ||
|
|
There was a problem hiding this comment.
Since all calls to sendSCACommand need to be protected against the ADC monitoring code, wouldn't it better to directly move the protection here?
There was a problem hiding this comment.
Yes, I think this was my initial proposal as well, but don't remember the reason for not implementing it.
In principle, it can also be removed at some point... but yeah... another time, another PR :-)
Description
Several issues were present with the SCA readout:
.ADC_MONITORING.MONITORING_OFFregister had not been forward propagatedutilsbool regExists(localArgs * la, const std::string & regName, lmdb::val * db_res)that is called in all the read/write functions in place of the register name lookup, as this allows other code to check whether the register exists in the LMDB, even if they don't need to create a validlmdb::valobjectTypes of changes
Motivation and Context
Needed to reliably monitor the sensors from the SCA
How Has This Been Tested?
Problem discovered and tested at P5