Skip to content

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]>
@MaureenHelm MaureenHelm requested a review from avisconti April 28, 2022 15:22
@stephanosio stephanosio requested a review from erwango August 18, 2022 08:34
@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