Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vl53l0x: Perform 'abs' operation before scaling and dividing mcps dif… #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keith-packard
Copy link
Contributor

…f value

If the computed value is negative, then it's important to flip the
sign before doing the division as that will not have the same result
on an unsigned value as it would on a signed value in the same bits.

Signed-off-by: Keith Packard [email protected]

…f value

If the computed value is negative, then it's important to flip the
sign before doing the division as that will not have the same result
on an unsigned value as it would on a signed value in the same bits.

Signed-off-by: Keith Packard <[email protected]>
@erwango
Copy link
Member

erwango commented Aug 22, 2022

Thanks for this fix. This seems valid but the original code is extracted from a library provided by ST.
I'm getting in touch with the dev team to make sure that the fix is valid and can be integrated in a next release of the lib.
Based on their feedback, we'll merge this fix, hopefully with a bug tracker so we can follow up.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advise from ST domain expert is to reject proposal:
"the fix is correct from mathematical perspective, but physically, xtalk can't be higher than signal otherwise status i invalid. Besides since function used for pulse standard deviation estimation isn't in the datasheet, ST can't guarantee perf on it".
@keith-packard Is that fine with you ?

@keith-packard
Copy link
Contributor Author

Their comment doesn't make much sense to me; if they're so sure the difference will always be positive, then why is there an abs in this code at all? In any case, the only reason I saw this bug was because I was working on zephyrproject-rtos/zephyr#45179. If you want to leave the code as-is, that's entirely up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants