-
Notifications
You must be signed in to change notification settings - Fork 614
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
[wpimath] Simplify pose estimator #6705
[wpimath] Simplify pose estimator #6705
Conversation
Since sampleAt() was merged after 2024 champs, it hasn't been in a release yet, so we can change the behavior to whatever we want. |
I didn't think of this earlier, but the information is still needed for |
Sounds like you should write some real code that uses this to make sure it actually does what a user might want before finalizing the PR. |
Based on further conversation on Discord, we don't want to add the assumption that vision measurements come in order (which is something that would happen with multiple coprocessors), so we'll keep the vision update buffer.
Is there something in addition to the existing tests that you think should be added? (Maybe a simulation of multiple coprocessors?) |
What's left for this PR? I'd be happy to add some tests (probably in a separate PR), but I'd need to know which behavior we want to test. |
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.
Please use blank lines to delimit related chunks of code. I'm having trouble reading and understanding it.
From local testing, the difference between the old implementation and the new implementation is on the order of 1e-12. (First, I changed debug to true in calls to Python code: from decimal import Decimal, InvalidOperation
import sys
with open(sys.argv[1]) as reader1, open(sys.argv[2]) as reader2:
max_err: Decimal = Decimal(-1)
for (lnum, (line1, line2)) in enumerate(zip(reader1, reader2, strict=True), start=1):
col1: int = 0
col2: int = 0
while True:
# -1 if not found, which using as the end index in a substring cuts off the last one (newline character)
next1: int = line1.find(',', col1)
next2: int = line2.find(',', col2)
part1: str = line1[col1:next1].strip()
part2: str = line2[col2:next2].strip()
float1: None | Decimal
try:
float1 = Decimal(part1)
except InvalidOperation:
float1 = None
float2: None | Decimal
try:
float2 = Decimal(part2)
except InvalidOperation:
float2 = None
if (float1 is None) ^ (float2 is None):
print(f"mismatched type: {part1 = !r}, {part2 = !r}")
elif float1 is not None and float2 is not None:
err: Decimal = abs(float2 - float1)
if err > max_err:
max_err = err
print(f"New max error {err} at {lnum} between {part1} and {part2}")
if next1 < 0 or next2 < 0:
break
col1 = next1 + 1
col2 = next2 + 1 Output:
|
Completely non-breaking pose estimator implementation parts of #5473 (that is, excluding the removal of the kinematics parameter of
PoseEstimator
(which is exposed to custom implementations) and the test unification) updated for the addition ofsampleAt()
. (Thanks @jlmcmchl for the inspiration!)Allocations analysis:
This assumes 0 allocations for map insertion,
isEmpty()
,firstKey()
/lastKey()
,floorKey()
, andentrySet()
and 1 allocation forheadMap()
andtailMap()
.sampleAt()
(Net -2 for diffy, -4 for mecanum, and -5 - 5n for swerve):InterpolationRecord
sample (if interpolating, 1 wheel positions interpolation, 6 allocations, 1kinematics.toTwist2d()
call).updateWithTime()
(Net 0 for diffy, 0 for mecanum, and -n for swerve):InterpolationRecord
object (1 allocation).addVisionMeasurement()
(With k odometry updates after vision measurement: Net +7 - 8k for diffy, +5 - 10k for mecanum, and +4 - 5n - (11 + 3n)k for swerve):InterpolationRecord
sample (if interpolating, 1 wheel positions interpolation, 6 allocations, 1kinematics.toTwist2d()
call) and anupdateWithTime()
call for every single odometry update after the vision measurement (1 odometry update (4 allocations, 1kinematics.toTwist2d()
call, 1 wheel positions copy), 1 wheel positions copy, 1 allocation).sampleAt()
call (6 allocations), aVisionUpdate
allocation (1 allocation), clearing latest vision updates (1 allocation), and updating latest pose estimate (2 allocations).Costs of kinematics operations:
kinematics.toTwist2d()
Notes:
sampleAt()
, we must maintain a record of past vision updates. This is the primary reason why this PR has a much greater line count than the original PR (which was able to effectively only have 1 offset). Changing the behavior ofsampleAt()
would allow for the complete removal ofm_visionUpdates
, but the goal of this PR was to be a completely non-visible change.m_visionUpdates
cannot be aTimeInterpolatableBuffer
, because a vision update may be needed for an indefinite amount of time (if no more vision updates come after).PoseEstimator
constructor keeps around the (now unused)kinematics
parameter in case people implemented a custom implementation ofPoseEstimator
. We could just remove it now, though, since the expected number of people in that category is quite low (and those people would probably already have to make updates because of [wpimath] Remove swerve wrappers for odometry and pose estimation, move wheel positions operations to kinematics #6673).GetInternalBuffer()
to C++'sTimeInterpolatableBuffer
. This could be moved to a separate PR for commit history if desired, but I included it in this PR to make the reason obvious.