patternrustMinor
Calculating word-wise CRC
Viewed 0 times
calculatingwordcrcwise
Problem
This is a small program to calculate the CRC for an STM32 program image, compatible with the CRC on the STM32 hardware. It's a word-wise CRC, and the bin file is encoded as a series of 32 bit little endian words.
This is my first serious attempt at Rust and I'm looking for general comments, as well as a few questions:
-
Is there a better way of writing the repetitive (
-
Where should the
This is my first serious attempt at Rust and I'm looking for general comments, as well as a few questions:
-
Is there a better way of writing the repetitive (
buff[3] as u32) when building the word?-
Where should the
T: Read trait bounds go? Should they also go on WordIterator?use std::fs::File;
use std::io::Read;
struct WordIterator {
rd: T
}
impl WordIterator {
fn new(f: T) -> WordIterator {
WordIterator {rd: f}
}
}
impl Iterator for WordIterator {
type Item = u32;
fn next(&mut self) -> Option {
let mut buf = [0 as u8; 4];
match self.rd.read(&mut buf) {
Ok(4) => {
let v: u32 = (buf[3] as u32) {
None
}
}
}
}
struct CrcCalculator {
lookup_table: [u32; 256]
}
impl CrcCalculator {
fn new() -> CrcCalculator {
let mut calc = CrcCalculator {lookup_table: [0; 256]};
// Calculate the table:
for v in 0..256 {
let mut v2: u32 = v (&self, f: T) -> (u32, u32) {
let mut crc: u32 = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(f) {
crc = crc ^ word;
crc = (crc > 24) & 0xFF) as usize];
crc = (crc > 24) & 0xFF) as usize];
crc = (crc > 24) & 0xFF) as usize];
crc = (crc > 24) & 0xFF) as usize];
count += 1;
}
(count, crc)
}
}
fn main() {
let lt = CrcCalculator::new();
let f = File::open("Image.bin").unwrap();
let (count, crc) = lt.calculate(f);
println!("count=0x{:X} crc=0x{:08X}", count, crc);
}Solution
Overall, this looks very reasonable! There are some small style nits like spaces inside of struct declarations. A few bigger points:
-
The Rust community loves crates and reusing existing work. In this case, you should use byteorder instead of bit-banging yourself. Beyond being less code, a nice bonus is that the fact that you use little-endian is now written in words in the code!
-
If you weren't doing this project for fun or learning, I'd recommend using one of the existing CRC crates to completely avoid reinventing the wheel!
-
Consider making the lookup table into a lazy_static global. This would allow it to only be initialized once.
-
Even better, consider hardcoding the lookup table. This would allow you to avoid any creation overhead. There are two paths:
-
There's a few extra places that have type specifications when they aren't needed, such as when building the table. It's better to allow inference to apply when you can. Because you switch back and forth between
-
You could extract a function for the repeated bit shuffling and/or use a loop. You may wish to profile if you are concerned about performance, as sometimes a manual loop unrolling is still required.
-
Use
-
Add some buffering to your file reading to prevent inefficient amounts of system calls.
-
Try to name your generic types so that the name hints at the type.
-
Expand on your variable names a bit.
-
I prefer to use the
-
I only add trait bounds where the function implementation requires it.
For fun, I wanted to see what the compile-time build solution would look like.
build.rs
And this means that the CRC functions don't need to be instance methods anymore, and can just be free functions:
```
include!(concat!(env!("OUT_DIR"), "/lookup_table.rs"));
fn crc_next_component(crc: u32) -> u32 {
CRC_LOOKUP_TABLE[((crc >> 24) & 0xFF) as usize]
}
fn crc(r: R) -> (u32, u32)
where R: Read,
{
let mut crc = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(r) {
crc = crc ^ word;
for _
-
The Rust community loves crates and reusing existing work. In this case, you should use byteorder instead of bit-banging yourself. Beyond being less code, a nice bonus is that the fact that you use little-endian is now written in words in the code!
-
If you weren't doing this project for fun or learning, I'd recommend using one of the existing CRC crates to completely avoid reinventing the wheel!
-
Consider making the lookup table into a lazy_static global. This would allow it to only be initialized once.
-
Even better, consider hardcoding the lookup table. This would allow you to avoid any creation overhead. There are two paths:
- You could copy-paste it in. Probably fine as the CRC table is unlikely to change often.
- Create a build script that generates it at compile time and then include the result. This has a little bit of overhead at build time, but captures the process used to generate the table.
-
There's a few extra places that have type specifications when they aren't needed, such as when building the table. It's better to allow inference to apply when you can. Because you switch back and forth between
usize and u32, there will still be some places it's needed.-
You could extract a function for the repeated bit shuffling and/or use a loop. You may wish to profile if you are concerned about performance, as sometimes a manual loop unrolling is still required.
-
Use
expect instead of unwrap, to help you and your users understand failures.-
Add some buffering to your file reading to prevent inefficient amounts of system calls.
-
Try to name your generic types so that the name hints at the type.
T is great when the type can be basically anything, but in this case I'd recommend R to reflect the Read bound.-
Expand on your variable names a bit.
rd has a larger scope as it's shared among multiple locations and should have an easier name.-
I prefer to use the
where clause to specify trait bounds. I find it easier to read.-
I only add trait bounds where the function implementation requires it.
extern crate byteorder;
use byteorder::{LittleEndian, ReadBytesExt};
use std::fs::File;
use std::io::{Read, BufReader};
struct WordIterator {
inner: R,
}
impl WordIterator {
fn new(r: R) -> WordIterator {
WordIterator { inner: r }
}
}
impl Iterator for WordIterator
where R: Read,
{
type Item = u32;
fn next(&mut self) -> Option {
self.inner.read_u32::().ok()
}
}
struct CrcCalculator {
lookup_table: [u32; 256],
}
impl CrcCalculator {
fn new() -> CrcCalculator {
let mut calc = CrcCalculator { lookup_table: [0; 256] };
// Calculate the table:
for v in 0..256 {
let mut v2 = v (&self, r: R) -> (u32, u32)
where R: Read,
{
let mut crc = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(r) {
crc = crc ^ word;
for _ in 0..4 {
crc = (crc u32 {
self.lookup_table[((crc >> 24) & 0xFF) as usize]
}
}
fn main() {
let calculator = CrcCalculator::new();
let f = BufReader::new(File::open("Image.bin").expect("Couldn't open input file"));
let (count, crc) = calculator.calculate(f);
println!("count=0x{:X} crc=0x{:08X}", count, crc);
}For fun, I wanted to see what the compile-time build solution would look like.
build.rs
use std::env;
use std::io::{self, Write, BufWriter};
use std::fs::File;
use std::path::Path;
fn write_table(mut w: W) -> io::Result
where W: Write,
{
try!(writeln!(w, "static CRC_LOOKUP_TABLE: [u32; 256] = ["));
for v in 0..256u32 {
let mut v2 = v << 24;
for _ in 0..8 {
if 0 != v2 & 0x80000000 {
v2 = (v2 << 1) ^ 0x04C11DB7;
} else {
v2 = v2 << 1;
}
}
if v % 8 == 0 { try!(write!(w, " ")) }
try!(write!(w, "0x{:08X}, ", v2));
if (v + 1) % 8 == 0 { try!(writeln!(w, "")) }
}
writeln!(w, "];")
}
fn main() {
let out_dir = env::var("OUT_DIR").expect("Could not determine where to place output");
let dest_path = Path::new(&out_dir).join("lookup_table.rs");
let f = File::create(&dest_path).expect("Could not create lookup table file");
let f = BufWriter::new(f);
write_table(f).expect("Could not write lookup table")
}And this means that the CRC functions don't need to be instance methods anymore, and can just be free functions:
```
include!(concat!(env!("OUT_DIR"), "/lookup_table.rs"));
fn crc_next_component(crc: u32) -> u32 {
CRC_LOOKUP_TABLE[((crc >> 24) & 0xFF) as usize]
}
fn crc(r: R) -> (u32, u32)
where R: Read,
{
let mut crc = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(r) {
crc = crc ^ word;
for _
Code Snippets
extern crate byteorder;
use byteorder::{LittleEndian, ReadBytesExt};
use std::fs::File;
use std::io::{Read, BufReader};
struct WordIterator<R> {
inner: R,
}
impl<R> WordIterator<R> {
fn new(r: R) -> WordIterator<R> {
WordIterator { inner: r }
}
}
impl<R> Iterator for WordIterator<R>
where R: Read,
{
type Item = u32;
fn next(&mut self) -> Option<u32> {
self.inner.read_u32::<LittleEndian>().ok()
}
}
struct CrcCalculator {
lookup_table: [u32; 256],
}
impl CrcCalculator {
fn new() -> CrcCalculator {
let mut calc = CrcCalculator { lookup_table: [0; 256] };
// Calculate the table:
for v in 0..256 {
let mut v2 = v << 24;
for _ in 0..8 {
if 0 != v2 & 0x80000000 {
v2 = (v2 << 1) ^ 0x04C11DB7;
} else {
v2 = v2 << 1;
}
}
calc.lookup_table[v as usize] = v2;
}
calc
}
fn calculate<R>(&self, r: R) -> (u32, u32)
where R: Read,
{
let mut crc = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(r) {
crc = crc ^ word;
for _ in 0..4 {
crc = (crc << 8) ^ self.next_component(crc);
}
count += 1;
}
(count, crc)
}
fn next_component(&self, crc: u32) -> u32 {
self.lookup_table[((crc >> 24) & 0xFF) as usize]
}
}
fn main() {
let calculator = CrcCalculator::new();
let f = BufReader::new(File::open("Image.bin").expect("Couldn't open input file"));
let (count, crc) = calculator.calculate(f);
println!("count=0x{:X} crc=0x{:08X}", count, crc);
}use std::env;
use std::io::{self, Write, BufWriter};
use std::fs::File;
use std::path::Path;
fn write_table<W>(mut w: W) -> io::Result<()>
where W: Write,
{
try!(writeln!(w, "static CRC_LOOKUP_TABLE: [u32; 256] = ["));
for v in 0..256u32 {
let mut v2 = v << 24;
for _ in 0..8 {
if 0 != v2 & 0x80000000 {
v2 = (v2 << 1) ^ 0x04C11DB7;
} else {
v2 = v2 << 1;
}
}
if v % 8 == 0 { try!(write!(w, " ")) }
try!(write!(w, "0x{:08X}, ", v2));
if (v + 1) % 8 == 0 { try!(writeln!(w, "")) }
}
writeln!(w, "];")
}
fn main() {
let out_dir = env::var("OUT_DIR").expect("Could not determine where to place output");
let dest_path = Path::new(&out_dir).join("lookup_table.rs");
let f = File::create(&dest_path).expect("Could not create lookup table file");
let f = BufWriter::new(f);
write_table(f).expect("Could not write lookup table")
}include!(concat!(env!("OUT_DIR"), "/lookup_table.rs"));
fn crc_next_component(crc: u32) -> u32 {
CRC_LOOKUP_TABLE[((crc >> 24) & 0xFF) as usize]
}
fn crc<R>(r: R) -> (u32, u32)
where R: Read,
{
let mut crc = 0xFFFFFFFF;
let mut count = 0;
for word in WordIterator::new(r) {
crc = crc ^ word;
for _ in 0..4 {
crc = (crc << 8) ^ crc_next_component(crc);
}
count += 1;
}
(count, crc)
}Context
StackExchange Code Review Q#129027, answer score: 8
Revisions (0)
No revisions yet.