cms-gem-daq-project / reg_utils

0 stars 9 forks source link

Bug Fix #21: sca.py no longer relies on sys.argv to obtain inputs #22

Closed bdorney closed 6 years ago

bdorney commented 6 years ago

Description

Exhibited several bugs with sca.py while testing v3 electronics. All of which seem to relate to lack of care with argument order of sys.argv. To be more pythonic I've implemented an argparser object and have done extensive testing.

Previous positional arguments have been maintained.

Noted significant divergences between sca.py on the texas account of CTP7 and our use case. This should be really frowned upon and active development on private area locations should be stopped.

Types of changes

Motivation and Context

Changes to sca.py over time have made the order of arguments challenging to keep up with and resulted in ambiguity. This addresses #21 and other incidents in which the addition of the <cardName> has caused an index error.

Additionally several undocumented instructions are now necessary to configure the OHv3c (bad). These are now documented.

How Has This Been Tested?

Extensively tested at this elog post Extensively tested using argparser at this elog post See post

New usage:

% sca.py -h
usage: sca.py [-h]
              cardName ohMask
              {compare-mcs-bit,h,hh,fpga-id,program-fpga,gpio-read-input,gpio-set-direction,gpio-set-output,r,sysmon}
              ...

Arguments to supply to sca.py

positional arguments:
  cardName              hostname of the AMC you are connecting too, e.g.
                        'eagle64'; if running on an AMC use 'local' instead
  ohMask                ohMask to apply, a 1 in the n^th bit indicates the
                        n^th OH should be considered
  {compare-mcs-bit,h,hh,fpga-id,program-fpga,gpio-read-input,gpio-set-direction,gpio-set-output,r,sysmon}
                        sca command help
    compare-mcs-bit     compares a *.mcs with a *.bit file to check if the
                        firmware is the same
    h                   FPGA hard reset will be done
    hh                  FPGA hard reset will be asserted and held
    fpga-id             FPGA ID will be read through JTAG
    program-fpga        Program OH FPGA with a bitfile or an MCS file
    gpio-read-input     Read GPIO settings of the SCA
    gpio-set-direction  Set the GPIO Direction Mask
    gpio-set-output     Set the GPIO output values
    r                   SCA reset will be done
    sysmon              Read FPGA sysmon data repeatedly

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

Additional option menus for sub_commands:

Comparing firmware files:

sca.py eagle60 0x2 compare-mcs-bit -h
usage: sca.py cardName ohMask compare-mcs-bit [-h] fwFileMCS fwFileBit

positional arguments:
  fwFileMCS   firmware mcs file to be used in the comparison
  fwFileBit   firmware bit file to be used in the comparison

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

Programming FPGA using JTAG via SCA:

% sca.py eagle60 0x2 program-fpga -h   
usage: sca.py cardName ohMask program-fpga [-h] fwFile

positional arguments:
  fwFile      firmware file to program fpga with, must end in either '*.bit'
              or '*.mcs'

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

Setting gpio direction:

% sca.py eagle60 0x2 gpio-set-direction -h
usage: sca.py cardName ohMask gpio-set-direction [-h] gpioValue

positional arguments:
  gpioValue   32 bit number where each bit represents a GPIO channel. If a
              given bit is high it means that this GPIO channel will be set to
              OUTPUT mode, and otherwise it will be set to INPUT mode

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

Setting gpio output:

% sca.py eagle60 0x2 gpio-set-output -h   
usage: sca.py cardName ohMask gpio-set-output [-h] gpioValue

positional arguments:
  gpioValue   32 bit number where each bit represents the 32 GPIO channels
              state

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

Comparing FW Files

sca.py eagle60 2 compare-mcs-bit OH-20180320-3.1.1.B.mcs OH-20180320-3.1.1.B.bit

Issuing Resets

sca.py eagle60 2 h
sca.py eagle60 2 hh
sca.py eagle60 2 r

Reading FPGA-ID

sca.py eagle60 0x2 fpga-id 

GPIO Commands

sca.py eagle60 0x2 gpio-read-input
sca.py eagle60 2 gpio-set-direction 0x0fffff8f
sca.py eagle60 2 gpio-set-output 0xf00000f0

Programming FPGA

This is not yet testable since it requires changes to setup_ctp7.sh and migration of reg_utils to the actual card. However the script correctly identifies that it should not be called from the DAQ machine with this command.

Reading sysmon

sca.py eagle60 0x2 sysmon

Checklist:

bdorney commented 6 years ago

Additionally the undocumented commands test1 and test2 are broken in the current software and it's not clear how to fix them or what they do. So I have dropped support for them at present.

If this is desired perhaps @mexanick or @evka85 can resolve the errors reported in the above elog which are beyond the scope of this PR.

jsturdy commented 6 years ago

I would be more in favour of implementing these changes using the ArgParse module: it's more future safe, and can persist the current usage, while improving the clarity of the usage. I had started to implement this quite some time ago, and then @mexanick made the change that prompted my start (card_name). The basics are effectively the same as OptParse, but there is a difference between an argument (mandatory), and an option (optional)

    import argparse
    parser = argparse.ArgumentParser(description='Arguments to supply to sca.py')
    parser.add_argument('card', metavar='card', type=str,
                        help='Birdname CTP7 hostname')
    parser.add_argument('oh_mask', metavar='oh_mask', type=str,
                        help='Mask of OptoHybrids to operate on, hex (0x) or binary (0b) accepted')
    parser.add_argument('command', metavar='command', type=str,
                        help='Command to send via SCA')
    parser.add_argument('-t', '--type', metavar='imgtype', type=str,
                        help='Firmware image type \'program-fpga\' command')
    parser.add_argument('-f','--file', metavar='imgfile', type=str,
                        help='Firmware image file to load for \'program-fpga\' command')
    parser.add_argument('--bitfile', metavar='bitfile', type=str,
                        help='BIT image file to load for \'compare-mcs-bit\' command')
    parser.add_argument('--mcsfile', metavar='mcsfile', type=str,
                        help='MCS image file to load for \'compare-mcs-bit\' command')

    args = parser.parse_args()

The syntax would remain identical for most use cases we've currently had, and the parsing and logic makes sure that for certain commands, the required options are specified.

bdorney commented 6 years ago

Ah I thought argparse was not available in python2.7 but only from 3.X.

Okay if it's available in 2.7 I can easily make this switch.

jsturdy commented 6 years ago

It's available even in 2.6, and officially, 2.7 deprecates optparse. I've been using argparse almost exclusively for any new thing I develop

bdorney commented 6 years ago

Switched to argparse

bdorney commented 6 years ago

Now using sub commands. Will test this afternoon.

bdorney commented 6 years ago

rebased onto 70d243e5d3513e734d289618876a22530633726d

bdorney commented 6 years ago

Okay so testing completed on the daq machine, everything behaves as expected.

I was unable to install the rpms (elog) on the ctp7 but would argue this is beyond the scope of this PR.

The only thing is needed to do from the ctp7 that cannot be done from the DAQ machine is to program the FPGA using JTAG via the SCA. However this is 100% superseded by the working promless functionality.

So if this meets your approval @jsturdy maybe we can merge it in (need the gpio functionality for OHv3c testing.)