Support for STK500 v1 and v2 devices with STK500_XTAL != 7372800 #1566

Ho-Ro · 2023-11-10T21:08:42Z

This PR replaces #1565 because it introduced a lot of whitespace-only changes. Sorry for the confusion.
1st part fixed some errors while 2nd commit does the real work.

1st part of PR for avrdudes#1560 - clean up `-x` command handling of stk500v1 2nd part will add `-x xtal` handling for stk500v1 Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

fix parsing of frequencies, values w/o k or M prefix were not recognised -x fosc=100k worked, -x fosc=100000 resulted in 0.000 now frequency parameter fosc=nnnn or fosc=nnnnH[z] are also ok more consistent XTAL frequency display - this parameter is constant for the used programmer contrary to variables like fosc and sck Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

Ho-Ro · 2023-11-11T12:18:21Z

STK500 v2 takes some more time. The source code is more confusing because so many different programmers are involved and the crystal frequency has also been coded into many different "magic constants".

stefanrueger · 2023-11-11T17:05:50Z

STK500 v2 takes some more time.

Thanks for looking into this! I think it will be worth our while. Let us know once you are finished with code and documentation in avrdude.1 and doc/avrdude.texi; then the team will be able to review and test.

unify xtra argument messages for v1 and v2, better error message wording update man and doc Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

Ho-Ro · 2023-11-12T15:44:44Z

stk500v2 support for -x xtal is done, also avrdude.1 and doc/avrdude.texi - please review.

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

stefanrueger · 2023-11-12T20:09:02Z

src/stk500.c

