-
Notifications
You must be signed in to change notification settings - Fork 139
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
new functions (#1570): get_vtarget, get_varef, get_fosc, get_sck_period #1574
Conversation
Thanks for the PR @Ho-Ro, a very welcome PR! I'll go through it soon. However, we should also look into adding this functionality to the JTAG1, JTAG2, and JTAG3 programmers as well. I'm sure it's very similar to how you did the stk500 and stk500v2 implementation. |
I'm getting this error when trying to read the ISP clock speed using an STK500 board running v2 firmware.
When using a board that's running v1 firmware, it works just fine. EDIT:
|
I see now that the ISP clock frequency is a bit fiddly. You'll have to either use AFAIK you can't just change
You should probably copy the implementation from |
I think it will need a bigger refactoring: extract the functions to retrieve the values and use these functions in |
73b0fb9
to
787770f
Compare
I've rebased my PR on the new main and tested with nanoSTK and ScrMo, please have a look if it works for the other stk500v2 based programmer types. |
Thanks @Ho-Ro. I've had a cursory look at the code,and all looks good (but will do a thorough review once no longer marked as draft). |
…sck_period this is only a proof of concept declarations for libavrdude, parser in term.c sample implementations in stk500v1.c Signed-off-by: Martin <[email protected]>
Signed-off-by: Martin <[email protected]>
Signed-off-by: Martin <[email protected]>
Signed-off-by: Martin <[email protected]>
difficult to test w/o programmer HW (only orig STK500 (v2) and SMo) Signed-off-by: Martin <[email protected]>
difficult to test w/o programmer HW (only orig STK500 (v2) and SMo) Signed-off-by: Martin <[email protected]> # Conflicts: # src/stk500v2.c
Signed-off-by: Martin <[email protected]>
Signed-off-by: Martin <[email protected]>
And again rebased on latest |
I did my last tests, all ok from my side. |
I'll test the PR again. BTW the whitespaces for the SCK line aren't correct (missing six whitespaces. It is, however, correct in git main.
|
I just rested this PR, again using two separate STK500 board running v1 and v2 firmware. I also tested the PR using an avrispmkii. Everything works great! The only issue I've found is the missing whitespaces, and different number of decimals between the
|
src/jtagmkI.c
Outdated
*v = 500e3; | ||
else if (dur == JTAG_BITRATE_250_kHz) | ||
*v = 250e3; | ||
else |
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.
Would it not be better to have sth like
else if(dur == JTAG_BITRATE_125_kHz)
*v = 125e3;
else
return -1;
AVRDUDE is normally good at checking all possible things that can go wrong. We might also want to sprinkle in a pmsg_error("(reason for returning -1)\n");
when a function couldn't do its job and returns -1. This makes debugging soooo much easier.
src/jtagmkI.c
Outdated
static int jtagmkI_get_vtarget(const PROGRAMMER *pgm, double *v) { | ||
unsigned char vtarget = 0; | ||
if (jtagmkI_getparm(pgm, PARM_OCD_VTARGET, &vtarget) < 0) | ||
return - 1; |
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.
(This is not wrong, but the monadic change sign operator normally isn't followed by a space.)
@@ -1252,8 +1281,9 @@ static int stk500_set_fosc(const PROGRAMMER *pgm, double v) { | |||
} | |||
} | |||
if (idx == sizeof(ps) / sizeof(ps[0])) { | |||
pmsg_warning("f = %u Hz too low, %u Hz min\n", fosc, PDATA(pgm)->xtal / (256 * 1024 * 2)); | |||
return -1; | |||
pmsg_warning("f = %u Hz too low, using %u Hz\n", fosc, PDATA(pgm)->xtal / (256 * 1024 * 2)); |
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.
Good thinking! I like that the code does the nearest best and warns
src/stk500.c
Outdated
return rc; | ||
|
||
if (prescale) { | ||
fosc = PDATA(pgm)->xtal / ((cmatch + 1) * 2 * ps[prescale - 1]); |
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.
Indentation looks wrong, braces not needed? Would fosc = prescale? ...: ....;
be crisper and clearer?
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.
Actually fosc
isn't needed either? *v = prescale? ...: ...;
would do.
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.
Strange, I will check the indentation, normally gcc warns about such stupid formatting.
But I would stick with the if / else construct (w/o braces, possibly a leftover of my previous more complicated calculation) b/c the ternary is harder to read:
fosc = prescale ? PDATA(pgm)->xtal / ((cmatch + 1) * 2 * ps[prescale - 1]) : 0;
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.
Pro tip: if one of the two branches is trivial then making that the first option aids readability:
*v = !prescale? 0: /* here comes what it really is */;
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.
Thx, I've also seen that fosc is not needed and removed it.
src/stk500.c
Outdated
unsigned dur; | ||
int rv = 0; | ||
|
||
if ((rv = stk500_getparm(pgm, Parm_STK_SCK_DURATION, &dur))< 0) { |
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.
Is the spacing around binary <
deliberate? It's a bit unusual to have asymmetric spacing (edit: around a binary operator).
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.
Typo when staring to long at the screen during nighttime (same as above return - 1;
). This is the reason that I do an automatic clang-format -i ...
before I check in my own projects. And my qtcreator
is told to do it automatically when saving.
src/stk500v2.c
Outdated
@@ -294,7 +301,10 @@ void stk500v2_setup(PROGRAMMER * pgm) | |||
memset(pgm->cookie, 0, sizeof(struct pdata)); | |||
PDATA(pgm)->command_sequence = 1; | |||
PDATA(pgm)->boot_start = ULONG_MAX; | |||
PDATA(pgm)->xtal = STK500V2_XTAL; | |||
if (str_starts(pgmid, "scratchmonkey")) |
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.
Crisper and clearer in one line, but that's a matter of taste (and it's your decision).
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.
ok, looks good
PDATA(pgm)->xtal = (str_starts(pgmid, "scratchmonkey")) ? SCRATCHMONKEY_XTAL : STK500V2_XTAL;
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.
Looks so much neater, don't you agree? You can even remove the parentheses for the test and the spaces before ?
and :
to crisp it up:
PDATA(pgm)->xtal = str_starts(pgmid, "scratchmonkey")? SCRATCHMONKEY_XTAL: STK500V2_XTAL;
src/stk500v2.c
Outdated
jtagmkII_getparm(pgmcp, PAR_OCD_VTARGET, vtarget_jtag); | ||
pgm_free(pgmcp); | ||
return b2_to_u16(vtarget_jtag) / 1000.0; | ||
} else if (PDATA(pgm)->pgmtype != PGMTYPE_JTAGICE3) { |
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.
Elsing a return is a bit unusual. It's defo not needed. And code might look clearer if a clear line is left
if(...) {
...
return ...;
}
if(...) {
...
return ...;
}
return 0;
}
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.
fully agree :)
src/stk500v2.c
Outdated
case PGMTYPE_STK500: | ||
stk500v2_getparm(pgm, PARAM_SCK_DURATION, &sck_duration); | ||
return stk500v2_sck_to_us(pgm, sck_duration); | ||
break; |
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.
Again break after a return is ... unnecessary distraction (many times)
src/stk500v2.c
Outdated
return 0.0; | ||
if (osc_pscale == 0) | ||
return 0.0; | ||
else { |
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.
Elsing this return looks confusing (to me)
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.
I reworked this part, looks nice and tidy. Normally I'm against multiple returns from a function, but this is probably my old-school thinking (as a result of being socialised with Pascal that has no return, just reaching the end of the function and also due to using compiler with bad data flow analysis (e.g. Turbo C or early gcc 2.something that yells at me "end of function reached w/o return statement"); BUT here it structures much better - thx for insisting.
I still see a little style discrepancy in case/switch indenting - here
case() {
switch A: ...
switch B: ...
}
vs. the non indenting avrdude style
case() {
switch A: ...
switch B: ...
}
but I left it indented - the last switch and the closing brace in one column looks confusing (to me) :)
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.
Yes, flow of control can be a philosophically charged discussion. There are a lot of pros for having one point of return, eg, that is very useful when resourced need to be freed.
And, yes, switch indentation also attracts various schools of thought. One fraction likens the case
to if
, else
and else if
role and indentation...
All good, and both are cool. (though one is still cooler 😉)
src/stk500v2.c
Outdated
dac = (clock_conf & 0x0ffc) >> 2u; | ||
fosc = pow(2, (double)oct) * 2078.0 / (2 - (double)dac / 1024.0); | ||
return fosc; | ||
break; |
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 default clause isn't needed as a return 0.0
follows the switch. (Or the latter return isn't needed.)
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.
I like the explicit default better than the implicit. I would better sacrifice the last return.
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.
Sure: your preference.
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.
Very happy with the code. A few tiny suggestions to improve readability and maintainability. Most pertinent: would appreciate an error message accompanying unsuccessful returns (unless a called function earlier already prints an error). In case things go wrong this is soooo much more helpful when there is an indication where things went wrong: pmsg_error()
with -v
prints source code file and line number.
I will push the proposed and discussed changes tonite when I'm back at my devel. laptop. |
Signed-off-by: Martin <[email protected]>
So, it was fun and I learned a lot of avrdude's internals! |
I'd like to carry out some sanity checking today or tomorrow even though I may not have the right HW to test. |
@@ -1158,7 +1189,8 @@ static void jtagmkI_display(const PROGRAMMER *pgm, const char *p) { | |||
|
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.
This is actually my mistake. I just realized that the two lines (in jtagmkI.c):
msg_info("%sICE HW version : 0x%02x\n", p, hw);
msg_info("%sICE FW version : 0x%02x\n", p, fw);
has an incorrect amount of spaces. Would you mind removing two spaces for each of them, so they'll look like this instead?
msg_info("%sICE HW version : 0x%02x\n", p, hw);
msg_info("%sICE FW version : 0x%02x\n", p, fw);
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.
Just a quick thought: Let the computer do this nasty work. Could a format like this help to maintain an uniform appearance more easily instead of manual space counting?
const int msg_space = 21;
msg_info("%s%-*s : 0x%02x\n", p, msg_space, "ICE HW version", hw);
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.
Absolutely. We should probably have done this in the first place.
However, I think this should be done at a global scale, and as its very own PR.
Adding "dumb" spaces is sufficient for the time being. Maybe I'll look into fixing it after 7.3 has been released.
It seems to work fine with my STK500v2 clone running official STK500v2 FW (using ATmega8535 MCU), but with only the programmer portion. The sequence of the output is not the same. This PR.
git main:
|
Okay also for AVRISP mkii clone running official Atmel FW but the alignment is a bit off. Edit: ignore this result as it is not with the latest commit. This PR:
git main:
|
Tomorrow I'll be back home and can try again with my STK500 (SW v2) if nobody else has this original AVR fossil.
yikes - where do these two decimals come? - oh, I see, it's from main branch ( |
Sorry I attached the wrong log for STK500v2, now it is okay, both the git main and this PR shows For AVRISP mkII, this PR has an issue with alignment. Both this PR and git main show '8.00us'. Edit: ignore my test results for this PR. I was not using the latest commit. I will update soon. |
@Ho-Ro For STK500v2, the sequence of the output is not the same. I tend to think it is better to be consistent with git main in this case.
No issues with AVRISP mkII. This PR is outputing
|
Signed-off-by: Martin <[email protected]>
Also stk500 parameter sequence is now ok. |
this is only a proof of concept
declarations for libavrdude, parser in term.c
sample implementations in stk500v1.c