-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Trace32 debugger support #1267
base: master
Are you sure you want to change the base?
Conversation
d9d75bb
to
f21d593
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1267 +/- ##
========================================
- Coverage 63.0% 62.7% -0.4%
========================================
Files 160 163 +3
Lines 11901 12145 +244
========================================
+ Hits 7502 7616 +114
- Misses 4399 4529 +130 ☔ View full report in Codecov by Sentry. |
Hi, Regards, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to share a second strategy how to handle the 'which ports are available on the exporter' problem in ecda73d. The solution is more like
ser2net
. The exporter itself handles the start of thet32tcpusb
agent and thus handles the tcp port reservation. This solution is already used since a couple of months. Let me know which variant you prefer.
The initial approach looks much better to us. In most setup, the exporter runs as a different user. That user should not run arbitrary code. There is also a life cycle conflict there: _start()
is called on acquire, but the corresponding binary is not present, yet (maybe from the last run). The hack using _get_start_params()
for finding the symlink change is pretty clever, but not the intended approach. It's used for cases where the USB connection changes and ser2net has to be restarted.
Let's start with a rather high level review. When we figured out the rough direction for this PR, we will take a more detailed look.
labgrid/protocol/debuggerprotocol.py
Outdated
import abc | ||
|
||
|
||
class DebuggerProtocol(abc.ABC): | ||
"""Abstract class for the DebuggerProtocol""" | ||
|
||
@abc.abstractmethod | ||
def start(self): | ||
""" | ||
Start a debugger session | ||
""" | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's not really useful, yet. Let's add a protocol when another driver with similar functionality is added. Until then, just drop this.
labgrid/resource/udev.py
Outdated
|
||
@target_factory.reg_resource | ||
@attr.s(eq=False) | ||
class USBLauterbachDebugger(USBResource): | ||
def filter_match(self, device): | ||
# udevadm info --attribute-walk /dev/lauterbach/trace32/* | ||
match = (device.properties.get('ID_VENDOR_ID'), device.properties.get('ID_MODEL_ID')) | ||
|
||
if match not in [("0897", "0002"), # PowerDebug USB1.0/USB2.0/Ethernet/II + PowerTrace | ||
("0897", "0004"), # PowerDebug USB3.0 + uTrace | ||
("0897", "0005"), # PowerDebug PRO/E40 | ||
("0897", "0006") # PowerDebug X50 | ||
]: | ||
return False | ||
|
||
return super().filter_match(device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
labgrid/resource/lauterbach.py
Outdated
Args: | ||
architecture (str): Architecture of the exporter userspace | ||
""" | ||
architecture = attr.ib(validator=attr.validators.instance_of(str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The architecture should be handled in the driver, see review comments there.
labgrid/util/helper.py
Outdated
|
||
def get_uname_machine(): | ||
if hasattr(get_uname_machine, "architecture"): | ||
return get_uname_machine.architecture | ||
|
||
# translate `uname -m` output to debian/ubuntu architecture terms | ||
uname_translate = { | ||
"x86_64": "amd64" | ||
} | ||
# default: use ``uname -m`` | ||
get_uname_machine.architecture = processwrapper.check_output(["uname","-m"]).decode("utf-8").rstrip() | ||
get_uname_machine.architecture = uname_translate.get(get_uname_machine.architecture, get_uname_machine.architecture) | ||
|
||
# check ELF header - inspired by /proc/sys/fs/binfmt_misc/qemu-* | ||
# Why? e.g. a AARCH64 Kernel can run a armhf userspace (buzzword: Raspian OS) | ||
binfmt = { | ||
"arm": (b'\x7f\x45\x4c\x46\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00' # e_ident | ||
b'\x02\x00\x28\x00' # e_type + e_machine | ||
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' # e_<version,entry,phoff,shoff> | ||
b'\x00\x00\x00\x00', # e_flags | ||
b'\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff' | ||
b'\xfe\xff\xff\xff' | ||
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x00\x00\x04\x00'), | ||
"armhf": (b'\x7f\x45\x4c\x46\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x02\x00\x28\x00' | ||
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x00\x00\x04\x00', | ||
b'\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff' | ||
b'\xfe\xff\xff\xff' | ||
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x00\x00\x04\x00'), | ||
"aarch64": (b'\x7f\x45\x4c\x46\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x02\x00\xb7\x00', | ||
b'\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff' | ||
b'\xfe\xff\xff\xff'), | ||
"amd64": (b'\x7f\x45\x4c\x46\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00' | ||
b'\x02\x00\x3e\x00', | ||
b'\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff' | ||
b'\xfe\xff\xff\xff'), | ||
} | ||
|
||
elfheader = b'' | ||
with open("/proc/self/exe", "rb") as exe: | ||
elfheader = exe.read(40) | ||
for arch, compare in binfmt.items(): | ||
magic = compare[0] | ||
mask = compare[1] | ||
|
||
match = True | ||
for i in range(min(len(elfheader),len(magic),len(mask))): | ||
match = match and ((elfheader[i] & mask[i]) == magic[i]) | ||
|
||
if match: | ||
get_uname_machine.architecture=arch | ||
break | ||
|
||
return get_uname_machine.architecture | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this into a dedicated Lauterbach agent (labgrid/util/agents/
). Agents are copied to the exporter on-the-fly and executed there via AgentWrapper. See labgrid/driver/gpiodriver.py
for an example.
labgrid/resource/lauterbach.py
Outdated
|
||
@target_factory.reg_resource | ||
@attr.s(eq=False) | ||
class NetworkUSBLauterbachDebugger(RemoteUSBResource): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be RemoteUSBLauterbachDebugger
. We tend to use the "Remote" prefix for resources which also exist as a local variant (USBLauterbachDebugger in this case) and the "Network" prefix for stand-alone network resources (such as the NetworkLauterbachDebugger).
labgrid/driver/lauterbachdriver.py
Outdated
"amd64": "bin/pc_linux64", | ||
"armhf": "bin/linux-armhf" | ||
} | ||
self.t32sys = os.environ.get("T32SYS", "/opt/t32") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other drivers use something like:
if self.target.env:
self.tool = self.target.env.config.get_tool('mybinary')
else:
self.tool = 'mybinary'
Since there is no single binary in this case, maybe use a path from paths:
via self.target.env.config.get_path()
from https://labgrid.readthedocs.io/en/latest/configuration.html#environment-configuration instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pystart
library expects only the basename of the executable and handles the rest internally including e.g. host architecture detection. This is put into t32_bin
now.
The t32_sys
is used to specify the base installation folder.
I reworked that completely in the latests force push.
"NetworkLauterbachDebugger" | ||
}, | ||
} | ||
t32_bin = attr.ib(default="t32marm", validator=attr.validators.instance_of(str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe have a look at the QEMUDriver's qemu_bin
attribute.
labgrid/remote/client.py
Outdated
|
||
drv = None | ||
try: | ||
drv = target.get_driver("DebuggerProtocol", name=name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's where the protocol is useful. I guess until a second debugger driver is added, we can simply reference the driver directly.
labgrid/remote/client.py
Outdated
|
||
def debugger(self): | ||
place = self.get_acquired_place() | ||
target = self._get_target(place) | ||
name = self.args.name | ||
from ..resource.lauterbach import (NetworkLauterbachDebugger, NetworkUSBLauterbachDebugger) | ||
from ..resource.udev import (USBLauterbachDebugger) | ||
|
||
drv = None | ||
try: | ||
drv = target.get_driver("DebuggerProtocol", name=name) | ||
except NoDriverFoundError: | ||
for resource in target.resources: | ||
if isinstance(resource, (NetworkLauterbachDebugger, NetworkUSBLauterbachDebugger, USBLauterbachDebugger)): | ||
drv = self._get_driver_or_new(target, "LauterbachDriver", activate=False, | ||
name=name) | ||
if drv: | ||
break | ||
|
||
if not drv: | ||
raise UserError("target has no compatible resource available") | ||
target.activate(drv) | ||
drv.start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def debugger(self): | |
place = self.get_acquired_place() | |
target = self._get_target(place) | |
name = self.args.name | |
from ..resource.lauterbach import (NetworkLauterbachDebugger, NetworkUSBLauterbachDebugger) | |
from ..resource.udev import (USBLauterbachDebugger) | |
drv = None | |
try: | |
drv = target.get_driver("DebuggerProtocol", name=name) | |
except NoDriverFoundError: | |
for resource in target.resources: | |
if isinstance(resource, (NetworkLauterbachDebugger, NetworkUSBLauterbachDebugger, USBLauterbachDebugger)): | |
drv = self._get_driver_or_new(target, "LauterbachDriver", activate=False, | |
name=name) | |
if drv: | |
break | |
if not drv: | |
raise UserError("target has no compatible resource available") | |
target.activate(drv) | |
drv.start() | |
def debugger(self): | |
place = self.get_acquired_place() | |
target = self._get_target(place) | |
name = self.args.name | |
drv = self._get_driver_or_new(target, "LauterbachDriver", name=name) | |
drv.start() |
This should be sufficient.
match USB connected - PowerDebug USB1.0/USB2.0/USB3.0/Ethernet/PRO/E40/X50 - PowerTrace - uTrace
Network devices support always UDP, the most recent devices e.g. PowerDebug X50 have an option to use TCP which enables LG_PROXY support.
On ARM (e.g. Raspian OS) runs a 32bit userspace (e.g. armhf) on a Aarch64/ARM64 machine. Thus `uname -m` might be misleading as it identifies the kernel and not the userspace. The introduced `get_uname_machine` checks the elf header of the python interpreter itself to detect the architecture for dynamically linked applications. The terms align with the debian/ubuntu architecture naming (arm/armhf/aarch64/amd64).
Looking at this PR I am wondering if it might make sense to split it into two stages? Firstly get
Once this is upstreamed, the Trace32 support can be merged separately |
Hi, |
cff4278
to
9e9c57c
Compare
start a debug session using the DebuggerProtocol
fixup endianness twist to detect HARDFP ABI for ARM
Hi,
as previously discussed we are working on adding a TRACE32 debugger support into Labgrid. The pull request is not yet finished thus I did not add any
signed-off
's to the commit.Nevertheless I want to sum up the status of what the work does:
t32tcpusb
is required to forward the USB traffic to a TCP portNetworkUSBLauterbachDebug
activate
DebuggerProtocol
allows tostart
a debugger sessionFuture work:
pytest
BootstrapProtocol
Questions:
t32tcpusb
t32tcpusb
is cleaned up on the exporter e.g. implementing aUSBLauterbachDebuggerExport._stop()
-tt
to thecommand_prefix
of aRemoteUSBResource
? Without that I cannot terminate a process started on the exportertests
NetworkUSB..
resource?Checklist