search

cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

[feature request] Function porting #160

Closed jsturdy closed 4 years ago

jsturdy commented 4 years ago

Brief summary of issue

This issue will track the porting of functions existing on the release/v1.1.X but not on the develop branch. We are only interested in tracking functions were added after the "freeze" on new features was implemented, i.e., when the branches diverged. Old functions that have since been removed in develop for various reasons, should stay removed.

Please try editing this message before posting a reply

Types of issue

release/v1.1.X functions not in develop

Extracted using, e.g.,

ctags -x --c-types=f --format=1 src/utils.cpp

utils.h/utils.cpp

1a2
> bitCheck           src/utils.cpp    uint32_t bitCheck(uint32_t word, int bit)
13a15,16
> regExists          src/utils.cpp    bool regExists(localArgs * la, const std::string & regName, lmdb::val * db_res)
> repeatedRegReadLocal   src/utils.cpp    slowCtrlErrCntVFAT repeatedRegReadLocal(localArgs * la, const std::string & regName, bool breakOnFailure, uint32_t nReads)

extras.h/extras.cpp

Empty diff

amc.h/amc.cpp

5a6
> repeatedRegRead    src/amc.cpp      void repeatedRegRead(const RPCMsg *request, RPCMsg *response)
134a136,140
> readAllSCAADCSensors   src/amc/sca.cpp  void readAllSCAADCSensors(const RPCMsg *request, RPCMsg *response)
> readSCAADCSensor   src/amc/sca.cpp  void readSCAADCSensor(const RPCMsg *request, RPCMsg *response)
> readSCAADCSignalStrengthSensors   src/amc/sca.cpp  void readSCAADCSignalStrengthSensors(const RPCMsg *request, RPCMsg *response)
> readSCAADCTemperatureSensors   src/amc/sca.cpp  void readSCAADCTemperatureSensors(const RPCMsg *request, RPCMsg *response)
> readSCAADCVoltageSensors   src/amc/sca.cpp  void readSCAADCVoltageSensors(const RPCMsg *request, RPCMsg *response)
141c147
< scaADCCommand      src/amc/sca.cpp  std::vector<uint32_t> scaADCCommand(localArgs* la, SCAADCChannelT const& ch, SCAADCCommandT const& cmd, uint8_t const& len, uint32_t data, uint16_t const& ohMask)
---
> scaADCCommand      src/amc/sca.cpp  std::vector<uint32_t> scaADCCommand(localArgs* la, SCAADCChannelT const& ch, uint16_t const& ohMask)
145c151
< scaI2CCommand       src/amc/sca.cpp  std::vector<uint32_t> scaI2CCommand(localArgs* la, SCAI2CChannelT const& ch, SCAI2CCommandT const& cmd, uint8_t const& len, uint32_t data, uint16_t const& ohMask)
---
> scaI2CCommand      src/amc/sca.cpp  std::vector<uint32_t> scaI2CCommand(localArgs* la, SCAI2CChannelT const& ch, SCAI2CCommandT const& cmd, uint8_t const& len, uint32_t data, uint16_t const& ohMask)

optohybrid.h/optohybrid.cpp

Empty diff

gbt.h/gbt.cpp

3,4c3,4
< scanGBTPhasesLocal    src/gbt.cpp      bool scanGBTPhasesLocal(localArgs *la, const uint32_t ohN, const uint32_t N, const uint8_t phaseMin, const uint8_t phaseMax, const uint8_t phaseStep)
< writeGBTConfig      src/gbt.cpp      void writeGBTConfig(const RPCMsg *request, RPCMsg *response)
---
> scanGBTPhasesLocal    src/gbt.cpp      bool scanGBTPhasesLocal(localArgs *la, const uint32_t ohN, const uint32_t nResets, const uint8_t phaseMin, const uint8_t phaseMax, const uint8_t phaseStep, const uint32_t nVerificationReads)
> writeGBTConfig     src/gbt.cpp      void writeGBTConfig(const RPCMsg *request, RPCMsg *response)

vfat3.h/vfat3.cpp

Empty diff

daq_monitor.h/daq_monitor.cpp

0a1
> getmonCTP7dump     src/daq_monitor.cpp void getmonCTP7dump(const RPCMsg *request, RPCMsg *response)

calibration_routines.h/calibration_routines.cpp

