Baudrate correction in Serial is not correctly implemented

[egilkv]

How much of a problem this is would depend on the correction of the individual chip. Async is quite robust to clock accuracy, but we should still try to make the clocks as accurate we can by using the correction methods in the right way.

The code in the pull request is now in accordance to the data sheet. The previous code would in effect either apply at least twice the compensation, or apply little or no compensation, depending on the sign of the compensation. Finally, as a matter of principle, non-corrected F_CPU is the basis, since that is what is assumed by the correction factors.

I have verified with a frequency counter that the new code gives the most accurate baudrate. On my samples the error was between 0,1% and 0,15%.

https://github.com/arduino/ArduinoCore-megaavr/pull/44

To do the test, the following code will work fine since it will give a pattern on the Tx pin equivalent to a square wave of half the frequency of the baudrate:

https://github.com/arduino/ArduinoCore-megaavr/files/3461514/Test_serial_bug.txt

Change the BAUD to test different frequencies.

[egilkv]

Going through the history, to see how we got here:

To begin with, it seems frequency correction was overall, with F_CPU_CORRECTED (I assume) being the corrected CPU frequency overall, and baudrate correction was based on this. If done right this would also make things like millis() being frequency corrected, but for some reason (may very well have been a good one) it seems this approach was abandoned.

Submit e7a0b8a (@agdl) introduced baudrate frequency correction for the UART, and was a correct implementation with the exception that it was based on F_CPU_CORRECTED, not F_CPU. (Which in practice may be a somewhat moot point if they are the same, but at least reads wrong).

The error was introduced by 642be1c0 (@agdl) which was titled "Improved baud rate settings formula". It was nothing of the sort, it made the compensation twice as large as it should be.

The submit d60f5a90 introducing abs was curious, it made zero correction or twice the correction based on the polarity of the correction.

I do believe I have fixed the problem, and hopefully made the code slightly clearer.

[facchinm]

Hi @egilkv, thanks for summing everything up. https://github.com/arduino/ArduinoCore-megaavr/commit/e7a0b8ac229cc059f5f625500563a907e5acd7c8 in fact also made F_CPU_CORRECTED = F_CPU (as per the code removed in wiring.c) so the implementation at that commit should be corrected.

Anyway, during the factory functionality test, we had a lot of failures. The investigation was showing that all the failing boards had a negative sigrow value and the resulting baud rate was horribly wrong; the abs() patch fixed this but, as you clearly report, it can apply double the correction or none.

I'm testing your patchset (#44) with a few boards and it looks ok but I still didn't find one with negative sigrow to confirm that it doesn't regress.

[egilkv]

@facchinm I am not sure how you measured the baudrate. If measuring flank to flank on an oscilloscope, for instance, it is really hard to get such a measurement paricularly accurate. My preferred method is to emit an alternating 1-0-1-0... bit pattern (including start and stop bits) and send continuously. The test program I made does just that. You can then accurately measure the resulting frequency, which will be half the baudrate, with a frequency counter.

I have a very limited set of CPUs to test from, but I did test both 16 and 20MHz compensation (code has to be altered to pick up the right one, trivial change, I can submit that too), and they where both good with that change in place. Alas, from memory, I do not have any samples that had negative compensation.

[facchinm]

Merged with #44 , publishing the core later today so it can get some broader testing