-
Notifications
You must be signed in to change notification settings - Fork 1
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
Clean up and simplify hvupdi handling, and set default hvupdi_variant… #3
Conversation
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.
Thanks for considering my comments. I like the way that you removed the dependency of the programmer names with defaulting hvupdi to -1.
Could I ask you or @MCUdude do a final sanity check as to whether all still works as expected (I don't have the HW).
I have one remaining recommendation: If the user is not supposed to supply the hvupdi variant on the command line (and that makes perfect sense to me), then please consider replacing
else if (matches(extended_param, "hvupdi")) {
with
else if (strcmp(extended_param, "hvupdi") == 0) {
The reason is that match() is strncmp() in diguise, so -xhvupdi=3.1415, -xhvupdi=0 or -xhvupdi=-1 would all still match and trigger PDATA(pgm)->use_hvupdi = true;
which seems counter-intuitive to me from a user point of view. The suggested strcmp() would only accept one string (as is documented).
If you decide to make this change, please remember I have done a "blind" review, ie, I have not compiled, let alone run the modified AVRDUDE. So, please test with a few command line options whether implementation matches documentation. Also imagine a user who hasn't read the documentation and predict whether or not they'd find the suggested command line logic reasonable. (From what you said earlier I understand this to be a "yes").
…The hvupdi type is implied by the part configuration.
I do build it before I push the changes, so at least it compiles. But I do not have the HW setup easily available at the moment, so I hope @MCUdude can do some quick sanity checks on the latest changes. |
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.
Brill, over to @MCUdude - once confirmed working consider merging with AVRDUDE main straight away
The PR works fine, but why change the behavior in
- else if ((matches(extended_param, "hvupdi") || matches(extended_param, "hvupdi=1")) &&
- (matches(ldata(lfirst(pgm->id)), "pickit4_updi") || matches(ldata(lfirst(pgm->id)), "powerdebugger_updi"))) {
+ else if (strcmp(extended_param, "hvupdi") == 0) {
PDATA(pgm)->use_hvupdi = true;
continue;
} |
I don't think it is necessary to expose the user for hvupdi type. The type for the selected target is given in avrdude.conf, so Avrdude already has that information. The users then only has to choose if they want to use HVUPDI or not.
Yes, Totally agree. Should do the check using hvupdi_support and not programmer name. |
I guess there are several ways to check if the programmer supports HVUPDI. Here's one suggesting in
For programmers that supports HVUPDI type 0 or 2 the EDIT: But this throws no error... so a slight rewrite of this suggestion... |
Do we want to add a
|
I don't think that's necessary. It will terminate before that code is even executed:
|
If you use a PowerDebugger and try to do hvupdi on a type 2 target, then you will get that message. Only case I can think of. |
It doesn't hurt to have it, so added Off topic: Do you think Microchip would either donate or let one of us spare time Avrdude developers borrow some hardware to ensure that Avrdude properly supports the hardware? There's a few issues we can't work on without testing it on actual hardware:
|
I'll see what I can do... |
… to -1.
Updated according to comments by @stefanrueger. (avrdudes#1015 (review))