Repository navigation
Pre-EKF Robot Localizer Clean up - #3953
Conversation
8df03a3 to
90378fc
Compare
90378fc to
f2e9dd6
Compare
e18f050 to
90378fc
Compare
2ba21c0 to
0d85f4d
Compare
6133e3c to
e462706
Compare
williamckha
left a comment
There was a problem hiding this comment.
LGTM, just two nits and a comment
| } | ||
|
|
||
| // If rollback point is at the end, vision is older than all history steps | ||
| // So rollback ever step |
There was a problem hiding this comment.
| // So rollback ever step | |
| // So rollback every step |
| updateRobotLocalizer(primitive.value()); | ||
| primitive_executor_->updatePrimitive(primitive.value(), robot_status_); | ||
| } | ||
| imu_service_->poll(robot_status_); |
There was a problem hiding this comment.
I don't think moving the IMU service poll here is a good idea (frankly it makes things inconsistent since now we're collecting IMU data at a different time than the motor data).
This raises a problem with the order we're doing things right now. Currently this is what we're doing:
- Run the predict step for robot localizer, which gives us the priori estimate for time
x - Run the update step for robot localizer using the sensor data for time
x-1we collected in the previous loop iteration. This is incorrect. We should be using the current sensor data (timex) for this update step to get the posteriori estimate for timex - Use the INCORRECT posteriori estimate to step the primitive executor
- Collect sensor data for the current time
x
What we should be doing is:
- Collect sensor data for the current time
x - Run the predict step for robot localizer, which gives us the priori estimate for time
x - Run the update step for robot localizer USING the sensor data we just collected, which gives the posteriori estimate for time
x - Use the posteriori estimate to step the primitive executor
What makes this difficult right now is that we're both collecting sensor data + commanding motors in the same MotorService::poll. We'd need to have separate methods so that we get sensor data before running robot localizer, and then command the motors after stepping primitive executor.
Food for thought
There was a problem hiding this comment.
I agree with that. I will open a ticket about current robot localizer issues including:
- RTT time being fixed to 15ms
- Lack of real robot testing :(
- seperating motor poll and motor update
On the note at this we might as well rename thepollmethods to smth more indicative. For now, I will revert the imu polling to after update robot localizer, so the time is at least consistent.
| getAngularVelocity()); | ||
| } | ||
|
|
||
| // TODO: Investigate process models/variances/etc |
There was a problem hiding this comment.
I'm not a fan of TODOs in-code tbh because we never clean them up lol. If you must put a TODO, attach an issue number so we can check (in the future) if it's complete or not and get rid of the TODO
There was a problem hiding this comment.
I'll just remove this. The process model and varaicnes seems to be fine and we can debug this if robot testing ends up bad
Description
This PR is the base of the "redesign robot localizer" PR stack. It fixes the following issues:
FilterStepstruct and checking for them to determine update/prediction, we store an enum that tells us what type of step this was.Testing Done
Resolved Issues
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue