#362 research current architecture of planning component #417
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
doc/planning/README.md (1)
13-16: Improve readability with proper punctuation and clearer structure.The description accurately reflects the global planner's responsibilities, but could benefit from better punctuation and structure.
Apply these changes to improve readability:
-The global planner is responsible for collecting and preparing all data from the leaderboard and other internal -components that is needed for the preplanning component. -After finishing that this node initiates the calculation of a trajectory based on the OpenDriveConverter -from preplanning_trajectory.py. In the end the computed trajectory and prevailing speed limits are published. +The global planner is responsible for collecting and preparing all data from the leaderboard and other internal +components that is needed for the preplanning component. +After finishing that, this node initiates the calculation of a trajectory based on the OpenDriveConverter +from preplanning_trajectory.py. In the end, the computed trajectory and prevailing speed limits are published.🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: Possible missing comma found.
Context: ... preplanning component. After finishing that this node initiates the calculation of ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~16-~16: A comma might be missing here.
Context: ... from preplanning_trajectory.py. In the end the computed trajectory and prevailing ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
doc/research/paf24/planning/local_v_global_planning.md (4)
1-1: Fix typo in title.Correct the spelling of "beteween" to "between".
-# Relation beteween local and global planning +# Relation between local and global planning
12-12: Fix typo in global planning description.Correct the spelling of "consits" to "consists".
-Global planning consits of the PrePlanner Node and the OpenDriveConverter. +Global planning consists of the PrePlanner Node and the OpenDriveConverter.
20-20: Consider breaking down the long line for better readability.The line exceeds the recommended length of 300 characters.
-Local planning consists of the ACC, CollisionCheck and MotionPlanning nodes. It uses information from the Global Planning to make decisions in the local environment of the ego vehicle. It creates a trajectory based on the global trajectory and the current position of the vehicle that is used by the acting component. +Local planning consists of the ACC, CollisionCheck and MotionPlanning nodes. It uses information from the Global Planning to make decisions in the local environment of the ego vehicle. +It creates a trajectory based on the global trajectory and the current position of the vehicle that is used by the acting component.🧰 Tools
🪛 Markdownlint
20-20: Expected: 300; Actual: 317
Line length(MD013, line-length)
31-31: Add missing newline at end of file.Add a newline character at the end of the file to comply with markdown standards.
- receives global trajectory from `/paf/hero/trajectory_global` +🧰 Tools
🪛 Markdownlint
31-31: null
Files should end with a single newline character(MD047, single-trailing-newline)
doc/planning/Global_Planner.md (1)
Line range hint
26-57: Consider enhancing component interaction documentation.Given that this PR is focused on researching the planning component's architecture (issue #362), consider adding more details about:
- How the global planner interacts with pre-planning and local planning phases
- The data flow and dependencies between these components
- Any architectural constraints or considerations
This would align with the research objectives of understanding component interactions.
Would you like help drafting a new section that details these architectural aspects?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: A comma might be missing here.
Context: ...to the ODC. After smaller outliners are removed the x and y coordinates as well as the ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
code/planning/src/global_planner/global_planner.py (1)
Line range hint
1-248: Consider architectural improvements for better separation of concernsWhile the implementation is functional, there are opportunities for architectural improvements:
The
PrePlannerclass currently handles multiple responsibilities:
- OpenDrive map processing
- Trajectory calculation
- ROS topic management
- Coordinate transformations
Consider splitting these responsibilities into separate components:
- A dedicated map processor
- A trajectory calculator service
- A ROS interface handler
This would align with the PR's research objectives by making the planning component's architecture more modular and easier to understand.
Would you like assistance in designing a more modular architecture for these components?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
doc/assets/research_assets/planning_internal.pngis excluded by!**/*.png
📒 Files selected for processing (5)
code/planning/src/global_planner/global_planner.py(2 hunks)doc/planning/Collision_Check.md(1 hunks)doc/planning/Global_Planner.md(3 hunks)doc/planning/README.md(1 hunks)doc/research/paf24/planning/local_v_global_planning.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/planning/Collision_Check.md
🧰 Additional context used
🪛 LanguageTool
doc/planning/README.md
[uncategorized] ~15-~15: Possible missing comma found.
Context: ... preplanning component. After finishing that this node initiates the calculation of ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~16-~16: A comma might be missing here.
Context: ... from preplanning_trajectory.py. In the end the computed trajectory and prevailing ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 Markdownlint
doc/research/paf24/planning/local_v_global_planning.md
20-20: Expected: 300; Actual: 317
Line length
(MD013, line-length)
31-31: null
Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (5)
doc/research/paf24/planning/local_v_global_planning.md (2)
1-31: Documentation effectively explains planning architecture.
The documentation successfully achieves the PR objective of evaluating and documenting the planning component architecture. It clearly explains the relationship between global and local planning components, their responsibilities, and communication patterns via ROS topics.
🧰 Tools
🪛 Markdownlint
20-20: Expected: 300; Actual: 317
Line length
(MD013, line-length)
31-31: null
Files should end with a single newline character
(MD047, single-trailing-newline)
8-8: Verify the referenced image exists.
Let's ensure the image file exists at the specified path.
✅ Verification successful
Image path is correctly referenced and file exists ✓
The image file /doc/assets/research_assets/planning_internal.png exists at the specified location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced image exists
if [ -f "doc/assets/research_assets/planning_internal.png" ]; then
echo "Image exists"
else
echo "Warning: Image not found at doc/assets/research_assets/planning_internal.png"
fi
Length of output: 96
doc/planning/Global_Planner.md (2)
11-15: LGTM! Well-structured table of contents.
The table of contents is clear, hierarchical, and accurately reflects the document structure.
26-26: LGTM! Improved clarity and consistency.
The terminology and sentence structure improvements make the documentation clearer and more professional.
Also applies to: 56-57
code/planning/src/global_planner/global_planner.py (1)
4-12: LGTM: Import reorganization maintains proper grouping
The reorganization of imports follows good practices by grouping standard library, third-party, and local imports while consolidating geometry_msgs imports.
johannaschwarz
force-pushed
the
362-research-current-architecture-of-planning-component
branch
2 times, most recently
from
57343d4 to
2e6729b
Compare
johannaschwarz
force-pushed
the
362-research-current-architecture-of-planning-component
branch
from
2e6729b to
30d12b4
Compare
Co-authored-by: Niklas Rousset <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
doc/planning/README.md (2)
15-18: Improve readability with proper punctuation.The content changes look good, but consider adding commas for better readability:
-The global planner is responsible for collecting and preparing all data from the leaderboard and other internal components that is needed for the preplanning component. After finishing that this node initiates the calculation of a trajectory based on the OpenDriveConverter from preplanning_trajectory.py. In the end the computed trajectory and prevailing speed limits are published. +The global planner is responsible for collecting and preparing all data from the leaderboard and other internal components that is needed for the preplanning component. After finishing that, this node initiates the calculation of a trajectory based on the OpenDriveConverter from preplanning_trajectory.py. In the end, the computed trajectory and prevailing speed limits are published.🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: A comma might be missing here.
Context: ... preplanning component. After finishing that this node initiates the calculation of ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~18-~18: A comma might be missing here.
Context: ... from preplanning_trajectory.py. In the end the computed trajectory and prevailing ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
15-18: Consider adding architectural interaction details.Since this PR focuses on evaluating the planning component's architecture, consider enhancing this section with:
- How the global planner interacts with pre-planning and local planning phases
- The data flow between these components
- Any dependencies or constraints in the interaction
Would you like me to help draft additional documentation sections covering these architectural aspects?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: A comma might be missing here.
Context: ... preplanning component. After finishing that this node initiates the calculation of ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~18-~18: A comma might be missing here.
Context: ... from preplanning_trajectory.py. In the end the computed trajectory and prevailing ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
doc/research/paf24/planning/local_v_global_planning.md (2)
1-1: Fix typo in heading-# Relation beteween local and global planning +# Relation between local and global planning
23-33: Consider standardizing ROS topic formattingWhile the topic documentation is thorough, consider using consistent formatting for ROS topics, perhaps using code formatting (backticks) for better readability.
- receives Speed from `/carla/hero/Speed` + receives Speed from `\`/carla/hero/Speed\``Apply similar formatting to all ROS topics in the document for consistency.
code/planning/src/local_planner/ACC.py (2)
2-9: Remove commented-out import if unused.The
CarlaWorldInfoimport is commented out. If it's no longer needed, please remove it entirely rather than leaving it commented.
Line range hint
190-245: Consider extracting speed calculation logic for better maintainability.The speed calculation logic in the
loopfunction is quite complex. Consider extracting it into separate methods likecalculate_safe_speed_with_obstacleandcalculate_safe_speed_without_obstacleto improve readability and maintainability.Example refactor:
def calculate_safe_speed_with_obstacle(self): safety_distance = calculate_rule_of_thumb(False, self.__current_velocity) if self.obstacle_distance < safety_distance: safe_speed = self.obstacle_speed * (self.obstacle_distance / safety_distance) safe_speed = interpolate_speed(safe_speed, self.__current_velocity) return 0 if safe_speed < 1.0 else safe_speed return 0 if self.__current_velocity < 1.0 else self.__current_velocity def loop(self, timer_event=None): if self.obstacle_distance is not None and self.obstacle_speed is not None and self.__current_velocity is not None: safe_speed = self.calculate_safe_speed_with_obstacle() self.velocity_pub.publish(safe_speed) elif self.speed_limit is not None: self.velocity_pub.publish(self.speed_limit) else: self.velocity_pub.publish(0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
code/planning/src/global_planner/global_planner.py(4 hunks)code/planning/src/global_planner/preplanning_trajectory.py(3 hunks)code/planning/src/local_planner/ACC.py(1 hunks)doc/planning/Global_Planner.md(3 hunks)doc/planning/README.md(1 hunks)doc/research/paf24/planning/local_v_global_planning.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- code/planning/src/global_planner/preplanning_trajectory.py
🚧 Files skipped from review as they are similar to previous changes (2)
- code/planning/src/global_planner/global_planner.py
- doc/planning/Global_Planner.md
🧰 Additional context used
🪛 LanguageTool
doc/planning/README.md
[uncategorized] ~17-~17: A comma might be missing here.
Context: ... preplanning component. After finishing that this node initiates the calculation of ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~18-~18: A comma might be missing here.
Context: ... from preplanning_trajectory.py. In the end the computed trajectory and prevailing ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (7)
doc/research/paf24/planning/local_v_global_planning.md (3)
10-17: LGTM! Clear and accurate description of Global Planning components
The section effectively describes the Global Planning components and their responsibilities, with clear documentation of the ROS topic used for communication.
18-22: LGTM! Comprehensive overview of Local Planning
The introduction to Local Planning effectively explains its relationship with Global Planning and its role in the system.
8-8: Verify the referenced image exists
Let's ensure the image file exists at the specified path.
✅ Verification successful
Image reference is valid and accessible ✓
The image file exists at the specified path doc/assets/research_assets/planning_internal.png, confirming that the markdown reference is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the image exists
if [ -f "doc/assets/research_assets/planning_internal.png" ]; then
echo "Image exists"
else
echo "Warning: Image not found at doc/assets/research_assets/planning_internal.png"
fi
Length of output: 96
code/planning/src/local_planner/ACC.py (4)
14-15: LGTM! Clear and informative class documentation.
The updated class documentation clearly describes the ACC's purpose and its dependencies on various inputs.
Line range hint 17-89: LGTM! Well-organized initialization with improved type safety.
The subscriber and publisher initialization is well-structured with:
- Clear type hints for better code maintainability
- Logical grouping of related subscriptions
- Consistent naming and documentation
Line range hint 1-245: Verify test coverage for this critical safety component.
Given that ACC is a critical safety component, we should ensure comprehensive test coverage, especially for:
- Edge cases in collision detection
- Speed calculation logic
- Safety distance calculations
#!/bin/bash
# Search for test files related to ACC
fd -g "*test*ACC*.py" -g "*ACC*test*.py"
# Search for any pytest fixtures or test cases related to ACC
rg -l "def test.*ACC|class.*ACC.*Test" Line range hint 91-106: Consider adding validation for negative distance/speed values.
While the method correctly handles infinite values, it might be worth adding validation for negative values in data.data[0] (distance) and data.data[1] (speed) to ensure robustness.
def __collision_callback(self, data: Float32):
if np.isinf(data.data[0]):
# If no obstacle is in front, we reset all values
self.obstacle_speed = None
self.obstacle_distance = None
return
+ # Validate distance and speed
+ if data.data[0] < 0 or data.data[1] < 0:
+ self.logwarn("Received negative distance or speed values")
+ return
self.obstacle_speed = data.data[1]
self.obstacle_distance = data.data[0]
johannaschwarz
deleted the
362-research-current-architecture-of-planning-component
branch
Description
Adds research for planning and some minor documentation updates for planning.
Fixes #362
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
no
Most important changes
Update to research of planning
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor