Re: [avrdude-dev] chip_erase and cycle_count cleanup

[Theodore A. Roth]

On Tue, 2 Dec 2003, Jan-Hinnerk Reichert wrote:

> On Monday 01 December 2003 23:27, Theodore A. Roth wrote:
> > On Mon, 1 Dec 2003, Jan-Hinnerk Reichert wrote:
> > > > > - Why is cycle count reset at 0xffff?
> > > >
> > > > Good question.  I think this is a bug.  I think it should be
> > > > compared against 0xffffffff.  A long time ago the cycle count
> > > > was 16 bits, but was later changed to 32 bits.  This code was
> > > > progably missed.
> > >
> > > There was another goodie in avr_get_cycle(). If the count was
> > > 0xffffffff it was set to 0xffff. So, every check against
> > > 0xffffffff was not doing anything.
> >
> > Instead of checking against 0xffffffff, shouldn't it check against
> > a macro?
> >
> >   #define CYCLES_ERASED ((uint32_t)-1)
> >
> > Then the code becomes:
> >
> >   if (!do_cycles && ((rc >= 0) && (cycles != CYCLES_ERASED))) {
> >
> > This should guarantee that the number of ff's if correct for the
> > size of the storage used. Plus you eliminate a magic number, and
> > the code becomes a bit more readable.
>
> Nice idea ;-)
>
> However, there is only one occurance of this magic number left in my
> version and this is in avr_get_cycle_count():
>
>   if (cycle_count == 0xffffffff) {
>     cycle_count = 0;
>   }

Even if it's only used once, it still improves readability by avoiding
the magic number. 0xfffffffff doesn't infer the eeprom is erased without
a few extra brain cycles.

>
> So, a zero count is read, if the counter has not been initialized.
>
> This removes the need for additional adjustment in avr_chip_erase()
>
> The only other check is in "main.c". Here the cycle count isn't
> output, if it isn't been used. Now, this check is for zero (which has
> the semantics: "We haven't counted anything, yet").
>
> We could even drop the check in "main.c", but that would result in a
> cycle count message at each invocation, probably confusing users who
> don't use (or even know about) the cycle_count ;-)
>
> BTW: Would you mind testing the patch on STK500 or PPI?

Sure, but it may take a few days since I'm swamped with day job
and simulavr work right now.

Ted Roth