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

Invalid firmware version on CMSIS-DAP #1143

Closed kongr45gpen closed 3 years ago

kongr45gpen commented 3 years ago

I am trying to interface with an ATSAMV71-XULT, and getting this error:

INFO:pyocd.board.board:Target type is atsamv71q21b
Traceback (most recent call last):
  File "./test.py", line 22, in <module>
    with ConnectHelper.session_with_chosen_probe(unique_id=args.unique_id) as session:
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/core/session.py", line 339, in __enter__
    self.open()
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/core/session.py", line 453, in open
    self._probe.open()
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/probe/cmsis_dap_probe.py", line 170, in open
    self._link.open()
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/utility/concurrency.py", line 28, in _locking
    return func(self, *args, **kwargs)
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/probe/pydapaccess/dap_access_cmsis_dap.py", line 644, in open
    self._read_protocol_version()
  File "/home/kongr45gpen/.local/lib/python3.7/site-packages/pyocd/probe/pydapaccess/dap_access_cmsis_dap.py", line 624, in _read_protocol_version
    patch = int(fw_version[2] if len(fw_version) > 2 else 0)
ValueError: invalid literal for int() with base 10: '01B6'

The reported firmware version for CMSIS-DAP is:

fw_version_str = "03.25.01B6"

I'm not sure if the documentation supports a hexadecimal string as the patch version or not, and not sure how to go around this issue. Also note that working with this board worked perfectly before commit bbf361e3b289f19d1ed354fa017a460b119d9cd8.

phsommer commented 3 years ago

I have observed a similar problem with the Particle Debugger running firmware version v0254.2:

File "/Users/xyz/venv/lib/python3.9/site-packages/pyocd/probe/pydapaccess/dap_access_cmsis_dap.py", line 645, in open
    self._read_protocol_version()
  File "/Users/xyz/venv/lib/python3.9/site-packages/pyocd/probe/pydapaccess/dap_access_cmsis_dap.py", line 621, in _read_protocol_version
    patch = int(fw_version[1][1])
ValueError: invalid literal for int() with base 10: '\x01'

The reported firmware version in my case is:

fw_version_str = "0254.2A"
mcbridejc commented 3 years ago

I am getting the same error with the Microchip SAMG55 Xplained Pro development board, which has the microchip embedded debugger on it. It returns 03.25.01B6 for the DAP version, just as in the original case.

It's clear from https://arm-software.github.io/CMSIS_5/DAP/html/group__DAP__Info.html that this field should be one of the valid DAP protocol versions, not a vendor software revision, but it seems microchip has implemented it incorrectly.

I've worked around it in a fork, here: https://github.com/mcbridejc/pyOCD

I also submitted PR #1201 in case @flit wants to pull it in.

flit commented 3 years ago

Hi all, thanks for reporting these issues.

Prior to CMSIS-DAP v2.1, the description of the DAP_Info value 0x04 was actual "firmware version", but they really meant protocol version. In v2.1 this was made very clear, and a separate Product Firmware Version = 0x09 value was added. I thought I was the only one to misunderstand the pre-2.1 description! (In DAPLink.) So it's very interesting to see other firmware doing the same.

We'll have to add some workarounds for these firmwares. An exception handler is needed for the conversion to int.

Then there's the question of whether to try and detect the product and map to a CMSIS-DAP protocol version. Currently pyocd only uses the protocol version to decide whether to read the new Product Firmware Version value. So, for now, it will be fine if it just defaults to CMSIS-DAP v1.0.0 for an invalid protocol version.

However, that may not always be the case in the future, so it might be wise to go ahead and implement a version mapping for the products we know about now.

mcbridejc commented 3 years ago

Thanks for the context. I didn't realize the older protocol docs described it differently. Supporting all the things in the embedded world is always a never ending chore!

The PR I submitted was basically the narrowest change I thought I could make to get my case working. I don't actually know what other version strings are out there now or will be tomorrow on microchip EDBG devices....so it may not be robust at all.

Perhaps the best path for now is to check the reported version against the list of CMSIS-DAP protocol versions and if it's not found default to v1.0.0 as you say? I like that approach. I'm not at all offended if you reject that PR to do something different, or I can rework it to check for valid protocol versions instead of having specific vid/pid + version exceptions.

flit commented 3 years ago

Definitely need to wrap with an exception handler.

Checking against known CMSIS-DAP versions is probably ok. My only concern with this is that it means that new CMSIS-DAP protocol versions will require an immediate pyocd update. For now that's unlikely to be an issue since I'm involved with or at least aware of CMSIS-DAP protocol changes. And, this is probably better than explicit checks against non-compliant firmware versions, which will also have to be updated and have less visibility about.

If you would like to update your PR that would be great! I'm always happy to have more contributors. 😄 (I'll comment on the PR too.)

flit commented 3 years ago

This issue should be resolved now with version 0.32.0. If you still see problems, please reopen.