[feature request] Add ADC readout from SCA

[jsturdy]

Brief summary of issue

This issue is to outline what is expected from the SCA ADC readout implementation. The behaviour of the SCA is outlined in the GBT-SCA users manual

Wrapper functions to the various interfaces of the SCA have already been implemented (#97), and this issue is to simply utilize the ADC interface to read back any information that we are interested in.

Types of issue

• [x] Feature request (request for change which adds functionality)

Expected Behavior

• Functions that read a specific ADC, for all ADCs
• Function that will read all ADCs?

Possible Solution (for bugs)

Context (for feature requests)

• GE1/1 OptoHybrid V3 SCA pin connections:

[jsturdy]

@ram1123, based on this I have a couple of clarifying comments. 1) The expectation is that these SCA related functions will be implemented in the amc/sca.cpp unit 2) They will use the interface(s) that has(have) been implemented in #97, and as discussed in #93

[ram1123]

I added the functionalities af requested in this issue #100.

You can have a look at them here: https://github.com/ram1123/ctp7_modules/commit/6f2b856a18ac9f82fdbb4178a3ab875db24d4689

Please let me know if it seems fine. Then I will make a pull request.

[jsturdy]

A couple of quick questions/comments (others left inline on your code):

• your diff shows that the config submodule changed, did you make changes here?
  • remember, before making your PR you will have to do the following (nothing else should have gone into this branch, so these should actually not be necessary): 1) git rebase -i feature/sca-interface (your changes should have started from this branch, and been made back to this branch) 1) git submodule update

[jsturdy]

I see that you've started from develop, so I'll just rebase feature/sca-interface onto that and you can make your PR to this branch

[jsturdy]

Hi @ram1123, referencing your reply to my comment here in the issue for trackability.

1) You do not need to write any new local functions. 1) For each of the new functions (RPC callbacks) you have implemented, they need to return the data to the calling PC, so the results of the sensor reads need to be turned into an RPC reply message, and the format of the data you will be sending back defined 1) see, e.g., optohybrid::getUltraScanResults or vfat3::broadcastRead (or maybe still optohybrid::broadcastRead), or rpctest.cpp for some examples of how this has been done in some other cases 1) Feel free to reply here with your proposed data format for comment 1) My suggestion would be that the results are stored as a 2d-array or as a 32-bit word array 1) you need 3 bits to identify the link 1) you need 5 bits to identify the sensor 1) you need 12 bits to store the value

word = <link ID 26:24><0s 23:21><ADC ID 20:16><0's 15:12><data 11:0>

[ram1123]

@jsturdy, Please check below if this is fine[1]. Please, let me know if I need to make any further improvements.

Still I have one more doubt. As you set that the word should be like:

word = <link ID 26:24><0s 23:21><ADC ID 20:16><0's 15:12><data 11:0>

where ADC ID is of 4 bits. But, I think it should be of 8 bits as it goes from 0 to 31 (ref).

---

[1]

void readADCTemperatureChannel(const RPCMsg *request, RPCMsg *response)
{
  // struct localArgs la = getLocalArgs(response);
  GETLOCALARGS(response);

  uint32_t ohMask = request->get_word("ohMask");
  LOGGER->log_message(LogManager::DEBUG, stdsprintf("Optohybrids to read: %x",ohMask));
  uint32_t outData;

  SCAADCChannelT chArr[5] = {
                  static_cast<SCAADCChannelT>(SCAADCChannelT::ADC_CH00),
                  static_cast<SCAADCChannelT>(SCAADCChannelT::ADC_CH04),
                  static_cast<SCAADCChannelT>(SCAADCChannelT::ADC_CH07),
                  static_cast<SCAADCChannelT>(SCAADCChannelT::ADC_CH08),
                  static_cast<SCAADCChannelT>(SCAADCChannelT::ADC_CH31),
                };

  std::vector<uint32_t> result;
  int count;
  for(auto const& channelName : chArr){
    result = scaADCCommand(&la, channelName, 0, 0, ohMask);
    count = 0;
    for(auto const& val : result){
      if (val != 0){
        LOGGER->log_message(LogManager::DEBUG, stdsprintf("Temperature for OH-0x%x, SCA-ADC channel 0x%x = %i ",ohMask, channelName, val));
        outData = (count<<24) | (channelName<<16) | val;
        response->set_word("outData",outData);
        count++;
      }
    }
  }

  rtxn.abort();
}

[jsturdy]

where ADC ID is of 4 bits. But, I think it should be of 8 bits as it goes from 0 to 31 (ref).

