Feature/ge21compatibility #287

mexanick · 2019-06-13T09:38:51Z

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

please rebase

bdorney · 2019-06-14T07:16:38Z

In the CTP7 modules PR we added things like NUM_OH_PER_AMC. This is covered in HwAMC by a read of the register that says how many OH's are in the FW but one thing that's potentially overlooked are these lines:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L409-L411

e.g. the 0xfff bit mask used in this operation is specific to 12 OH case...should this also be protected by a per gemType constant?

bdorney · 2019-06-14T07:17:35Z

In the CTP7 modules PR we added things like NUM_OH_PER_AMC. This is covered in HwAMC by a read of the register that says how many OH's are in the FW but one thing that's potentially overlooked are these lines:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L409-L411

e.g. the 0xfff bit mask used in this operation is specific to 12 OH case...should this also be protected by a per gemType constant?

Same question here:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L464-L470

bdorney · 2019-06-14T07:18:56Z

I think you need to add self.NVFATs here:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L665-L669

Specifically:

        lenExpected = self.nOHs * (maxVfat3DACSize[dacSelect][0] - 0+1)*24 / dacStep

Also the following locations:

Also need self.parentAMC.NVFATs here:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/optohybrid_user_functions_xhal.py#L486

bdorney · 2019-06-14T07:20:05Z

Need self.NOHs in following locations:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L828

bdorney · 2019-06-14T07:21:20Z

Need documentation update in following locations I think:

bdorney · 2019-06-14T07:25:58Z

Probably change how mask in broadcast methods works:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/optohybrid_user_functions_xhal.py#L106-L141

Propose to add to HwOptohybrid:

self.defaultVFATMask as a per gemType parameter in the __init__() method
Broadcast methods then have mask argument default to None
If mask argument is None in broadcast methods the method will use self.defaultVFATMask which is the gemType specific usage; otherwise it uses the input mask argument.

Otherwise I think you'll run into bus errors at some point.

If above solution is adopted then the following method would also need a similar modification:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/vfat_user_functions_xhal.py#L346-L347

bdorney · 2019-06-14T07:27:35Z

Looks like no changes to this method, e.g. documentation illustrates that outData should be of a size that expects 24 VFATs and 12 OHs. Do we want to have unused memory like this?

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/optohybrid_user_functions_xhal.py#L143-L176

Same question for the below:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/optohybrid_user_functions_xhal.py#L350-L392

bdorney · 2019-06-14T07:32:19Z

Probably need a per gemType constant for the number of channels in a detector, e.g.:

bdorney

Outstanding questions and/or clarification needed. Some additional compatibility issues raised.

bdorney · 2019-06-14T07:34:23Z

I'm not sure I caught everything...

This reverts commit f4ffbf3.

mexanick · 2019-06-18T13:34:54Z

In the CTP7 modules PR we added things like NUM_OH_PER_AMC. This is covered in HwAMC by a read of the register that says how many OH's are in the FW but one thing that's potentially overlooked are these lines:
https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L409-L411
e.g. the 0xfff bit mask used in this operation is specific to 12 OH case...should this also be protected by a per gemType constant?

Same question here:

https://github.com/mexanick/cmsgemos/blob/4cc3741fae8358643c885443a674c694686775ec/gempython/tools/amc_user_functions_xhal.py#L464-L470

I don't think a protections is needed here

bdorney · 2019-06-25T14:37:17Z

gempython/tools/amc_user_functions_xhal.py

@@ -185,6 +185,12 @@ def __init__(self, cardName, debug=False, gemType="ge11"):
        else:
            raise KeyError("Unrecognized gemType {0}".format(gemType))

+        #Determine the number of VFATs per geb based on the gemType
+        if gemType in vfatsPerGemVariant.keys():
+            self.NVFAT = vfatsPerGemVariant[gemType]


we won't have a situation where detType has a different number of VFATs correct?
e.g. {M1,...,M8} all have 12 VFATs?

Also in HwOptohybrid this is self.nVFATs vs. self.NVFAT.

Should we keep both these data members...in retrospect probably the better way to go would have been to have:

