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

373 unstuck behavior #425

Merged
merged 11 commits into from
Nov 3, 2024
Merged

373 unstuck behavior #425

merged 11 commits into from
Nov 3, 2024

Conversation

bilinskaA
Copy link
Collaborator

@bilinskaA bilinskaA commented Nov 2, 2024

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Does this PR introduce a breaking change?

e.g. is old functionality not usable anymore

Most important changes

Which files functionalities are most important in this PR. On which part should the reviewer be focussed on?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (might be obsolete with CI later on)
  • New and existing unit tests pass locally with my changes (might be obsolete with CI later on)

Summary by CodeRabbit

  • Documentation
    • Introduced a comprehensive analysis of the vehicle's unstuck and overtake behaviors, highlighting critical issues and potential improvements.
    • Added new methods related to behavior management and trajectory changes for better vehicle navigation.
  • New Features
    • Enhanced vehicle capabilities for managing unstuck scenarios and overtaking maneuvers.

@bilinskaA bilinskaA linked an issue Nov 2, 2024 that may be closed by this pull request
Copy link
Contributor

coderabbitai bot commented Nov 2, 2024

Walkthrough

The changes in this pull request involve updates to the document "Unstuck/Overtake Behavior," which analyzes the vehicle's behaviors related to becoming unstuck and overtaking other vehicles. The document details critical issues in the current implementation, introduces new methods for managing behaviors, and suggests improvements for trajectory planning and collision detection. Key methods added include __set_curr_behavior, change_trajectory, and overtake_fallback, which enhance the vehicle's ability to navigate obstacles and manage overtaking maneuvers effectively.

Changes

File Path Change Summary
doc/research/paf24/planning/Unstuck_Overtake Behavior.md Added methods: __set_curr_behavior(self, data: String), change_trajectory(self, distance_obj), and overtake_fallback(self, distance, pose_list, unstuck=False). Document details vehicle behaviors, issues, and suggestions for improvements.

Poem

In the land of speed and grace,
A rabbit hops to find its place.
With new paths to weave and steer,
Overtake and unstuck, have no fear!
Through obstacles, we’ll dance and play,
With every hop, we find our way! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (8)
doc/research/paf24/planning/Unstuck_Overtake Behavior.md (4)

1-22: Document structure is well-organized but needs completion.

The table of contents provides a clear roadmap of the document. However, there are a few areas for improvement:

  • The document lacks version information or last updated date
  • Some sections like "Relevant rostopics" are incomplete

Consider adding:

+# Unstuck/Overtake Behavior
+
+Version: 1.0
+Last Updated: November 2024

60-62: Complete the rostopics section.

The TODO comment indicates incomplete documentation of relevant rostopics, which are crucial for understanding system integration.

Would you like me to help create a comprehensive list of rostopics by analyzing the codebase? I can create a script to search for all relevant topic publications and subscriptions.


86-88: Important architectural questions need answers.

The questions raised about behavior status and overtake validation are critical for understanding the system's control flow. These questions should be answered in the documentation to prevent potential implementation issues.

Consider adding a sequence diagram to illustrate:

  1. The behavior status flow between components
  2. The validation steps during overtake initiation

141-147: Comprehensive improvement suggestions need prioritization.

The potential improvements section provides valuable suggestions but lacks prioritization and implementation details. The suggestions span multiple critical areas:

  • Trajectory planning
  • Dynamic offset calculations
  • Emergency handling
  • Speed control
  • Corner point calculation

Consider:

  1. Prioritizing these improvements based on safety impact
  2. Adding specific acceptance criteria for each improvement
  3. Breaking down larger improvements into smaller, manageable tasks
🧰 Tools
🪛 LanguageTool