The example has ADC ID as 5 bits (bits 20 down to 16), and this is sufficient to cover numbers from 0 to 31.

Your proposed function is not quite correct 1) You shouldn't check if the data is non-zero, all data should be returned 1) You have to write the response at the end 1) so set it up as a std::vector and fill the vector as you go through the loop 1) after the loop, set a word_array response with the std::vector::data() values, and a size of std::vector::size()

Furthermore, from Yifan, shortly we will have the mapping of ADC channels to PT100 sensor location, so we should add a sensible naming convention to be able to identify the various sensors, and these should be the inputs to the array you're using (do you have to do the static_cast when defining the array, or does it not compile with, e.g.,

std::array<SCAADCChannelT, 5> chArr = {
                  SCAADCChannelT::ADC_CH00),
                  SCAADCChannelT::ADC_CH04),
                  SCAADCChannelT::ADC_CH07),
                  SCAADCChannelT::ADC_CH08),
                  SCAADCChannelT::ADC_CH31),
                };

[ram1123]

@jsturdy , Thanks for your detailed comments. I updated the patch. Please have a look at it.

void readADCTemperatureChannel(const RPCMsg *request, RPCMsg *response)
{
  // struct localArgs la = getLocalArgs(response);
  GETLOCALARGS(response);

  uint32_t ohMask = request->get_word("ohMask");
  LOGGER->log_message(LogManager::DEBUG, stdsprintf("Optohybrids to read: %x",ohMask));
  std::vector<uint32_t> outData;

  SCAADCChannelT chArr[5] = {
                  SCAADCChannelT::ADC_CH00,
                  SCAADCChannelT::ADC_CH04,
                  SCAADCChannelT::ADC_CH07,
                  SCAADCChannelT::ADC_CH08,
                  SCAADCChannelT::ADC_CH31
                };

  std::vector<uint32_t> result;
  int count;
  for(auto const& channelName : chArr){
    result = scaADCCommand(&la, channelName, 0, 0, ohMask);
    count = 0;
    for(auto const& val : result){
        LOGGER->log_message(LogManager::DEBUG, stdsprintf("Temperature for OH-0x%x, SCA-ADC channel 0x%x = %i ",ohMask, channelName, val));
    outData.push_back((count<<24) | (channelName<<16) | val);
    count++;
    }
  }

  response->set_word_array("Temperature for SCA-ADC Channels", outData.data(), outData.size());

  rtxn.abort();
}

[jsturdy]

These comments generally apply to all the SCA ADC functions that have been added:

• For the generic readADCCommands that reads a single ADC
  • Change the function name to readSCAADCSensor
  • This addition might be nice, but I think it can also wait on some further refinement
  • Add a string word (set_string("converted")) that represents the double value to some precision, I would suggest using a stringstream and std::fixed << std::setprecision(10) << converted_value
    • This "converted" value should have the appropriate conversion applied, whether to voltage or temperature or current, dependent on the channel being sampled.
• Change the name of the function to readAllSCATempSensors (or as appropriate for the read all functions)
• Your log message is not correct:
  • the reading is for a specific sensor (correctly identified by channelName) connected to a specific OptoHybrid (should be identified by count)
• I would modify my proposed data format slightly (to indicate that a given OH has been masked by ohMask):
  • word = <OH masked 1, otherwise 0 27><link ID 26:24><0s 23:21><ADC ID 20:16><0's 15:12><data 11:0>
• Change the identifier of the word_array to data, the calling code knows what it's asking for, and having a common interface will be simpler for these functions
• Add the information to the enum that @bdorney emailed from Yifan and put these names in the array

[jsturdy]

• this (and similar) still doesn't follow the specification listed in my next-to-last bullet point above:

  • Change the identifier of the word_array to data, the calling code knows what it's asking for, and having a common interface will be simpler for these functions

• this (and similar) do not conform to my request:

  • Add the information to the enum that @bdorney emailed from Yifan and put these names in the array

• change this from readALLSCAADCSensor to readAllSCAADCSensors

• change the group read function names from <blah>Sensor to <blah>Sensors

[ram1123]

Hi @bdorney and @evka85,

Jared suggested the name of PT100 sensors as following:

VTTX_VTRX0_PT100 = 0x00, ///< Right (viewed from above) VTTx/VTRx temperature sensor
VTTX_VTRX1_PT100 = 0x04, ///< Left (viewed from above) VTTx/VTRx temperature sensor
SCA_PT100      = 0x07, ///< SCA temperature sensor
V6_FPGA_PT100  = 0x08, ///< Virtex6 temperature sensor

