rp-rs / rp-hal

A Rust Embedded-HAL for the rp series microcontrollers
https://crates.io/crates/rp2040-hal
Apache License 2.0
1.45k stars 234 forks source link

Faulty UART baudrate divisor formula #820

Open KisImre opened 3 months ago

KisImre commented 3 months ago

The fractional part of the UART baudrate divisor can overflow for certain baudrates. In this example I picked the calculate_baudrate_dividers function and iterated though a range of baudrates while printing the integer and fractional divisor values if they change. In the output this is the interesting part:

baudrate=57861 divint=135 divfrac=1
baudrate=57868 divint=135 divfrac=0
baudrate=57871 divint=134 divfrac=64 <----
baudrate=57874 divint=134 divfrac=63
baudrate=57881 divint=134 divfrac=62

The function outputs fractional divisor value of 64 which gets truncated once it's written into the register, so the effective fractional value will be zero.

The issue is in this line. (baudrate_div >> 7, ((baudrate_div & 0x7F) + 1) / 2 The recommended +0.5 correction is only added to the fractional part but the overflowing carry bit is not added to the integer part. Adding 1 and dividing by 2 before splitting into integer and fractional parts would solve the issue.

I've tested the issue on a Raspberry Pico board using the uart example and changing the baudrate in line 94. I've measured the actual baudrate (with ~10Hz precision) using a logic analyzer and I got the following results:

baudrate=57861 -> 57870
baudrate=57868 -> 57870
baudrate=57871 -> 58300 <----
baudrate=57874 -> 57870
baudrate=57881 -> 57870

The issue occurs for each baudrate where the integer divisor is about to change, or more precisely, where (baudrate_div & 0x7f) == 0x7f.

jannic commented 3 months ago

Hi @KisImre!

Thanks for this great analysis. How did you even notice this? I'd guess that the difference is small enough that it doesn't impact the usability of the UART. (https://pdfserv.maximintegrated.com/en/an/AN2141.pdf)

Impressive that you didn't stop with pointing out the bug in the source code, but also confirmed it by measuring the actual timing on the wire.

Given the comment above that code ("Code inspired from the C SDK.") I wonder if the C SDK has the same issue. Did you check that? if it's present there as well, do you want to file a bug yourself, or should I do it?

KisImre commented 3 months ago

I just noticed the affected line while I was tinkering with the UART and the (... + 0x7F) + 1 part seemed like a place where an overflow can happen. I put together the attached example to verify my theory and then tested it in the hardware just to see it in practice.

I guess it doesn't affect much on lower baudrates because the error is approx. 1 / divint. However, for a faulty baudrate in the 500000 baud range the error would be around 6% which is not good.

Now I tested the C SDK too, which indeed has the same bug. I will open bug report in their repo as well.