search

avrdudes / avrdude

AVRDUDE is a utility to program AVR microcontrollers
GNU General Public License v2.0
696 stars 136 forks source link

JTAGICE3 EDBG communication mode relies on hardcoded USBVID #1838

Closed askn37 closed 1 month ago

askn37 commented 1 month ago

Hello everyone

I am currently creating a jtagice3_*** compatible firmware that works with the AVR-DU series. It can be recognized as USB-HID + USB-CDC using only the standard drivers for Windows and macOS without installing any special drivers. (For the time being, the goal is to support UPDI and TPI, and debugWire cannot be used.)

To meet this condition and be recognized as a programmer by AVRDUDE, it must be in EDBG (CMSIS-DAP) communication mode, at least like pkobn_updi. However, I noticed that it is hard-coded to change to EDBG only when usbvid is 0x03EB ('ATMEL'). Changing usbvid in .avrduderc does not work, and it is always fixed to max_xfer = USBDEV_MAX_XFER_3 (912). The conditional branch is in usb_hidapi.c:173.

I could redesign the firmware to create a dedicated endpoint that can handle 912 byte packets, but I personally don't like the hard-coded part. This wouldn't have been an issue before. All the chips with jtagice3 were "ATMEL" branded.

The AVR-DU series is a "Microchip" (MCHP) product, so I'm reluctant to use the "ATMEL" VID. I'd like to use at least 0x04DB (MCHP) or another VID if possible.

Any ideas on how to solve this problem? It seems like I need to add a configuration option or internal flag so that the condition at usb_hidapi.c:173 is always true.

stefanrueger commented 1 month ago

This is up @dl8dtl's road. Joerg is currently unavailable but I am sure he'll reply.

askn37 commented 1 month ago

Additional information

Several questions came to mind while looking into this issue. All of these were found on M1 Mac + avrdude main and should be considered as separate discussions if necessary, but they all relate to the same source files.

(07-24 update edit)

askn37 commented 1 month ago

Experimental patch ideas to improve JTAGICE3+hid_open.