[uncategorized] ~141-~141: Loose punctuation mark.
Context: ... improvements - def change_trajectory: Consider implementing a more sophistica...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~142-~142: Loose punctuation mark.
Context: ...oming traffic. - def overtake_fallback: Instead of fixed offsets for unstuck an...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~143-~143: Loose punctuation mark.
Context: ...ffic conditions. - __get_speed_unstuck: Include checks for nearby vehicles and ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~144-~144: Loose punctuation mark.
Context: ...DISTANCEdynamic. -__check_emergency`: Currently, it only considers whether th...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~145-~145: Loose punctuation mark.
Context: ...or rerouting). - get_speed_by_behavior: Consider feedback from sensors regardin...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~146-~146: Loose punctuation mark.
Context: ...fic conditions. - __calc_corner_points: Consider a more intelligent approach ra...

(UNLIKELY_OPENING_PUNCTUATION)

code/planning/src/local_planner/motion_planning.py (4)

Line range hint 243-284: Refactor overtake_fallback for better maintainability and safety.

The method needs several improvements:

  1. Extract magic numbers into named constants
  2. Split the complex logic into smaller functions
  3. Add bounds checking for array operations
+# At class level
+NORMAL_OVERTAKE_OFFSET = 2
+UNSTUCK_OVERTAKE_OFFSET = 3
+WAYPOINT_BUFFER = 2

 def overtake_fallback(self, distance, pose_list, unstuck=False):
+    if not pose_list:
+        raise ValueError("Empty pose list provided")
+
+    currentwp = self._validate_waypoint_index(self.current_wp)
-    normal_x_offset = 2
-    unstuck_x_offset = 3
+    x_offset = UNSTUCK_OVERTAKE_OFFSET if unstuck else NORMAL_OVERTAKE_OFFSET
+    
+    selection = self._select_waypoints(currentwp, distance, pose_list, unstuck)
+    waypoints = self.convert_pose_to_array(selection)
+    waypoints_off = self._calculate_offset_waypoints(waypoints, x_offset)

Consider extracting the waypoint selection logic:

def _select_waypoints(self, currentwp, distance, pose_list, unstuck):
    if unstuck:
        start_idx = max(0, int(currentwp) - WAYPOINT_BUFFER)
        end_idx = min(len(pose_list), int(currentwp) + int(distance) + WAYPOINT_BUFFER + NUM_WAYPOINTS)
    else:
        start_idx = int(currentwp) + int(distance / 2)
        end_idx = min(len(pose_list), int(currentwp) + int(distance) + NUM_WAYPOINTS)
    return pose_list[start_idx:end_idx]

Line range hint 285-299: Add safety checks for path reconstruction.

The path reconstruction logic needs validation to prevent index out of bounds errors.

     path = Path()
     path.header.stamp = rospy.Time.now()
     path.header.frame_id = "global"
+    
+    def safe_slice(poses, start, end):
+        start = max(0, start)
+        end = min(len(poses), end)
+        return poses[start:end]
+    
     if unstuck:
-        path.poses = (
-            pose_list[: int(currentwp) - 2]
-            + result
-            + pose_list[int(currentwp) + int(distance) + 2 + NUM_WAYPOINTS :]
-        )
+        prefix = safe_slice(pose_list, 0, int(currentwp) - 2)
+        suffix = safe_slice(pose_list, int(currentwp) + int(distance) + 2 + NUM_WAYPOINTS, None)
+        path.poses = prefix + result + suffix

446-450: Improve robustness of overtake initiation checks.

The collision point check could be more robust, and the status management could be more explicit.

-    if data.data == bs.ot_enter_init.name:
-        if np.isinf(self.__collision_point):
-            self.__overtake_status = -1
-            self.overtake_success_pub.publish(self.__overtake_status)
-            return
-        self.change_trajectory(self.__collision_point)
+    if data.data == bs.ot_enter_init.name:
+        if not self._is_valid_collision_point():
+            self._cancel_overtake("No valid collision point detected")
+            return
+        try:
+            self.change_trajectory(self.__collision_point)
+        except Exception as e:
+            self._cancel_overtake(f"Failed to initiate overtake: {e}")
+
+def _is_valid_collision_point(self):
+    return self.__collision_point is not None and not np.isinf(self.__collision_point)
+
+def _cancel_overtake(self, reason):
+    self.logwarn(reason)
+    self.__overtake_status = -1
+    self.overtake_success_pub.publish(self.__overtake_status)

Line range hint 485-522: Enhance unstuck behavior implementation.

The unstuck behavior implementation could benefit from several improvements:

  1. Move the global constant to class level
  2. Extract complex conditions into named methods
  3. Add comprehensive logging
-global UNSTUCK_OVERTAKE_FLAG_CLEAR_DISTANCE
+# At class level
+UNSTUCK_OVERTAKE_CLEAR_DISTANCE = 7.0
+MIN_OBSTACLE_DISTANCE = 6.0  # meters

 def __get_speed_unstuck(self, behavior: str) -> float:
-    global UNSTUCK_OVERTAKE_FLAG_CLEAR_DISTANCE
     speed = 0.0
     if behavior == bs.us_unstuck.name:
         speed = bs.us_unstuck.speed
     elif behavior == bs.us_stop.name:
         speed = bs.us_stop.speed
     elif behavior == bs.us_overtake.name:
         pose_list = self.trajectory.poses
 
         if self.unstuck_distance is None:
-            self.logfatal("Unstuck distance not set")
+            self.logerr("Cannot plan unstuck maneuver: distance to obstacle unknown")
             return speed
 
-        if self.init_overtake_pos is not None and self.current_pos is not None:
-            distance = np.linalg.norm(
-                self.init_overtake_pos[:2] - self.current_pos[:2]
-            )
-            if distance > UNSTUCK_OVERTAKE_FLAG_CLEAR_DISTANCE:
-                self.unstuck_overtake_flag = False
-                self.logwarn("Unstuck Overtake Flag Cleared")
+        if self._should_clear_unstuck_flag():
+            self._clear_unstuck_flag()
 
         if self.unstuck_overtake_flag is False:
-            self.overtake_fallback(self.unstuck_distance, pose_list, unstuck=True)
-            self.logfatal("Overtake Trajectory while unstuck!")
-            self.unstuck_overtake_flag = True
-            self.init_overtake_pos = self.current_pos[:2]
+            self._initiate_unstuck_overtake(pose_list)
 
         speed = bs.us_overtake.speed
         return speed

+def _should_clear_unstuck_flag(self):
+    return (self.init_overtake_pos is not None and 
+            self.current_pos is not None and 
+            np.linalg.norm(self.init_overtake_pos[:2] - self.current_pos[:2]) > 
+            self.UNSTUCK_OVERTAKE_CLEAR_DISTANCE)
+
+def _clear_unstuck_flag(self):
+    self.unstuck_overtake_flag = False
+    self.loginfo("Cleared unstuck overtake flag: sufficient distance covered")
+
+def _initiate_unstuck_overtake(self, pose_list):
+    self.loginfo(f"Initiating unstuck overtake maneuver at distance: {self.unstuck_distance}")
+    self.overtake_fallback(self.unstuck_distance, pose_list, unstuck=True)
+    self.unstuck_overtake_flag = True
+    self.init_overtake_pos = self.current_pos[:2]
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cd6c47a and 8ff02f1.

📒 Files selected for processing (2)
  • code/planning/src/local_planner/motion_planning.py (7 hunks)
  • doc/research/paf24/planning/Unstuck_Overtake Behavior.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/planning/Unstuck_Overtake Behavior.md

[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ey components - self.unstuck_distance: Tracks the distance to the object causi...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...required. - self.unstuck_overtake_flag: Prevents repeated or "spam" overtakes b...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...R_DISTANCE). - self.init_overtake_pos`: Saves the position where the unstuck be...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...y components - self.__overtake_status: Manages the status of overtaking, where...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~49-~49: Loose punctuation mark.
Context: ...ng isn't required. - change_trajectory: Initiates the overtake by calling `over...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~50-~50: Loose punctuation mark.
Context: ...ems of the change. - overtake_fallback: Calculates a new trajectory path that o...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~81-~81: Loose punctuation mark.
Context: ...ake?" - self.__overtake_status = -1: Unsuccessful or canceled overtake - `...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~141-~141: Loose punctuation mark.
Context: ... improvements - def change_trajectory: Consider implementing a more sophistica...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~142-~142: Loose punctuation mark.
Context: ...oming traffic. - def overtake_fallback: Instead of fixed offsets for unstuck an...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~143-~143: Loose punctuation mark.
Context: ...ffic conditions. - __get_speed_unstuck: Include checks for nearby vehicles and ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~144-~144: Loose punctuation mark.
Context: ...DISTANCEdynamic. -__check_emergency`: Currently, it only considers whether th...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~145-~145: Loose punctuation mark.
Context: ...or rerouting). - get_speed_by_behavior: Consider feedback from sensors regardin...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~146-~146: Loose punctuation mark.
Context: ...fic conditions. - __calc_corner_points: Consider a more intelligent approach ra...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 Markdownlint
doc/research/paf24/planning/Unstuck_Overtake Behavior.md

38-38: Expected: 300; Actual: 365
Line length

(MD013, line-length)


54-54: Expected: 300; Actual: 434
Line length

(MD013, line-length)


46-46: null
Multiple headings with the same content

(MD024, no-duplicate-heading)

🔇 Additional comments (1)
doc/research/paf24/planning/Unstuck_Overtake Behavior.md (1)

99-101: Clarify overtake success criteria.

The documentation raises a valid concern about the ambiguity of overtake success status. This needs to be addressed to ensure proper system monitoring and debugging.

Let's verify the current implementation:

Comment on lines +117 to +128
### Aggressive lane changes

The vehicle exhibits aggressive lane changes, leading to emergency stops to avoid oncoming traffic. This behavior suggests that the decision-making logic for overtaking does not adequately assess the surrounding traffic conditions before executing maneuvers.

### Inadequate collision detection

The vehicle fails to avoid obstacles, such as open car doors or stationary vehicles, indicating that the collision detection mechanisms are either insufficient or not effectively integrated into the overtaking logic.

### Improper trajectory planning

The trajectory generated for overtaking often leads to incorrect paths, such as attempting to overtake trees or parked cars without considering oncoming traffic. A better filtering and validation of potential overtaking targets is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical safety issues need immediate attention.

The documented issues with aggressive lane changes, inadequate collision detection, and improper trajectory planning present significant safety risks:

  1. Emergency stops due to aggressive lane changes
  2. Failure to avoid obstacles
  3. Incorrect path planning around static objects

These issues require immediate attention and should be tracked with high-priority tickets. Consider implementing:

  1. A pre-overtake validation phase
  2. Enhanced obstacle classification
  3. Multi-stage trajectory planning with safety checks

Would you like me to create GitHub issues for tracking these safety-critical improvements?

Comment on lines 235 to 242
pose_list = self.trajectory.poses #vehicle's planned path.

# Only use fallback
self.overtake_fallback(distance_obj, pose_list)
self.__overtake_status = 1
self.overtake_fallback(distance_obj, pose_list) #likely implements a simpler, predefined maneuver for overtaking, which adjusts the trajectory if more sophisticated planning is unavailable or as a safety fallback option.
self.__overtake_status = 1 #overtake successfully planed
self.overtake_success_pub.publish(self.__overtake_status)
return

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and validation in change_trajectory method.

The method needs additional safeguards:

  1. Validate pose_list is not empty
  2. Verify the success of overtake_fallback before setting __overtake_status
  3. Add error handling for potential exceptions during trajectory modification
 def change_trajectory(self, distance_obj):
+    if not self.trajectory or not self.trajectory.poses:
+        self.logwarn("No trajectory available for modification")
+        return
     pose_list = self.trajectory.poses
 
-    self.overtake_fallback(distance_obj, pose_list)
-    self.__overtake_status = 1
+    try:
+        self.overtake_fallback(distance_obj, pose_list)
+        self.__overtake_status = 1
+    except Exception as e:
+        self.logerr(f"Failed to modify trajectory: {e}")
+        self.__overtake_status = -1
     self.overtake_success_pub.publish(self.__overtake_status)
     return

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
doc/research/paf24/planning/Unstuck_Overtake Behavior.md (1)

68-70: Complete the rostopics section

The TODO comment indicates this section is incomplete. Please add all relevant rostopics for comprehensive documentation.

Would you like me to help compile a complete list of rostopics related to the unstuck/overtake behaviors?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between dfe78a8 and 968ad94.

📒 Files selected for processing (1)
  • doc/research/paf24/planning/Unstuck_Overtake Behavior.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/research/paf24/planning/Unstuck_Overtake Behavior.md

[uncategorized] ~31-~31: Loose punctuation mark.
Context: ...ey components - self.unstuck_distance: Tracks the distance to the object causi...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~32-~32: Loose punctuation mark.
Context: ...required. - self.unstuck_overtake_flag: Prevents repeated or "spam" overtakes b...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...R_DISTANCE). - self.init_overtake_pos`: Saves the position where the unstuck be...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...y components - self.__overtake_status: Manages the status of overtaking, where...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~53-~53: Loose punctuation mark.
Context: ...ng isn't required. - change_trajectory: Initiates the overtake by calling `over...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~54-~54: Loose punctuation mark.
Context: ...ems of the change. - overtake_fallback: Calculates a new trajectory path that o...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~89-~89: Loose punctuation mark.
Context: ...ake?" - self.__overtake_status = -1: Unsuccessful or canceled overtake - `...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~149-~149: Loose punctuation mark.
Context: ... improvements - def change_trajectory: Consider implementing a more sophistica...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~150-~150: Loose punctuation mark.
Context: ...oming traffic. - def overtake_fallback: Instead of fixed offsets for unstuck an...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~151-~151: Loose punctuation mark.
Context: ...ffic conditions. - __get_speed_unstuck: Include checks for nearby vehicles and ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...DISTANCEdynamic. -__check_emergency`: Currently, it only considers whether th...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~153-~153: Loose punctuation mark.
Context: ...or rerouting). - get_speed_by_behavior: Consider feedback from sensors regardin...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~154-~154: Loose punctuation mark.
Context: ...fic conditions. - __calc_corner_points: Consider a more intelligent approach ra...

(UNLIKELY_OPENING_PUNCTUATION)

🪛 Markdownlint
doc/research/paf24/planning/Unstuck_Overtake Behavior.md

50-50: null
Multiple headings with the same content

(MD024, no-duplicate-heading)

🔇 Additional comments (3)
doc/research/paf24/planning/Unstuck_Overtake Behavior.md (3)

15-15: ⚠️ Potential issue

Update method name for consistency

As noted in a previous review, the method name overtake_fallback has been changed to generate_overtake_trajectory in the codebase. Please update all occurrences in the documentation to maintain consistency.

Also applies to: 34-34, 54-54, 112-119


152-152: ⚠️ Potential issue

Remove or implement unused emergency check

The __check_emergency method is currently unused (issue #413). Either implement the method with proper emergency condition checks or remove it from the documentation.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...DISTANCEdynamic. -__check_emergency`: Currently, it only considers whether th...

(UNLIKELY_OPENING_PUNCTUATION)


125-136: ⚠️ Potential issue

Prioritize addressing critical safety issues

The documented safety concerns are significant and require immediate attention:

  1. Aggressive lane changes causing emergency stops
  2. Insufficient collision detection
  3. Incorrect trajectory planning

These issues align with the previous review comments and present serious safety risks. Consider adding specific mitigation strategies and acceptance criteria for each issue.

Let's verify the frequency and severity of these issues:

✅ Verification successful

Safety concerns are accurately documented and implementation evidence confirms the issues

The documented safety concerns are verified through the codebase evidence:

  1. The aggressive lane changes and emergency stops are confirmed in multiple documents:

    • doc/research/paf24/general/current_state.md: "Lane change planning is overly aggressive, causing the vehicle to abruptly veer, triggering emergency stops"
    • Implementation in code/planning/src/local_planner/motion_planning.py shows emergency stop handling
  2. Collision detection issues are reflected in the implementation:

    • doc/planning/Collision_Check.md shows the collision detection system exists but has limitations
    • The emergency handling in code/acting/src/acting/vehicle_controller.py confirms reactive rather than preventive measures
  3. The overtaking behavior implementation in code/planning/src/behavior_agent/behaviours/overtake.py lacks robust safety checks mentioned in the documentation.

