[feature_request] writeGBTPhase.py takes input textfile and writes all phases

[bdorney]

Brief summary of issue

Request to change writeGBTPhase.py to be able to write all phases instead of the phase for a single VFAT.

Types of issue

• Bug report (report an issue with the code)
• Feature request (request for change which adds functionality)

Expected Behavior

Add subparser to have different commands, e.g. single and all where single is the current behavior.

Then all command should have a required input argument filename that is something like:

/data/bigdisk/GEM-Data-Taking/GE11_QC8//GE11-X-S-INDIA-0008/gbtPhaseSetPoints_GE11-X-S-INDIA-0008_current.log

Then for each (link,vfat) defined in the file it writes the phase to the GBT. After all phases are written it should issue a link reset.

This enables you to write either one OH link worth of phases or all phases on an AMC depending on file contents.

Current Behavior

Each call can only write one phase.

Context

Right now there's no easy way to write a set of known phases at once to the frontend. QC8 shifters generally end up having to relaunch testConnectivity.py to do a phase scan (slow, especially with many links) or need to call writeGBTPhase.py 24 times per OH to set known phases.

[ram1123]

@jsturdy , @bdorney: Please see the updated script ram1123@2b6b271 .

Also, I tried to test it on gem904qc8daq machine. It seems it run without any error. You can see the output below.

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py single 0 7 4 2 5
Open pickled address table if available  /opt/cmsgemos/etc/maps//amc_address_table_top.pickle...
Initializing AMC gem-shelf02-amc05
Initial value to write: 1, register GEM_AMC.GEM_SYSTEM.CTRL.LINK_RESET
Goodbye

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py all /data/bigdisk/GEM-Data-Taking/GE11_QC8/GE11-X-S-INDIA-0015/gbtPhaseSetPoints_GE11-X-S-INDIA-0015_current.log 2 5
List of link and its phases: 4 [7, 9, 10, 5, 5, 12, 7, 8, 7, 9, 4, 4, 6, 4, 11, 6, 9, 9, 8, 6, 10, 10, 13, 7]
Open pickled address table if available  /opt/cmsgemos/etc/maps//amc_address_table_top.pickle...
Initializing AMC gem-shelf02-amc05
Initial value to write: 1, register GEM_AMC.GEM_SYSTEM.CTRL.LINK_RESET
Goodbye
[gemuser@gem904qc8daq ~]$ 

Also, its help argument goes like below:

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py -h
usage: writeGBTPhase.py [-h] {single,all} ... shelf slot

Tool for writing GBT phase for a single or all elink

positional arguments:
  {single,all}  Choose single/all for writing the GBT phase
    single      write GBT phase for single VFAT
    all         write GBT phase for all VFAT
  shelf         uTCA shelf number
  slot          AMC slot number in the uTCA shelf

optional arguments:
  -h, --help    show this help message and exit

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py single -h
usage: writeGBTPhase.py single [-h] vfat phase link

positional arguments:
  vfat        VFAT number on the OH
  phase       GBT Phase Value to Write
  link        OH number on the AMC

optional arguments:
  -h, --help  show this help message and exit

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py all -h
usage: writeGBTPhase.py all [-h] gbtPhaseFile

positional arguments:
  gbtPhaseFile  file having link, vfat and phase info

optional arguments:
  -h, --help    show this help message and exit

[bdorney]

@ram1123 I provided some change requests on the commit message you linked above.

[jsturdy]

This is not the best way to go, as comments in linked code are not easily traceable through the issue.
Some tips:

• The issue should be for discussing the mechanics and usage, and once that is squared away, a PR made.
• In the PR comments on the implementation details should be made.
• If you want to link to snippets, do inline snippet linking in replies to the issue, such that they'll show up in the issue thread
• Don't rebase until you're making the PR because the history will be garbled in the issue itself.

[ram1123]

@bdorney

    import argparse
    parser = argparse.ArgumentParser(description="Tool for writing GBT phase for a single or all elink")

    subparsers = parser.add_subparsers(help='Choose single/all for writing the GBT phase', dest='command')
    parser_single = subparsers.add_parser("single", help='write GBT phase for single VFAT')
    parser_all = subparsers.add_parser("all", help='write GBT phase for all VFAT')

    parser.add_argument("shelf", type=int, help="uTCA shelf number")
    parser.add_argument("slot", type=int, help="AMC slot number in the uTCA shelf")

    # create the sub-parser for writing the GBT phase for "single" elink
    parser_single.add_argument("vfat", type=int, help="VFAT number on the OH")
    parser_single.add_argument("phase", type=int, help="GBT Phase Value to Write")
    parser_single.add_argument("link", type=int, help="OH number on the AMC")

    # create the sub-parser for writing the GBT phase for "all" elink
    parser_all.add_argument("gbtPhaseFile", type=str, help="file having link, vfat and phase info")

    args = parser.parse_args()

These I would have as a parent parser. For example I create a parent parser here:

https://github.com/cms-gem-daq-project/vfatqc-python-scripts/blob/54711c0191d20c2c3cfa026b56d01eb6a393f07c/run_scans.py#L521-L524

And then it is used when defining the sub commands, e.g.:

https://github.com/cms-gem-daq-project/vfatqc-python-scripts/blob/54711c0191d20c2c3cfa026b56d01eb6a393f07c/run_scans.py#L536

If we define it like this then it does not shows the common options as shown below:

(base) visitor-48597916:XDAQ ram$ python writeGBTPhase.py -h
usage: writeGBTPhase.py [-h] {single,all} ...

Tool for writing GBT phase for a single or all elink

positional arguments:
  {single,all}  Available subcommands and their descriptions.To view the sub
                menu callwriteGBTPhase.py COMMAND -h
                e.g.'writeGBTPhase.py single -h'
    single      write GBT phase for single VFAT
    all         write GBT phase for all VFAT

optional arguments:
  -h, --help    show this help message and exit

The current version (as I did) shows the help argument as:

[gemuser@gem904qc8daq ~]$ python writeGBTPhase.py -h
usage: writeGBTPhase.py [-h] {single,all} ... shelf slot

Tool for writing GBT phase for a single or all elink

positional arguments:
  {single,all}  Choose single/all for writing the GBT phase
    single      write GBT phase for single VFAT
    all         write GBT phase for all VFAT
  shelf         uTCA shelf number
  slot          AMC slot number in the uTCA shelf

optional arguments:
  -h, --help    show this help message and exit

So, this shows that there are two different sub-arguments along with two common arguments shelf and slot. Thus, I would prefer the current way. If you still want me to do it like as you suggested I can do. Let me know.

[ram1123]

@bdorney

from xhal.reg_interface_gem.core.gbt_utils_extended import setPhaseAllVFATs            setPhaseAllVFATs(cardName, int(link), listOfPhases)

Does setPhaseAllVFATs check that the inputs it's given are valid before attempting to write? If not you might want to make those checks here before calling the function.

yes it checks, here

[ram1123]

@bdorney

The GEM_AMC.GEM_SYSTEM.CTRL.LINK_RESET register is applied to the full FW block, e.g. it is not link specific.

You should change lines 49 to 55 to a single line:

vfatBoard.parentOH.parentAMC.writeRegister("GEM_AMC.GEM_SYSTEM.CTRL.LINK_RESET", 0x1)

This could be done inside your writeSinglePhase and writeAllPhases functions mentioned above

In the above command the vfatBoard is defined as

cmsgemos/gempython/tests/writeGBTPhase.py

Lines 21 to 22 in ab72ed6

	vfatBoard = HwVFAT(cardName,args.link)
	vfatBoard.parentOH.parentAMC.writeRegister("GEM_AMC.GEM_SYSTEM.CTRL.LINK_RESET",0x1)

Here vfatBoard takes args.link as one of argument. As per the current format of the text file, e.g. gbtPhaseSetPoints_GE11-X-S-INDIA-0008_current.log, it has only one link. But, this code will also work, even if the input text file has multiple links. So, it should be in the loop of link.

[bdorney]

Addressed by #283