Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: GlasgowEmbedded/glasgow
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 1e0ae593ec6a
Choose a base ref
...
head repository: GlasgowEmbedded/glasgow
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 2c7828f6fd7e
Choose a head ref
  • 2 commits
  • 4 files changed
  • 1 contributor

Commits on Nov 5, 2020

  1. applet.interface.jtag_probe: completely rework error handling. 

    JTAGProbeInterface historically suffered from underuse of exceptions;
it was originally written to return void values on error paths since
it strictly validates all input at every opportunity, and this does
not survive collision with the real world. (Can you believe there are
production JTAG devices where IDCODE is 32 zeroes?) Scanning, as well
as any future manual overrides, have to relax validation greatly.

Unfortunately, this approach caused proliferation of identical error
handling code in every caller, error propagation boilerplate, and
polymorphic return values. It was awful. This commit changes all
remaining code to use exceptions and, in most cases, accept a check=
argument to opt into returning void values where truly necessary.

The check= argument is not forwarded; callers that need to customize
error handling in a fine-grained way should inline and tweak the code
that needs to behave differently.
    @whitequark
    whitequark committed Nov 5, 2020
    Copy the full SHA
    89ae50c View commit details

  2. Stuff all of the custom_repl infrastructure into a bonfire. 

    Custom REPLs are very nice. custom_repl is awful for several reasons:
  1. It required the user to watch out for a warning and then
     remember to switch to a different, inconsistent command, or
     else experience strange breakage.
  2. It lacked any way to provide arguments to the REPL runner,
     which in turn was justifying the behavior in (1).
  3. It involved some incredibly gross metaclass code.
  4. It was completely unnecessary. All of the metaclass stuff has
     been replaced by a pair of methods that's easier to understand
     *and* provides more useful functionality (REPL arguments).

I don't know what I was thinking.
    @whitequark
    whitequark committed Nov 5, 2020
    Copy the full SHA
    2c7828f View commit details
Showing with 179 additions and 173 deletions.
  1. +11 −7 software/glasgow/applet/__init__.py
  2. +5 −6 software/glasgow/applet/debug/mips/__init__.py
  3. +156 −149 software/glasgow/applet/interface/jtag_probe/__init__.py
  4. +7 −11 software/glasgow/cli.py
18 changes: 11 additions & 7 deletions software/glasgow/applet/__init__.py
View file
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
import argparse
from abc import ABCMeta, abstractmethod

from ..support.pyrepl import *
from ..gateware.clockgen import *


@@ -21,11 +22,6 @@ def __new__(metacls, clsname, bases, namespace, name=None, **kwargs):
raise NameError(f"Applet {name!r} already exists")
namespace["name"] = name

# Any class that overrides interact() no longer has its superclass' custom REPL, so be
# helpful and reset that attribute.
if "has_custom_repl" not in namespace and "interact" in namespace:
namespace["has_custom_repl"] = False

cls = ABCMeta.__new__(metacls, clsname, bases, namespace, **kwargs)
if name is not None:
metacls.all_applets[name] = cls
@@ -37,7 +33,6 @@ class GlasgowApplet(metaclass=GlasgowAppletMeta):
help = "applet help missing"
description = "applet description missing"
required_revision = "A0"
has_custom_repl = False

@classmethod
def add_build_arguments(cls, parser, access):
@@ -71,9 +66,18 @@ async def run(self, device, args):
def add_interact_arguments(cls, parser):
pass

async def interact(self, device, args, interface):
async def interact(self, device, args, iface):
pass

@classmethod
def add_repl_arguments(cls, parser):
pass

async def repl(self, device, args, iface):
self.logger.info("dropping to REPL; use 'help(iface)' to see available APIs")
await AsyncInteractiveConsole(locals={"device":device, "iface":iface},
run_callback=device.demultiplexer.flush).interact()


class GlasgowAppletTool:
def __init_subclass__(cls, applet, **kwargs):
11 changes: 5 additions & 6 deletions software/glasgow/applet/debug/mips/__init__.py
View file
Original file line number Diff line number Diff line change
@@ -830,7 +830,6 @@ class DebugMIPSApplet(JTAGProbeApplet, name="debug-mips"):
Other configurations might or might not work. In particular, it certainly does not currently
work on little-endian CPUs. Sorry about that.
"""
has_custom_repl = True

@classmethod
def add_run_arguments(cls, parser, access):
@@ -879,9 +878,9 @@ async def interact(self, device, args, ejtag_iface):
if ejtag_iface.target_attached():
await ejtag_iface.target_detach()

if args.operation == "repl":
await AsyncInteractiveConsole(locals={"iface":ejtag_iface}).interact()
async def repl(self, device, args, ejtag_iface):
await super().repl(device, args, ejtag_iface)

# Same as above.
if ejtag_iface.target_attached():
await ejtag_iface.target_detach()
# Same reason as above.
if ejtag_iface.target_attached():
await ejtag_iface.target_detach()
Loading