The review comment's concerns are valid and supported by multiple code artifacts. Continue with the proposed mitigation strategies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related error logs and emergency stops
rg -i "emergency.*stop|collision.*detect|aggressive.*lane" --type=log

# Look for related issue reports
gh issue list --label "safety,critical" --state open

Length of output: 273


Script:

#!/bin/bash
# Search for safety-related patterns in the codebase
rg -i "emergency.*stop|collision.*detect|aggressive.*lane|overtake.*behavior"

# Search for test files related to overtaking
fd -e test.js -e test.ts -e spec.js -e spec.ts | grep -i "overtake"

# Search for implementation files related to overtaking
fd -e js -e ts -e py -e cpp | grep -i "overtake"

# Look for configuration or parameter files related to overtaking
fd -e yaml -e json -e config | grep -i "overtake"

# Search for related documentation
fd -e md | grep -i "overtake"

Length of output: 6909

Comment on lines +147 to +155
## Potential improvements

- `def change_trajectory`: Consider implementing a more sophisticated trajectory planning algorithm that considers dynamic obstacles and traffic conditions rather than relying on a fallback method. Verify that the area into which the vehicle is moving is clear of obstacles and oncoming traffic.
- `def overtake_fallback`: Instead of fixed offsets for unstuck and normal situations, consider dynamically calculating offsets based on current speed, vehicle dimensions, and surrounding traffic conditions.
- `__get_speed_unstuck`: Include checks for nearby vehicles and their speeds. Make `UNSTUCK_OVERTAKE_FLAG_CLEAR_DISTANCE` dynamic.
- `__check_emergency`: Currently, it only considers whether the vehicle is in a parking behavior state. Expand this method to evaluate various emergency conditions (e.g., obstacles detected by sensors) and initiate appropriate responses (e.g., stopping or rerouting).
- `get_speed_by_behavior`: Consider feedback from sensors regarding current traffic conditions.
- `__calc_corner_points`: Consider a more intelligent approach rather than relying on simple angle thresholds.

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance improvement suggestions with concrete implementation details

