search

esp-rs / rust

Rust for the xtensa architecture. Built in targets for the ESP32 and ESP8266
https://www.rust-lang.org
Other
729 stars 34 forks source link

Possible miscompilation error? #164

Closed vojty closed 1 year ago

vojty commented 1 year ago

Hello, I've found a strange error and I'm not sure what the problem might be. I've been trying to use ESP32 + SCD41 with no luck. I tracked down the problem to this piece of code:

// source https://github.com/Sensirion/sensirion-i2c-rs/blob/master/src/crc8.rs#L11-L25
pub fn calculate(data: &[u8]) -> u8 {
    const CRC8_POLYNOMIAL: u8 = 0x31;
    let mut crc: u8 = 0xff;
    for byte in data {
        crc ^= byte;
        for _ in 0..8 {
            if (crc & 0x80) > 0 {
                crc = (crc << 1) ^ CRC8_POLYNOMIAL;
            } else {
                crc <<= 1;
            }
        }
    }
    crc
}

For the given example data [9, 94] the correct result should be 35 but I'm getting 55. It works correctly on the blank Rust project using cargo new but it fails when I deploy the code to my ESP32.

I've created an example here https://wokwi.com/projects/345876589283639891 with 2 identical calculate functions, the only difference is that one of them has println! macros inside. However, the result differs.

Does anyone know what's going on? What am I missing?

Configuration:

➜  rust-esp32-std git:(master) ✗ rustup show         
Default host: aarch64-apple-darwin
rustup home:  /Users/tomas.vojtasek/.rustup

installed toolchains
--------------------

stable-aarch64-apple-darwin (default)
nightly-aarch64-apple-darwin
esp
esp-1.64.0.0

active toolchain
----------------

esp (directory override for '/Users/tomas.vojtasek/fun/rust-esp32-std')
rustc 1.64.0-nightly (f53d2361e 2022-09-20)

----------------

esp (directory override for '/Users/tomas.vojtasek/fun/rust-esp32-std')
rustc 1.64.0-nightly (f53d2361e 2022-09-20)
➜  rust-esp32-std git:(master) ✗ ./install-rust-toolchain.sh 
Processing configuration:
--build-target          = esp32,esp32s2,esp32s3
--cargo-home            = /Users/tomas.vojtasek/.cargo
--clear-cache           = YES
--esp-idf-version       = 
--export-file           = export-esp.sh
--extra-crates          = ldproxy cargo-espflash
--installation-mode     = install
--llvm-version          = esp-14.0.0-20220415
--minified-esp-idf      = NO
--minified-llvm         = YES
--nightly-version       = nightly
--rustup-home           = /Users/tomas.vojtasek/.rustup
--toolchain-version     = 1.64.0.0
--toolchain-destination = /Users/tomas.vojtasek/.rustup/toolchains/esp
/Users/tomas.vojtasek/.cargo/bin/rustup
nightly-aarch64-apple-darwin

