Suggest compatible command when SNAP/PICKit4 in PIC mode is detected

[MCUdude]

With this PR Avrdude suggests either switching the SNAP/PICkit4 to AVR mode (-xmode=avr), or providing an alternative command where the appropriate pickit5 programmer option is selected. And now it wont print -p (null) if no -p option is specified.

$ avrdude -c pickit4_isp

avrdude error: PICkit 4 in PIC mode detected
        to switch into AVR mode try
        $ avrdude -c pickit4_isp -P usb -x mode=avr
        or use PIC mode by using the pickit5_isp programmer option:
        $ avrdude -c pickit5_isp -P usb

This has only been implemented for SNAP/PICkit4 in PIC mode. Similar functionality should be implemented for the pickit5 programmer option if a SNAP/PICkit4 in AVR mode is detected. @MX682X probably has thoughts on how something like this could be implemented. Ideally something like this:

$ avrdude -c pickit5_isp

avrdude error: PICkit 4 in AVR mode detected
        to switch into AVR mode try
        $ avrdude -c pickit4_isp -P usb -x mode=pic
        or use AVR mode by using the pickit4_isp programmer option:
        $ avrdude -c pickit4_isp -P usb

[MX682X]

Alright, I'm not sure how I can add something to this PR, so I'll just copy paste the added code here.
It compiles, but without a PK4 I can't test it.

The code fragment has to be pasted into this line:

avrdude/src/pickit5.c

Line 629 in 7668a1f

  // No PICkit4 / SNAP (PIC mode) nor PICkit5 found, look for programmer in AVR mode
  if(rv < 0) {
    for(LNODEID ln = lfirst(pgm->id); ln; ln = lnext(ln)) {
      if(str_starts(ldata(ln), "snap") || str_starts(ldata(ln), "pickit4")) {
        bool is_snap_pgm = str_starts(ldata(ln), "snap");

        pinfo.usbinfo.vid = USB_VENDOR_MICROCHIP;
        pinfo.usbinfo.pid = is_snap_pgm? USB_DEVICE_SNAP_PIC_MODE: USB_DEVICE_PICKIT4_PIC_MODE;
        const int bl_pid = is_snap_pgm? USB_DEVICE_SNAP_PIC_MODE_BL: USB_DEVICE_PICKIT4_PIC_MODE_BL;
        const char *pgmstr = is_snap_pgm? "MPLAB SNAP": "PICkit 4";

        int pic_mode = serial_open(port, pinfo, &pgm->fd);

        if(pic_mode < 0) {
          // Retry with bootloader USB PID
          pinfo.usbinfo.pid = bl_pid;
          pic_mode = serial_open(port, pinfo, &pgm->fd);
        }

        if(pic_mode >= 0) {
          msg_error("\n");
          cx->usb_access_error = 0;
          const char *pgm_suffix = strchr(pgmid, '_')? strchr(pgmid, '_'): "";
          char part_option[32] = {};  // 32 should be enough
          char c_option[32] = {};

          snprintf(c_option, 31, "-c %s_%s", (is_snap_pgm? "snap" : "pickit4"), pgm_suffix);
          if (partdesc) {
            snprintf(part_option, 31, "-p %s", partdesc);
          } else {
            strcpy(part_option, "");
          }

          pmsg_error("%s in %s mode detected\n", pgmstr, pinfo.usbinfo.pid == bl_pid? "bootloader": "AVR");

          imsg_error("to switch into PIC mode try\n");
          imsg_error("$ %s %s %s -P %s -x mode=pic\n", progname, c_option, part_option, port);
          imsg_error("or use AVR mode by using the %s programmer option:\n", &(c_option[3]));  // avoid the "-c "
          imsg_error("$ %s %s %s -P %s\n", progname, c_option, part_option, port);

          serial_close(&pgm->fd);
          return LIBAVRDUDE_EXIT;;
        }
      }
    }
    pmsg_error("no device found matching VID 0x%04x and PID list: ", (unsigned) pinfo.usbinfo.vid);
    int notfirst = 0;
    for(usbpid = lfirst(pgm->usbpid); usbpid; usbpid = lnext(usbpid)) {
      if(notfirst)
        msg_error(", ");
      msg_error("0x%04x", (unsigned int) (*(int *) (ldata(usbpid))));
      notfirst = 1;
    }
  }

