WIP [feature] templated RPC methods migration #164
Conversation
include/amc.h
Outdated
| uint32_t getOHVFATMaskLocal(localArgs * la, uint32_t ohN); | ||
| /*! | ||
| * \brief Returns AMC FW version | ||
| * In the case FW version is not 1.X or 3.X, throws std::runtime_error |
There was a problem hiding this comment.
/*!
* \throws std::runtime_error if the FW version is not 1.X or 3.X
*/There was a problem hiding this comment.
Guess this will have to be updated globally, as there are quite some places that now throw
include/amc.h
Outdated
| /*! | ||
| * \brief Returns AMC FW version | ||
| * In the case FW version is not 1.X or 3.X, throws std::runtime_error | ||
| * \param \c caller_name Name of method which called the FW version check FIXME necessary? |
There was a problem hiding this comment.
I guess the parameter is not needed, now would be a good time to drop it or at least make it optional.
There was a problem hiding this comment.
What were the use cases originally for including it?
Just to pass it to a potential LOGGER message?
| */ | ||
| struct getOHVFATMask : public xhal::rpc::Method | ||
| { | ||
| uint32_t operator()(const uint32_t &ohN) const; |
There was a problem hiding this comment.
Why constant references for parameters? I can't think of any reason to prefer them over values (for scalars; for heavier objects there is a good reason).
There was a problem hiding this comment.
Support for returning std::bitset would come handy here.
| */ | ||
| void getOHVFATMaskMultiLink(const RPCMsg *request, RPCMsg *response); | ||
| /*! | ||
| * \brief As \c getOHVFATMask but for all optical links specified in ohMask on the AMC |
There was a problem hiding this comment.
I suggest repeating the documentation instead of referring to another. If not, use \ref instead of \c.
There was a problem hiding this comment.
I think \c was an artifact, so not sure who introduced it nor what it does
There was a problem hiding this comment.
\c displays the following word in typewriter font. \ref is for a reference.
| * * if the size is more, an error will be thrown | ||
| * * if the size is less, only the number of words specified will be read from the RAM (maybe) | ||
| * * if ALL is specified, the size must be the total RAM size | ||
| * - GBT_RAM_SIZE*N_GBTX*N_OH + OH_RAM_SIZE*N_OH + VFAT_RAM_SIZE*N_VFAT*N_OH |
There was a problem hiding this comment.
Could introduce a function to calculate this.
include/amc/blaster_ram.h
Outdated
| * GEM_AMC.CONFIG_BLASTER.STATUS.VFAT_RAM_SIZE | ||
| */ | ||
| struct readConfRAM : xhal::rpc::Method { | ||
| uint32_t operator()(BLASTERTypeT const& type, uint32_t* blob, size_t const& blob_sz); |
There was a problem hiding this comment.
This method cannot be called remotely because of the pointer parameter. Return a std::vector? Same issue for several RPC methods after this one.
There was a problem hiding this comment.
Oops, this was a byproduct of partial implementation...
include/amc/blaster_ram_defs.h
Outdated
| * \brief With its intrusive serializer | ||
| */ | ||
| template<class Message> void serialize(Message & msg) { | ||
| msg & EBLASTERType; |
There was a problem hiding this comment.
Does this compile? EBLASTERType is the enum type, shouldn't it be BLASTERType?
There was a problem hiding this comment.
This particular commit probably doesn't, but the compilation is fixed in a later commit.
As I go through to fix the comments here, I'll backport that
There was a problem hiding this comment.
What do ya know, it actually does compile :-D, but this still needs the update viz scoped enums
|
I opened #167 to supersede this (it contains all the changes mentioned in the description of this, and is the hopeful starting point for integrated development between |
This PR holds the code of the migration to the templated methods.
It will be built up as the individual modules are merged into my fork, and will only become non WIP after all code has found its way into that fork (so review of the independent parts can still happen in bite-size chunks).
As it stands here, the changes have been applied to the core libraries as well as the
utilsandamcmodules.Description
Significant and breaking
Types of changes
Motivation and Context
Necessary to bring the code base into the future.
How Has This Been Tested?
Only with the compiler so far, will envision a set of operational tests to follow before final merge.
Checklist: