gd32v-rust / gd32vf103-hal

Hardware abstract layer (HAL) `embedded-hal` for RISC-V microcontroller GD32VF103 in Rust. Contributions welcomed!
MIT License
62 stars 17 forks source link

Delay multiplication overflow #6

Closed martindisch closed 4 years ago

martindisch commented 4 years ago

I'm coming from https://github.com/martindisch/gd32vf103-demo/issues/1 and noticed a problem with the delay implementation after trying it as you suggested.

Using a delay of 1000 ms resulted in a panic. I tried other values and found out that 537 and higher values fail, while 536 and below work. Because I have trouble with my debugger and can't get semihosting to work, it was pretty hard to exactly get to the bottom of it, but the fact that the panic didn't happen in release mode gave me the final hint I needed to figure it out.

By first multiplying the number of milliseconds with the clock frequency (which is often a large number, e.g. 8000000) before dividing it further, it was easy to run into an overflow case starting at 537 ms, as 537 * 8000000 = 4296000000 which is greater than 2^32 = 4294967296. A possible fix is to only multiply the intended delay with the already divided clock frequency, which allows for larger delays. This only shifts the problem though, the maximum delay is now 1073741 ms assuming a clock frequency of 8 MHz. The only way to improve this further is to switch to u64.

Edit: You can see what happened in my case on this playground. If you run in debug mode it panics, in release mode it overflows and produces a wrong result.

luojia65 commented 4 years ago

Nice! Thank you for testing. That's an unsound bug I have not noticed