Hw access optimization #327

mexanick · 2020-03-19T13:03:40Z

Description

This PR brings:

Complete removal of uhal dependencies everywhere in gemhardware except amc13-related code
Conversion of all the RPC calls in gemhardware to templated ones
Partial removal of obsolete functionality where it is obvious
- hardware monitoring classes are removed as not used
Cleaning of the FM transitions

The quality of the code is not up to industrial standards, but this is one more technical stop on the way to a better structured clean project. A number of issues were spotted during the work on this PR which have to be fixed before we can advance.

Notably:

namespaces accross the and has to be uniformized
Compile-time definition for GEM_VARIANT added to makefiles of gemhardware/managers and gemhardware/devices
- must be fixed with new build system. Eventually has to be converted into a runtime constant as we want the same backend software to work with different generation front-ends
lots of obsolete functions which require interface redesign
argument passing between and has to be uniformized - see New Feature: Adding setAllChannelRegisters to HwVFAT #173 of ctp7_modules

Cleaning the FSM transition, getting rid of uhal library.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

How Has This Been Tested?

Not yet tested on the hardware. Help is needed on building/deployment instructions for ctp7_modules. So far build system is unclear. However the code compiles and in general should run. Would also prefer a code review before stepping into testing phase.

Screenshots (if appropriate):

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

jsturdy

Just a few quick comments before it's ready for reveiw:

would be better to replace all the // Deprecated comments with [[deprecated]] or __attribute__((__deprecated__)) (if the functions themselves aren't going to be removed in this rework)
why are we reintroducing the duplication of remote vs local function calls? if the method in the CTP7 modules is desired outside of the modules themselves, then why not just export it directly, i.e., why duplicate readReg with the readRemoteReg wrapper, when the templating was introduced with one of the intended goals of removing this duplication?

I think everything else can wait for proper review

lpetre-ulb · 2020-03-20T16:51:49Z

* would be better to replace all the `// Deprecated` comments with `[[deprecated]]` or `__attribute__((__deprecated__))` (if the functions themselves aren't going to be removed in this rework)

👍 I would even remove the deprecated placeholder functions since they can lead to unexpected misbehavior (if cmsgemos still compiles without them).

* why are we reintroducing the duplication of remote vs local function calls? if the method in the CTP7 modules is desired outside of the modules themselves, then why not just export it directly, i.e., why duplicate `readReg` with the `readRemoteReg` wrapper, when the templating was introduced with one of the intended goals of removing this duplication?

Wouldn't these remote RPC methods be only temporary until we no longer need raw register accesses from cmsgemos? And replacing all function calls by functor calls in the ctp7_modules is not going to be pretty...

jsturdy · 2020-03-20T17:06:07Z

why are we reintroducing the duplication of remote vs local function calls? if the method in the CTP7 modules is desired outside of the modules themselves, then why not just export it directly, i.e., why duplicate readReg with the readRemoteReg wrapper, when the templating was introduced with one of the intended goals of removing this duplication?

Wouldn't these remote RPC methods be only temporary until we no longer need raw register accesses from cmsgemos? And replacing all function calls by functor calls in the ctp7_modules is not going to be pretty...

I don't know, either there's a use case for having these low level functions exposed on on the PC side, in which case I'd just export them as is and do a refactor in ctp7_modules now (it's a pretty simple regex replace to get it all done), or we don't want them outside, in which case why not just make that change now, i.e., create the ctp7_modules functions that replace the ones that are using these one-off calls. That's what I did with the HwGenericAMC -> amc module (although I didn't replace the calls in cmsgemos at the time).
The third alternative is to have the exported name the same as the "local" name, as they'll be in a different namespace, right?

lpetre-ulb · 2020-03-23T08:53:23Z

I don't know, either there's a use case for having these low level functions exposed on on the PC side, in which case I'd just export them as is and do a refactor in ctp7_modules now (it's a pretty simple regex replace to get it all done), or we don't want them outside, in which case why not just make that change now, i.e., create the ctp7_modules functions that replace the ones that are using these one-off calls. That's what I did with the HwGenericAMC -> amc module (although I didn't replace the calls in cmsgemos at the time).

I think the use case is only a temporary compatibility layer for migration purposes. I prefer to see multiple easily reviewable small PR rather than one huge PR with many changes. The mid-term goal could be like:

Get rid of uHAL (this PR)
Create RPC methods that replace the direct read/write accesses (one or more PR)
Create a RPC module for monitoring purposes (one or more PR)

The third alternative is to have the exported name the same as the "local" name, as they'll be in a different namespace, right?

For now, at least, they are in the same namespace, in this case, utils. But, the function would hide the class, so the behavior would be the one you expect, i.e. the function available for the ctp7_modules and the functor available for remote calls. That's terrible design though.

…roducing [[deprecated]] attributes

…resent in xml configurations as it relates to base class GEMApplication. Not sure what to do with it right now

…removing obsolete GLIB-related functionality, obsolete register access methods and HwGlib device class

…package. Lots of dirty hacks which won't be fixed in the current PR in order to keep it compact. Notably: * namespaces accross the and has to be uniformized * Compile-time definition for added to makefiles of and * must be fixed with new build system. Eventually has to be converted into a runtime constant as we want the same backend software to work with different generation front-ends * lots of obsolete functions which require interface redesign * argument passing between and has to be uniformized - see cms-gem-daq-project#173 of Code compiles and this is IMHO enough for the current PR

mexanick · 2020-03-25T14:34:29Z

please check updated PR description

lpetre-ulb

Nothing outstanding, can be compiled. Should be good for hardware tests. I would be careful where and how the INPUT_ENABLE_MASK is set.

lpetre-ulb · 2020-03-27T17:06:44Z

gemhardware/managers/src/common/amc/AMCManager.cc

-
-  // FIXME make me more streamlined
+  // this maybe shouldn't be done? Commenting out for now, but needs testing
+  //info.slotID  = slot+1;


To be tested. A changelog, or announcement, is required if the configuration has to be changed somehow.

lpetre-ulb · 2020-03-27T17:07:00Z

gemhardware/managers/src/common/amc/AMCManager.cc

-        // FIXME should not need this here?
-        amc->disableDAQLink();


To be tested, since it changes the behavior.

Indeed, but the main purpose if this PR is to have something which uses the templated RPC framework and can cycle through the FSM states. Proper implementation of the state transitions IMHO deserves another PR (and at the moment it has to be also chained with the corresponding changes of the ctp7_modules). As a lot of things are candidates for refactoring, simply making it work "as is but without uHAL" doesn't appeal too much to me

In general, I believe that regressions should not be allowed in PR. However, this PR as well as the next round of PR are kind of special due to the huge refactoring in progress. But what about the other developments which happen in parallel? Once this PR gets merged, how would the calibration suite be tested, for example?

I'd say there's more to the calibration suite tests... The system also can't be easily recovered from cold boot state. But if you insist, I can retrofit this back. However I can't test the real data taking until we fix the cooling

gemhardware/devices/include/gem/hw/devices/GEMHwDevice.h

gemhardware/devices/src/common/amc/HwGenericAMC.cc

gemhardware/devices/src/common/optohybrid/HwOptoHybrid.cc

gemhardware/managers/src/common/optohybrid/OptoHybridManager.cc

…the cmsgemos side, I make it compliant with the corresponding method of ctp7_modules

gemhardware/devices/src/common/optohybrid/HwOptoHybrid.cc

…faces (candidates for refactoring in base class) and linking issues against xhal-base (temporary hacks)

mexanick · 2020-04-02T11:40:05Z

Test report:

templated RPC calls could be successfully executed from the cmsgemos
linking problem against xhal-base has been discovered, which prevented correct exceptions treatment and subsequent code crash. Issue reported in xhal repository, suggested there temporary fix is implemented.
discovered quite a few attempts to read registers which are not leaf nodes and do not have "read" permissions. This part of the code wasn't changed in the current PR and I suspect that the problem was hidden by ipbus transactions ignoring register permissions. Since these bugs don't affect state transitions, I propose to address them in subsequent PR
FSM transition between "Initial" and "Halted" states is executed correctly. If the HW is present, the system goes to "Halted" state, if HW is not accessible, the system goes to "Error"
FSM transition between "Halted" and "Configured" state resulted in "Error" due to HW problem
"Reset" action from "Halted" state hangs up indefinitely

Following this I'd propose to merge this PR and address remaining and future issues in separate PRs. As a proof of concept, the system can work without uhal and with templated rpc.

jsturdy · 2020-04-02T14:08:45Z

gemhardware/devices/Makefile

@@ -55,7 +59,8 @@ LibraryDirs+=$(BUILD_HOME)/gemutils/lib/$(XDAQ_OS)/$(XDAQ_PLATFORM)
 LibraryDirs+=$(uHALROOT)/lib


is this still needed?

gemhardware/devices/Makefile

jsturdy · 2020-04-02T14:09:28Z

gemhardware/devices/Makefile

@@ -44,6 +47,7 @@ IncludeDirs+=$(BUILD_HOME)/gemhardware/utils/include
 IncludeDirs+=$(BUILD_HOME)/gemutils/include
 IncludeDirs+=$(uHALROOT)/include


Likewise, still refs uhal

gemhardware/devices/include/gem/hw/devices/GEMHwDevice.h

gemhardware/devices/include/gem/hw/devices/amc/HwGenericAMC.h

gemhardware/devices/src/common/amc/HwGenericAMC.cc

jsturdy · 2020-04-02T14:28:23Z

gemhardware/devices/src/common/amc/HwGenericAMC.cc

 }

-uint32_t gem::hw::HwGenericAMC::getTTCCounter(AMCTTCCommandT const& cmd)
+uint32_t gem::hw::amc::HwGenericAMC::getTTCCounter(AMCTTCCommandT const& cmd)


but in cases like this, where there is much more than just a single (read|write)Reg it would have made sense to use the ctp7_modules functor call

yes, but this is out of the scope of the current PR

jsturdy · 2020-04-02T14:28:31Z

gemhardware/devices/src/common/amc/HwGenericAMC.cc

 }

-uint32_t gem::hw::HwGenericAMC::getOptoHybridTriggerLinkCount(uint8_t const& oh, uint8_t const& link, AMCOHLinkCountT const& count)
+uint32_t gem::hw::amc::HwGenericAMC::getOptoHybridTriggerLinkCount(uint8_t const& oh, uint8_t const& link, AMCOHLinkCountT const& count)


~~likewise here~~ i guess not, as i see this one was not ported over...

gemhardware/managers/Makefile

gemhardware/managers/src/common/optohybrid/OptoHybridManager.cc

mexanick · 2020-04-02T15:01:30Z

Test update
With a cool enough front end all the FSM transitions are executed as before

…cludes in hw devices

lpetre-ulb

Approved as a technical PR. I prefer to see all new developments use the templated RPC and run into easily fixable issues rather than waiting months for the perfect PR; all issues from such a large change cannot be discovered by simple testing.

Changes implemented

mexanick added Priority: Medium Status: In Progress labels Mar 19, 2020

mexanick self-assigned this Mar 19, 2020

jsturdy reviewed Mar 20, 2020

View reviewed changes

mexanick added 7 commits March 23, 2020 15:06

Refactoring FSM transitions, still temporary

fd13d33

Another drop of obsolete FW support

c71e77c

Removing obsolete legacy (GLIB) support and fixing minor typos

2e97321

Implementing readReg, readBlock and writeReg

e05fd00

Techincal: fixing makefiles to include CTP7 headers where needed, int…

b5ddc08

…roducing [[deprecated]] attributes

Removing device monitors. The parameter DisableMonitoring is still pl…

ede0eb6

…resent in xml configurations as it relates to base class GEMApplication. Not sure what to do with it right now

Dropping uhal support from hardware devices with exception of AMC13, …

3b2e2f5

…removing obsolete GLIB-related functionality, obsolete register access methods and HwGlib device class

mexanick force-pushed the hw_access_optimization branch from 71bbaee to 3b2e2f5 Compare March 23, 2020 20:55

mexanick requested review from lpetre-ulb and cgalloni March 25, 2020 14:34

mexanick added Priority: High Status: Review Needed and removed Priority: Medium labels Mar 25, 2020

lpetre-ulb mentioned this pull request Mar 27, 2020

Uniformize types in RPC method arguments cms-gem-daq-project/ctp7_modules#173

Closed

3 tasks

lpetre-ulb reviewed Mar 27, 2020

View reviewed changes

This was referenced Mar 27, 2020

Feature/read write export cms-gem-daq-project/ctp7_modules#171

