cms-gem-daq-project / reg_utils

0 stars 9 forks source link

[code refactoring] improve gbt.py arguments and options with sub-commands (a la sca.py) #38

Closed AndrewLevin closed 5 years ago

AndrewLevin commented 5 years ago

gbt.py and gbt_utils.py have a somewhat inflexible command-line structure, which is not suitable as we add more functions to them.

Brief summary of issue

gbt.py uses positional arguments for the link, gbtx, and command:

        ohSelect = int(sys.argv[1])
        gbtSelect = int(sys.argv[2])
        command = sys.argv[3]

Then, in gbt_utils.py, what follows the command is interpreted as the arguments to the command, for example:

https://github.com/cms-gem-daq-project/reg_utils/blob/master/python/reg_interface/arm/gbt_utils.py#L36-L38

This implementation works OK for a small number of commands, but there are more robust solutions that will work better as we increase the number of commands, such as the use of ArgumentParser as was done in sca.py:

https://github.com/cms-gem-daq-project/reg_utils/blob/master/python/reg_interface/scripts/sca.py#L115

Types of issue

Expected Behavior

We would like to be able to do something like:

eagle64:~$ gbt.py <link> <command> --vfat <vfat> --phase <phase> 

(not the absence of a GBT number).

Current Behavior

eagle64:~$ gbt.py <link> <gbt number> <command> <command arguments>          

Context (for feature requests)

This issue was realized in the PR to add set-phase and read-phase commands to gbt.py: https://github.com/cms-gem-daq-project/reg_utils/pull/36. These commands take a VFAT number argument, which uniquely determine the GBT number.

bdorney commented 5 years ago

I think we missed the mark here. Look at how sca.py of this repo and run_scans.py of vfatqc (rpc-playground) are laid out.

The idea is to use ArgumentParser to make a set of sub-parsers where each sub-parser has it's own help menu and represents itself it's own commands. e.g. calling:

gbt.py -h

Would show the commands available:

gbt.py help menu

   config <config description>
   phase-scan <phase scan description>
   set-phase <set phase scan description>
   read-phase <read phase description>

Then calling:

gby.py config -h

   gbt.py config <OH0> <GBTx> <file>

   Description of positional arguments
    ...

   Description of optional arguments
    ...

Then each sub_parser would use the set_defaults(func=<somefunction>) mechanism to call a function when supplied and pass the necessary arguments.

This is exactly what we have done with sca.py of this repo and run_scans.py of the rpc-playground branch of vfatqc.

This is what myself and @jsturdy where asking for.

Before you start why not try to sketch an outline here of what you think you should be doing and then we can confirm that/provide feedback before starting. Take a look at these two scripts to get an idea.

mexanick commented 5 years ago

given that the gbt functionality goes towards implementation as an RPC module, do we still need to consider this?

bdorney commented 5 years ago

One could imagine a gbt.py python tool that uses this new rpc modules. Additionally a python based tool would be strongly useful in the lab. I think it's useful to keep this open. What do you think?

mexanick commented 5 years ago

Yes, I agree. But this would be rather a new tool/script development than a refactoring of the existing one.

bdorney commented 5 years ago

Closing as outdated; now GBT python functionality incorporated into xhal.