Added Dataflow/Architecture Diagram & Checked Documentation for Planning #406
Conversation
Changed confusing naming scheme in Readme. Checked current Documentation.
WalkthroughThe changes introduce a new diagram file, Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (2)
doc/planning/README.md (2)
9-11: Improve grammar and readability in the OpenDrive Converter section.Apply these improvements to enhance clarity:
-This module focuses on creating a trajectory out of -an OpenDrive map (ASAM OpenDrive). As input it receives an xodr file (OpenDrive format) and the target points -from the leaderboard with the belonging actions. For example action number 3 means, drive through the intersection. +This module focuses on creating a trajectory from an OpenDrive map (ASAM OpenDrive). As input, it receives an xodr file (OpenDrive format) and target points +from the leaderboard with their associated actions. For example, action number 3 means drive through the intersection.🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: A comma is probably missing here.
Context: ...f an OpenDrive map (ASAM OpenDrive). As input it receives an xodr file (OpenDrive for...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[misspelling] ~10-~10: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... (ASAM OpenDrive). As input it receives an xodr file (OpenDrive format) and the ta...(EN_A_VS_AN)
[typographical] ~11-~11: After the expression ‘for example’ a comma is usually used.
Context: ...erboard with the belonging actions. For example action number 3 means, drive through th...(COMMA_FOR_EXAMPLE)
33-36: Enhance readability of the Local Planning section.Apply these improvements to fix grammar and enhance clarity:
-This module includes the Nodes: ACC, CollisionCheck, MotionPlanner +This module includes the following nodes: +- ACC +- CollisionCheck +- MotionPlanner -The Local Planning package is responsible for evaluating short term decisions in the local environment of the ego vehicle. It containes components responsible for detecting collisions and reacting e. g. lowering speed. -The local planning also executes behaviors e.g. changes the trajectory for an overtake. +The Local Planning package is responsible for evaluating short-term decisions in the local environment of the ego vehicle. It contains components responsible for detecting collisions and reacting (e.g., lowering speed). +The local planning also executes behaviors (e.g., changing the trajectory for overtaking).🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...g package is responsible for evaluating short term decisions in the local environment of t...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~35-~35: A comma might be missing here.
Context: ...esponsible for detecting collisions and reacting e. g. lowering speed. The local plannin...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~35-~35: There should be no space here.
Context: ...e for detecting collisions and reacting e. g. lowering speed. The local planning also...(EG_SPACE)
[uncategorized] ~36-~36: A comma might be missing here.
Context: ...speed. The local planning also executes behaviors e.g. changes the trajectory for an over...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
doc/assets/planning/planning_structure.pngis excluded by!**/*.png
📒 Files selected for processing (2)
doc/assets/planning/planning_structure.drawio(1 hunks)doc/planning/README.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/assets/planning/planning_structure.drawio
🧰 Additional context used
🪛 LanguageTool
doc/planning/README.md
[uncategorized] ~10-~10: A comma is probably missing here.
Context: ...f an OpenDrive map (ASAM OpenDrive). As input it receives an xodr file (OpenDrive for...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[misspelling] ~10-~10: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... (ASAM OpenDrive). As input it receives an xodr file (OpenDrive format) and the ta...
(EN_A_VS_AN)
[typographical] ~11-~11: After the expression ‘for example’ a comma is usually used.
Context: ...erboard with the belonging actions. For example action number 3 means, drive through th...
(COMMA_FOR_EXAMPLE)
[uncategorized] ~35-~35: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...g package is responsible for evaluating short term decisions in the local environment of t...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~35-~35: A comma might be missing here.
Context: ...esponsible for detecting collisions and reacting e. g. lowering speed. The local plannin...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~35-~35: There should be no space here.
Context: ...e for detecting collisions and reacting e. g. lowering speed. The local planning also...
(EG_SPACE)
[uncategorized] ~36-~36: A comma might be missing here.
Context: ...speed. The local planning also executes behaviors e.g. changes the trajectory for an over...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🔇 Additional comments (2)
doc/planning/README.md (2)
Line range hint 13-20: LGTM! Clear and well-structured description.
The section effectively explains the Global Planner's role and its interaction with other components.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~10-~10: A comma is probably missing here.
Context: ...f an OpenDrive map (ASAM OpenDrive). As input it receives an xodr file (OpenDrive for...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[misspelling] ~10-~10: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ... (ASAM OpenDrive). As input it receives an xodr file (OpenDrive format) and the ta...
(EN_A_VS_AN)
[typographical] ~11-~11: After the expression ‘for example’ a comma is usually used.
Context: ...erboard with the belonging actions. For example action number 3 means, drive through th...
(COMMA_FOR_EXAMPLE)
3-3: Verify the planning structure diagram accessibility.
Let's ensure the diagram is properly accessible and rendered.
✅ Verification successful
Image path is correctly configured and accessible
The planning structure diagram exists at the correct path doc/assets/planning/planning_structure.png and is properly referenced in the documentation using the relative path ../assets/planning/planning_structure.png. The image file is accessible and the markdown reference is correctly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence and accessibility of the planning structure diagram
if [ -f "doc/assets/planning/planning_structure.png" ]; then
echo "✓ Planning structure diagram exists"
file "doc/assets/planning/planning_structure.png"
else
echo "✗ Planning structure diagram not found at expected path"
fi
Length of output: 258
Script:
#!/bin/bash
# Let's verify if the image exists in other potential locations and check the actual path
fd "planning_structure.png" .
# Also check if there are any references to this image in other files
rg "planning_structure.png" --type md
Length of output: 190
| </Array> | ||
| </mxGeometry> | ||
| </mxCell> | ||
| <mxCell id="VXw4u54tnH0GYowiBxqF-12" value="<div><font style="font-size: 13px;"><b>Motion Planning</b></font></div><div><font style="font-size: 12px;">- Take most disposable Information to make final trajectory Decision</font></div>" style="ellipse;whiteSpace=wrap;html=1;fillColor=#f5f5f5;strokeColor=#666666;gradientColor=#b3b3b3;" parent="1" vertex="1"> |
There was a problem hiding this comment.
I’m not sure what you mean by ‚disposable information‘. Can you give me a bit more detail please?
There was a problem hiding this comment.
It meant that the node takes most available information into account when generating the desired trajectory and velocity.
I changed the description, it should be less confusing...
…n-making-for-subsystem-planning
| <mxPoint x="-4" y="-3" as="offset" /> | ||
| </mxGeometry> | ||
| </mxCell> | ||
| <mxCell id="VXw4u54tnH0GYowiBxqF-1" value="<div style="font-size: 14px;"><b>GlobalPlanner</b></div><div>(PrePlanner):</div><div>- Processes Map and Path Information</div><div>- Generates Global Path</div><div><i>Node GlobalPlanDistance</i></div><div><i>provides Distance to next WP and to LaneCHange</i><br></div>" style="ellipse;whiteSpace=wrap;html=1;fillColor=#fad7ac;strokeColor=#b46504;" parent="1" vertex="1"> |
There was a problem hiding this comment.
Fix Typo "LaneCHange"
| <mxPoint x="-4" y="-3" as="offset" /> | ||
| </mxGeometry> | ||
| </mxCell> | ||
| <mxCell id="VXw4u54tnH0GYowiBxqF-1" value="<div style="font-size: 14px;"><b>GlobalPlanner</b></div><div>(PrePlanner):</div><div>- Processes Map and Path Information</div><div>- Generates Global Path</div><div><i>Node GlobalPlanDistance</i></div><div><i>provides Distance to next WP and to LaneCHange</i><br></div>" style="ellipse;whiteSpace=wrap;html=1;fillColor=#fad7ac;strokeColor=#b46504;" parent="1" vertex="1"> |
There was a problem hiding this comment.
I wouldn't mention the GlobalPlanDistance Node here because it is part of another component. When I first read it, I thought you were describing this node as part of the GlobalPlanner. I think it would be better to just leave it out here and stick to the basic role of the GlobalPlanner.
There was a problem hiding this comment.
Yeah, would make sense to just leave it out.
Since the information is so closely related, I thought I briefly mention it.
But I can remove it :)
| <mxPoint x="-3" y="7" as="offset" /> | ||
| </mxGeometry> | ||
| </mxCell> | ||
| <mxCell id="VXw4u54tnH0GYowiBxqF-13" value="<div><b><font style="font-size: 14px;">ACC</font><br></b></div><div>- Recommend Velocity for current State<br></div>" style="ellipse;whiteSpace=wrap;html=1;fillColor=#b0e3e6;strokeColor=#0e8088;" parent="1" vertex="1"> |
There was a problem hiding this comment.
Maybe add (Adaptive Cruise Control) behind the abbr. ACC. :D
There was a problem hiding this comment.
Do you think it might be a good idea to combine the PrePlanner and the OpenDrive Components with a box named Global Planning like the Local Planning? Then you could rename the GlobalPlanner to PrePlanner corresponding to the class name in the code.
There was a problem hiding this comment.
What does the sentence on the bottom right mean?
There was a problem hiding this comment.
The Blackboard is used in the behavior tree. I thought there might be messages exchanged that are not shown on the node graph.
But on closer inspection, it should be just an internal thing.
Will remove it
There was a problem hiding this comment.
It might be a good idea in general to not explicitly write the ROS topics used for communication but more the concept of what is being transferred. ( i.e. Speed Limits). That will probably make it easier to keep the diagram up to date in the future, as it will be less prone to change.
There was a problem hiding this comment.
Since it is just a diagram of the current architecture, I would leave it in for now.
We might want to change more major parts anyway ;)
Description
Added new diagram of current planning architecture/dataflow to Readme. (including drawio asset)
Changed confusing naming scheme in Readme.md to the scheme used in the other parts of the documentation.
Validated current Documentation.
Type of change
Documentation Additions/Changes
Summary by CodeRabbit
New Features
Documentation