@@ -707,7 +704,7 @@ static int stk500_parseextparms(const PROGRAMMER *pgm, const LISTID extparms)
            if (str_eq(fosc_str, "off"))
              PDATA(pgm)->fosc_data = 0.0;
            else {
-              pmsg_error("cannot parse fosc value %s\n", fosc_str);
+              pmsg_error("invalid fosc value '%s'\n", fosc_str);


ever so slightly prefer %s without single quotes in common with other messages

stefanrueger · 2023-11-12T20:10:59Z

src/stk500.c

@@ -590,7 +590,7 @@ static int stk500_initialize(const PROGRAMMER *pgm, const AVRPART *p) {
    stk500_getparm(pgm, Parm_STK_OSC_CMATCH, &osc_cmatch);
    if(osc_pscale) {
      int prescale = 1;
-      f_get = STK500_XTAL / 2;
+      f_get = PDATA(pgm)->xtal / 2;


better divide by 2.0 (otherwise lsb is lost)

stefanrueger · 2023-11-12T20:11:53Z

src/stk500.c

+        char xtal_str[16] = {0};
+        int sscanf_success = sscanf(extended_param, "xtal=%10s", xtal_str);
+        if (sscanf_success < 1) {
+          pmsg_error("invalid xtal value '%s'\n", extended_param);


no single quotes please in message (multiple times)

stefanrueger · 2023-11-12T20:13:56Z

src/stk500.c

+        }
+        char *endp;
+        double v = strtod(xtal_str, &endp);
+        if (endp == xtal_str){


Would be neat to allow space between number and unit; eg, with code to the effect of

if(*endp == ' ') endp++;

Good POLA point, I will check.
EDIT: Does not work as expected, "1.23 MHz" -> v=1.23, *endp=''
But I do not expect the user to provide frequencies with space inside as you must put it into quotes:
-x"xtal=1.23 MHz" instead of -xxtal=1.23M

Users are unlikely to use quotes but scripts that extract the frequency from another AVRDUDE or otherwise call might have a frequency as 1.23 MHz and find it easy to use quotes but marginally harder to remove the space between 3 and MHz. It is always neat if the output of AVRDUDE (correctly done with space between number and unit) can be used as input without mods.

ok, c983e30 allows spaces in extra parameter, e.g. -x "fosc= .1e3 kHz"

stefanrueger · 2023-11-12T20:16:48Z

src/stk500.c

@@ -1167,31 +1206,31 @@ static int stk500_set_fosc(const PROGRAMMER *pgm, double v) {

  prescale = cmatch = 0;
  if (v > 0.0) {
-    if (v > STK500_XTAL / 2) {
+    if (v > PDATA(pgm)->xtal / 2) {


2.0 (multiple times)

stefanrueger · 2023-11-12T20:19:41Z

src/stk500.c

+    unit = "kHz";
+  } else
+    unit = "Hz";
+  fmsg_out(fp, "%sXTAL frequency  : %.3f %s\n", p, f, unit);


Is .3 sufficient? Might .5 or .6 be better?

Yes, I changed to variable decimal count 6 for MHz, 3 for kHz, 0 for Hz

stefanrueger · 2023-11-12T20:21:05Z

src/stk500.h

@@ -57,6 +57,9 @@ struct pdata {
  bool fosc_get;
  bool fosc_set;
  double fosc_data;
+


Perhaps neater to have inline comment at column 32 without empty line? So adding a component is one line, not three.

src/stk500v2.c

stefanrueger

Code looks vvv good; I only have minor (minor) suggestions, which need applying multiple times; still needs to be tested independently, but all looks vvv good

Ho-Ro · 2023-11-13T10:46:14Z

@stefanrueger et. al. What's the next step, should I add another commit with all requested changes and you "squash and merge" them as one commit?
Or should I tidy up on my side?

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

mcuee · 2023-11-14T05:15:04Z

Unfortunately my original STK500 was dead so I can not carry out real test. I have a few STK500 clones which only implement the programming portion (including one with HVPP capability) but not other features.

@MCUdude
Just wondering if you can carry out the real test.

MCUdude · 2023-11-14T07:15:15Z

Unfortunately my original STK500 was dead so I can not carry out real test. I have a few STK500 clones which only implement the programming portion (including one with HVPP capability) but not other features.
@MCUdude
Just wondering if you can carry out the real test.

Sure! I'll give it a test tonight.

"How dead" are your STK500 board? I accidentally bricked one of mine, but I was able to get it working again by re-flashing the firmware for the ATtiny2313 (running AVR910) and the ATmega8515 (running STK500 FW). I think I have the correct hex files somewhere if that's your issue.

mcuee · 2023-11-14T08:15:35Z

"How dead" are your STK500 board? I accidentally bricked one of mine, but I was able to get it working again by re-flashing the firmware for the ATtiny2313 (running AVR910) and the ATmega8515 (running STK500 FW). I think I have the correct hex files somewhere if that's your issue.

I have the necessary STK500 V1/V2 FW hex files -- that is why I can switch between STK500 V1 and STK500 V2 for two clones using ATmega8535 MCU.

Unfortunately for my original STK500 it is HW failure and it is hard to repair now due to broken traces. It was a very old unit (probably made in 2005/2006) and I got from my colleague a few years ago.

MCUdude · 2023-11-14T21:01:52Z

I just tested this PR on two separate STK500 boards running V1 and v2 firmware, and when I changed -xxtal to 14.7456 MHz, I got the reported fosc was twice the frequency that I measured. All good then!

Two things:
*-xxtal is not an option for -c stk500 even though it probably should be.

Could it be an idea to add xtal as a command in terminal mode?

avrdude> xtal
Syntax: xtal <value>[M|k]
avrdude> xtal 16M

$ ./avrdude -cstk500 -xhelp
avrdude -c stk500 extended options:
  -xhelp    Show this help menu and exit

MCUdude · 2023-11-14T21:33:44Z

Could it be an idea to add xtal as a command in terminal mode?

When thinking a bit; probably not, as it doesn't change anything on the board other than a constant used for calculation. However, it should probably not be present when running the parms command.

Ho-Ro · 2023-11-14T22:37:28Z

However, it should probably not be present when running the parms command.

I agree, as it cannot be changed from terminal cmd, but @stefanrueger had a good argument for leaving it in.

stefanrueger · 2023-11-15T01:37:10Z

src/stk500.c

-      fmsg_out(fp, "%.3f %s\n", f, unit);
+      decimals = get_decimals(f);
+      f = f_to_kHz_MHz(f, &unit);
+      fmsg_out(fp, "%.*f %s\n", f, decimals, unit);


Must be fmsg_out(fp, "%.*f %s\n", decimals, f, unit); ie, decimals first.

Right, star comes before f in the string - I know I checked it later with the C standard, but only corrected line 1465 below and forgot the other occurrences here and in v2.
Funny thing is that gcc doesn't complain when I do it the wrong way round and does "what I mean" - I don't know what clang does though.

does "what I mean"

A fluke maybe? Maybe doubles are passed in a different frame to functions than smaller data types via the stack?

gcc doesn't complain

gcc does not realise that the called function resolves to or calls printf()

stefanrueger · 2023-11-15T01:39:18Z

src/stk500v2.c

        f = f_to_kHz_MHz(f, &unit);
-        fmsg_out(fp, "%.3f %s\n", f, unit);
+        fmsg_out(fp, "%.*f %s\n", f, decimals, unit);


change order of f and decimals arguments (multiple times)

yep, 2 times

stefanrueger · 2023-11-15T01:52:18Z

src/stk500v2.c

@@ -1306,7 +1314,7 @@ static int stk500v2_initialize(const PROGRAMMER *pgm, const AVRPART *p) {
      stk500v2_getparm(pgm, PARAM_OSC_CMATCH, &osc_cmatch);
      if(osc_pscale) {
        int prescale = 1;
-        f_get = STK500V2_XTAL / 2;
+        f_get = PDATA(pgm)->xtal / 2;


Not a fault of this PR, but osc_pscale and osc_cmatch ought to be set to 0 above b/c stk500v2_getparm() might not set them! Ideally, also the return value of stk500v2_getparm() calls ought to be checked to guard against not setting the osc_... variables.

This will be a bigger separate commit, because stk500v2_getparm() (as well as stk500_getparm() in stk500.c) is used quite often w/o checking the return value. Nevertheless I can adopt this issue and fix it in the course of my stk500* overhaul.

Thanks for cleaning this in your overhaul of stk500* - much appreciated.

stefanrueger · 2023-11-15T01:53:51Z

src/stk500v2.c

@@ -1620,7 +1628,7 @@ static int stk500hv_initialize(const PROGRAMMER *pgm, const AVRPART *p, enum hvm
      stk500v2_getparm(pgm, PARAM_OSC_CMATCH, &osc_cmatch);
      if(osc_pscale) {
        int prescale = 1;
-        f_get = STK500V2_XTAL / 2;
+        f_get = PDATA(pgm)->xtal / 2;


Same as above:

Not a fault of this PR, but osc_pscale and osc_cmatch ought to be set to 0 above b/c stk500v2_getparm() might not set them! Ideally, also the return value of stk500v2_getparm() calls ought to be checked to guard against not setting the osc_... variables.

stefanrueger · 2023-11-15T02:00:50Z

src/stk500v2.c

      // STK500: Write analog reference voltage
      else {
-        msg_info("Changing analog reference voltage from %.2f to %.2fV\n",
+        msg_info("Changing analog reference voltage from %.2f V to %.2f V\n",


unsigned char varef_read above should be initialised to 0 (same reason as below, four times in this code)

stefanrueger · 2023-11-15T02:02:45Z

src/stk500v2.c

    // Write target voltage value
    else {
-      msg_info("Changing target voltage from %.2f to %.2fV\n", (vtarg_read / 10.0), PDATA(pgm)->vtarg_data);
+      msg_info("Changing target voltage from %.2f V to %.2f V\n", (vtarg_read / 10.0), PDATA(pgm)->vtarg_data);


unsigned char vtarg_read; should be initialised with 0 (same reason as below, needs to be done twice in this file).

stefanrueger · 2023-11-15T02:10:04Z

What's the next step

Normally someone reviews all changes and also the surrounding code (sometimes there are problems that are outside the PR but should be fixed "while at it"). Then things are tested. Then the PR hangs around for a few days (can be weeks) until a bunch of PRs is merged. I sometimes squash and merge (eg, when a few ideas were tested) or merge when it's not problematic that a few fixes were committed. If it's important to you that a PR goes in with a particular pristine commit sequence it is normally better to recreate a new PR. Force pushes are not really encouraged as they can go wrong.

Ho-Ro · 2023-11-15T08:30:00Z

I sometimes squash and merge

I think it would then be ok to stack some more commits e.g. the check of *_getparm() on top of my previuous work.

minor format change remove ':' from pmsg_error() to be consistent with other output lines PR avrdudes#1566 should be done Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

stefanrueger · 2023-11-16T00:40:08Z

Code of new commits looks good. Thank you, @Ho-Ro.

@mcuee @MCUdude Once this is thoroughly tested I think it can be merged

stefanrueger · 2023-11-18T07:48:56Z

@mcuee @MCUdude please test the -xxtal functionality and test against regressions so this PR can be merged at the next occasion.

mcuee · 2023-11-18T08:04:55Z

@mcuee @MCUdude please test the -xxtal functionality and test against regressions so this PR can be merged at the next occasion.

Unfortunately I do not have the right HW to test this one as mentioned earlier. Hans should be able to test.

Ho-Ro · 2023-11-18T10:34:46Z

Unfortunately I do not have the right HW to test this one as mentioned earlier. Hans should be able to test.

I'm back at my home desk and can also provide some evidence for tests with my (original) STK500 with v2 FW.

MCUdude · 2023-11-20T19:40:16Z

Just tested with two separate STK500 board running v1 and v2 firmware. Everything seems to work fine! I've not found any issues. I also verified using an oscilloscope to measure the clock input frequency on the target chip.

Ho-Ro · 2023-11-21T09:12:41Z

I measured the SCK timing

nanoSTK (stk500v1)

$ avrdude_Ho-Ro -pm328p -cstk500v1 -v -xxtal=16M -U flash:r:/dev/null:r -B1
...

               Programmer Type : STK500
               Description     : Atmel STK500 version 1.x firmware
               Hardware Version: 2
               Firmware Version: 1.29
               Vtarget         : 4.8 V
               Varef           : 0.0 V
               Oscillator      : 1.000000 MHz
               SCK period      : 1.0 us
               XTAL frequency  : 16.000000 MHz
avrdude_Ho-Ro: AVR device initialized and ready to accept instructions
avrdude_Ho-Ro: device signature = 0x1e950f (probably m328p)

avrdude_Ho-Ro: processing -U flash:r:/dev/null:r
avrdude_Ho-Ro: reading flash memory ...
Reading | ################################################## | 100% 4.47 s 
avrdude_Ho-Ro: writing output file /dev/null

avrdude_Ho-Ro done.  Thank you.

SCK timing nanoSTK (stk500v1)

Original STK500

$ avrdude_Ho-Ro -pm328p -cstk500v2 -v -U flash:r:/dev/null:r -B8

...

               Programmer Type : STK500V2
               Description     : Atmel STK500 version 2.x firmware
               Programmer Model: STK500
               Hardware Version: 2
               Firmware Version Controller : 2.01
               Topcard         : Unknown
               Vtarget         : 5.0 V
               Varef           : 0.0 V
               Oscillator      : 3.686400 MHz
               SCK period      : 8.7 us
               XTAL frequency  : 7.372800 MHz
avrdude_Ho-Ro: AVR device initialized and ready to accept instructions
avrdude_Ho-Ro: device signature = 0x1e950f (probably m328p)

avrdude_Ho-Ro: processing -U flash:r:/dev/null:r
avrdude_Ho-Ro: reading flash memory ...
Reading | ################################################## | 100% 16.46 s 
avrdude_Ho-Ro: writing output file /dev/null

avrdude_Ho-Ro done.  Thank you.

SCK timing of STK500 (stk500v2)

And with half speed xtal setting it calculates the values correctly based on this setting (but uses normal speed as expected)

$ avrdude_Ho-Ro -pm328p -cstk500v2 -v -U flash:r:/dev/null:r -xxtal=3.686400M -B17

...

               Programmer Type : STK500V2
               Description     : Atmel STK500 version 2.x firmware
               Programmer Model: STK500
               Hardware Version: 2
               Firmware Version Controller : 2.01
               Topcard         : Unknown
               Vtarget         : 5.0 V
               Varef           : 0.0 V
               Oscillator      : 1.843200 MHz
               SCK period      : 17.4 us
               XTAL frequency  : 3.686400 MHz

stefanrueger · 2023-11-21T11:02:58Z

Brilliant, thanks for confirming and tests @Ho-Ro and @MCUdude

Ho-Ro added 2 commits November 10, 2023 21:52

fix -x vtarg handling (HAS_VTARG_READ vs. HAS_VTARG_ADJ)

0d851db

1st part of PR for avrdudes#1560 - clean up `-x` command handling of stk500v1 2nd part will add `-x xtal` handling for stk500v1 Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

Ho-Ro mentioned this pull request Nov 10, 2023

Support for STK500 v1 devices with STK500_XTAL != 7372800 #1565

Closed

mcuee added the enhancement New feature or request label Nov 11, 2023

stk500v2: xtra argument -x xtal=nnnn

4f0ac16

unify xtra argument messages for v1 and v2, better error message wording update man and doc Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

stk500v1: add option -B handling identical to stk500v2

373e2e0

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

stefanrueger reviewed Nov 12, 2023

View reviewed changes

src/stk500v2.c Outdated Show resolved Hide resolved

stefanrueger reviewed Nov 12, 2023

View reviewed changes

update of minor findings from PR review

69255c6

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

Ho-Ro mentioned this pull request Nov 13, 2023

Query a programmer to figure out its current clock speed (low priority) #1329

Closed

MCUdude mentioned this pull request Nov 14, 2023

Certain terminal commands may be confusing #1570

Closed

stefanrueger reviewed Nov 15, 2023

View reviewed changes

Ho-Ro added 2 commits November 15, 2023 23:18

fix review findings; init values before get_parameter; check ret values

9434e36

minor format change remove ':' from pmsg_error() to be consistent with other output lines PR avrdudes#1566 should be done Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

allow spaces in extra parameter, e.g. -x "fosc= .1e3 kHz"

c983e30

Signed-off-by: Martin <Ho-Ro@users.noreply.github.com>

mcuee changed the title ~~Support for STK500 v1 devices with STK500_XTAL != 7372800~~ Support for STK500 v1 and v2 devices with STK500_XTAL != 7372800 Nov 18, 2023

stefanrueger mentioned this pull request Nov 20, 2023

Update part cmd #1568

Merged

stefanrueger merged commit 92c576d into avrdudes:main Nov 22, 2023
10 checks passed

Ho-Ro deleted the stk500v1-x_xtal branch December 1, 2023 08:45

Support for STK500 v1 and v2 devices with STK500_XTAL != 7372800 #1566

Support for STK500 v1 and v2 devices with STK500_XTAL != 7372800 #1566

Conversation

Ho-Ro commented Nov 10, 2023

Ho-Ro commented Nov 11, 2023

stefanrueger commented Nov 11, 2023

Ho-Ro commented Nov 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanrueger Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ho-Ro Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanrueger left a comment

Choose a reason for hiding this comment

Ho-Ro commented Nov 13, 2023

mcuee commented Nov 14, 2023

MCUdude commented Nov 14, 2023

mcuee commented Nov 14, 2023

MCUdude commented Nov 14, 2023

MCUdude commented Nov 14, 2023

Ho-Ro commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanrueger commented Nov 15, 2023

Ho-Ro commented Nov 15, 2023

stefanrueger commented Nov 16, 2023

stefanrueger commented Nov 18, 2023

mcuee commented Nov 18, 2023

Ho-Ro commented Nov 18, 2023

MCUdude commented Nov 20, 2023 • edited Loading

Ho-Ro commented Nov 21, 2023

nanoSTK (stk500v1)

Original STK500

stefanrueger commented Nov 21, 2023

stefanrueger Nov 12, 2023 •

edited

Loading

Ho-Ro Nov 13, 2023 •

edited

Loading

Ho-Ro commented Nov 14, 2023 •

edited

Loading

MCUdude commented Nov 20, 2023 •

edited

Loading