Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible miscompilation error? #164

Closed
vojty opened this issue Oct 18, 2022 · 9 comments
Closed

Possible miscompilation error? #164

vojty opened this issue Oct 18, 2022 · 9 comments

Comments

@vojty
Copy link

vojty commented Oct 18, 2022

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
Copy link

andresv commented Oct 19, 2022

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

@vojty
Copy link
Author

vojty commented Oct 19, 2022

I've updated the comment with my configuration

@andresv
Copy link

andresv commented Oct 19, 2022

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
Copy link
Member

Hi @vojty, sorry you've been running into this issue. Fortunately, its a duplicate of #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: #134.

@vojty
Copy link
Author

vojty commented Oct 19, 2022

@MabezDev Thank you!

@vojty
Copy link
Author

vojty commented Nov 5, 2022

@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
Copy link
Member

MabezDev commented Nov 7, 2022

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?

@MabezDev MabezDev reopened this Nov 7, 2022
@vojty
Copy link
Author

vojty commented Nov 7, 2022

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

@MabezDev
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants