[ECOS] Re: Bug in crc32 routine?

[Jonathan Larmour]

Andrew Lunn wrote:
> I checked around and found a few more CRC calculations. In summary
> 
> 1) zlib
> 2) jffs2
> 3) gdb stubs
> 4) cygmon
> 5) ks32c5000 net driver
> 6) Redboot - already using cyg_crc32()
[snip]

Belated reply from me (what's new!), but just a few comments:

First of all, we aren't the master for jffs2. If there's a problem with 
the crc32 calc for jffs2 we should tell <dwmw2@redhat.com>. But then I'm 
not convinced a mismatch with zlib counts as a problem. If you agree, then 
this is a nop :).

> --- packages/devs/eth/arm/ks32c5000/current/cdl/ks32c5000_eth.cdl	23 May 2002 23:00:40 -0000	1.2
> +++ packages/devs/eth/arm/ks32c5000/current/cdl/ks32c5000_eth.cdl	11 Oct 2002 11:32:31 -0000
> @@ -58,6 +58,8 @@
>  	active_if     CYGPKG_IO_ETH_DRIVERS
>      implements    CYGHWR_NET_DRIVERS
>      implements    CYGHWR_NET_DRIVER_ETH0
> +    requires      (CYGPKG_CRC || (!(CYG_HAL_CPUTYPE == \"KS32C5000A\" )))
> +

More out of interest than anything, CDL has an implies operator which 
allows you to use:

  requires { (CYG_HAL_CPUTYPE == \"KS32C5000A\" ) implies CYGPKG_CRC }

which is more readable.

One thing I'm concerned about though is that the "default" requirements of 
the eth driver include a package that is not included by default. It shows 
up two long-standing CDL issues:

- only hardware packages can be listed in ecos.db targets
- there is no way for the user to request automatically loading required 
packages not currently in the config

the former would be easy to fix, but would be a band-aid for the latter. 
Bart's on holiday for a week, but I'd be interested in his feedback. A 
bugzilla bug should probably be submitted to track this (depending on what 
the correct value of "this" is ;-)).

> +++ packages/services/crc/current/doc/crc.sgml	11 Oct 2002 11:32:33 -0000
> @@ -5,7 +5,7 @@
>  The CRC package provides implementation of CRC algorithms. This
>  includes the POSIX CRC calculation which produces the same result as
>  the cksum command on Linux, another 32 bit CRC by Gary S. Brown and a
> -16bit CRC.
> +16bit CRC. The CRC used for Ethernet FCS is also implemented.
>  </PARA>
>  </PARTINTRO>
>  <CHAPTER id="crc-functions">
> @@ -35,17 +35,45 @@
>  <sect2 id="services-crc-api-cyg-crc32">
>  <title>cyg_crc32</title>
>  <para>
> -This function implements a 32 bit CRC by Gary S. Brown. It uses the
> +These functions implements a 32 bit CRC by Gary S. Brown. It uses the

implement :). I've just gone ahead and tweaked this in the repo, and a few 
other trivial speeling typos.

> Index: packages/services/crc/current/include/crc.h
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/services/crc/current/include/crc.h,v
> retrieving revision 1.1
> diff -u -r1.1 crc.h
> --- packages/services/crc/current/include/crc.h	9 Aug 2002 10:27:04 -0000	1.1
> +++ packages/services/crc/current/include/crc.h	11 Oct 2002 11:32:33 -0000
> @@ -65,6 +65,23 @@
>  unsigned long 
>  cyg_crc32(unsigned char *s, int len);
>  
> +// Gary S. Brown's 32 bit CRC, but accumulate the result from a
> +// previous CRC calculation
> +
> +unsigned long 
> +cyg_crc32_accumulate(unsigned long crc, unsigned char *s, int len);
[snip rest]

These should have C++ safe extern definitions. I've added these (change 
attached).

> +  if (1667500021 != cyg_ether_crc32(license_txt,sizeof(license_txt)-1)) {

Mumble, mumble, 32-bit reliance, mumble ;-). Really, that's what the type 
defs in <cyg/infra/cyg_type.h> are for - rather than using unsigned longs. 
(Although in due course we should use the C99 <inttypes.h> when GCC gets 
round to implementing it).

Jifl
-- 
eCosCentric       http://www.eCosCentric.com/       <info@eCosCentric.com>
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: crc.externc.pat
URL: <http://sourceware.org/pipermail/ecos-patches/attachments/20021017/5983a222/attachment.ksh>