Skip to content
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

fix(vehicle_cmd_gate): fix publisher HZ in the unit test #6661

Closed

Conversation

takayuki5168
Copy link
Contributor

@takayuki5168 takayuki5168 commented Mar 21, 2024

Description

Related to #6612.
Based on the following analysis and suggestion by @xmfcx, the unit test in the vehicle_cmd_gate uses more precise 100Hz publisher.
#6612 (comment)

Tests performed

Unit test

Effects on system behavior

Nothing

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Takayuki Murooka <takayuki5168@gmail.com>
@github-actions github-actions bot added the component:control Vehicle control algorithms and mechanisms. (auto-assigned) label Mar 21, 2024
@takayuki5168 takayuki5168 marked this pull request as ready for review March 21, 2024 04:23
@takayuki5168 takayuki5168 requested a review from xmfcx March 21, 2024 04:23
@takayuki5168 takayuki5168 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Mar 21, 2024
@xmfcx
Copy link
Contributor

xmfcx commented Mar 21, 2024

@takayuki5168 the updates look good, thanks.

It seems the tests are still failing:

constexpr auto threshold_scale = 1.1;
if (std::abs(lon_vel) > 0.01) {
ASSERT_LT_NEAR(std::abs(lon_acc), max_lon_acc_lim, threshold_scale);
ASSERT_LT_NEAR(std::abs(lon_jerk), max_lon_jerk_lim, threshold_scale);
ASSERT_LT_NEAR(std::abs(lat_acc), max_lat_acc_lim, threshold_scale);
ASSERT_LT_NEAR(std::abs(lat_jerk), max_lat_jerk_lim, threshold_scale);
}

It might make sense to just increase threshold_scale to 1.2 or 1.3, but I'm not sure if this contradicts with the test itself.

@xmfcx
Copy link
Contributor

xmfcx commented Mar 21, 2024

const auto dt = (cmd_received_times_.at(i_curr) - cmd_received_times_.at(i_prev)).seconds();
const auto lon_vel = cmd_curr->longitudinal.speed;
const auto lon_acc = cmd_curr->longitudinal.acceleration;
const auto lon_jerk = (lon_acc - cmd_prev->longitudinal.acceleration) / dt;

In the current implementation, acceleration and jerk are calculated based on instantaneous measurements, leading to potential abrupt changes due to the high sensitivity to small fluctuations in data.

Especially for these 1nd and 2rd order derivatives of speed.

To achieve smoother and more reliable readings, I suggest averaging these values over a 20-30 ms window.

This method will help reduce noise and better reflect the vehicle's dynamics, improving the test's reliability.

@xmfcx
Copy link
Contributor

xmfcx commented Mar 21, 2024

To achieve smoother and more reliable readings, I suggest averaging these values over a 20-30 ms window.

@takayuki5168 -san, I've implemented this in:

I hope it works. (It is continued from your commit)

@xmfcx
Copy link
Contributor

xmfcx commented Mar 26, 2024

Closing this since:

includes the changes in this PR and is merged.

@xmfcx xmfcx closed this Mar 26, 2024
@takayuki5168 takayuki5168 deleted the fix/vehicle-cmd-gate-test branch March 26, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:control Vehicle control algorithms and mechanisms. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants