-
Notifications
You must be signed in to change notification settings - Fork 20.2k
AP_Math: Add an 8-bit constrain function #32004
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
AP_Math: Add an 8-bit constrain function #32004
Conversation
peterbarker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change any compilation at all? Didn't seem to change anything here...
Would be nice to find a spot where this does make a difference before merging it.
libraries/AP_Math/AP_Math.cpp
Outdated
| template signed char constrain_value<signed char>(const signed char amt, const signed char low, const signed char high); | ||
| template unsigned char constrain_value<unsigned char>(const unsigned char amt, const unsigned char low, const unsigned char high); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't you use the stdint types here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These explicit instantiations are intentional and new.
Although constrain_value<T> is templated, signed char and unsigned char values are promoted to int during expression evaluation, so constrain_value<int> was being selected and the result then cast back to 8-bit.
By explicitly instantiating constrain_value<signed char> and constrain_value<unsigned char>, constraining is actually performed in the intended 8-bit domain, avoiding unnecessary integer promotion and re-casting.
This is the same class of issue that was previously addressed in commit 666b4e0.
666b4e0#diff-cbed7b32d0d646f76e1e63e3bd9ce1523dd8114a21e5b4f7654ca338d6e0fe68R421
I kept signed char / unsigned char here to stay consistent with existing APIs that already use these types, but I’m happy to switch to int8_t / uint8_t if that is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept signed char / unsigned char here to stay consistent with existing APIs that already use these types, but I’m happy to switch to int8_t / uint8_t if that is preferred.
Which API? Neither "signed" nor "unsigned" exist in AP_Math.h.
So yeah, please change them :-) (unless I'm missing something here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterbarker san
Agreed, thanks for the clarification.
I’ve updated this to use int8_t / uint8_t instead of signed char / unsigned char, to make the intended 8-bit width explicit and align better with AP_Math conventions.
I also checked the generated assembly in ArduCopter/mode_zigzag.cpp: this change affects which constrain_value<> overload is selected, keeping the constrain operation in the 8-bit domain.
I rechecked the generated assembly, and with current compiler settings the final instruction sequence ends up being the same.
However, this change still affects which constrain_value<> overload is selected and makes the intended 8-bit behavior explicit, rather than relying on integer promotion.
So this is more about correctness and maintainability than a measurable runtime difference.
INT8_T
3e: 227f movs r2, #127 ; 0x7f
40: 1b04 subs r4, r0, r4
42: 2100 movs r1, #0
44: f995 007b ldrsb.w r0, [r5, #123] ; 0x7b
48: f7ff fffe bl 0 <signed char constrain_value(signed char, signed char, signed char)>
4c: f44f 737a mov.w r3, #1000 ; 0x3e8
50: b280 uxth r0, r0
52: 4358 muls r0, r3
54: 4284 cmp r4, r0
56: bf34 ite cc
58: 2000 movcc r0, #0
5a: 2001 movcs r0, #1
ORIGINAL
3e: 227f movs r2, #127 ; 0x7f
40: 1b04 subs r4, r0, r4
42: 2100 movs r1, #0
44: f995 007b ldrsb.w r0, [r5, #123] ; 0x7b
48: f7ff fffe bl 0 <short constrain_value(short, short, short)>
4c: f44f 737a mov.w r3, #1000 ; 0x3e8
50: b280 uxth r0, r0
52: 4358 muls r0, r3
54: 4284 cmp r4, r0
56: bf34 ite cc
58: 2000 movcc r0, #0
5a: 2001 movcs r0, #1
5c:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which function in mode_zigzag.cpp uses constrain_value?
b8684d3 to
085a8cd
Compare
|
I had a bit of a play around with this to see if we could convince the compiler to whinge if we called the wrong function. |
peterbarker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks for taking a look at this and experimenting with it. That makes sense to me. My intent in #32004 was mainly to make the 8-bit behavior explicit and avoid relying on integer promotion; I agree that being able to catch the “wrong” constrain call at compile time would be even better. Happy to see this explored further in #32015, and I’m fine with keeping #32004 focused on the explicit 8-bit instantiation as a prerequisite for this. |
Currently, there is no constrain function for 8-bit values.
As a result, a 16-bit constrain function is being used, and the constrained result is then cast back to an 8-bit type.
This approach requires unnecessary type promotion and redefinition, which reduces code clarity and intent.
This change introduces a dedicated 8-bit constrain function, allowing 8-bit values to be constrained directly without relying on 16-bit operations or additional type casting.
This improves readability and keeps the data type consistent with its intended use.