pyocd / pyOCD

Open source Python library for programming and debugging Arm Cortex-M microcontrollers
https://pyocd.io
Apache License 2.0
1.13k stars 484 forks source link

serial now required in 0.29.0? #1076

Open nerdralph opened 3 years ago

nerdralph commented 3 years ago

My probe firmware does not provide a serial descriptor string, and pyOCD 0.28.x worked fine. 0.29.0 seems to require it now:

0000542:CRITICAL:__main__:uncaught exception: 'NoneType' object is not subscriptable
Traceback (most recent call last):
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/__main__.py", line 401, in run
    self._COMMANDS[self._args.cmd](self)
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/__main__.py", line 807, in do_commander
    PyOCDCommander(self._args, cmds).run()
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/commands/commander.py", line 65, in run
    if not self.connect():
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/commands/commander.py", line 166, in connect
    unique_id=self.args.unique_id,
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/core/helpers.py", line 138, in choose_probe
    allProbes = ConnectHelper.get_all_connected_probes(blocking=blocking, unique_id=unique_id)
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/core/helpers.py", line 83, in get_all_connected_probes
    sortedProbes = sorted(allProbes, key=lambda probe:probe.description + probe.unique_id)
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/core/helpers.py", line 83, in <lambda>
    sortedProbes = sorted(allProbes, key=lambda probe:probe.description + probe.unique_id)
  File "/home/ralphd/.local/lib/python3.6/site-packages/pyocd/probe/cmsis_dap_probe.py", line 99, in description
    board_id = self.unique_id[0:4]
TypeError: 'NoneType' object is not subscriptable

After modifying my probe firmware to provide a serial descriptor, pyocd 0.29.0 works.

flit commented 3 years ago

That's really weird, I don't know why it would have worked in 0.28.x. That line has been there for several years. 🤷🏽

pyocd requires a serial number for every probe so it can be identified and selected if there are multiple connected devices. I hadn't thought that requiring a serial number string would be a problem; I'd never seen a USB device without some serial number.

1031 addresses this issue by generating a stable-ish unique ID from other USB device info. It only applies to pyusb for CMSIS-DAPv1, but needs to be extended to pyusb for CMSIS-DAPv2. I'm not sure if hidapi and pywinusb need to be handled similarly since I don't have a probe to test with that doesn't report a serial number.

flit commented 3 years ago

If you can, would you mind testing #1077? It would be nice to confirm it fixes the issue with your probe before merging.

nerdralph commented 3 years ago

I've been switching back and forth between Win7 and Linux for testing the CMSIS-DAP firmware (running on CH55x MCUs). Since you say the unique ID code has been in pyOCD for a while, I'm guessing Win7 cached the serial descriptor from before I removed it from the firmware. When I switched to testing on Linux, and happened to upgrade to 0.29.0 at the same time, I noticed the "bug". I haven't found an actual CMSIS-DAPv1 spec, so I wasn't sure if the protocol requires a serial descriptor or not.

I'll test #1077 once I've figured out my problem getting local pyOCD builds working.

flit commented 3 years ago

That makes sense, Windows does like to cache a lot of USB device attributes in the registry.

The only specification is the official CMSIS-DAP documentation. The protocol doesn't require a serial number afaik, but pyocd wants one. And it's largely my fault for assuming that devices would always have a serial string.

If you have any questions about the specification for CMSIS-DAP v1 or v2, please either create an issue on the CMSIS_5 repo and cc me, or you can just email me. I'm collecting notes for things to improve in the CMSIS docs to make requirements more explicitly stated.

nerdralph commented 3 years ago

I looked at the CMSIS-DAP docs, but there doesn't seem to be any versioning so I could filter out the v2.0 stuff that I'm not interested in. I see the revision history, but for each command I couldn't see any reference saying which version it was added in.

To be blunt, I think it would be impossible to implement a debug probe based only on the CMSIS-DAP documentation. I've mostly been looking through the pyOCD and daplink source to figure out the protocol. I couldn't find any mention in the CMSIS-DAP docs about v2 supporting bulk endpoints; I only found out about that in one of the pyOCD issues talking about performance. I also couldn't find any documentation about probe detection based on finding the string 'CMSIS-DAP' in the device product descriptor string.

I currently have a working CMSIS-DAP v1.1 firmware working on CH551 & CH552 MCUs. They are 8051 cores with limited RAM; there's only 512 bytes that can be used for USB rx & tx buffers on the CH551. Flashing a STM32F103 with pyOCD runs at ~8kB/sec. I'm working on adding CDC serial, so I don't think I'll have enough RAM to increase DAP_PACKET_COUNT (currently 1). I see that pyOCD supports control transfers, which is something I may try. https://github.com/pyocd/pyOCD/blob/7abe4c03674f2769ecc2292e2e3ad85c72083018/pyocd/probe/pydapaccess/interface/pyusb_backend.py#L190

