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 lanechange start_llt_id picking logic #2397

Merged
merged 12 commits into from
Jun 17, 2024
Merged

Conversation

MishkaMN
Copy link
Contributor

@MishkaMN MishkaMN commented Jun 4, 2024

PR Details

Description

Main change is the plan_delegator is not filtering lanelet on shortest path when determining previous lanelet for cooperative lanechange maneuver. This makes the tactical plugin not able to generate trajectory because it may pick a lanelet that doesn't have adjacent lanelet for example in an intersection. Therefore, it needs to pick a lanelet on the actual shortest path.

Also since filtering a lanelet that's on the shortest path functionality is often repeated and needed, I refactored it out to carma_wm.

Related GitHub Issue

resolves: #2391

Related Jira Key

https://usdot-carma.atlassian.net/browse/CAR-6050

Motivation and Context

Discovered during unsignalized workzone demo for 06/11/2024. Please see the issue.

How Has This Been Tested?

Tested on passenger vehicles on TFHRC with testing environment enabled for unsignalized workzone. checked out from carma-system-4.5.0

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@MishkaMN MishkaMN changed the title Car 6037 unsig workzone fix lanechange start_llt_id picking logic Jun 4, 2024
@MishkaMN MishkaMN requested review from JonSmet and adev4a June 4, 2024 18:25
@MishkaMN MishkaMN self-assigned this Jun 4, 2024
@MishkaMN MishkaMN added the anomaly Something isn't working label Jun 4, 2024
carma_wm/include/carma_wm/WorldModel.hpp Outdated Show resolved Hide resolved
carma_wm/src/CARMAWorldModel.cpp Outdated Show resolved Hide resolved
lci_strategic_plugin/src/lci_strategic_plugin.cpp Outdated Show resolved Hide resolved
plan_delegator/src/plan_delegator.cpp Outdated Show resolved Hide resolved
@MishkaMN MishkaMN requested review from adev4a and JonSmet June 6, 2024 20:17
@MishkaMN MishkaMN requested a review from JonSmet June 10, 2024 13:29
JonSmet
JonSmet previously approved these changes Jun 10, 2024
Copy link
Contributor

@JonSmet JonSmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but just noticed that the 'CI: Run tests' workflow is failing due to the cpplint checks on motion_computation after PR #2399. My guess is that these edits might have been done in an editor without settings for removing whitespace, etc. in VS Code. Would you mind taking a look and we can address it in a separate PR?

adev4a
adev4a previously approved these changes Jun 11, 2024
@adev4a
Copy link
Contributor

adev4a commented Jun 11, 2024

Looks good to me as well. Linking the error log from the cpplint tests here for tracking https://github.com/usdot-fhwa-stol/carma-platform/actions/runs/9449278789/job/26037602316?pr=2397#step:13:23837

@MishkaMN
Copy link
Contributor Author

Code coverage says less than 80%, let me try to increase those

@MishkaMN MishkaMN dismissed stale reviews from adev4a and JonSmet via f80dfc7 June 13, 2024 18:03
@MishkaMN MishkaMN requested review from JonSmet and adev4a June 13, 2024 19:52
JonSmet
JonSmet previously approved these changes Jun 14, 2024
@MishkaMN
Copy link
Contributor Author

@JonSmet Is this okay to merge with the above sonarcloud status?

[22.2% Duplication on New Code](https://sonarcloud.io/component_measures?id=usdot-fhwa-stol_CARMAPlatform&pullRequest=2397&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)
 [79.17% Line Coverage on New Code](https://sonarcloud.io/dashboard?id=usdot-fhwa-stol_CARMAPlatform&pullRequest=2397) (required ≥ 80%)

@JonSmet
Copy link
Contributor

JonSmet commented Jun 17, 2024

@JonSmet Is this okay to merge with the above sonarcloud status?

[22.2% Duplication on New Code](https://sonarcloud.io/component_measures?id=usdot-fhwa-stol_CARMAPlatform&pullRequest=2397&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)
 [79.17% Line Coverage on New Code](https://sonarcloud.io/dashboard?id=usdot-fhwa-stol_CARMAPlatform&pullRequest=2397) (required ≥ 80%)

I'm okay with this code duplication since it is a very small code block. Additionally, the unit test coverage is failing our target (80%) because of a temporarily disabled unit test in light_controlled_intersection_tactical_plugin, so that is fine as well. I did just notice that lci_strategic_plugin code does not seem to be analyzed by sonar, would you mind taking a brief look at this?

Copy link

sonarcloud bot commented Jun 17, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
20.5% Duplication on New Code (required ≤ 3%)
65.52% Line Coverage on New Code (required ≥ 80%)
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Contributor

@JonSmet JonSmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the mention of the temporarily disabled light_controlled_intersection_tactical_plugin unit test, there is a similar disabled test for lci_strategic_plugin. These unit test fixes are being captured in a separate effort, and will improve the test coverage for these code changes.

@MishkaMN MishkaMN merged commit 05902d4 into develop Jun 17, 2024
3 of 4 checks passed
@MishkaMN MishkaMN deleted the car-6037-unsig-workzone branch June 17, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plan_delegator is assigning arbitrary lanelet_ids when adjusting maneuver's starting distance
3 participants