diff --git a/src/usb_hidapi.c b/src/usb_hidapi.c
index 59c6dee4..3e60efaf 100644
--- a/src/usb_hidapi.c
+++ b/src/usb_hidapi.c
@@ -54,6 +54,40 @@ static int usbhid_open(const char *port, union pinfo pinfo, union filedescriptor

   if (fd->usb.max_xfer == 0)
     fd->usb.max_xfer = USBDEV_MAX_XFER_3;
+  /* Note that this setting only works with the older JTAGICE3 (Bulk, not Interrupt). */
+
+  /*
+   * The syntax for usb devices is defined as:
+   *
+   * -P usb:<usbvid>:<usbpid>
+   * 
+   * If it contains multiple colons, it interprets this style first,
+   * and then tries the next style if no matching device is found.
+   */
+  size_t plen = strlen(port), pidx = 0;
+  char *ptok[2] = {NULL, NULL}, *p = (char*)port;
+  for (size_t i = 0; i < plen && pidx < 2; i++) {
+    if (*p++ == ':') ptok[pidx++] = p;
+  }
+  if (ptok[1] != NULL) {
+    /* This will not be done because the second colon will    */
+    /* be lost after the serial number is checked after this. */
+    int vid, pid;
+    if (sscanf(ptok[0], "%x", &vid) > 0 && sscanf(ptok[1], "%x", &pid) > 0) {
+      dev = hid_open(vid, pid, NULL);
+      if (dev != NULL) {
+        /* We'll close it later, because we'll reopen it. */
+        hid_close(dev);
+        pmsg_notice2("USB device with VID: 0x%04x and PID: 0x%04x\n", vid, pid);
+        /* Now that we have confirmed that the vid and */
+        /* pid exist, we will use them from now on.    */
+        pinfo.usbinfo.vid = vid;
+        pinfo.usbinfo.pid = pid;
+        *(ptok[0] - 1) = '\0';
+        /* Prevents serialnumber style from being called. */
+      }
+    }
+  }

   /*
    * The syntax for usb devices is defined as:
@@ -144,6 +178,26 @@ static int usbhid_open(const char *port, union pinfo pinfo, union filedescriptor

   fd->usb.handle = dev;

+  /* If a device contains the string "CMSIS-DAP" anywhere in its       */
+  /* product name string, it claims to be a CMSIS-DAP spec HID device. */
+  /* HID interrupt packets can only be 64 byte (Full-Speed) or         */
+  /* 512 byte (High-Speed) in length and cannot be any other value.    */
+  /* Standard-Speed (8 bytes) will not work. */
+  wchar_t ps[256];
+  if (hid_get_product_string(dev, ps, 255) == 0) {
+    size_t n = wcstombs(NULL, ps, 0) + 1;
+    if (n) {
+      char cn[255];
+      if (wcstombs(cn, ps, n) != (size_t) -1 && str_contains(cn, "CMSIS-DAP")) {
+        /* The JTAGICE3 running the CMSIS-DAP firmware doesn't */
+        /* use a separate endpoint for event reception.        */
+        fd->usb.eep = 0;
+        fd->usb.max_xfer = 64;
+        pmsg_debug("usbhid_open(): product string: %s\n", cn);
+      }
+    }
+  }
+
   /*
    * Try finding out the endpoint size.  Alas, libhidapi doesn't
    * provide us with an API function for that, nor for the report
stefanrueger commented 1 month ago

The same page erase command XMEGA_ERASE_APP_PAGE=0x04 is sent to both BOOTROW and PROGMEM of AVR-EB/DU, so it is not possible to distinguish which address space it is, meaning that the wrong area may be erased.

Thanks for alerting us to this problem, @askn37. I really wonder what the Microchip protocols do in that case. @xedbg do you know what's expected from the protocol for a bootrow page erase?

reluctant to use the "ATMEL" VID. I'd like to use at least 0x04DB (MCHP) or another VID if possible

In absence if @dl8dtl's input I have had a look at this. I think it is vvv reasonable to also allow the Microchip VID here. This would also be hardcoded. The open routines don't seem to have access to the part AVRPART *p or programmer PROGRAMMER *pgm which makes it hard to use vid/pid settings from avrdude.conf.

-P usb:serialnumber is limited to a maximum of 12 digits

The code matches from right to left, so even one digit is normally enough (unless you have more devices with serial numbers ending in the same digit). But yes, I'll create a PR that makes this 63 digits.

Thanks @askn37 for the code snippets you provided. That makes it clearer what you are after. Ideally, provide a PR.

Have a look at the linked PR. Would that be sufficient for your purposes? It does what you suggest -P usb:vid:pid. I am unclear about the second part of your suggested changes, particularly the fd->usb.max_xfer = 64; line. Looks like that setting would be overwritten further below? The other setting fd->usb.eep = 0; won't (good)

Anyway, have a look at the PR and tell us what other changes you'd like.

When -vvvv is used, the JTAG3 transmit commands are not dumped

Yes, makes sense to be able to have the transmit commands logged. jtag.c has 3400+ lines of code. Please provide a PR so we can consider inclusion [edit: but wait until PR #1849 has been merged - that PR will change log levels].

askn37 commented 1 month ago

When -vvvv is used, the JTAG3 transmit commands are not dumped

Yes, makes sense to be able to have the transmit commands logged. jtag.c has 3400+ lines of code. Please provide a PR so we can consider inclusion [edit: but wait until PR #1849 has been merged - that PR will change log levels].

I'm in no rush to do it, but I have every reason and desire to work on it, so I'll wait for the opportunity.

askn37 commented 1 month ago

I am unclear about the second part of your suggested changes, particularly the fd->usb.max_xfer = 64; line.

Yes, ideally you should put fd->usb.max_xfer = 64; at the beginning of usbhid_open(), but I don't know which known devices this affects, so I'll change the default once I'm sure.

Currently, all USB-HID devices that link at Full-Speed must have max=64 to work, if their "USB Report Descriptor" requests a report size of 64. The only exception I know of is ATMELICE3, which has a report size of 512.

xedbg commented 1 month ago

. @xedbg do you know what's expected from the protocol for a bootrow page erase? Good question - I actually suspect that page erase for UPDI parts is just not supported (missing/forgotten)