HwOptohybrid inherit from HwAMC and HwVFAT inherit from HwOptohybrid. Then all this parentX crap which is, admittedly, messy could disappear. Although whether that type of change is appropriate for this PR (or ever given this is #legacy) is unclear. Alas...my programming skills have improved since the original design...

I would caution against making API changes at this point, since this legacy stuff is supposed to be frozen anyway...
We will have to potentially think of how to map this in the updated API, where currently there is not going to be a HwVFAT, though it could be added if deemed absolutely critical (would just have to think of how best to do so...)

I would caution against making API changes at this point, since this legacy stuff is supposed to be frozen anyway...

Indeed. For this PR then I would suggest we keep both data members but just standardized the data member name in both classes for QOL; so it should be either self.NVFAT or self.nVFATs with a slight preference for self.nVFATs simply because it pre-dates this PR in HwOptohybrid.

gempython/tools/optohybrid_user_functions_xhal.py

bdorney

some typos in function documentation and one spot where I think an AttributeError will be raised at runtime.

jsturdy · 2019-06-27T14:55:33Z

If @bdorney is satisfied with changes, please rebase to clean up the commits (not before as it will make the review more difficult)

bdorney · 2019-06-28T08:31:29Z

If @bdorney is satisfied with changes, please rebase to clean up the commits (not before as it will make the review more difficult)

My only outstanding comment was here.

mexanick · 2019-07-03T12:51:01Z

I believe all the comments has been addressed now

jsturdy

OK from my side

jsturdy · 2019-07-08T10:15:52Z

@mexanick, merging this wouldn't break backwards compatibility (GE1/1) until cms-gem-daq-project/xhal#119 and cms-gem-daq-project/ctp7_modules#132 are merged, right?

mexanick · 2019-07-08T10:16:59Z

It should not break GE1/1 compatibility at all

jsturdy · 2019-07-08T10:18:28Z

Grrr, @mexanick you merged... (I was going to squash)

mexanick · 2019-07-08T10:31:19Z

ups. We can rewind if you insist

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

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

mexanick self-assigned this Jun 13, 2019

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

jsturdy reviewed Jun 13, 2019

View reviewed changes

bdorney suggested changes Jun 14, 2019

View reviewed changes

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

mexanick added 8 commits June 18, 2019 15:33

Changing NGBT in hw_constants, adding additional debug info

30b4fb9

Fixing gemType propagation to HwAMC from HwOH

2770c29

Revert "Fixing gemType propagation to HwAMC from HwOH"

4681866

This reverts commit f4ffbf3.

Fixing gemType propagation to HwAMC from HwOH

fdf2853

Fixing hardcoded NVFATs

9bca633

Fixing GBT scan

edf11e5

Trying another phase

e37907d

adding default phases for GE2/1 from Evaldas's tests

391ceef

Fixing doc

8d1c662

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

bdorney reviewed Jun 25, 2019

View reviewed changes

gempython/tools/optohybrid_user_functions_xhal.py Outdated Show resolved Hide resolved

bdorney reviewed Jun 25, 2019

View reviewed changes

gempython/tools/optohybrid_user_functions_xhal.py Outdated Show resolved Hide resolved

bdorney reviewed Jun 25, 2019

View reviewed changes

gempython/tools/optohybrid_user_functions_xhal.py Outdated Show resolved Hide resolved

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

Fixing nVFAT typos

a5999e0

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

mexanick requested review from jsturdy and bdorney June 26, 2019 07:24

Switching NVFAT to nVFATs

8b022ff

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 approved these changes Jul 8, 2019

View reviewed changes

mexanick merged commit 0dedd40 into cms-gem-daq-project:generic-amc-RPC-v3-short-term 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/ge21compatibility #287

Feature/ge21compatibility #287

mexanick commented Jun 13, 2019

jsturdy left a comment

bdorney commented Jun 14, 2019

bdorney commented Jun 14, 2019

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019

bdorney left a comment •

edited

Loading

bdorney commented Jun 14, 2019

mexanick commented Jun 18, 2019

bdorney Jun 25, 2019

bdorney Jun 25, 2019

jsturdy Jun 26, 2019

bdorney Jun 26, 2019 •

edited

Loading

bdorney left a comment

jsturdy commented Jun 27, 2019

bdorney commented Jun 28, 2019

mexanick commented Jul 3, 2019

jsturdy left a comment

jsturdy commented Jul 8, 2019

mexanick commented Jul 8, 2019

jsturdy commented Jul 8, 2019

mexanick commented Jul 8, 2019

Feature/ge21compatibility #287

Feature/ge21compatibility #287

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 14, 2019

bdorney commented Jun 14, 2019

bdorney commented Jun 14, 2019 • edited Loading

bdorney commented Jun 14, 2019

bdorney commented Jun 14, 2019 • edited Loading

bdorney commented Jun 14, 2019 • edited Loading

bdorney commented Jun 14, 2019 • edited Loading

bdorney commented Jun 14, 2019

bdorney left a comment • edited Loading

Choose a reason for hiding this comment

bdorney commented Jun 14, 2019

mexanick commented Jun 18, 2019

bdorney Jun 25, 2019

Choose a reason for hiding this comment

bdorney Jun 25, 2019

Choose a reason for hiding this comment

jsturdy Jun 26, 2019

Choose a reason for hiding this comment

bdorney Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

bdorney left a comment

Choose a reason for hiding this comment

jsturdy commented Jun 27, 2019

bdorney commented Jun 28, 2019

mexanick commented Jul 3, 2019

jsturdy left a comment

Choose a reason for hiding this comment

jsturdy commented Jul 8, 2019

mexanick commented Jul 8, 2019

jsturdy commented Jul 8, 2019

mexanick commented Jul 8, 2019

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney commented Jun 14, 2019 •

edited

Loading

bdorney left a comment •

edited

Loading

bdorney Jun 26, 2019 •

edited

Loading