Statically assert that delays are less than 25,769,803,784 cycles

[lord-ne]

The maximum delay that the Delayer efficiently supports is 25,769,803,784 cycles. Beyond this, previously the implementation would support slightly longer delays (about 0.3% longer) but use many more instructions to do so, and for even longer delays would overflow silently.

This PR adds a static check that the requested delay is less than 25,769,803,784 cycles. If the delay is longer than that, the compilation panics with an error message stating that the delay is too long.

[lord-ne]

The error message that results from having too large of a delay is as follows:

error[E0080]: evaluation of `avr_delay::delay_cycles::Delayer::<18446744073709551614_u64, 16000000_u64, 1000_u64>::TOTAL_CYCLES` failed
   --> C:\Users\Noam\Desktop\Programs\Rust\delay\src\delay_cycles.rs:82:9
    |
82  |         panic!("Error: Tried to delay for too many cycles. The maximum supported delay is 25_769_803_784 cycles");
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         the evaluated program panicked at 'Error: Tried to delay for too many cycles. The maximum supported delay is 25_769_803_784 cycles', C:\Users\Noam\Desktop\Programs\Rust\delay\src\delay_cycles.rs:82:9
    |         inside `avr_delay::delay_cycles::compute_total_cycles` at C:\Users\Noam\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\panic.rs:28:9
...
122 |     const TOTAL_CYCLES: u64 = compute_total_cycles(CYCLES, MUL, DIV);
    |                               -------------------------------------- inside `avr_delay::delay_cycles::Delayer::<18446744073709551614_u64, 16000000_u64, 1000_u64>::TOTAL_CYCLES` at C:\Users\Noam\Desktop\Programs\Rust\delay\src\delay_cycles.rs:122:31
    |
    = note: this error originates in the macro `$crate::panic::panic_2015` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn avr_delay::delay_cycles::Delayer::<18446744073709551614_u64, 16000000_u64, 1000_u64>::delay_impl`
  --> C:\Users\Noam\Desktop\Programs\Rust\delay\src\lib.rs:30:5
   |
30 |     Delayer::<MS, {avr_config::CPU_FREQUENCY_HZ as u64}, 1_000>::delay_impl()
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0080`.
error: could not compile `arduino_test` due to previous error

Ideally the error message would show the lines where you called delay_ms, but I wasn't able to get that to work (due to limitations because of feature(generic_const_exprs) not being complete yet). It's not super likely that a user will overflow this function anyway (max delay is about 25 minutes at 16 MHz), so I think the error message not showing the line is okay for now.

[stappersg]

Is blocked by #35, "Which branch of the library to release?".

[lord-ne]

I think it isn't? This is a PR into the cyacc branch, not master. We can continue improving each of the branches, even if we haven't decided if we're using them in the end. I mean, that's what we've already been doing for the past few weeks.

[stappersg]

A commit message is a letter to your co-worker and your future self. And a letter is more as a single line.

[stappersg]

A commit message is a letter to your co-worker and your future self. And a letter is more as a single line.

Oops, sorry for not clicking on show me the commit message button earlier.

[bombela]

I had completely missed that panic was supported in const expression. That's pretty sweet.

[stappersg]

On Tue, Jun 28, 2022 at 11:23:11PM -0700, lord-ne wrote: @lord-ne commented on this pull request. + const MAX_SUPPORTED_CYCLES: u64 = 25_769_803_784; At worst it's like four 64-bit multiplies, or sixteen 32-bit multiplies, and some additions. Compared to the cost of running the rust compiler in the first place, this is nothing.

Please document what is the reason that MAX_SUPPORTED_CYCLES = 25_769_803_784.

[lord-ne]

Please document what is the reason that MAX_SUPPORTED_CYCLES = 25_769_803_784.

@stappersg It's in the doc comments for the Delayer:

delay/src/delay_cycles.rs

