libc: minimal: math: sqrt: fix Incorrect minimal libc sqrt #58479
Conversation
stephanosio
changed the title
There was a problem hiding this comment.
The code is looking good, but there are a bunch of formatting issues. Get this past checkpatch and the CI and it should be ready to merge.
I'm thinking we might want to borrow tests from picolibc to validate this code during CI too; I'll see if I can find a bit of time to pull that out separately.
lawrencek52
force-pushed
the
55962_fix_typo
branch
from
0993ac6 to
d26a6b0
Compare
tests/lib/c_lib/src/test_sqrt.c
Outdated
There was a problem hiding this comment.
This comment seems incorrect here -- you're using abs(a-b) instead of xor.
|
@lawrencek52 Please fix the compliance and coding guidline issues listed in the CI failures |
The coding guideline error refers to defining the reserved identifier 'sqrt', which this patch kinda needs to do. The compliance checks look to be commit message formatting errors which should be fixed. |
|
Hi Keith:
I did fix the commit formatting errors but for some reason Git hasn't seen
these changes. My level of skill with Git is VERY low and I am not sure
what I did wrong, or how to go back and fix the errors.
…-Lawrence-
On Sun, Jul 9, 2023 at 3:53 PM Keith Packard ***@***.***> wrote:
@lawrencek52 <https://github.com/lawrencek52> Please fix the compliance
and coding guidline issues listed in the CI failures
The coding guideline error refers to defining the reserved identifier
'sqrt', which this patch kinda needs to do. The compliance checks look to
be commit message formatting errors which should be fixed.
—
Reply to this email directly, view it on GitHub
<#58479 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE57NSKUR25ZLPQ3DC2LNXTXPMD4VANCNFSM6AAAAAAYVOOS2A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Changed initial guess from a simple x/3 to dividing the exponent by 2. This makes large or small numbers like 10e10 and 01e-10 converge in a few loops. Added a loop counter to ensure that the algorithm breaks out of the loop in the case that the algorithm doesn't converge (toggling between two numbers). Added test cases for sqrt and sqrtf in libc. Tested with a range of numbers between 10e10 and 10e-10. Verify good accuracy in test case. Closes: zephyrproject-rtos#55962 Signed-off-by: Lawrence King <lawrencek52@gmail.com>
Yeah, it's a complicated operation. I've generated a single commit which contains all of the changes in this series along with a reformatted commit message and stuck that in my Zephyr tree here: https://github.com/keith-packard/zephyr/tree/55962_fix_typo -- I did that by using |
|
Btw, I just tried enabling the sqrt/sqrtf tests on all targets and it works correctly for everything except picolibc under x86 with soft floats and leon3. Once I've got picolibc working for that configuration, I'll submit a PR that enables the tests everywhere -- Zephyr should have floating point support on all targets, although without an FPU you will end up using the soft float support provided by the toolchain. |
|
The changes look good, please go ahead with pushing it into my repository.
What do I need to do to finish it up?
…On Mon, Jul 10, 2023 at 2:56 AM Keith Packard ***@***.***> wrote:
Hi Keith: I did fix the commit formatting errors but for some reason Git
hasn't seen these changes. My level of skill with Git is VERY low and I am
not sure what I did wrong, or how to go back and fix the errors.
Yeah, it's a complicated operation. I've generated a single commit which
contains all of the changes in this series along with a reformatted commit
message and stuck that in my Zephyr tree here:
https://github.com/keith-packard/zephyr/tree/55962_fix_typo -- I did that
by using git rebase -i e824b24 and then
editing the provided file to change the 'pick' to 'squash' for all but the
first commit. That presented me with another file where I edited the commit
message to make it conform to Zephyr requirements (I think). If you're
happy with the result, I can put this into your repository with git push
-f lawrencek52 55962_fix_typo. That should get it ready for merging.
—
Reply to this email directly, view it on GitHub
<#58479 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE57NSL4UY76SSDQTN5AQI3XPORSXANCNFSM6AAAAAAYVOOS2A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
keith-packard
force-pushed
the
55962_fix_typo
branch
from
d26a6b0 to
908cc95
Compare
We should be all set. As you learn more about git, I suspect you'll figure out how to do the rebase and push. It's a slightly advanced git feature that lets you do development in tiny increments (as you had done) and then clean things up before they get pulled into the project. I'd bet there are plenty of tutorials on how this all works. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @lawrencek52!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!
To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.
Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁
6 participants
I crashed my computer and I am having difficulty with git getting back to where I was, but I think this code is correct.
Continuation of #56881
Fixes #55962