Incorrect ADC readings after toolchain bump

[tronje]

Hi! We're using avr-hal and avr-device with an atmega164pa uC.

We tried bumping our Rust toolchain, as you have done with 2eb28fa.
This causes incorrect ADC readings. We've literally changed nothing else, and suddenly an analog_read on Pin<Analog, PA0> returns usually 0, sometimes 1, but never a value in the hundreds, as would be expected in our situation.

I guess it could be due to some unsafe weirdness in our codebase, but I mean the ADC is set up normally, and then it's just a call to analog_read, really. Not sure how much of our codebase I can share; I could probably build a minimal example to show the behavior. Just figured I'd ask first if anyone has any ideas about this. Works just fine on nightly-2023-12-28.

Thanks!

[Rahix]

Paging @Patryk27 (sorry for always bugging you with these topics!)

@tronje, your best bet is building a minimal reproducer example that still triggers this compiler regression. You can do this by continuously removing code from your application until you cannot remove anything without the regression vanishing.

Then people here can investigate where this problem originates from.

[tronje]

Hi Rahix, thanks for your response.

@tronje, your best bet is building a minimal reproducer example that still triggers this compiler regression. You can do this by continuously removing code from your application until you cannot remove anything without the regression vanishing.

I've done that, you can see it here: https://github.com/tronje/adc-read-compiler-bug

I'm guessing it'd be possible to minimize it even further. For my setup, I have to set up a GPIO expander and pull two of its outputs for my pin of interest to report correct values. I'd guess any other pin should exhibit the same bugged behavior.
Let me know if this is anything to go on, or if I should try to find an even simpler example that eliminates the need for the custom hardware I have here...

[Patryk27]

Hi, thanks for pinging! I'll check it tomorrow 🙂

sorry for always bugging you with these topics!

No worries, glad to be helpful 😄

[Patryk27]

Alright, I've been comparing the disassembly between the working and broken firmware, and one important change that stands out is the inlining of MCP23017::update_register_bit() (i.e. it's inlined in the broken firmware).

Now, while on its own inlining shouldn't cause any issues (or rather, if inlining a function breaks the binary, then it's a bug in the codegen), narrowing the scope down is always a step forward.

So, @tronje would you mind compiling your code with this updated config.toml and reporting back whether it works on the real hardware?

[build]
target = "avr-specs/avr-atmega164pa.json"
rustflags = ["-Zcross-crate-inline-threshold=0"]

[unstable]
build-std = ["core"]
build-std-features = ["compiler-builtins-mangled-names"]

[tronje]

Hey @Patryk27, thanks a lot for taking the time to look into this!

Unfortunately, your suggested change made no difference. I get the correct values with the old toolchain, and mostly 0's with the newer toolchain, just like before.

[Patryk27]

@tronje does the issue still occur if you compare the values instead of printing them? Like:

loop {
    let val = pin.analog_read(&mut adc);

    if val >= 128 {
        ufmt::uwriteln!(serial, "high").ok();
    } else {
        ufmt::uwriteln!(serial, "low").ok();
    }

    delay_ms(100);
}

(trying to narrow it down as to whether it's the measurement that's wrong or rather the printing part)

[tronje]

Your example always prints low on the new toolchain. I also tried this:

    loop {
        let val = pin.analog_read(&mut adc);

        if val == 0 {
            ufmt::uwriteln!(serial, "zero\r").ok();
        } else if val < 5 {
            ufmt::uwriteln!(serial, "small\r").ok();
        } else {
            ufmt::uwriteln!(serial, "large\r").ok();
        }

        delay_ms(100);
    }

Which mostly prints zero, soemtimes small on the new toolchain, and always large on the older toolchain.

[Patryk27]

I see, thanks for checking!

I'll try reproducing the issue in simavr and report back.

[tronje]

I see, thanks for checking!

No, thank you for taking all this time to help me out! I really, really appreciate it.

[Patryk27]

Alright, I've been testing the firmware under simavr and I can't reproduce the problem there (i.e. everything behaves correctly).

Let's try another angle:

• Does the issue still occur if you treat the pin as digital?
• Just a random guess but - could you try with let pin = pins.pa0.into_analog_input(&mut adc);?

[tronje]

* Does the issue still occur if you treat the pin as digital?

Looks that way; is_high always returns false on the new toolchain, and always true on the older one. Interesting.
I've simplified my example and pushed it. I guess the ADC is not involved in the issue then.

* Just a random guess but - could you try with `let pin = pins.pa0.into_analog_input(&mut adc);`?

Doesn't make a difference.

[AsmPrgmC3]

I looked at the assembly, but couldn't find anything obvious.
The instruction sequences for setting up and reading from the ADC are the same and the rest of the assembly also looks about the same.

Could you upload your verified working and broken versions in case it's system dependent?

EDIT: that was on the original version, didn't see that you simplified it further

[tronje]

EDIT: that was on the original version, didn't see that you simplified it further

Yep. Anyway, I've dumped the binaries using the following commands:

$ avr-objcopy target/avr-atmega164pa/release/adc-read-compiler-bug.elf -O ihex -j .data -j .text firmware.hex
$ avr-objdump -d -s -m avr5 -j .sec1 firmware.hex

and attached them here.

built-with-nightly-2023-12-28.txt
built-with-nightly-2024-03-22.txt

[AsmPrgmC3]

I compared those dumps with mine and they're bit for bit identical.