AP_HAL: fix up ftoa_engine to avoid wrong results and hangs
#30758
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Subnormal values were formatted as 1/2 of their true value because it was assumed the mantissa's significance corresponded to an exponent of 0, not the correct exponent of 1 (which represents 2x the value). Fix by setting the correct exponent in this case. This affects the ~16.7 million floats closest to zero.
Contributor
Author
|
Empirically |
Subnormal floats with mantissas of 7 or less (14 before the previous fix) have less digits to format than the conversion loop expects, causing it to never meet the condition to exit and looping forever. This cannot occur with normal floats because the implicit leading 1 always produces a sufficiently large value to format. This causes a hang with the 12 floats closest to zero (26 before the previous fix) if a suitably large precision is passed (which it is by default using e.g. `%f`). Fix by exiting the conversion loop if we get in the case where we can't add any more digits. Co-authored-by: JasonMDavey <[email protected]>
Ensures subnormals aren't formatted grossly wrong nor cause hangs. Reveals two additional bugs: * The requested number of digits is not always produced. * The precision isn't enough to distinguish every float. Outputs from the (nominally) original assembly implementation running on a real AVR (avr-gcc 7.3.0) are included. That implementation exhibits none of these flaws so it's likely they have been introduced during the C port. Co-authored-by: Peter Barker <[email protected]>
Contributor
Author
|
Changed to check for the problematic case in the loop and bail no matter how it happens, instead of checking the numbers which cause it going into the loop. Should be more robust but might be slightly slower. |
tridge
approved these changes
Jul 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alternative to #30754 to close #30722 . I have been severely nerd-sniped but here goes... Please see the commit messages for further detail.
This applies to anything that uses
%fwithhal.util->snprintfor the things we use linker wraps (augh) to point to that.Resolves two problems. Out of the ~4.2 billion floats:
0.<sufficient zeros>2was printed as0.<same zeros>1. This affects the ~16.7 million non-zero valuesfabsf(v) < 1.175494e-38f(FLT_MIN).fabsf(v) <= 2e-44f) will get stuck in an infinite loop if formatted, causing a watchdog reboot even in flight if executed on the main thread!Notices and does not resolve two additional problems:
This study of
%fproblems is not exhaustive. I did dig out an OG Genuino Arduino Uno R3 and avr-gcc 7.3.0 to test the (nominally) original assembly this function was ported from and it has none of these problems. Fixing the latter two concerns me slightly for overflows, and none of these problems have been obvious for a decade or more.We should perhaps consider modernizing our implementation (or digging deeper into the corresponding assembly to fix the other port problems) but this is a surprisingly rich and subtle field. We are also unfortunately not fully consistent in using this implementation.