Check yaw rates instead of positions when yoyoing #124
Conversation
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
This PR depends on #89, #96. Recommended review order is #89 > #96 > this PR. If we want I can cherry pick this branch as the changes are independent of the underlying code. The merge order should be this PR > #96 > #89. There is a slight change in the center of rotation of the yoyo mission with #96. This leads to test expectations failing. Rather than use position to check whether the vehicle is yoyoing, I think we should use @tfoote's suggestion and use yaw rate which makes the test independent of the center of rotation and hence more robust while at the same time ensuring that the vehicle actually moves in a circle. Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
There was a problem hiding this comment.
I think this is ok, just a couple of questions
yaw rate which makes the test independent of the center of rotation and hence more robust
Is that more robust because future dynamics updates won't affect the yaw rate? I can imagine fixes to lift/drag changing the forces on the rudder, which may change the yaw rate for example.
ensuring that the vehicle actually moves in a circle.
The tolerance for the yaw rate now is in the same order of magnitude than the rate itself (0.037 +- 0.02). So we should just make sure that we're not actually regressing and that's why we're not getting a circle anymore.
| if (i > 6000) | ||
| { | ||
| // Once the vehicle achieves its full velocity the vehicle should have a | ||
| // nominal yaw rate of around 0.37-0.38rad/s. This means that the vehicle |
There was a problem hiding this comment.
Was this number chosen empirically through the test or is this something we got from the real vehicle?
There was a problem hiding this comment.
I believe I used empirical means. But we should verify the actual rate with @braanan. BTW its missing a few zeros above.
There was a problem hiding this comment.
Gotcha. It would be good to document this on the comment so we know in the future that this isn't a target that we're trying to get to, just what we've observed so far. This is in contrast with the 1 m/s velocity for example, which is an actual target
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Thanks for spotting this 🤦♂️ . Yeah we don't need search tolerances I forgot to bump the tolerance back down. its now 0.037 +/- 0.002.
I do not believe yaw rate should change. The reason is that for this regime, the current |
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
|
I cherry-picked this PR onto |
…o main) (#133) * bump depth expectations Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org> * Check yaw rates instead of positions when yoyoing This PR depends on #89, #96. Recommended review order is #89 > #96 > this PR. If we want I can cherry pick this branch as the changes are independent of the underlying code. The merge order should be this PR > #96 > #89. There is a slight change in the center of rotation of the yoyo mission with #96. This leads to test expectations failing. Rather than use position to check whether the vehicle is yoyoing, I think we should use @tfoote's suggestion and use yaw rate which makes the test independent of the center of rotation and hence more robust while at the same time ensuring that the vehicle actually moves in a circle. Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org> * remove unused variable Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org> * Fix tolerances. :man-facepalming: Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org> * increase ramp up time Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org> * Fix bad merge and increase tolerance Signed-off-by: Louise Poubel <louise@openrobotics.org> Co-authored-by: Arjo Chakravarty <arjo@openrobotics.org>
…atch #124 Signed-off-by: Louise Poubel <louise@openrobotics.org>
This PR depends on #89, #96.
Recommended review order is #89 > #96 > this PR. If we want I can cherry pick this branch as the changes are independent of the underlying code. The merge order should be this PR > #96 > #89.
There is a slight change in the center of rotation of the yoyo mission with #96. This leads to test expectations failing. Rather than use position to check whether the vehicle is yoyoing, I think we should use @tfoote's suggestion and use yaw rate which makes the test independent of the center of rotation and hence more robust while at the same time ensuring that the vehicle actually moves in a circle.