Is using a control transfer instead of a HID OUT packet something that is officially part of the protocol, or just a pyOCD proprietary option?

Thanks for offering to help. I'll probably take you up on it, likely via email, since I'm not sure of the right way to create an issue on the CMSIS_5 repo without starting an argument with Robert. :-)

flit commented 3 years ago

I see the revision history, but for each command I couldn't see any reference saying which version it was added in.

All commands that are not explicitly mentioned in the revision history were present from version 0.01. But I agree, the version specification leaves a lot to be desired.

I couldn't find any mention in the CMSIS-DAP docs about v2 supporting bulk endpoints

The v2 docs are here. Instead of "bulk" they use the term "WinUSB", which is possibly misleading but also somewhat correct because WinUSB descriptors are used to enable working on Windows without requiring a kernel driver or manual driver install via Zadig. However, the WinUSB descriptors aren't actually part of the CMSIS-DAP protocol; it's just an enablement thing—this needs to be made much more clear in the specification.

Is using a control transfer instead of a HID OUT packet something that is officially part of the protocol, or just a pyOCD proprietary option?

Honestly, I'm not totally sure. pyOCD, I think (it was implemented by someone else before I took over), supports control transfers primarily as a fallback based on the HID class specification. DAPLink supports CMSIS-DAP via control transfers so that WebUSB can be supported without requiring an additional set of interrupts endpoints—but this interface is not used by pyOCD.

It's really cool that you're able to implement CMSIS-DAP with such a small MCU!

There are plans for some upcoming additions to CMSIS-DAP. I'll try to get improvements to the docs added to the list, so we can get a more formal specification instead of screenshots of Keil MDK demos. 😅

nerdralph commented 3 years ago

I couldn't find any mention in the CMSIS-DAP docs about v2 supporting bulk endpoints

The v2 docs are here. Instead of "bulk" they use the term "WinUSB", which is possibly misleading but also somewhat correct because WinUSB descriptors are used to enable working on Windows without requiring a kernel driver or manual driver install via Zadig. However, the WinUSB descriptors aren't actually part of the CMSIS-DAP protocol; it's just an enablement thing—this needs to be made much more clear in the specification.

Someone seems to be conflating documentation for daplink with CMSIS-DAP. I understand daplink is a reference implementation, but things like the following sentence don't belong in the CMSIS-DAP protocol spec: The USB Device setting high-speed / full-speed USB must be reflected in the DAP_config.h file as described under Firmware Configuration.

Is using a control transfer instead of a HID OUT packet something that is officially part of the protocol, or just a pyOCD proprietary option?

Honestly, I'm not totally sure. pyOCD, I think (it was implemented by someone else before I took over), supports control transfers primarily as a fallback based on the HID class specification. DAPLink supports CMSIS-DAP via control transfers so that WebUSB can be supported without requiring an additional set of interrupts endpoints—but this interface is not used by pyOCD.

I could just try changing my firmware to remove the OUT endpoint and handle the DAP requests through control transfers. It would save some RAM on the CH55x by reusing the EP0 buffer. I'm a bit hesitant to do it if it is not officially part of the CMSIS-DAP protocol, since I want my firmware to work with OpenOCD too.

flit commented 3 years ago

I understand daplink is a reference implementation

Technically, DAPLink is a product implementation. Meaning it builds firmware meant for end users on a wide variety of dev platforms, and it includes features beyond basic CMSIS-DAP. The CMSIS-DAP code in the CMSIS_5 repo is the reference implementation. The CMSIS_5 repo also contains a couple buildable examples that use the reference code.

things like the following sentence don't belong in the CMSIS-DAP protocol spec: The USB Device setting high-speed / full-speed USB must be reflected in the DAP_config.h file as described under Firmware Configuration.

Yep, totally agreed. There needs to be a clear dividing line between the protocol spec, and reference implementation documentation.

I could just try changing my firmware to remove the OUT endpoint and handle the DAP requests through control transfers. It would save some RAM on the CH55x by reusing the EP0 buffer. I'm a bit hesitant to do it if it is not officially part of the CMSIS-DAP protocol, since I want my firmware to work with OpenOCD too.

Using control transfers instead of the OUT endpoint is part of the USB HID class spec, so whether it works really depends on how well OpenOCD and other tools implemented HID.

nerdralph commented 3 years ago

I could just try changing my firmware to remove the OUT endpoint and handle the DAP requests through control transfers. It would save some RAM on the CH55x by reusing the EP0 buffer. I'm a bit hesitant to do it if it is not officially part of the CMSIS-DAP protocol, since I want my firmware to work with OpenOCD too.

Using control transfers instead of the OUT endpoint is part of the USB HID class spec, so whether it works really depends on how well OpenOCD and other tools implemented HID.

Thanks for looking into that.