The potential improvements section would benefit from:

  1. Specific implementation examples for each suggestion
  2. Acceptance criteria for improvements
  3. Priority levels for each enhancement
  4. Dependencies between improvements

Consider structuring this section as a roadmap with clear milestones and success metrics.

Would you like me to help create a detailed implementation roadmap for these improvements?

🧰 Tools
🪛 LanguageTool

[uncategorized] ~149-~149: Loose punctuation mark.
Context: ... improvements - def change_trajectory: Consider implementing a more sophistica...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~150-~150: Loose punctuation mark.
Context: ...oming traffic. - def overtake_fallback: Instead of fixed offsets for unstuck an...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~151-~151: Loose punctuation mark.
Context: ...ffic conditions. - __get_speed_unstuck: Include checks for nearby vehicles and ...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~152-~152: Loose punctuation mark.
Context: ...DISTANCEdynamic. -__check_emergency`: Currently, it only considers whether th...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~153-~153: Loose punctuation mark.
Context: ...or rerouting). - get_speed_by_behavior: Consider feedback from sensors regardin...

(UNLIKELY_OPENING_PUNCTUATION)


[uncategorized] ~154-~154: Loose punctuation mark.
Context: ...fic conditions. - __calc_corner_points: Consider a more intelligent approach ra...

(UNLIKELY_OPENING_PUNCTUATION)

- [Methods overview](#methods-overview)
- [def \_\_set_curr_behavior](#def-set_curr_behavior)
- [def change_trajectory](#def-change_trajectory)
- [def overtake_fallback](#def-overtake_fallback)
Copy link
Collaborator

@niklasr22 niklasr22 Nov 3, 2024

Choose a reason for hiding this comment

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

When documenting the motion_planning.py file, I changed the method name from overtake_fallback to generate_overtake_trajectory to better reflect the actual behavior of the method. So it might be good to replace all of the occurrences here as well

- `def change_trajectory`: Consider implementing a more sophisticated trajectory planning algorithm that considers dynamic obstacles and traffic conditions rather than relying on a fallback method. Verify that the area into which the vehicle is moving is clear of obstacles and oncoming traffic.
- `def overtake_fallback`: Instead of fixed offsets for unstuck and normal situations, consider dynamically calculating offsets based on current speed, vehicle dimensions, and surrounding traffic conditions.
- `__get_speed_unstuck`: Include checks for nearby vehicles and their speeds. Make `UNSTUCK_OVERTAKE_FLAG_CLEAR_DISTANCE` dynamic.
- `__check_emergency`: Currently, it only considers whether the vehicle is in a parking behavior state. Expand this method to evaluate various emergency conditions (e.g., obstacles detected by sensors) and initiate appropriate responses (e.g., stopping or rerouting).
Copy link
Collaborator

@niklasr22 niklasr22 Nov 3, 2024

Choose a reason for hiding this comment

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

__check_emergency never gets called (#413) and i think eventually, we should remove it entirely. But for now I think it is ok to mention it here

Copy link
Collaborator

@niklasr22 niklasr22 left a comment

Choose a reason for hiding this comment

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

Good job

@niklasr22 niklasr22 added this to the Sprint 00 2024-11-04 milestone Nov 3, 2024
@bilinskaA bilinskaA closed this Nov 3, 2024
@bilinskaA bilinskaA reopened this Nov 3, 2024
@bilinskaA bilinskaA merged commit d73d457 into main Nov 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unstuck behavior
2 participants