Do you have better suggestation for these?

[bdorney]

Hi @bdorney and @evka85,

Jared suggested the name of PT100 sensors as following:

VTTX_VTRX0_PT100 = 0x00, ///< Right (viewed from above) VTTx/VTRx temperature sensor
VTTX_VTRX1_PT100 = 0x04, ///< Left (viewed from above) VTTx/VTRx temperature sensor
SCA_PT100      = 0x07, ///< SCA temperature sensor
V6_FPGA_PT100  = 0x08, ///< Virtex6 temperature sensor

Do you have better suggestation for these?

Is this to be inside an enum? Or where are these defined? I would assume inside hw_constants.h

[ram1123]

hi @bdorney ,

Yes, its defined in enum. But in file sca_enums.h#L211-L267

[jsturdy]

Is this to be inside an enum?

Indeed, this is only to give a sensible (readable) name to the channels for usage within the code. The only two that I'm not sure about are the VTTX_VTRX0/1, since one could conceive of a more descriptive name, based on the specific neighboring components.

[bdorney]

Is this to be inside an enum?

Indeed, this is only to give a sensible (readable) name to the channels for usage within the code. The only two that I'm not sure about are the VTTX_VTRX0/1, since one could conceive of a more descriptive name, based on the specific neighboring components.

You could go with VTTX_VRTX_GEMTRIG and VTTX_VRTX_CSCTRIG and that would more easily locate the sensors on the physical board since the VTTx for GEM and CSC Trigger links are on two different tongues of the OH board.

[bdorney]

hi @bdorney ,

Yes, its defined in enum. But in file sca_enums.h#L211-L267

@jsturdy why do we have HW constants defined in two different locations? If there is a desire to not have a giant include of a single file then I would suggest something more like:

ctp7_modules/hw_constants/object_constants.h

Like gbt_constants.h or sca_constants.h and this would provide a single entry point/location for such information rather than scattered around.

[jsturdy]

OK, then @ram1123, I'd suggest (modulo a switch if I got the locations wrong: in this GEM is in block of 3, CSC is in block of 2):

VTTX_CSC = 0x00, ///< Transceiver block next to the CSC VTTX
VTTX_GEM = 0x04, ///< Transceiver block next to the GEM VTTX

[jsturdy]

hi @bdorney , Yes, its defined in enum. But in file sca_enums.h#L211-L267

@jsturdy why do we have HW constants defined in two different locations? If there is a desire to not have a giant include of a single file then I would suggest something more like:

ctp7_modules/hw_constants/object_constants.h

Like gbt_constants.h or sca_constants.h and this would provide a single entry point/location for such information rather than scattered around.

These are not HW constants per se; they are module specific enums, and scoping them by their module fits the development model herein utilized to scope functionality. The current hw_constants.h has a more general, and different use case, and it is of interest to a broader scope

[bdorney]

The current hw_constants.h has a more general, and different use case, and it is of interest to a broader scope

What would be the preferred use case?

General comment overtime you've been providing a lot of feedback regarding how things should be structured. Is it possible for you to start to include these in a generic "contributing guidelines page" that could be used as a quick reference for developers?

[ram1123]

Hi @jsturdy ,

I updated my git repo (https://github.com/ram1123/ctp7_modules/commit/3e2546d52bbabc46e81f7de0e9fd295bf3f31a7c). Also, I commened out the not-connected sensors. Please have a look at it. Now, things should be fine.

Let me know if I can make a pull request now?

[jsturdy]

• change the group read function names from <blah>Sensor to <blah>Sensors

this has still not been addressed

[jsturdy]

in sca.cpp

• at L134, the integrated SCA temperature sensor does not need a current source to function (as described in the manual)
• at L141, you are not turning off the current sink here, and you should

[ram1123]

• change the group read function names from Sensor to Sensors

Here sca.cpp#L244

I will keep the name readSCAADCSensor as it reads only one sensor at at time.

[jsturdy]

When accessing the enum values, use, e.g., SCAADCChannel::VTTX_CSC instead of SCAADCChannelT::VTTX_CSC. SCAADCChannelT was typedef'd to give the "type", while SCAADCChannel was typedef'd to access the values

[jsturdy]

• change the group read function names from Sensor to Sensors

Here sca.cpp#L244

I will keep the name readSCAADCSensor as it reads only one sensor at at time.

Correct

[jsturdy]

Address these, and I think your code will be ready for review

[ram1123]

@jsturdy . I updated your comments. Please check.

[jsturdy]

please make PR