tinyfpga / TinyFPGA-Bootloader

An open source USB bootloader for FPGAs
Apache License 2.0
357 stars 94 forks source link

Handle port errors gracefully and print proper error messages #45

Closed mbuesch closed 5 years ago

mbuesch commented 5 years ago

Currently tinyprog just throws the exception to the console in case of communication errors on the port:

Serial:

$ tinyprog -p test.bin                                                                                                                  

    TinyProg CLI                                                                                                                      
    ------------                                                                                                                      
    Using device id 1d50:6130                                                                                                         
    Only one board with active bootloader, using it.                                                                                  

Traceback (most recent call last):                                                                                                    
  File "/usr/lib/python3/dist-packages/serial/serialposix.py", line 265, in open                                                     
    self.fd = os.open(self.portstr, os.O_RDWR | os.O_NOCTTY | os.O_NONBLOCK)                                                         
PermissionError: [Errno 13] Permission denied: '/dev/ttyACM0'                                                                         

During handling of the above exception, another exception occurred:                                     

Traceback (most recent call last):                            
  File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main                                                               
    "__main__", mod_spec)                                     
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code                                                                          
    exec(code, run_globals)                       
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__main__.py", line 474, in <module>                             
    main()                                                                                                                           
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__main__.py", line 375, in main                                 
    with active_port:                           
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__init__.py", line 97, in __enter__
    self.port_name, timeout=1.0, writeTimeout=1.0).__enter__()
  File "/usr/lib/python3/dist-packages/serial/serialutil.py", line 240, in __init__
    self.open()                                                                                                                      
  File "/usr/lib/python3/dist-packages/serial/serialposix.py", line 268, in open                                                      
    raise SerialException(msg.errno, "could not open port {}: {}".format(self._port, msg))
serial.serialutil.SerialException: [Errno 13] could not open port /dev/ttyACM0: [Errno 13] Permission denied: '/dev/ttyACM0'

USB:

$ tinyprog --libusb -b
Traceback (most recent call last):
  File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__main__.py", line 478, in <module>
    main()
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__main__.py", line 296, in main
    active_boards = get_ports(device) + get_ports("1209:2100")
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__init__.py", line 73, in get_ports
    for d in usb.core.find(idVendor=vid, idProduct=pid, find_all=True)
  File ".../TinyFPGA-Bootloader/programmer/tinyprog/__init__.py", line 74, in <listcomp>
    if not d.is_kernel_driver_active(1)
  File "/usr/lib/python3/dist-packages/usb/core.py", line 1061, in is_kernel_driver_active
    self._ctx.managed_open()
  File "/usr/lib/python3/dist-packages/usb/core.py", line 102, in wrapper
    return f(self, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/usb/core.py", line 120, in managed_open
    self.handle = self.backend.open_device(self.dev)
  File "/usr/lib/python3/dist-packages/usb/backend/libusb1.py", line 786, in open_device
    return _DeviceHandle(dev)
  File "/usr/lib/python3/dist-packages/usb/backend/libusb1.py", line 643, in __init__
    _check(_lib.libusb_open(self.devid, byref(self.handle)))
  File "/usr/lib/python3/dist-packages/usb/backend/libusb1.py", line 595, in _check
    raise USBError(_strerror(ret), ret, _libusb_errno[ret])
usb.core.USBError: [Errno 13] Access denied (insufficient permissions)

To fix this all low level exceptions are caught and translated to a generic port exception. That port exception is then translated to a proper error message on stderr:

Serial:

$ tinyprog -p test.bin                                                        

    TinyProg CLI                                               
    ------------                                                   
    Using device id 1d50:6130                                                                                                        
        Only one board with active bootloader, using it.

Failed to open serial port:                                                     
[Errno 13] could not open port /dev/ttyACM0: [Errno 13] Permission denied: '/dev/ttyACM0'

USB:

$ tinyprog --libusb -b
Failed to open USB:
[Errno 13] Access denied (insufficient permissions)
mbuesch commented 5 years ago

I just want to add that the changes to __main__.py look more intrusive than they are. It basically just indents the code one more level and adds a try-except block for handling and printing the exception. You can check that by looking at the diff with whitespace changes ignored (-w).

Does this look reasonable, or is a change needed? Thanks a lot for your review.

mithro commented 5 years ago

Only issue is the change to the output caused by increasing the indenting level of the stuff inside the print.