PICkit 5 support #1863
PICkit 5 support #1863
Conversation
|
Brilliant @MX682X We'll have to carefully review and test. I have just quickly scrolled through the code and it looks competently written. One thing I noted is that the But yes, vvv pleased to have a pickit5 implementation. With a bit of fair wind we could get that into v 8.0. |
OK, as long as the pointer to the local array on the stack is passed down to callees and not returned to callers. Do you want to share an example? |
|
Ok, so MacOS rellay needs usbhid and can't compile with libusb.... this is bad, because from what I know I have to talk to the Specific Endpoints I could try to figure out what they doing in fruit-land, if anyone with MacOS, MPLAB and Wireshark could send me the capture files.
https://github.com/MX682X/avrdude/blob/88e78565125102ff27158c050dbecbdbd1e4043a/src/pickit5.c#L829
The whole thing was an attempt at optimization. The way the reading is done is that the start address and the length is transmitted to the PICkit and it runs the for-loop internally and sends the data back in one chunk. Requesting only one byte at a time creates a significant overhead on the USB side I'm trying to avoid. At least when it comes to Unpaged memory like SRAM or EEPROM, and it seemed like that if there is no paging, avrdude won't allow reading multiple bytes. |
|
@MX682X Your examples with usage of local arrays are standard and good practice.
They are only treated as unpaged if their Surprisingly, many AVRnnDxmm parts have an EEPROM size of This may or may not be needed. I don't know why that is! You could try to overwrite This will ensure the To cut a long short: please don't change Did I say that it's exciting to have pickit5 in AVRDUDE? Brilliant stuff. I will review and help fold your code into AVRDUDE with a view that it fits with the philosophy and can be maintained by us. Please bear with me. |
|
Sorry for the closing/reopening, still figuring out git... PS: I haven't touched the High Voltage Programming yet, so this feature s not supported yet |
@MX682X I don't quite get what the problem is with macOS and libusb/hidapi. |
|
@MCUdude I need to write to specific Endpoints. I was not able to find a way to specify an endpoint in the usbhid files.
same goes for read. But now that I'm thinking about it, maybe it could work if there are two hid descriptors, the second being a copy, except for the Endpoints, which are changed? Could this work? But then again the whole ordeal fails as I have no way to test it. |
Excellent, @MX682X. It's now a much neater separation of programmer code and AVRDUDE infrastructure A couple of things I noted whilst scrolling through the PR (without reading code)
Not sure when I have time for a proper review etc, but will be in next few days.
|
|
@MX682X so the reason for using libusb here is to be able to write to specific endpoints, which hidapi does not support. But the way libusb is implemented in If I am, why is this only an issue on macOS, and not all platforms? |
It is not possible (or nearly impossible) to detach kernel hid driver under macOS to use libusb. The bug with libusb fallback for USB HID device is a different story. I think it does not work for full speed USB device like PICKit 4 and SNAP. I do not have Atmel ICE and Power Debugger to test whether the fallback works for them or not. You may want to test them under Linux or Windows (you can not test under macOS as libusb will not work). You can refer to the comments by @askn37
|
The recommendation is really to use hidapi and not libusb for USB HID device. As for the translation between libusb and hidapi, I think the USB HID spec is quite useful. You can read CH7 and CH8. Basically hidapi talks in HID report (input, output and feature report). libusb talks by endpoint with different transfer type (only Control Transfer and Interrupt transfer are involved for USB HID device). That being said, you can continue your work with libusb. If you make it work, that helps a lot already. We can leave macOS to the later. BTW, PICKit 2 avrdude support does not work under macOS for the same reason -- it uses libusb (or Windows native HID API under Windows). It needs to be ported to hidapi as well in order for macOS support to work. |
|
You can use the following tool to parse the USB HID report descriptors under Linux (Windows is not good at this, macOS is good). Or you can use hidapi's hidtest utility under Linux to dump the raw USB HID report descriptor and then uses online tools to parse. This is the method I used in Issue #1221. Example HID Report Descriptor dump by @askn37 for Atmel ICE using macOS. |
@stefanrueger Thanks for the reminder. The reason why I decided to name it that way at the beginning was that the scripts are not exclusive to the PICkit5, they should work with the PICkit4 (when in PIC mode) as well. So by changing the PID locally on your end to match the PICkit4, the 5 code should run with the 4.
The reason for the different files is that I have written a Python script that grabs the scripts.xml file from a Toolpack, filters the UPDI functions, compresses them through a look-up-table and creates the C and H files necessary for compilation (a config file or something would be probably better, but I have no experience with that topic)
Go Ahead
Fine by me
I prefer a squash merge, as it is more clean Oh, and one more question: Can you tell me how I can cross-compile avrdude for Windows/MacOS? |
@MCUdude I'm not sure, but it worked High-Speed on my end, I think. Transfer Size is 512 bytes. However, I also found out, that the Endpoints are not HID, but Vendor Specific (0xFF), thus usbhid fails to open, I think. |
|
@mcuee usbhid-dump did not find any HID devices (Ubuntu) with the debugger connected. Also, avrdude reports the following on init: lsusb says it is connected, though: |
That's good to know. According to a Microchip developer, this PR should also work with the ICE4 and ICD5 programmers as well. And if it works with PICkit4 in PIC mode, it works with the MPLAB SNAP in PIC mode as well. @MX682X I do have a PICkit4 I can use with MPLAB X to get some Wireshark data on my Mac, if that's useful to you. |
Preferably one capture on Readout of Program (empty chip is fine) and one Upload of a small Program, preferably < 1023 bytes |
|
I'll see what I can do. Is it OK if I email you the capture files? |
|
I'd prefer a zip attached to a comment, but I guess you can send it to *************** (without spaces) EDIT by MCUdude: removed email address |
|
@MX682X email sent! |
|
Yes, thanks. What I can say, is that the payloads are identical, so pickit 4 in PIC mode should be supportable. Looking further into the matter, I think the build Problem lies in bad #if defines #include at the top of the file |
I made the same guess when AVR-EB was announced. Another reason is that USERROW and BOOTROW in AVR-Dx/Ex families are FLASH memory characteristics, not EEPROM, and therefore need to support page erase commands. They need to be pages in one block, so the base address needs to be a clean paragraph, otherwise it will clutter the on-die address calculation circuitry. It seems they also wanted to standardize the EEPROM base address across the family to 0x1400. Next to that is where the microcode for UPDI, OCD, etc. is stored, and they didn't want to cut that out and extend it, so 0x1100 was sacrificed. |
The mentioned fix calculates the offset at runtime, while I'm using "hard-coded" instructions with the scripts. I just have some if conditions that check the allowed range from 0 to 3 and 4 to 9, and decide on that which script pointer to return. And as a fallback version, there is still the look up by name.
I see, this helps. Here is the testrun summary ./test-avrdude -e ../build_linux/src/avrdude -d1 -p "-c pickit5_updi -p avr32dd28 -b900000"
Testing ../build_linux/src/avrdudePrepare "-c pickit5_updi -p avr32dd28 -b900000" and press 'enter' or 'space' to continue. Press any other key to skip
❌ 2,005 s: fuse access: clear, set and read eesave fuse bit (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "config eesave=0; config eesave=1; config eesave"
❌ 1,764 s: fuse access: set eesave fusebit to delete EEPROM on chip erase (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "config eesave=ee*erased"
✅ 2,723 s: chip erase
✅ 3,310 s: flash -U write/verify holes_rjmp_loops_32768B.hex
❌ 1,870 s: flash -T write/verify holes_rjmp_loops_32768B.hex (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "write flash ./test_files/holes_rjmp_loops_32768B.hex:a"
❌ 3,237 s: flash -T write/verify rjmp_loops_for_bootloaders_32768B.hex (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "write flash ./test_files/rjmp_loops_for_bootloaders_32768B.hex:a"
✅ 2,630 s: eeprom check whether programmer can flip 0s to 1s
✅ 2,607 s: eeprom -U write/verify holes_pack_my_box_256B.hex
❌ 3,694 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_256B.hex (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "write eeprom ./test_files/holes_the_five_boxing_wizards_256B.hex:a" -T flush -T "write eeprom ./test_files/holes_pack_my_box_256B.hex:a"
❌ 5,637 s: eeprom -T write/verify lorem_ipsum_256B.srec (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "write eeprom ./test_files/lorem_ipsum_256B.srec:a"
✅ 2,793 s: chip erase and spot check flash is actually erased
✅ 1,650 s: spot check eeprom is erased, too
❌ 1,643 s: usersig -T/-U write/read random_data_32B.bin (failed command below)
$ ../build_linux/src/avrdude -qq -c pickit5_updi -p avr32dd28 -b900000 -T "erase usersig; write usersig ./test_files/random_data_32B.bin" -T flush -U usersig:r:/dev/shm/test-avrdude.tmp.MsuY4u:r -U usersig:v:/dev/shm/test-avrdude.tmp.MsuY4u:r -T "erase usersig" -T flush -U usersig:v:./test_files/0xff_32B.hex:iThe first fail returns this: All others have just the warning. I haven't thought much about it as it seems to work and thought it might be just on my end. With -vvvv I get |
|
@MX682X Great, thanks for sharing results and finding the source of the warning in Best would be to figure out how to avoid the set configuration 1 error (and avoid it). If pickit5 cannot avoid that warning and if it is of no consequence, we could downgrade the warning from |
|
BTW, the logic of And the logic of not wanting warnings in |
|
As a quick solution I locally changed the warning in usb_libusb.c to notice, and the test completed successfully Tested AVR32DD28 and Attiny1614./test-avrdude -e ../build_linux/src/avrdude -d1 -p "-c pickit5_updi -p avr32dd28 -b900000"
Testing ../build_linux/src/avrdudePrepare "-c pickit5_updi -p avr32dd28 -b900000" and press 'enter' or 'space' to continue. Press any other key to skip
✅ 1,644 s: fuse access: clear, set and read eesave fuse bit
✅ 1,611 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅ 2,726 s: chip erase
✅ 3,215 s: flash -U write/verify holes_rjmp_loops_32768B.hex
✅ 1,713 s: flash -T write/verify holes_rjmp_loops_32768B.hex
✅ 2,603 s: eeprom check whether programmer can flip 0s to 1s
✅ 2,668 s: eeprom -U write/verify holes_pack_my_box_256B.hex
✅ 3,727 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_256B.hex
✅ 2,829 s: chip erase and spot check flash is actually erased
✅ 1,742 s: spot check eeprom is erased, too
✅ 1,645 s: usersig -T/-U write/read random_data_32B.bin
./test-avrdude -e ../build_linux/src/avrdude -d1 -p "-c pickit5_updi -pt1614 -b900000"
Testing ../build_linux/src/avrdudePrepare "-c pickit5_updi -pt1614 -b900000" and press 'enter' or 'space' to continue. Press any other key to skip
✅ 1,617 s: fuse access: clear, set and read eesave fuse bit
✅ 1,708 s: fuse access: set eesave fusebit to delete EEPROM on chip erase
✅ 2,856 s: chip erase
✅ 3,632 s: flash -U write/verify holes_rjmp_loops_16384B.hex
✅ 1,870 s: flash -T write/verify holes_rjmp_loops_16384B.hex
✅ 1,645 s: eeprom check whether programmer can flip 0s to 1s
✅ 1,872 s: eeprom -U write/verify holes_pack_my_box_256B.hex
✅ 1,710 s: eeprom -T write/verify holes_{the_five_boxing_wizards,pack_my_box}_256B.hex
✅ 3,118 s: chip erase and spot check flash is actually erased
✅ 1,613 s: spot check eeprom is erased, too
✅ 1,644 s: usersig -T/-U write/read random_data_32B.bin
|
|
@MX682X Looks like we are converging? Likely to review tonight and push commits with small changes onto this PR. Let me know if I should wait |
|
@MX682X I have done some minor formatting etc. Most looks good:
I might have another look in a couple of days, but so far, I'm very pleased with this new |
I already had the thought, that it might be pretty useless, if not documented properly, and I agree, it would make more sense to do it globally, especially as it would allow to improve the
The datasheet says, that it is recommended to change the BOD setting to the highest value, so no need to change fuses, setting the register directly would be enough. However, MPLAB isn't following the datasheet either, so I guess it should be fine to keep it like that.
I managed to do .texi, but the avrdude.1 has some weird syntax. Would be better if somone with more experience would do that part. Also: How do I "compile" the docs? Haven't seen any doc files.
Done While doing the .texi, I noticed that the SNAP doesn't support HV and can't provide power, so I added some checks for that, just to make sure. Couldn't add the checks in extended params already, as we only find out the device we're connected with later in open() |
|
Also, I wonder, if it's possible to overwrite the functions associated with a programmer to use the functions of another programmer during runtime. EDIT: What I want to say is, that it will end up being pretty confusing specifing pickit 5 to talk to pickit 4. It would be significantly better, user-interface wise, not to clobber them together. I did expect the scripts to be backwards compatible, but not that well. Maybe the PIC mode support could be designated experimental, impling that it will be changed in the future? |
I run |
It sets expectations and either binds future contributors to the strings used here or risks breaking something that users might have started relying on in scripts etc. For example, a global solution might not use mc and at but instead the full names potentially abbreviated to the smallest unique string (m, mi, mic, ... but not mc). That wouldn't be compatible.
Ahh, got you. AVRDUDE knows where these registers are. Do you need bod.ctrla? libavrdude has functions on how to get at the required address, eg, something along the lines should enable you to do what you want.
Can be done, and has been done. It's a nightmare for maintenance, though. And I absolutely discourage that. Much better to tell the user of a PK4 in PIC mode they can either switch it to AVR mode and continue to use |
|
@MX682X I have decided it is better at this point to not commit to specific pid/vid aliases in the Here how the LaTex docu looks like. There is a slight problem that SNAP appears three times and that the third time the options don't seem to pertain to SNAP (or do they?). @MX682X what is correct? @MCUdude Should we do something about a single physical programmer having multiple entries here? Unrelated to this PR: Should this section not be about
|
I was just about to do a commit, when I saw your pushes. Removed some lingering function calls that were useless at this point.
When I copy pasted the paragraph above, I did not know that SNAP has no voltage converter at all, and it kinda persisted. So the last "SNAP" should be replaced with "PICkit 4", I guess |
|
OK, this is now almost ready to merge
Unless I hear otherwise I will squash merge (I think that was @MX682X's preference). We normally credit contributors of substantial pieces of work in AUTHORS, typically real name and optional email address. @MX682X how do you want to appear there? Aliases or usernames should be OK if that's what your pref is. |
Forgot to mention: I thought about it and I think it will be kinda annoying to implement. There are a couple of script functions that activate UPDI System Reset, meaning that the BOD Register will be reset after it, so I think it might be better to go without this. MPLAB isn't doing it either, and we have one voltage measurement anyway.
I'd rather prefer my alias. |
|
I'm not sure if it matters, but I'd like to point out that I haven't committed the workaround in |
|
@MX682X Thanks! Done now |
|
Actually the better approach would be to find a way not to get the warning (perhaps not configuring if already configured)... [edit On the other hand now is not the time to tinker with central code] |
Alright, should be complete now, or at least complete enough for experimental release.
Supports: Writing and reading of all memories, chip erase, Setting Baudrate (which is the UPDI Baudrate in this case), Supplying Power through "-xvoltage="[mV]
As the PICkit5 can read and write arbitrary amounts, I've added read_array()/write_array() functions, allowing a significant reduction in overhead, compared to single byte reads/writes of e.g. SRAM
When testing the terminal mode, I noticed that some functions were checking "rc" against non zero after byte reads, which didn't make sense for me, as the read functions are allowed to return number of read bytes (meaning returning 1 for 1 byte read was treated as error), so I decided to change it to check against negative rc
Oh, and one more thing:
I haven't programmed much for OS based systems, so I can't tell if it's bad or not: There are some functions, where I declare a small local array and pass it as pointer to another function. So far, no problem my test, but I can imagine it might cause Problems. Any suggestions on that topic?