search

df8oe / UHSDR

SDR firmware and bootloader with configuration files for use with Eclipse, EmBitz and Makefile
Other
359 stars 189 forks source link

PPM_Error_Reduction #1916

Closed M0FDA closed 3 years ago

M0FDA commented 3 years ago

Storage of the PPM figure as a decimal fraction introduces conversion errors. By handling as a PPM figure eliminates this source of error. Adding an extra single multiplication at the fdiff calculation to compensate reduces the error generated by the division by the factor of the multiplication.

2 code changes in the SI570.c file can reduce the PPM calculation errors. Dividing PPM figure by 1,000,000 results in fundamental calculation errors due to non-exact binary division. Using the PPM figure directly removes this conversion error as the figure used is an exact representation. The original error is compounded in the "small frequency switching range" checking process when a very small but accurate figure is divided by a very large number and gives rise to the problems described in the SI570.c file

M0FDA commented 3 years ago

This is my first attempt at contributing to a Git project, so any comments are appreciated. Thanks. Alan

db4ple commented 3 years ago

@M0FDA : Well spotted. Honestly. Github-wise you did all things right.

But if I am not mistaken, the error corrected is ~2e-17. This is by all means a small error, considering the the better of the two German atomic standard clocks has an error of ~2e-16, which is an order of magnitude worse.

So integrating or not integrating your change would not make any difference for any practical scenario. At 10 Ghz we would have a frequency difference of 2e-7 Hz if we keep the old formula. I hope you agree, that we can live with that. Don't get me wrong, if I was not right about the impact, no problem, just tell us.

I am fairly sure, that you will find more problematic parts, and we are looking forward to your contributions.

M0FDA commented 3 years ago

Thanks for your comments, it is very much appreciated.

I noticed the error creeping in while writing an SI569 driver, but the error reduced the allowed small frequency change switching range in that case.

I do agree that the error is indeed very small and is not one I have observed in practice on my McHF, but previous coders had reported a problem with the small frequency change switching range and this calculation does not significantly alter the code, only the resulting errors.

I believe the errors are compounded by effectively being repeated in the two lines of code I have outlined.  The first line eliminates one source of error completely (this is the ~2e-17 you show below), the second reduces a calculation error by a factor of 1E6 in the calculation of fdiff by making better use of the integer part of the storage of this number, provided the scaling is done before the division, this at the cost of one extra floating point calculation. It is the second of these errors that I consider the most significant and benefits your code the most, but this change cannot be made without the first.

I am happy to leave it with you to decide if you wish to include the 2 changes in the next release of the firmware and am glad I have had the opportunity to present my arguments for the change.

This is a great product and one which I will continue to support in the future.

On 7/28/2021 10:49 AM, db4ple wrote:

@M0FDA https://github.com/M0FDA : Well spotted. Honestly. Github-wise you did all things right.

But if I am not mistaken, the error corrected is ~2e-17. This is by all means a small error, considering the the better of the two German atomic standard clocks has an error of ~2e-16, which is an order of magnitude worse.

So integrating or not integrating your change would not make any difference for any practical scenario. At 10 Ghz we would have a frequency difference of 2e-7 Hz if we keep the old formula. I hope you agree, that we can live with that. Don't get me wrong, if I was not right about the impact, no problem, just tell us.

I am fairly sure, that you will find more problematic parts, and we are looking forward to your contributions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/df8oe/UHSDR/pull/1916#issuecomment-888171983, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF26QCOLUXWPBIOMLECHBEDTZ7HDBANCNFSM5A7P6MAA.

-- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus