Re: [avrdude-dev] [bug #17487] AVR RC oscillator calibration routine not supported (feature request)

[Joerg Wunsch]

Hi John, (Cc to the list, mainly to discuss the ideas below)

> Follow-up Comment #1, bug #17487 (project avrdude):
> 
> Here is my update patch

Just browsing through it.  First of all, thanks so much for your
effort!  When I first read appnote AVR053, I simply didn't get the
idea what their intention was, now I've got the picture.

There are a few things I don't like about the patch, like arbitrary
changes to the code which are unrelated to the oscillator calibration
issue.  That doesn't mean there would be no reason to perhaps change a
few things -- but please, don't mix that with other changes, and often
it would be better to first discuss it in the mailing list.  For
example, the -r option you added is not really needed: just running
avrdude with -v (and no other arguments) also gives you that
information.

However, I've got a few question where I'm simply not sure about your
changes.  In avr.c, you added

   wsize = m->size;
+  
+  if (size == 0) {
+       size = m->size;
+  }
+  
   if (size < wsize) {
     wsize = size;
   }

Is there any reason for this?  I'm not sure whether this is really
related to the oscillator calibration or not.

Just as a side note, the master text for the texinfo documentation is
avrdude.texi -- avrdude.info is a compiled version.  But I'll be able
to merge your documentation changes, no worries.  I'll also add a note
that the calibration routine will clobber EEPROM cell 0.

I wonder whether the "c" update specification is a good idea.  I see
your point, and I guess it's in some way compatible with what
stk500.exe and jtagicemkii.exe are doing.  OTOH, one could think of
similar situations that would require yet another option then, like
pulling the oscillator calibration fuse value, and storing it in an
arbitrary location.  The traditional Unix approach for that would be
to rather read out EEPROM cell 0 in a first call (e. g. using one of
the new "h", "o" or "d" memory "formats", so you get the value as 0x42
or such), store it (somehow) in the caller, and re-run avrdude to put
that value into an arbitrary EEPROM or flash location.  Say, if the
caller is a shell script, you just store the value in a variable:

var=$(avrdude ...)

The only thing currently missing for that job is to allow specifying a
start address for the memory write commands, so in particular when
using the "m" format, you can tell it where to write the data to.  I
think adding such a feature has been considered a good idea some time
ago already.  An idea for the syntax would be

-U eeprom:w:address@hidden:m

(${var} is filled in by the calling shell script.)

For the read operation, a limitation about how many bytes to read will
be useful, too, so the above could be like

var=$(avrdude -p m8 -U ee:r:address@hidden/1:h)

to just read a single byte from EEPROM at address 0.

Of course, the ideal partner to handle this would be avrdude-gui, but
that requires some active developers first.

While the above changes seem to be more intrusive at first, I think
they are more generally useful beyond the scope of the oscillator
calibration then.

Further remarks while continuing to read your patch:

The -O option should check for the perform_osscal function pointer to
be non-NULL before calling it, and issue a respective message
otherwise.

     if (rc) {
-      exitrc = 1;
+      exitrc = -1;
       break;

Command exit codes are usually non-negative integers (and often
limited to 0..255 in Unix environments).  Only 0 is considered
success, anything else is an error state.  So there's no need to make
that -1.

As for stk500v2_check_response_status() which you've added, that's one
of those unrelated changes: perhaps it's not a bad idea, but it
doesn't belong into the patch for the oscillator calibration.  Also,
when moving to a central check, the already existing decentral checks
for the response status can be removed.  The other option would be to
fill in the missing checks instead -- most callers already check for
STATUS_CMD_OK but not all do.  It's probably even sufficient to add
the check to stk500v2_command().

-- 
cheers, J"org               .-.-.   --... ...--   -.. .  DL8DTL

http://www.sax.de/~joerg/                        NIC: JW11-RIPE
Never trust an operating system you don't have sources for. ;-)