Chip: Espressif ESP32-WROOM-32 (https://www.laskakit.cz/laskakit-esp-vindriktning-esp-32-i2c/)

andresv commented 1 year ago

Tried on ESP32-WROOM-32 and got 35 using esp rust v1.61.0-nightly

vojty commented 1 year ago

I've updated the comment with my configuration

andresv commented 1 year ago

Tried with both versions from https://wokwi.com/projects/345876589283639891 and also with overflowing_shl. Both of them return 35. I don't want to update my toolchain yet to 1.64.

pub fn calculate(data: &[u8]) -> u8 {
    const CRC8_POLYNOMIAL: u8 = 0x31;
    let mut crc: u8 = 0xff;
    for byte in data {
        crc ^= byte;
        println!("crc tmp: {:02x}", crc);
        for _ in 0..8 {
            if (crc & 0x80) > 0 {
                println!("top bit is high");
                //crc = (crc.overflowing_shl(1)).0 ^ CRC8_POLYNOMIAL;
                crc = (crc << 1) ^ CRC8_POLYNOMIAL;
            } else {
                println!("top bit is low");
                //crc = crc.overflowing_shl(1).0;
                crc <<= 1;
            }
        }
    }
    crc
}

pub fn calculate2(data: &[u8]) -> u8 {
    const CRC8_POLYNOMIAL: u8 = 0x31;
    let mut crc: u8 = 0xff;
    for byte in data {
        crc ^= byte;
        for _ in 0..8 {
            if (crc & 0x80) > 0 {
                //crc = (crc.overflowing_shl(1)).0 ^ CRC8_POLYNOMIAL;
                crc = (crc << 1) ^ CRC8_POLYNOMIAL;
            } else {
                //crc = crc.overflowing_shl(1).0;
                crc <<= 1;
            }
        }
    }
    crc
}
MabezDev commented 1 year ago

Hi @vojty, sorry you've been running into this issue. Fortunately, its a duplicate of https://github.com/esp-rs/rust/issues/134 which we already have a fix for! It will be released in the next 2 weeks with the 1.65 Rust toolchain. I tested your code with the new LLVM version locally and it works correctly with your input data :).

I'll close this for now, in favour of the tracking issue here: https://github.com/esp-rs/rust/issues/134.

vojty commented 1 year ago

@MabezDev Thank you!

vojty commented 1 year ago

@MabezDev I've upgraded the toolchain to 1.65 using https://github.com/esp-rs/espup

➜  rust-esp32-std git:(master) ✗ rustup show         
Default host: aarch64-apple-darwin
rustup home:  /Users/tomas.vojtasek/.rustup

installed toolchains
--------------------

stable-aarch64-apple-darwin (default)
nightly-aarch64-apple-darwin
esp

active toolchain
----------------

esp (directory override for '/Users/tomas.vojtasek/fun/rust-esp32-std')
rustc 1.65.0-nightly (bf1b78e4a 2022-11-02)

Unfortunately, the calculation is still wrong. For given [171, 13] the result should be 40. It works in the Rust playground https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=35171f35fd49a47b9055dbf7a9834c4f

The program executed on the ESP chip outputs 246

Code:

pub fn calculate(data: &[u8]) -> u8 {
    const CRC8_POLYNOMIAL: u8 = 0x31;
    let mut crc: u8 = 0xff;
    for byte in data {
        crc ^= byte;
        for _ in 0..8 {
            if (crc & 0x80) > 0 {
                crc = (crc << 1) ^ CRC8_POLYNOMIAL;
            } else {
                crc <<= 1;
            }
        }
    }
    crc
}

fn main() -> Result<()> {
    // Temporary. Will disappear once ESP-IDF 4.4 is released, but for now it is necessary to call this function once,
    // or else some patches to the runtime implemented by esp-idf-sys might not link properly.
    esp_idf_sys::link_patches();

    // Bind the log crate to the ESP Logging facilities
    esp_idf_svc::log::EspLogger::initialize_default();

    println!("dec={}", calculate(&[171, 13]));

    Ok(())
}

Log

I (27) boot: ESP-IDF v4.4.1-dirty 2nd stage bootloader
I (27) boot: compile time 20:54:11
I (27) boot: chip revision: 1
I (30) boot_comm: chip revision: 1, min. bootloader chip revision: 0
I (37) boot.esp32: SPI Speed      : 40MHz
I (42) boot.esp32: SPI Mode       : DIO
I (46) boot.esp32: SPI Flash Size : 4MB
I (51) boot: Enabling RNG early entropy source...
I (56) boot: Partition Table:
I (60) boot: ## Label            Usage          Type ST Offset   Length
I (67) boot:  0 nvs              WiFi data        01 02 00009000 00006000
I (75) boot:  1 phy_init         RF data          01 01 0000f000 00001000
I (82) boot:  2 factory          factory app      00 00 00010000 00300000
I (90) boot: End of partition table
I (94) boot_comm: chip revision: 1, min. application chip revision: 0
I (101) esp_image: segment 0: paddr=00010020 vaddr=3f400020 size=12e58h ( 77400) map
I (137) esp_image: segment 1: paddr=00022e80 vaddr=3ffb0000 size=02240h (  8768) load
I (141) esp_image: segment 2: paddr=000250c8 vaddr=40080000 size=0a6b8h ( 42680) load
I (161) esp_image: segment 3: paddr=0002f788 vaddr=50000000 size=00010h (    16) load
I (161) esp_image: segment 4: paddr=0002f7a0 vaddr=00000000 size=00878h (  2168) 
I (167) esp_image: segment 5: paddr=00030020 vaddr=400d0020 size=587e4h (362468) map
I (311) boot: Loaded app from partition at offset 0x10000
I (311) boot: Disabling RNG early entropy source...
I (323) cpu_start: Pro cpu up.
I (323) cpu_start: Starting app cpu, entry point is 0x40081170
I (0) cpu_start: App cpu up.
I (337) cpu_start: Pro cpu start user code
I (337) cpu_start: cpu freq: 160000000
I (337) cpu_start: Application information:
I (342) cpu_start: Project name:     libespidf
I (347) cpu_start: App version:      1
I (351) cpu_start: Compile time:     Nov  5 2022 20:54:06
I (357) cpu_start: ELF file SHA256:  0000000000000000...
I (363) cpu_start: ESP-IDF:          v4.4.1-dirty
I (369) heap_init: Initializing. RAM available for dynamic allocation:
I (376) heap_init: At 3FFAE6E0 len 00001920 (6 KiB): DRAM
I (382) heap_init: At 3FFB2BC8 len 0002D438 (181 KiB): DRAM
I (388) heap_init: At 3FFE0440 len 00003AE0 (14 KiB): D/IRAM
I (394) heap_init: At 3FFE4350 len 0001BCB0 (111 KiB): D/IRAM
I (401) heap_init: At 4008A6B8 len 00015948 (86 KiB): IRAM
I (408) spi_flash: detected chip: generic
I (412) spi_flash: flash io: dio
I (417) cpu_start: Starting scheduler on PRO CPU.
I (0) cpu_start: Starting scheduler on APP CPU.
dec=246

Also the previous example [9, 94] with the correct value of 35, now outputs 32 (previously 55, also wrong) 😕

EDIT: If I add println! macros to debug the problem like this:

pub fn calculate(data: &[u8]) -> u8 {
    const CRC8_POLYNOMIAL: u8 = 0x31;
    let mut crc: u8 = 0xff;
    for byte in data {
        crc ^= byte;
        println!("outer {}", crc);
        for _ in 0..8 {
            if (crc & 0x80) > 0 {
                crc = (crc << 1) ^ CRC8_POLYNOMIAL;
            } else {
                crc <<= 1;
            }
            println!("inner {}", crc);
        }
    }
    crc
}

then the result is correct:

...
I (0) cpu_start: Starting scheduler on APP CPU.
outer 84
inner 168
inner 97
inner 194
inner 181
inner 91
inner 182
inner 93
inner 186
outer 183
inner 95
inner 190
inner 77
inner 154
inner 5
inner 10
inner 20
inner 40
dec=40
MabezDev commented 1 year ago

Thanks for the update, it seems like it's not entirely fixed. For me, it works on all opt-levels apart from when opt-level = 's', is this the case for you?

vojty commented 1 year ago

I can confirm that opt-level = "z" works just fine

MabezDev commented 1 year ago

With the 1.69 release, this should now be fixed. Please let me know if you still run into issues!