-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for high-voltage UPDI #1015
Conversation
Thanks for the PR @janegilruud!
// Generate 12V UPDI pulse if user asks for it and hardware supports it
if (p->flags & AVRPART_HAS_UPDI &&
PDATA(pgm)->use_hvupdi == true &&
p->hvupdi_variant == HV_UPDI_VARIANT_0) {
parm[0] = PARM3_UPDI_HV_SIMPLE_PULSE;
if (jtag3_setparm(pgm, SCOPE_AVR, 3, PARM3_OPT_12V_UPDI_ENABLE, parm, 1) < 0)
return -1;
} EDIT: And we're not going to do anything with the |
Yes, we should update that snippet. As I mentioned in #861 (comment) we could add a hvupdi_variant parameter to the programmers description too, and check if the target hvupdi_variant is present in the programmers hvupdi_variant-list. |
The |
Thanks. I think this PR is ready then! |
@MCUdude The grammar change looks good and is self-documenting (grammar variable same name as struct component, etc). Tiny request, though: please rename K_HVUPDI_IMPLEMENTATION to K_HVUPDI_VARIANT |
Thanks for the input @stefanrueger! I've updated the PR and replaced |
@janegilruud from what I can tell, the Pickit4 still outputs its 12V pulse on the UPDI pin (pin 4 on PK4 the programmer) instead of on the RESET pin (pin 6 on the PK4 programmer) with this PR. However, the same happens when using pymcuprog as well with the following command: Note that I don't own any DD or EA hardware (wish I did), but my tongue says that there must be a HV pulse on the UPDI pin when trying to communicate with an EA target, even when using pymcuprog. (I'm currently on vacation and only brought essentials, so I can't measure the UPDI pin with my oscilloscope). Here are the Pickit4 information Avrdude prints:
|
Update PICkit4 to latest FW. It is now possible to update with Microchip Studio too. |
@janegilruud I only brought a mac on vacation, so I can only update the tool through MPLAB X. I'm very happy to hear Microchip Studio finally can be used to update the PK4 (and SNAP as well?). IMO, MPLAB X is not nearly as intuitive and elegant as MS7. I hope it doesn't get discontinued in favor of MPLAB. After updating it, the latest version seems to be 1.14.268
The RESET line (PK4 pin 6) on the PK4 appears to be "dead" relative to GND when trying HVUPDI with "variant 2" targets. |
With FW 1.14 you successfully updated the tool. With the latest FW in PK4 this branch should be able to send a UPDI HV pulse on the first pin on the PK4, provided that the target is a device with MPLAB X IDE/IPE 6.00 does not support this, so that won't work. The latest Studio do, but that won't help you much while you only have a Mac. (And yes, Studio can update Snap too.) How do you measure the pulse? Have you brought a oscilloscope on vacation? :-) |
I did, once! But my wife won't let me anymore. She says that I'm supposed to "leave the geek stuff at home" when going on vacation. I have no idea why 😉
That explains it! For some reason, I've always thought that the HV pulse is outputted on the PK4's RESET pin (pin 6) instead of pin one. I'll give it a try tomorrow. I'm home for a few days, so I can verify this with an oscilloscope and post some screenshots for good measure. |
Sorry, I missed the part of your comment about looking at PK4 pin 6, but I still think it was necessary to update the FW. Anyway, the screenshots look good :-) |
@MCUdude, I created a PR to your branch, have you looked at it? |
@janegilruud thanks for the PR. I can't get it to work, see my comment here MCUdude#2 (comment).
Out of curiosity. To convert a PK4 or a SNAP from PIC to AVR mode is this just a simple command, or is the tool reflashed by MPLABX/MS7? If it's just a simple command, this is something that would be neat to either implement here or create a simple FOSS CLI tool that does just this. |
It's "just a simple" command. Yes, I agree there should be an option outside the IDEs. |
Great, that means it's possible for tools like Avrdude to "quickly" switch to AVR mode. My dream would be something like this:
|
Add HVUPDI_SUPPORT list for programmers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is easy to read and to easy to follow. I don't have the kit to check the hardware effect of the changes, but this has been done elsewhere with osci-shots to prove the functionality correct. Particularly good that the documentation has been updated, too!
I have only a few (very small) comments below. In my opinion the PR can be merged without addressing them.
Why is sup
below unsigned int?
Lines 1262 to 1263 in 913509d
unsigned int sup = (unsigned int)(*(int *)(ldata(hvupdi_support))); | |
if(sup == p->hvupdi_variant) { |
p->hvupdi_variant is of type int. These lines could be shortened to sth like
if(*(int *) ldata(hvupdi_support) == p->hvupdi_variant) {
and you might get one fewer compiler warning (integer comparison of different signedness or some such). Similarly, why not shorten
Lines 1653 to 1658 in 913509d
unsigned int hv_sup; | |
avrdude_message(MSG_NOTICE2, "%s: HV UPDI support:", progname); | |
for (ln = lfirst(pgm->hvupdi_support); ln; ln = lnext(ln)) { | |
hv_sup = (unsigned int)(*(int *)ldata(ln)); | |
avrdude_message(MSG_NOTICE2, " %d", hv_sup); | |
} |
to sth like
avrdude_message(MSG_NOTICE2, "%s: HV UPDI support:", progname);
for (ln = lfirst(pgm->hvupdi_support); ln; ln = lnext(ln))
avrdude_message(MSG_NOTICE2, " %d", *(int *) ldata(ln));
Line 1499 in 913509d
else if ((matches(extended_param, "hvupdi") || matches(extended_param, "hvupdi=1")) && |
If I understand this correctly, matches(extended_param, "hvupdi")
implies matches(extended_param, "hvupdi=1")
, so is sufficient in the or clause. I know that the documentation does only mentions -xhvupdi
and not -xhvupdi=...
. However, if the code allows the user to supply -xhvupdi=0
might they expect the functionality to be switched off? Maybe do a quick check with different -x
arguments to see whether the current parsing is how you want it to be.
Line 1500 in 913509d
(matches(ldata(lfirst(pgm->id)), "pickit4_updi") || matches(ldata(lfirst(pgm->id)), "powerdebugger_updi"))) { |
Is the match against the programmer names necessary and useful? If so OK. It's not uncommon that techniques like these are used in AVRDUDE, but in general I would have an (ever so slight) preference to use properties of programmers rather than the name of the programmer.
All things considered, this PR is a smashing solution to a difficult problem, so yes please go for merging it.
Just old habit. No good reason. Should be changed to
With the introduction of Regarding the |
…The hvupdi type is implied by the part configuration.
Clean up and simplify hvupdi handling, and set default hvupdi_variant
Thank you @janegilruud for your extensive work, and @stefanrueger for providing excellent feedback! |
@MCUdude
|
Yes, I believe you need 2.14 for HV UPDI to work. @mcuee I'll look at the other issues you've tagged me in very soon! |
Somehow I still have a problem. pymcuprog seems to work, but not avrdude.
|
Any I can unlock the device now.
|
@mcuee I can't test it right now, but it should work on AVR-DDs. Lines 1200 to 1220 in b569966
Maybe you can try to add a few debug line to see what really happens?
|
This PR provides high-voltage UPDI support for the PICkit 4 and the Power Debugger.
It works perfectly with my hardware, but I would be very grateful if others also can test if they have compatible hardware.
Closes #861