Rewrote library to fix several issues

[lord-ne]

Fixes the issues https://github.com/avr-rust/delay/issues/13 and https://github.com/avr-rust/delay/issues/15. Supersedes the pull requests https://github.com/avr-rust/delay/pull/12, https://github.com/avr-rust/delay/pull/14, and https://github.com/avr-rust/delay/pull/16.

[lord-ne]

Marked as draft because I haven't had a chance to test this yet.

[lord-ne]

I decided it wasn't a good idea to make a breaking API change on such an inactive repository, so I put back the u32 delay function, and marked it as deprecated.

[lord-ne]

Inspecting the assembly output, both long and short delays work well. Constant delays (known at compile time) compile with negligible overhead. Tested with both speed (opt-level = 3) and size (opt-level = "z") optimizations.

[Patryk27]

Hi, thanks for this merge request - I've managed to run the code, but seems like on my atmega328p the delays are 10x as long as they are supposed to be: e.g. delay_ms(100); waits for one second.

I'm using AVR_CPU_FREQUENCY_HZ=20000000 and dropping one zero seems to help, but it kinda feels the value is alright and the calculation is off somewhere.

[lord-ne]

I double-checked the math and it looks right to me, and it seemed to work correctly on my 328p. I'm not sure what could be causing this issue. Can you post your code?

[Patryk27]

Sure - my code is nothing more than:

#![feature(llvm_asm)]
#![no_std]
#![no_main]

use ruduino::cores::current::port;
use ruduino::Pin;

#[no_mangle]
pub extern "C" fn main() {
    port::B0::set_output();

    loop {
        port::B0::set_high();
        avr_delay::delay_ms(1000); // in this case it's set to 1s, but in reality takes 10s 
        port::B0::set_low();
        avr_delay::delay_ms(1000);
    }
}

... compiled using:

AVR_CPU_FREQUENCY_HZ=20000000 cargo build --release

... with .cargo/config:

[build]
target = "avr-atmega328p.json"

[unstable]
build-std = ["core"]

Maybe 20 MHz is too much for AVR_CPU_FREQUENCY_HZ? For reference, atmega-hal's Delay seems to be working correctly (also on 20 MHz).

[lord-ne]

20MHz shouldn't be causing any overflow. Maybe this is related to the issue with avr_config where it doesn't rebuild when the AVR_CPU_FREQUENCY_HZ environment variable changes?

[Patryk27]

Yeah, I've read about that issue and I've done like a dozen of cargo clean-s to be sure I'm not running stale code 😄 Anyways, I don't have my 328p at hand now - I'll try to re-check when I have some free time; if everything works correctly on your side, then I might've just fused my processor incorrectly (I guess accidentally setting it to use its internal 8 MHz clock could yield similar results).

[stappersg]

For what it worth:

• 18 has been merged

• next work is on https://github.com/avr-rust/blink/issues/38

[stappersg]

Fixes the issues https://github.com/avr-rust/delay/issues/13 and https://github.com/avr-rust/delay/issues/15. Supersedes the pull requests https://github.com/avr-rust/delay/pull/12, https://github.com/avr-rust/delay/pull/14, and https://github.com/avr-rust/delay/pull/16.

That is no longer true due other "progress". The so called progress is a mix of

• being blocked by a LLVM ERROR
• switch from llvm_asm!() to asm!()
• merging merge requests

I hope that there will be smaller Merge Requests that replace this large MR.

[lord-ne]

Okay, this is ready for review again. I basically just switched the llvm_asm! to the asm! that's currently in the main repo (here) and tidied up the code a bit.

It only compiles in release mode for some reason (so does the current main branch of the main repo), but that's an issue with compiling core for AVR, not an issue with this code.

@bombela's point about delays that aren't known at compile time still stands, but I think it isn't a priority right now (AVR libc doesn't do it either), we first need to get the crate in a working state.

[lord-ne]

I might try something with magic-number division (https://ridiculousfish.com/blog/posts/labor-of-division-episode-i.html) to work with values not known at compile time. But that will definitely be a separate pull request

EDIT: It looks like this wouldn't be that helpful, since we would end up with 64x64->128 but multiplications, which would be very slow anyway.

[bombela]

This is fascinating. The math goes over my head sadly. On my side, I am trying to produce something similar to the intrinsic delay_cycles from gcc-avr. With asm! and const generics. I will make a pull request if I ever get something worthwhile.

[bombela]

I actually got it working. Here is a gist (https://gist.github.com/bombela/cc90e5c29f3e7667326de4c087c1e148) with the code for reference. I tested all the corner cases I could think of, and it appears to work beautifully. It requires that the delay functions take their parameters via const generic (delay_ms::<42>()). I will try to assemble a PR together someday.

[lord-ne]

@bombela

That looks really good. A couple minor points:

• I think that these compile time constant delay functions should probably be separate functions. There are still some use cases where the user knows that a value is known at compile time, but rust doesn't consider it valid in a const context for whatever reason (since the compiler's constant propagation is more powerful than rust's tools for determining const-ness). Although admittedly, I'm struggling to think of such cases off the top of my head. Maybe call the functions like delay_ms_const or something like that?
• I think the arguments to delay_us, delay_ms, delay_s should be u32, so they can't overflow the Delayer. In general, I think that the Delayer itself should be made private, since it can overflow. And to be honest, does the MUL and DIV part really need to be part of the Delayer, wouldn't it be simpler to just multiply and divide at the call site (that is, in the body of delay_us, delay_ms, and delay_s) and pass in a single CYCLES value? And if it really is a useful operation, make a seperate function that takes in a u32 cycles and mul, so it can't overflow. EDIT: Overflow actually causes a compile-time error, so this isn't really an issue.
• Some convenience macro rules so one can just write delay_s!(3 60) instead of delay_s::<{ 3 60 }>()
• You should probably wait until #16 and/or #17 is merged before making another PR.

[stappersg]

On Thu, Jun 09, 2022 at 01:34:07AM -0700, François-Xavier Bourlet wrote:

I actually got it working. Here is a gist (https://gist.github.com/bombela/cc90e5c29f3e7667326de4c087c1e148) with the code for reference. I tested all the corner cases I could think of, and it appears to work beautifully. It requires that the delay functions take their parameters via const generic (delay_ms::<42>()). I will try to assemble a PR together someday.

Request / idea

Idea / request

In directory examples in the rewrite branch

Groeten Geert Stappers -- Silence is hard to parse

[stappersg]

That is not for this rewrite.

I closing this merge request. That might be stupid. The idea is getting a fresh start. Use branch rewrite for it.