5a6
> confCalPulse      src/calibration_routines.cpp void confCalPulse(const RPCMsg *request, RPCMsg *response)
17,18c18,19
< sbitRateScanParallelLocal   src/calibration_routines.cpp void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t *outDataTrigRatePerVFAT, uint32_t *outDataTrigRateOverall, uint32_t ch, uint32_t dacMin, uint32_t dacMax, uint32_t dacStep, std::string scanReg, uint32_t ohMask=0xFFF)
< setSingleChanMask    src/calibration_routines.cpp std::unordered_map<uint32_t, uint32_t> setSingleChanMask(int ohN, int vfatN, unsigned int ch, localArgs *la)
---
> sbitRateScanParallelLocal   src/calibration_routines.cpp void sbitRateScanParallelLocal(localArgs *la, uint32_t *outDataDacVal, uint32_t *outDataTrigRatePerVFAT, uint32_t *outDataTrigRateOverall, uint32_t ch, uint32_t dacMin, uint32_t dacMax, uint32_t dacStep, std::string scanReg, uint32_t ohMask=0xFFF, uint32_t waitTime=1)
> setSingleChanMask    src/calibration_routines.cpp std::unordered_map<uint32_t, uint32_t> setSingleChanMask(unsigned int ohN, unsigned int vfatN, unsigned int ch, localArgs *la)
lpetre-ulb commented 4 years ago

[I cannot edit the first message. However this reply is more than just listing the added functions.]

First, I'm not sure what are the changes in the scaI2CCommand (amc.h/amc.cpp) and writeGBTConfig (gbt.h/gbt.cpp) functions except for a space?

Regarding purely the new features, the class slowCtrlErrCntVFAT (utils.h/utils.cpp) was implemented for usage in new repeatedReadRegLocal function. Also, constants file sca_enums.h must be adapted to match the requirements of the new functions in amc.h/amc.cpp.

The two functions scanGBTPhasesLocal and sbitRateScanParallelLocal do not only have new parameters nVerificationReads and waitTime, respectively, but also have changes their implementation. The GBT phase scan algorithm has been improved for increased robustness while the S-bit rate scan routine now uses the OH firmware persisting counter mode.

Finally, none of the GE2/1 generalization seems to be ported to the develop branch. This includes both the constants file hw_constants.h and the various loops and array sizes. One could also use this opportunity to adapt the sca_enums.h for GE2/1.

jsturdy commented 4 years ago

Indeed, rebasing may end up being the right choice, though it has implications on other developers... Currently develop is diverged from the code that was branched from it (templated-rpc-methods and friends) because of a hotfix and non-rebased children. To go this route I'd propose the following: 1) rebase develop onto release/v1.1.X (this is actually noop, simply because the hotfix that was patched is already in release/v1.1.X and nothing has been merged back into develop yet) 2) rebase children of develop (templated-rpc-methods) onto the newly rebaseddevelop All these get force pushed to thecms-gem-daq-projectrepository, and anyone who has outstanding branches should probably PR them into this branch before thisrebase` operation (at the moment, it may only be myself and @vetens)

3) Is to implement #161 and other optimizations of the legacy code base that are in need of cleanup (significant or otherwise)

jsturdy commented 4 years ago

OK, I've done a local rebase of

It looks like things have proceeded as expected, and I had to tweak a few things after the rebase to make sure all the new changes compile in the new framework.

I can force push these in the manner specified, provided all other outstanding developments are truly frozen. (@mexanick, @AndrewLevin, @lpetre-ulb)

lpetre-ulb commented 4 years ago

:+1: This is OK for me.

In any case, the rebase is necessary for a peaceful development. And one should be able to easily rebase his developments considering the small number of changes.

jsturdy commented 4 years ago

OK, as sign off has come from @AndrewLevin, @vetens, @lpetre-ulb I'm going to take the aforementioned actions now

jsturdy commented 4 years ago

And has been done, though after doing so, I see that @mexanick did some ninja merges last week. In any event, if you have local branches tracking these affected branches, do not merge the changes in, save your old branch (if you feel it should still live on), delete it, and pull the updated branch. Probably if you have pull set to pull with rebase as the default (which you should via one or both of git config --global pull.rebase true or git config --global branch.autosetuprebase always), then you won't have to delete the branch, but verify after the pull that things act as expected

Also mentioned in the meeting, as of this action no new features will be accepted onto release/v1.1.X as we will target complete migration to the templated framework for operations in 2020.

jsturdy commented 4 years ago

OK, I've finished the migration (checked that it compiles), and have 4 separate branches, each one addressing one module: 1) optohybrid 1) vfat3 1) daq_monitor 1) calibration_routines

There are a couple of things in the build process that I'd like to clean up, and for the packaging, I'd also like to separate out the exportable functions and the remote-only functions into separate headers, so that the host development can be better separated.

I'd also like to make clear which functions have been renamed, which arguments are changed, what return types are different, and what return types have had their formats changed, so I'll do this and add it as text to the eventual PR

lpetre-ulb commented 4 years ago

OK, I've finished the migration (checked that it compiles), and have 4 separate branches, each one addressing one module:

1. [`optohybrid`](https://github.com/jsturdy/ctp7_modules/compare/feature/templated-rpc-methods-migration...jsturdy:feature/templated-optohybrid-migration)
2. [`vfat3`](https://github.com/jsturdy/ctp7_modules/compare/feature/templated-optohybrid-migration...jsturdy:feature/templated-vfat-migration)
3. [`daq_monitor`](https://github.com/jsturdy/ctp7_modules/compare/feature/templated-vfat-migration...jsturdy:feature/templated-daqmon-migration)
4. [`calibration_routines`](https://github.com/jsturdy/ctp7_modules/compare/feature/templated-daqmon-migration...jsturdy:feature/templated-calib-migration)

Just a couple of comments after a (very, very) brief overview:

  1. I see that the logger variable is initialized as a global variable. Does it truly work? I think it should always work for the second loaded module, because the configuration is only reset at each module initialization. However, I'm not sure it will work for the first module to be loaded (if it happens that the first module gets a logger before initialization). Although log4cplus is maybe initialized, but not configured... This initialization would probably better done in the server...
  2. The default parameters are not supported in the templated RPC framework. This can somehow be added, but wasn't part of the initial requirements.

There are a couple of things in the build process that I'd like to clean up, and for the packaging, I'd also like to separate out the exportable functions and the remote-only functions into separate headers, so that the host development can be better separated.

:+1:

jsturdy commented 4 years ago 
1. I see that the `logger` variable is initialized as a global variable. Does it truly work? I _think_ it should always work for the second loaded module, because the configuration is only _reset_ at each module initialization. However, I'm not sure it will work for the first module to be loaded (if it happens that the first module gets a logger before initialization). Although `log4cplus` is maybe initialized, but not configured... This initialization would probably better done in the server...

Indeed, I have no idea if anything works at runtime yet, and I also dislike this current method of the logger so prefer the aforepromised abstraction away from the module code. What I've done was just to have things currently compile, while expecting that certain other features would still be optimized to the new framework.

2. The default parameters are not supported in the templated RPC framework. This can somehow be added, but wasn't part of the initial requirements.

I don't think it's necessary to add them, we can ensure that the defaults will be defined in the cmsgemos HW library, and as you mentioned previously, the defaults will work as expected on "local" calls. Unless there's a use case that I'm potentially missing where having the defaults actually work through the RPC would be a benefit... (I guess removing the defaults allows better compile time checks of API compatiblity...)

jsturdy commented 4 years ago

There are a couple of things in the build process that I'd like to clean up, and for the packaging, I'd also like to separate out the exportable functions and the remote-only functions into separate headers, so that the host development can be better separated.

While tackling this problem, I've created the following structure:

include
├── ctp7_modules
    ├── common
    │   ├── amc
    │   │   ├── blaster_ram_defs.h
    │   │   ├── blaster_ram.h
    │   │   ├── daq.h
    │   │   ├── sca_enums.h
    │   │   ├── sca.h
    │   │   └── ttc.h
    │   ├── amc.h
    │   ├── calib
    │   ├── calibration_enums.h
    │   ├── calibration_routines.h
    │   ├── daq_monitor.h
    │   ├── gbt
    │   ├── gbt.h
    │   ├── hw_constants_checks.h
    │   ├── hw_constants.h
    │   ├── oh
    │   │   └── optohybrid_enums.h
    │   ├── optohybrid.h
    │   ├── utils.h
    │   ├── vfat
    │   ├── vfat3.h
    │   └── vfat_parameters.h
    ├── packageinfo.h
    └── server
         ├── amc
         ├── calib
         ├── gbt
         ├── lmdb_cpp_wrapper.h
         ├── LockTools.h
         ├── LogManager.h
         ├── memhub.h
         ├── moduleapi.h
         ├── ModuleManager.h
         ├── oh
         ├── vfat
         └── wiscRPCMsg.h

There are some subfolder placeholders in the event that some things get broken up that way. I immediately ran into the issue that moduleapi.h is #includeed in register.h from xhal and this restructuring caused a problem that needs a solution:

@lpetre-ulb @lmoureaux, there was some discussion some time ago about certain files currently part of ctp7_modules that should migrate to xhal, was this one of them? (I know that wiscRPCMsg.h was one, and that gets #includeed by moduleapi.h)

lmoureaux commented 4 years ago

Since moduleapi.h is just a bunch of #includes, most of which come from the rpcsvc machinery, I think it can be dropped altogether.

jsturdy commented 4 years ago

After the templated migration, the only part of this that is seen in ctp7_modules code is the ModuleManger, the rest is only needed in the xhal code. The ctp7_modules have to #include xhal/common/rpc/register.h, so migrating this to xhal/extern like was done for wiscRPCMsg.h would make sense to me.

lmoureaux commented 4 years ago

Ah, I assumed the ModuleManager was already in xhal. Then:

This way the wiscrpc-specific classes are as hidden as possible.

jsturdy commented 4 years ago

One more, in ctp7_modules we have lmdb_cpp_wrapper.h, which is identical to lmdb++.h in xhal, so I suppose we should change the only reference in ctp7_modules to #include the file from xhal/extern

lpetre-ulb commented 4 years ago

Functions have been ported in #167.