Merged

Remove v2 electronics hardware compatibility cms-gem-daq-project/ctp7_modules#174

Merged

Remove obsolete RPC modules cms-gem-daq-project/ctp7_modules#175

Merged

mexanick added 3 commits March 31, 2020 16:28

Removing obsolete scan functionality

02665dc

Fixing remaining leftovers from uhal

86b1979

Added read/write exceptions logging

ba93668

lpetre-ulb mentioned this pull request Mar 31, 2020

Provide templated RPC calls as a XHALInterface member function cms-gem-daq-project/xhal#149

Closed

2 tasks

Changing interface of configGBT. As this function is not yet used on …

4fecfa0

…the cmsgemos side, I make it compliant with the corresponding method of ctp7_modules

lpetre-ulb reviewed Apr 1, 2020

View reviewed changes

gemhardware/devices/src/common/optohybrid/HwOptoHybrid.cc Outdated Show resolved Hide resolved

Adding missing change in configGBT interface

c5d3a93

mexanick linked an issue Apr 1, 2020 that may be closed by this pull request

FSM actions refactoring #316

Closed

3 tasks

mexanick mentioned this pull request Apr 1, 2020

GEM_AMC.GEM_SYSTEM.RELEASE is not readable but used in HwGenericAMC #331

Open

2 tasks

Fixing missed function implementations in hardware managers web inter…

d0fab37

…faces (candidates for refactoring in base class) and linking issues against xhal-base (temporary hacks)

mexanick requested review from jsturdy and lpetre-ulb April 2, 2020 13:28

jsturdy previously requested changes Apr 2, 2020

View reviewed changes

Removing uhal dependency from gemhardware/devices Makefile, fixing in…

17a39f7

…cludes in hw devices

mexanick requested a review from jsturdy April 2, 2020 16:00

lpetre-ulb approved these changes Apr 6, 2020

View reviewed changes

mexanick changed the title ~~WIP: Hw access optimization~~ Hw access optimization Apr 6, 2020

pr-triage bot added the PR: partially-approved label Apr 6, 2020

mexanick merged commit ba2bc52 into cms-gem-daq-project:develop Apr 6, 2020

pr-triage bot added PR: merged and removed PR: partially-approved labels Apr 6, 2020

mexanick deleted the hw_access_optimization branch April 6, 2020 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hw access optimization #327

Hw access optimization #327

mexanick commented Mar 19, 2020 •

edited

jsturdy left a comment

lpetre-ulb commented Mar 20, 2020

jsturdy commented Mar 20, 2020

lpetre-ulb commented Mar 23, 2020

mexanick commented Mar 25, 2020

lpetre-ulb left a comment

lpetre-ulb Mar 27, 2020

lpetre-ulb Mar 27, 2020

mexanick Apr 1, 2020

lpetre-ulb Apr 1, 2020

mexanick Apr 2, 2020

mexanick commented Apr 2, 2020

jsturdy Apr 2, 2020

jsturdy Apr 2, 2020

jsturdy Apr 2, 2020

mexanick Apr 2, 2020

jsturdy Apr 2, 2020

mexanick commented Apr 2, 2020

lpetre-ulb left a comment

		@@ -55,7 +59,8 @@ LibraryDirs+=$(BUILD_HOME)/gemutils/lib/$(XDAQ_OS)/$(XDAQ_PLATFORM)
		LibraryDirs+=$(uHALROOT)/lib

		@@ -44,6 +47,7 @@ IncludeDirs+=$(BUILD_HOME)/gemhardware/utils/include
		IncludeDirs+=$(BUILD_HOME)/gemutils/include
		IncludeDirs+=$(uHALROOT)/include

Hw access optimization #327

Hw access optimization #327

Conversation

mexanick commented Mar 19, 2020 • edited

Description

Types of changes

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

jsturdy left a comment

Choose a reason for hiding this comment

lpetre-ulb commented Mar 20, 2020

jsturdy commented Mar 20, 2020

lpetre-ulb commented Mar 23, 2020

mexanick commented Mar 25, 2020

lpetre-ulb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexanick commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexanick commented Apr 2, 2020

lpetre-ulb left a comment

Choose a reason for hiding this comment

mexanick commented Mar 19, 2020 •

edited