[ECOS] RedBoot: load.c srecord input offset fix

[ECOS] RedBoot: load.c srecord input offset fix

[Grant Edwards]

The "offset" variable in load_srec_image() isn't incremented
properly (assuming its purpose is to keep track of the current
byte offset in the input stream).  My version of load.c has
diverged enough that I can't generate a usable patch, so I'll
summarize the changes:

Increment at top of main while loop:

     while ((c = (*getc)()) > 0) {
+        ++offset;
         lp = line;  len = 0;
         // Start of line
         if (c != 'S') {

Double the byte-count for the address field to get the
character count since calling _hex2(func,N,&sum) consumes 2N
characters from the input stream::

         case '2':
         case '3':
             base_addr = addr = (unsigned char *)_hex2(getc, (type-'1'+2), &sum);
-            offset += (type-'1'+2);
+            offset += (type-'1'+2)*2;
             if (first_addr) {
                 if (base) {
                     addr_offset = (unsigned long)base - (unsigned long)addr;

Double the byte-count for the data field to get the character
count, and incrment by 2 instead of 1 for the checksum byte:

                 return;
             }
             count -= ((type-'1'+2)+1);
-            offset += count;
+            offset += count*2;
             while (count-- > 0) {
                 val = _hex2(getc, 1, &sum);
                 *addr++ = val;
             }
             cksum = _hex2(getc, 1, 0);
-            offset += 1;
+            offset += 2;
             sum = sum & 0xFF;
             cksum = (~cksum & 0xFF);
             if (cksum != sum) {

-- 
Grant Edwards
grante@visi.com

[ECOS] Re: RedBoot: load.c srecord input offset fix

[Grant Edwards]

On Thu, Apr 19, 2001 at 03:20:33PM -0500, Grant Edwards wrote:
> 
> The "offset" variable in load_srec_image() isn't incremented
> properly (assuming its purpose is to keep track of the current
> byte offset in the input stream).  My version of load.c has
> diverged enough that I can't generate a usable patch, so I'll
> summarize the changes:

I missed one in my previous post:

         case '7':
         case '8':
         case '9':
             addr = (unsigned char *)_hex2(getc, ('9'-type+2), &sum);
-            offset += ('9'-type+2);
+            offset += ('9'-type+2)*2;


-- 
Grant Edwards
grante@visi.com

[ECOS] Re: RedBoot: load.c srecord input offset fix

[Grant Edwards]

On Thu, Apr 19, 2001 at 03:25:50PM -0500, Grant Edwards wrote:

> > The "offset" variable in load_srec_image() isn't incremented
> > properly (assuming its purpose is to keep track of the current
> > byte offset in the input stream).  My version of load.c has
> > diverged enough that I can't generate a usable patch, so I'll
> > summarize the changes:

Oops, the "discard rest of line" loop at the bottom of the main
while loop also needs to be fixed:


         default:
             printf("Invalid S-record at offset 0x%lx, type: %x\n", 
                    (unsigned long)offset, type);
             return;
         }
-        while ((c = (*getc)()) != '\n') offset++;
+        do {
+             c = (*getc)();
+             ++offset;
+        } while (c != '\n');
     }
 }

-- 
Grant Edwards
grante@visi.com

Re: [ECOS] RedBoot: load.c srecord input offset fix

[Jonathan Larmour]

Grant Edwards wrote:
> 
> The "offset" variable in load_srec_image() isn't incremented
> properly (assuming its purpose is to keep track of the current
> byte offset in the input stream).  My version of load.c has
> diverged enough that I can't generate a usable patch, so I'll
> summarize the changes:
[snip]

I'm probably being dumb but this doesn't seem right to me either. If it's
purely the offset within the I/O stream, then there should be one per getc.
If it's the offset of the decoded data bytes, there should be one for every
two chars of actual encoded srec data, and the offset shouldn't be
incremented for any of the header or checksum.

What is the "offset" actually meant to _be_ if neither of those two
options?

Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine

Re: [ECOS] RedBoot: load.c srecord input offset fix

[Gary Thomas]

On 20-Apr-2001 Jonathan Larmour wrote:
> Grant Edwards wrote:
>> 
>> The "offset" variable in load_srec_image() isn't incremented
>> properly (assuming its purpose is to keep track of the current
>> byte offset in the input stream).  My version of load.c has
>> diverged enough that I can't generate a usable patch, so I'll
>> summarize the changes:
> [snip]
> 
> I'm probably being dumb but this doesn't seem right to me either. If it's
> purely the offset within the I/O stream, then there should be one per getc.
> If it's the offset of the decoded data bytes, there should be one for every
> two chars of actual encoded srec data, and the offset shouldn't be
> incremented for any of the header or checksum.
> 
> What is the "offset" actually meant to _be_ if neither of those two
> options?

It's only purpose is to try and provide more information when there is
damage in the input [stream].  In this case, it _should_ be just a count
of the calls to getc().

I'll look at Grant's suggestions and make sure that it really does so.

Re: [ECOS] RedBoot: load.c srecord input offset fix

[Grant Edwards]

On Fri, Apr 20, 2001 at 06:40:31AM -0600, Gary Thomas wrote:

> >> The "offset" variable in load_srec_image() isn't incremented
> >> properly (assuming its purpose is to keep track of the current
> >> byte offset in the input stream).  My version of load.c has
> >> diverged enough that I can't generate a usable patch, so I'll
> >> summarize the changes:
> > [snip]
> > 
> > I'm probably being dumb but this doesn't seem right to me either. If it's
> > purely the offset within the I/O stream, then there should be one per getc.
> > If it's the offset of the decoded data bytes, there should be one for every
> > two chars of actual encoded srec data, and the offset shouldn't be
> > incremented for any of the header or checksum.
> > 
> > What is the "offset" actually meant to _be_ if neither of those two
> > options?
> 
> It's only purpose is to try and provide more information when there is
> damage in the input [stream].  In this case, it _should_ be just a count
> of the calls to getc().
> 
> I'll look at Grant's suggestions and make sure that it really does so.

I added a debugging line to print out the offset value when the
load is finished, and (with my changes) the value printed now
matches the size of the s-record file.  Previously it was
somewhere between the size of the binary and the size of the
s-record file.

I also noticed that the checksum of the entry-address record
(S[789]...) wasn't being verified.

One other change I made that I've found quite useful is to save
the lowest/highest/entry addresses in a globally visible spot
and have the "fis create" command use those values as defaults
if none are provided on the command line.  Loading a file from
a tftp server into flash is now something that can be scripted 
a bit easier using Kermit or some other programmatic interface.

RedBoot> load -h 10.0.0.1 filename.srec
RedBoot> fis create filename


-- 
Grant Edwards
grante@visi.com