[MCUdude]

@MX682X Thanks! I pasted your code, but for some reason I'm getting the following error:

$ avrdude -cpickit5_isp -patmega328p
Error: no devicce found matching VID 0x04d8 and PID list: 0x9036, 0x9012, 0x9018
Error: unable to open port usb for programmer pickit5_isp

Avrdude done.  Thank you.

However, it is connected, and in AVR mode.

[MCUdude]

Oh, I see. pgmid doesn't contain snap or pickit4. The only way to "know" which programmer the user is trying to use is to look at the USB VID/PID.

[MX682X]

Oops, my fault, forgot to change the defines (like the VID), here is the fix.
Also, I'm not sure about the bootloader mode, for now I removed it.

if(rv < 0) {
    for(LNODEID ln = lfirst(pgm->id); ln; ln = lnext(ln)) {
      if(str_starts(ldata(ln), "snap") || str_starts(ldata(ln), "pickit4")) {
        bool is_snap_pgm = str_starts(ldata(ln), "snap");

        pinfo.usbinfo.vid = USB_VENDOR_ATMEL;
        pinfo.usbinfo.pid = is_snap_pgm? USB_DEVICE_SNAP_AVR_MODE: USB_DEVICE_PICKIT4_AVR_MODE;
        const char *pgmstr = is_snap_pgm? "MPLAB SNAP": "PICkit 4";

        int pic_mode = serial_open(port, pinfo, &pgm->fd);

        if(pic_mode >= 0) {
          msg_error("\n");
          cx->usb_access_error = 0;
          const char *pgm_suffix = strchr(pgmid, '_')? strchr(pgmid, '_'): "";
          char part_option[32] = {};  // 32 should be enough
          char c_option[32] = {};

          snprintf(c_option, 31, "-c %s_%s", (is_snap_pgm? "snap" : "pickit4"), pgm_suffix);
          if (partdesc) {
            snprintf(part_option, 31, "-p %s", partdesc);
          } else {
            strcpy(part_option, "");
          }

          pmsg_error("%s in AVR mode detected\n", pgmstr);

          imsg_error("to switch into PIC mode try\n");
          imsg_error("$ %s %s %s -P %s -x mode=pic\n", progname, c_option, part_option, port);
          imsg_error("or use AVR mode by using the %s programmer option:\n", &(c_option[3]));  // avoid the "-c "
          imsg_error("$ %s %s %s -P %s\n", progname, c_option, part_option, port);

          serial_close(&pgm->fd);
          return LIBAVRDUDE_EXIT;;
        }
      }
    }
    pmsg_error("no device found matching VID 0x%04x and PID list: ", (unsigned) pinfo.usbinfo.vid);
    int notfirst = 0;
    for(usbpid = lfirst(pgm->usbpid); usbpid; usbpid = lnext(usbpid)) {
      if(notfirst)
        msg_error(", ");
      msg_error("0x%04x", (unsigned int) (*(int *) (ldata(usbpid))));
      notfirst = 1;
    }
  }

[MCUdude]

It still doesn't work. I added a msg_info where I print the iteration number and the pgm->id string. As you can see, we're only getting pickit5_isp, so if(str_starts(ldata(ln), "snap") || str_starts(ldata(ln), "pickit4")) will never be true, even though my PICkit4 (in PIC mode) is connected.

  // No PICkit4 / SNAP (PIC mode) nor PICkit5 found, look for programmer in AVR mode
  if(rv < 0) {
    int i = 0;
    for(LNODEID ln = lfirst(pgm->id); ln; ln = lnext(ln)) {
      msg_info("i: %d, pgm->id: %s\n", i, ldata(ln));
      if(str_starts(ldata(ln), "snap") || str_starts(ldata(ln), "pickit4")) {
        bool is_snap_pgm = str_starts(ldata(ln), "snap");

        pinfo.usbinfo.vid = USB_VENDOR_ATMEL;
        pinfo.usbinfo.pid = is_snap_pgm? USB_DEVICE_SNAP_AVR_MODE: USB_DEVICE_PICKIT4_AVR_MODE;
        const char *pgmstr = is_snap_pgm? "MPLAB SNAP": "PICkit 4";

// ...

[MX682X]

You're right, sorry for not thinking through everything - I've noticed from the screenshot that the VIDs and PIDs didn't align and focused too much on that.
I have rewritten the code to just probe the usb system with hard-coded PIDs and VID for snap and pickit 4. It should do the trick for testing purposes for now. Maybe expand it later to have a linked list populated with the bootloader and AVR mode PIDs. (code to be pasted into lines 629, 630, 631).

If this won't work, first thing I'll do after is to figure out how to commit to this PR, copy pasting code is awful...

EDIT: I just realized that by redefining the defines I am able to test it with the PICkit 5.... Exams are really draining.
EDIT 2: Made some quick fixes based on test results.

 if(rv >= 0)  // if a programmer in PIC mode found, we're done
    return rv; 

  // otherwise, try to see if there is a SNAP or PK4 in AVR mode
  pinfo.usbinfo.vid = USB_VENDOR_ATMEL;
  pinfo.usbinfo.pid = USB_DEVICE_SNAP_AVR_MODE;
  
  const char *pgm_suffix = strchr(pgmid, '_')? strchr(pgmid, '_'): "";
  char part_option[32] = {};  // 32 should be enough

  if(partdesc) {
    snprintf(part_option, 31, "-p %s ", partdesc);
  } else {
    strcpy(part_option, "");
  }

  rv = serial_open(port, pinfo, &pgm->fd);  // Try SNAP PID
  
  if(rv >= 0) {
    msg_error("\n");
    cx->usb_access_error = 0;

    pmsg_error("MPLAB SNAP in AVR mode detected\n");

    imsg_error("to switch into PIC mode try\n");
    imsg_error("$ %s -c snap%s %s-P %s -x mode=pic\n", progname, pgm_suffix, part_option, port);
    imsg_error("or use the programmer in AVR mode with the following command:\n");
    imsg_error("$ %s -c snap%s %s-P %s\n", progname, pgm_suffix, part_option, port);

    serial_close(&pgm->fd);
    return LIBAVRDUDE_EXIT;
  }

  pinfo.usbinfo.pid = USB_DEVICE_PICKIT4_AVR_MODE;
  rv = serial_open(port, pinfo, &pgm->fd);  // Try PICkit4 PID

  if(rv >= 0) {
    msg_error("\n");
    cx->usb_access_error = 0;

    pmsg_error("PICkit 4 in AVR mode detected\n");

    imsg_error("to switch into PIC mode try\n");
    imsg_error("$ %s -c pickit4%s %s-P %s -x mode=pic\n", progname, pgm_suffix, part_option, port);
    imsg_error("or use the programmer in AVR mode with the following command:\n");
    imsg_error("$ %s -c pickit4%s %s-P %s\n", progname, pgm_suffix, part_option, port);

    serial_close(&pgm->fd);
    return LIBAVRDUDE_EXIT;
  }

  pmsg_error("no device found matching VID 0x%04x and PID list: 0x%04x, 0x%04x, 0x%04x\n", USB_VENDOR_MICROCHIP,
              USB_DEVICE_PICKIT5, USB_DEVICE_PICKIT4_PIC_MODE, USB_DEVICE_SNAP_PIC_MODE);
  imsg_error("nor VID 0x%04x with PID list: 0x%04x, 0x%04x\n", USB_VENDOR_ATMEL, USB_DEVICE_PICKIT4_AVR_MODE, USB_DEVICE_SNAP_AVR_MODE);

  return LIBAVRDUDE_EXIT;
}

[MCUdude]

You're right, sorry for not thinking through everything - I've noticed from the screenshot that the VIDs and PIDs didn't align and focused too much on that.

No worries @MX682X! I'm just happy you're willing to look into it.

It does work, but I've never seen these perimission denied errors before...

$ avrdude -cpickit5_isp -patmega328p
Warning: (i/face 0) Permission denied
Warning: (i/face 1) Permission denied
Warning: no usable interface found
Error: no device found matching VID 0x04d8 and PID list: 0x9036, 0x9012, 0x9018
nor VID 0x03eb with PID list: 0x2177, 0x2180

USB access errors detected; this could have many reasons; if it is
USB permission problems, avrdude is likely to work when run as root
but this is not good practice; instead you might want to
check out USB port permissions on your OS and set them correctly

Avrdude done.  Thank you.

$ sudo avrdude -cpickit5_isp -patmega328p

Error: PICkit 4 in AVR mode detected
to switch into PIC mode try
$ avrdude -c pickit4_isp -p atmega328p -P usb -x mode=pic
or use the programmer in AVR mode with the following command:
$ avrdude -c pickit4_isp -p atmega328p -P usb

Avrdude done.  Thank you.

$ avrdude -cpickit5_isp -patmega328p
Error: no device found matching VID 0x04d8 and PID list: 0x9036, 0x9012, 0x9018
nor VID 0x03eb with PID list: 0x2177, 0x2180

Avrdude done.  Thank you.

[MCUdude]

Weird. If I don't use sudo, I'm not able to connect to the SNAP/PICkit4 when using -c pickit5 when the programmer is in AVR mode. I'm just getting a permission denied error. However, when the programmer is in PIC mode, I can connect just fine. No idea why this is...

[MX682X]

I have never seen those errors either.

I think I found the culprit though - wrong endpoints. Unluckily, Atmel and Microchip decided on using the exact opposite numbering. (AVR Mode is using 0x82 while PIC mode is using 0x81 on bulk reads, as an example). Trying to use the endpoints the wrong way might be a reason for permission problems.

if(rv >= 0)  // if a programmer in PIC mode found, we're done
    return rv; 

  // otherwise, try to see if there is a SNAP or PK4 in AVR mode
  pgm->fd.usb.max_xfer = USBDEV_MAX_XFER_3;
  pgm->fd.usb.rep = USBDEV_BULK_EP_READ_3;
  pgm->fd.usb.wep = USBDEV_BULK_EP_WRITE_3;
  #if defined(HAVE_LIBUSB)
  pgm->fd.usb.eep = USBDEV_EVT_EP_READ_3;
  #endif

  pinfo.usbinfo.vid = USB_VENDOR_ATMEL;
  pinfo.usbinfo.pid = USB_DEVICE_SNAP_AVR_MODE;

[MCUdude]

Adjusting the USB parameters is a good thing, but changing the endpoints (and the other USB parameters) didn't make a difference. However, what did work was if I added serdev = &usbhid_serdev; before connecting to the PICkit4 in AVR mode. This means that using hidapi instead of libusb works better. But why? And why aren't there any permission issues when using libusb with a programmer that's in PIC mode?

I would prefer to only have to rely on one driver, so it would be neat if we could figure out why libusb requires sudo.

Anyways, I've pushed a commit that uses hidapi if present.

[mcuee]

For macOS we should really only use hidapi for HID devices. And in fact I will say we should use hidapi whenever it is avaialble for any OS and then only use libusb as a fall-back in case hidapi is not available.

References from libusb FAQ.
https://github.com/libusb/libusb/wiki/FAQ#user-content-Does_libusb_support_USB_HID_devices

• How to use the new macOS kernel detach feature for non-root user libusb/libusb#1014

[mcuee]

And why aren't there any permission issues when using libusb with a programmer that's in PIC mode?

Maybe you want to post the output of lsusb -vvv of the PIC mode and AVR mode to see if there are any differences. Or you can use USB Prober.

You need to install usbutil package by brew install usbutils in order to use lsusb -vvv under macOS.

[MCUdude]

IIRC we have to use libusb when using a PICkit 5 or a PICKIT4/SNAP in PIC mode. See PR #1863 where @MX682X added UPDI support. And the reason is that we need to write to specific endpoints.

Maybe you want to post the output of lsusb -vvv of the PIC mode and AVR mode to see if there are any differences. Or you can use USB Prober.

I'll try this later today

[mcuee]

IIRC we have to use libusb when using a PICkit 5 or a PICKIT4/SNAP in PIC mode. See PR #1863 where @MX682X added UPDI support. And the reason is that we need to write to specific endpoints.

Ah, I see. Now I remember from the comments in PR #1863. PICKit 4/SNAP in PIC mode is not an HID device, unlike in AVR mode, in that case, libusb has to be used.

• PICkit 5 support #1863 (comment)

Yes, thanks. What I can say, is that the payloads are identical, so pickit 4 in PIC mode should be supportable. There are also High-Speed Transfers with 4 Endpoints still, no HID packets, only Vendor Specific (0xFF). This means, that I will throw out my attempt of usbhid support alltogether, as the PICkits don't talk in HID at all.

[mcuee]

Maybe you want to post the output of lsusb -vvv of the PIC mode and AVR mode to see if there are any differences. Or you can use USB Prober.

I'll try this later today

Yes, this is still good to understand why sudo is required. By right you should not need to use sudo under macOS for generic vendor specific USB device.

But if you use Linux, then you need to set up the right udev rules in order to use libusb without using sudo.

[MX682X]

Maybe you want to post the output of lsusb -vvv of the PIC mode and AVR mode to see if there are any differences. Or you can use USB Prober.

I'll try this later today

Yes, this is still good to understand why sudo is required. By right you should not need to use sudo under macOS for generic vendor specific USB device.

But if you use Linux, then you need to set up the right udev rules in order to use libusb without using sudo.

I know that MPLAP has added the udev rules for the PICkit5 (and the other debuggers) during the installation. That's why I didn't had any problems using it. Can avrdude also add the udev rules when installed through apt when there aren't any rules yet? I can imagine there will be people trying to use the PK5 without an installed MPLAB.

[stefanrueger]

Can avrdude also add the udev rules when installed through apt when there aren't any rules yet?

That is the responsibility of the discerning respective package maintainer within the many distros. Fedora's avrdude package installs udev rules when avrdude is installed, I believe. Avrdude offers help to formulate rules through its -c .../u developer option; for example, try

$ avrdude -c pickit5\*/u
1. Examine the suggested udev rules below; to install run:

avrdude -c "pickit5*/u" | tail -n +11 | sudo tee /etc/udev/rules.d/55-avrdude-pickit5X.rules
sudo chmod 0644 /etc/udev/rules.d/55-avrdude-pickit5X.rules

2. Unplug any AVRDUDE USB programmers and plug them in again
3. Enjoy user access to the USB programmer(s)

Note: To install all udev rules known to AVRDUDE follow: avrdude -c "*/u" | more

# Generated from avrdude -c "pickit5*/u"

ACTION!="add|change", GOTO="avrdude_end"

# pickit5, pickit5_jtag, pickit5_dw, pickit5_isp, pickit5_pdi, pickit5_tpi, pickit5_updi
SUBSYSTEM=="usb", ATTRS{idVendor}=="04d8", ATTRS{idProduct}=="9012", MODE="0660", TAG+="uaccess"
SUBSYSTEM=="usb", ATTRS{idVendor}=="04d8", ATTRS{idProduct}=="9018", MODE="0660", TAG+="uaccess"
SUBSYSTEM=="usb", ATTRS{idVendor}=="04d8", ATTRS{idProduct}=="9036", MODE="0660", TAG+="uaccess"

LABEL="avrdude_end"

[MCUdude]

@mcuee here is the lsusb -vvv output:

$ lsusb -vvv                                                                                                                              7s

Bus 001 Device 001: ID 03eb:2177  
Negotiated speed: High Speed (480Mbps)
Device Descriptor:
  bLength                18
  bDescriptorType         1
  bcdUSB               2.00
  bDeviceClass          239 [unknown]
  bDeviceSubClass         2 [unknown]
  bDeviceProtocol         1 
  bMaxPacketSize0        64
  idVendor           0x03eb 
  idProduct          0x2177 
  bcdDevice            1.00
  iManufacturer           1 
  iProduct                2 
  iSerial                 3 
  bNumConfigurations      1
  Configuration Descriptor:
    bLength                 9
    bDescriptorType         2
    wTotalLength       0x006b
    bNumInterfaces          3
    bConfigurationValue     1
    iConfiguration          0 
    bmAttributes         0x80
      (Bus Powered)
    MaxPower              500mA
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        0
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass         3 [unknown]
      bInterfaceSubClass      0 [unknown]
      bInterfaceProtocol      0 
      iInterface              0 
        HID Device Descriptor:
          bLength                 9
          bDescriptorType        33
          bcdHID               1.11
          bCountryCode            0 Not supported
          bNumDescriptors         1
          bDescriptorType        34 (null)
          wDescriptorLength      35
          Report Descriptors: 
            ** UNAVAILABLE **
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x81  EP 1 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               1
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x02  EP 2 OUT
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval               1
    Interface Association:
      bLength                 8
      bDescriptorType        11
      bFirstInterface         1
      bInterfaceCount         2
      bFunctionClass          2 [unknown]
      bFunctionSubClass       2 [unknown]
      bFunctionProtocol       0 
      iFunction               5 PICkit 4 Virtual COM Port
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        1
      bAlternateSetting       0
      bNumEndpoints           1
      bInterfaceClass         2 [unknown]
      bInterfaceSubClass      2 [unknown]
      bInterfaceProtocol      0 
      iInterface              0 
      CDC Header:
        bcdCDC               1.10
      CDC ACM:
        bmCapabilities       0x02
          line coding and serial state
      CDC Union:
        bMasterInterface        1
        bSlaveInterface         2 
      CDC Call Management:
        bmCapabilities       0x03
          call management
          use DataInterface
        bDataInterface          2
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x84  EP 4 IN
        bmAttributes            3
          Transfer Type            Interrupt
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0040  1x 64 bytes
        bInterval              16
    Interface Descriptor:
      bLength                 9
      bDescriptorType         4
      bInterfaceNumber        2
      bAlternateSetting       0
      bNumEndpoints           2
      bInterfaceClass        10 [unknown]
      bInterfaceSubClass      0 [unknown]
      bInterfaceProtocol      0 
      iInterface              0 
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x83  EP 3 IN
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
      Endpoint Descriptor:
        bLength                 7
        bDescriptorType         5
        bEndpointAddress     0x05  EP 5 OUT
        bmAttributes            2
          Transfer Type            Bulk
          Synch Type               None
          Usage Type               Data
        wMaxPacketSize     0x0200  1x 512 bytes
        bInterval               0
Device Qualifier (for other device speed):
  bLength                10
  bDescriptorType         6
  bcdUSB               2.00
  bDeviceClass          239 [unknown]
  bDeviceSubClass         2 [unknown]
  bDeviceProtocol         1 
  bMaxPacketSize0        64
  bNumConfigurations      1
can't get debug descriptor: No such file or directory
Device Status:     0x0000
  (Bus Powered)