Conversation
e632ecc
to
37e4bb7
Compare
I fixed all the unit tests and placed the modifications in the commit that breaks them: 99f2e33 |
This PR started to have too many changes so I pulled the sensor_simulator bugfix in #838 |
Extracted the aid bitmask feedback to #839 |
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.
Nice, thanks a lot for the fixes too. I really thought I got the signs for the flow right.
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.
Cool, nice work. I am happy that you also start to use the test frame work regularly.
@@ -89,8 +89,8 @@ TEST_F(EkfFlowTest, resetToFlowVelocityInAir) | |||
const Vector2f simulated_horz_velocity(0.5f, -0.2f); | |||
flowSample flow_sample = _sensor_simulator._flow.dataAtRest(); | |||
flow_sample.flow_xy_rad = | |||
Vector2f(- simulated_horz_velocity(1) * flow_sample.dt / estimated_distance_to_ground, | |||
simulated_horz_velocity(0) * flow_sample.dt / estimated_distance_to_ground); | |||
Vector2f( simulated_horz_velocity(1) * flow_sample.dt / estimated_distance_to_ground, |
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.
Do you know why this was passing the test before even tough I got the signs wrong.
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.
Yes, because the test was just for the reset counter. There is still a bug in the flow reset that always initializes with zero. The value isn't important.
EXPECT_TRUE(_ekf_wrapper.isIntendingTerrainFlowFusion()); | ||
|
||
const float estimated_distance_to_ground = _ekf->getTerrainVertPos(); | ||
EXPECT_NEAR(estimated_distance_to_ground, flow_height, 0.5f); |
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.
Do you have an idea why the error is here higher?
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 think this is because the range finder fusion is stronger than the optical flow one. If we let it run more, it will converge to the true value.
@kamilritz Thanks for the review, I'll make the required modifications |
one can set the position rate = velocity to obtain consistent gps data
@kamilritz Updated, please could you review again? |
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.
Thanks for updating.
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.
Awesome work, really happy to see more of the system being able to be tested automatically.
When we track down why the HAGL estimation is weird during fast attitude changes this will be really helpful!
I tried to split the fixes properly in different commits.
Summary of the fixes:
get feedback in the log from the aiding sources currently used by the terrain estimator (will require Firmware update to actually log the data)edit: moved to Terrain estimate: Added height above ground source bitmask #839fix sensor simulator that was updating the ekf faster than IMU data (this created instability of the filter in dynamic unit tests) FYI @kamilritzedit: moved to sensor_simulator: update EKF at IMU rate #838Due to theedit: fixed in #838sensor_simulator
update rate fix, a lot of unit tests are now failing (because EKF prediction step runs slower) and I'm currently fixing them.