Feature/ge21 compatibility #119

mexanick · 2019-06-13T09:42:41Z

Description

Implements GE2/1 compatibility. All new parameters are backed with default values set to GE1/1 defaults. Thus the rest of the software should be able to work unchanged. Requires cms-gem-daq-project/ctp7_modules#132 to work with GE2/1

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

Need to be able to use the current legacy software for GE2/1 integration stand

How Has This Been Tested?

Tested with GE2/1 integration stand.

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

cursory

bdorney · 2019-06-13T18:30:49Z

Are all the commits in your development area uploaded here?

E.g. many files remain unmodified and I would expect some changes are needed to calibration functions, VFAT functions, optohybrid functions (broadcast read/write come immediately to mind). I recall that there are multiple locations in which array sizes are checked; while empty elements for VFATs from 13-23 are not an issue they are a potential memory overhead. Not sure if that should be addressed here or wait for cmsgemos and retirement of this branch completely.

Not sure if these were also updated in the ctp7_modules repo...? On my phone so it’s hard to check.

bdorney · 2019-06-25T09:12:24Z

xhalcore/src/common/rpc_manager/daq_monitor.cc

@@ -458,9 +458,15 @@ DLLEXPORT uint32_t getmonVFATLink(struct VFATLinkMonitor *vfatLinkMon, uint32_t
                for(int vfatN = 0; vfatN < 24; ++vfatN){


nvfats missing here I think.

Isn't this still outstanding?

bdorney · 2019-06-25T09:13:42Z

xhalcore/src/common/rpc_manager/vfat3.cc

@@ -174,7 +175,7 @@ DLLEXPORT uint32_t readVFAT3ADCMultiLink(uint32_t ohMask, uint32_t *ohVfatMaskAr
        return 1;
    }

-    const uint32_t size = 12*24;
+    const uint32_t size = 12*nvfats;


do we want this 12 to be replaced with an nohs? I'm fine with the answer being "no". But again this would present a potential issue for a setup driven by GLIB.

bdorney · 2019-06-25T09:18:50Z

Still need some GE2/1 vs. GE1/1 fixes, e.g.:

number of GBT's here:

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L254-L261

Number of VFATs here:

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L263-L269
https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L458-L469

For GE2/1 this might generate key errors?

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L339-L366

Aside from these issues I think you're good; the SCA one could also be a separate issue.

bdorney

Still a few places where nvfats or NGBT's is not used appropriately.

jsturdy · 2019-06-25T09:53:48Z

xhalcore/src/common/rpc_manager/vfat3.cc

@@ -115,7 +115,7 @@ DLLEXPORT uint32_t getVFAT3ChipIDs(uint32_t * chipIDData, uint32_t ohN, uint32_t
    }

    std::string regBase = "GEM_AMC.OH.OH" + std::to_string(ohN) + ".GEB.VFAT";
-    for(int vfat=0; vfat < 24; ++vfat)
+    for(int vfat=0; vfat < nvfats; ++vfat)


if you're changing to ensure signed/unsigned comparisons, probably you want to change here too.
also comment made here re unsigned int vs unsigned vs uintX_t

jsturdy · 2019-06-27T14:51:22Z

Needs rebase (preferably after addressing @bdorney's comments), and within the rebase preferably some cleanup of the commit history

…4 assuring the backwards compatibility

mexanick · 2019-07-03T13:46:42Z

I've rebased. The compatibility in the daq_monitor part deserves a separate issue in my opinion, as this also concerns the cmsgemos part and in general is not currently used, but will be used later eventually with refactored version of ctp7 modules, where the corresponding changes have to be implemented.

jsturdy

cursory

mexanick · 2019-07-05T08:16:55Z

@bdorney please see my comment considering outstanding issues

bdorney · 2019-07-08T08:32:11Z

Still need some GE2/1 vs. GE1/1 fixes, e.g.:

number of GBT's here:

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L254-L261

Number of VFATs here:

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L263-L269
https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L458-L469

For GE2/1 this might generate key errors?

https://github.com/mexanick/xhal/blob/58984c8cc70ffa03620a1388ce20fab7ba646da4/xhalcore/src/common/rpc_manager/daq_monitor.cc#L339-L366

Aside from these issues I think you're good; the SCA one could also be a separate issue.

@mexanick In addition to this comment here I see the above issues are still outstanding.

bdorney

Still seems like some outstanding issues have not been addressed, comments here

bdorney · 2019-07-08T08:34:42Z

I've rebased. The compatibility in the daq_monitor part deserves a separate issue in my opinion, as this also concerns the cmsgemos part and in general is not currently used, but will be used later eventually with refactored version of ctp7 modules, where the corresponding changes have to be implemented.

The SCA function I agree is a separate issue due to HW changes between boards; but the other issues I pointed out in daq_monitor.cc just require using nVFAT or nGBT in for loops and should be addressed before this is merged.

mexanick added Type: Enhancement Priority: Medium labels Jun 13, 2019

mexanick requested review from jsturdy, bdorney and lpetre-ulb June 13, 2019 09:42

mexanick self-assigned this Jun 13, 2019

pr-triage bot added the PR: unreviewed label Jun 13, 2019

jsturdy previously approved these changes Jun 13, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jun 13, 2019

mexanick dismissed jsturdy’s stale review via 16b8cee June 20, 2019 12:19

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Jun 20, 2019

bdorney reviewed Jun 25, 2019

View reviewed changes

bdorney suggested changes Jun 25, 2019

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jun 25, 2019

jsturdy reviewed Jun 25, 2019

View reviewed changes

mexanick added 8 commits July 3, 2019 15:44

modifying the GBT-related code to work with GE2/1

2119856

minor bugfixes

098dc9e

Fixing gbt scan to accept nVFAT arg

837ba34

Continue testing

5a11d67

temporary hack to locate incompatibility places

cb49a83

Another hack to test the compatibility

2cd1a02

Another hack to test the compatibility

eb9d119

Another hack to test the compatibility

ea8e31f

mexanick added 5 commits July 3, 2019 15:44

Improving logging for gbt functionality

39e0551

Introducing nvfats variable in appropriate places. Default value is 2…

fcf0c7c

…4 assuring the backwards compatibility

minor typo fixes

4b0f0b9

Implementing nVerificationReads for GBT phase scan

d83cdc0

Fixing GBT header

11532f2

mexanick force-pushed the feature/ge21-compatibility branch from 58984c8 to 11532f2 Compare July 3, 2019 13:45

pr-triage bot added PR: unreviewed and removed PR: reviewed-changes-requested labels Jul 3, 2019

mexanick requested review from jsturdy and bdorney July 3, 2019 15:39

jsturdy approved these changes Jul 4, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jul 4, 2019

bdorney suggested changes Jul 8, 2019

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested and removed PR: reviewed-approved labels Jul 8, 2019

bdorney approved these changes Jul 8, 2019

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: reviewed-changes-requested labels Jul 8, 2019

jsturdy mentioned this pull request Jul 8, 2019

Feature/ge21compatibility cms-gem-daq-project/cmsgemos#287

Merged

8 tasks

mexanick merged commit 721e3e8 into cms-gem-daq-project:legacyPreBoost Jul 8, 2019

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ge21 compatibility #119

Feature/ge21 compatibility #119

mexanick commented Jun 13, 2019

jsturdy left a comment

bdorney commented Jun 13, 2019 •

edited

Loading

bdorney Jun 25, 2019

bdorney Jul 8, 2019

bdorney Jun 25, 2019

bdorney commented Jun 25, 2019

bdorney left a comment

jsturdy Jun 25, 2019

jsturdy commented Jun 27, 2019

mexanick commented Jul 3, 2019

jsturdy left a comment

mexanick commented Jul 5, 2019

bdorney commented Jul 8, 2019

bdorney left a comment

bdorney commented Jul 8, 2019

		@@ -458,9 +458,15 @@ DLLEXPORT uint32_t getmonVFATLink(struct VFATLinkMonitor *vfatLinkMon, uint32_t
		for(int vfatN = 0; vfatN < 24; ++vfatN){

Feature/ge21 compatibility #119

Feature/ge21 compatibility #119

Conversation

mexanick commented Jun 13, 2019

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

bdorney commented Jun 13, 2019 • edited Loading

bdorney Jun 25, 2019

Choose a reason for hiding this comment

bdorney Jul 8, 2019

Choose a reason for hiding this comment

bdorney Jun 25, 2019

Choose a reason for hiding this comment

bdorney commented Jun 25, 2019

bdorney left a comment

Choose a reason for hiding this comment

jsturdy Jun 25, 2019

Choose a reason for hiding this comment

jsturdy commented Jun 27, 2019

mexanick commented Jul 3, 2019

jsturdy left a comment

Choose a reason for hiding this comment

mexanick commented Jul 5, 2019

bdorney commented Jul 8, 2019

bdorney left a comment

Choose a reason for hiding this comment

bdorney commented Jul 8, 2019

bdorney commented Jun 13, 2019 •

edited

Loading