Lines 39 to 59 in 31dbd31

	/// The maximum number of cycles is 25_769_803_784 when `delay_cycles_u32()` iterates 2^32
	/// times, `delay_2cycles()` is used twice, and `delay_1cycle()` once.
	///
	/// Every `delay_cycles_u*()` function has a minimum and maximum number of cycles it can consume.
	/// The minimum is: (cycles per run).
	/// The maximum is: (cycles per run) + (cycles per iteration) * (counter-1).
	/// Note that a counter of zero iterates 2^bits time.
	///
	/// Example with `delay_cycles_u32()`.
	/// Minimum: 9 cycles with 1 iteration.
	/// Maximum: 9 + 6 * (2^32-1) == 25_769_803_779 cycles with 2^32 iterations.
	///
	/// Cycles 1..=5 are implemented by a combination of up to two `delay_2cycles()` and up to one
	/// `delay_1cycle()`. Which gets us our maximum of 25_769_803_779 + 5 == 25_769_803_784.
	///
	/// Technically, beyond this value, the counters of various sizes will be combined until they are
	/// all used up. This means the absolute limit is the sum of the maximum cycles of all counters
	/// combined plus five:
	/// (3+3*0xFF) + (5+4*0xFFFF) + (7+5*0xFF_FFFF) + (9+6*0xFFFF_FFFF) + 5 == 25_853_952_779.
	/// But at this point, this is costing 23 instructions, for very little gain (~3.5s at 24Mhz).
	/// Calling delay_cycles twice would be far more efficient.

[stappersg]

I'm done with merge request., there is too much doubt that it is an improvement.

[stappersg]

I'm done with this merge request., there is too much doubt that it is an improvement.

[lord-ne]

there is too much doubt that it is an improvement.

What? That isn't the impression that I got from the discussion at all. Me and @bombela were just discussing a fairly minor detail (the u128 multiply).

@stappersg, are you saying that based on the previous discussion there is too much doubt it is an improvement, OR are you saying that you personally doubt that it is an improvement? Because if it is based on the previous discussion, I don't think you are correct

[bombela]

ahah woot. I think this PR is just fine and should be merged.

[stappersg]

Cycle accuracy makes sense for small delays.
Delaying 16_000_000 cycles on a 16MHz clocked AVR would be a delay of one second.
When already delaying one second there is no use in further cycle accurate delay.

I'm really done with this merge request.

[bombela]

This merge request ensures that there can never be any overflow in case somebody types out an overly long delay. Right now the overflow is silent. It would be annoying, difficult, and a waste of time to debug.

I do agree with you that delay_cycles is probably best reserved for small delays. Not minutes. Nonetheless, like everything in software ; and life in general; there is a physical limit. As far as I know, it is always better to inform the user when the limit is reached. Rather than surprising them with a seemingly random behavior.

The fastest AVR is 50Mhz (FPGA stuff). The fastest AVR MCU is 32Mhz. I personally have some ATTiny at 20Mhz. So for one second of delay, you need to be able to count to at least 50_000_000.

Here are the limits per counter as a reminder:

counter	calculation	max cycles	@16mhz	@50MHz
-	Up to 5 cycles without counter	5	312.5 ns	100 ns
8	$3+3*(2^8-1)+5$	773	48.31 µs	15.46 µs
16	$5+4*(2^{16} -1)+5$	262_150	16.38 ms	5.24 ms
24	$7+5*(2^{24} -1)+5$	83_886_087	5.24 s	1.68 s
32	$9+6*(2^{32} -1)+5$	25_769_803_784	26.84 min	8.59 min

I have personally used delays of up to a minute at @20Mhz when debugging hardware and code together. So I see an appeal to the 32 bits. And avr-gcc does the same thing, so why not follow. It is zero-cost anyways (ie: cost only if used).

Anyways, no matter the biggest counter width we ultimately support, there is always a limit. And we should produce an error when attempting to overflow this limit. This is what this PR is adding. An assertion to prevent a silent overflow.