608 feature integrate teb local planner #660
Conversation
Dockerfile now automatically installs the teb_local_planner. Started on documentation of usage.
This includes a guide on tuning and links to all relevant sources.
WalkthroughThe pull request introduces the TEB (Timed Elastic Bands) Local Planner into the project's build structure. This involves modifying the Docker image Dockerfile to install the Changes
Assessment against linked issues
Poem
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: 2
🧹 Nitpick comments (4)
doc/acting/teb_local_planner.md (4)
4-5: Enhance the introduction with more context.Consider expanding the summary to include:
- The purpose and benefits of using TEB local planner
- Key features and capabilities
- Target audience for this documentation
75-82: Improve frame configuration documentation.The explanation of frame configuration could be clearer. Consider adding:
- Why these specific frames are used
- The impact of frame selection on the planner's behavior
130-146: Document footprint configuration trade-offs.The section about footprint configuration could be enhanced by:
- Explaining why the polygon shape causes optimizer failures
- Documenting the performance implications of different footprint types
- Adding a comparison table of footprint configurations
🧰 Tools
🪛 LanguageTool
[typographical] ~141-~141: It appears that a comma is missing.
Context: ...ing_radius: 12 ``` In an ideal case the polygon shape would be optimal for ...(DURING_THAT_TIME_COMMA)
[typographical] ~142-~142: Consider adding two commas here.
Context: ... this planning. In testing the optimizer however failed miserable when trying this. We a...(HOWEVER_COMMA)
147-159: Improve move_base section formatting and clarity.Consider these improvements:
- Add subsections for better organization
- Use bullet points for listing challenges
- Add a diagram showing the relationship between move_base and TEB planner
🧰 Tools
🪛 LanguageTool
[uncategorized] ~155-~155: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...full move_base interface into our code. However the complete stack relies heavily on bi...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~156-~156: A comma may be missing after the conjunctive/linking adverb ‘Also’.
Context: ...e constantly changing in our situation. Also we are only interested in the local_pla...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~158-~158: Use a comma after an introductory phrase.
Context: ..._planner with any other planner easily. For this we would need to integrate move_base.(COMMA_INTRODUCTORY_WORDS_PHRASES)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
doc/assets/acting/single_teb.pngis excluded by!**/*.png
📒 Files selected for processing (2)
build/docker/agent/Dockerfile(2 hunks)doc/acting/teb_local_planner.md(1 hunks)
🧰 Additional context used
🪛 Hadolint (2.12.0)
build/docker/agent/Dockerfile
[error] 106-106: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
🪛 LanguageTool
doc/acting/teb_local_planner.md
[grammar] ~33-~33: Did you mean to write ‘therefore’?
Context: ...is integrated with dynamic reconfigure, therefor RQT reconfigure panel can be used to ad...
(THEREFORE)
[typographical] ~89-~89: Consider adding two commas here.
Context: ...r dt_h beeing 0.1 * dt_ref. These values however worked good at testing. Smaller dts lea...
(HOWEVER_COMMA)
[formatting] ~97-~97: Consider adding a comma after ‘Obviously’ for more clarity.
Context: ...ons: 3 optimization_activate: True ``` Obviously optimization should be activated. The m...
(CONJUNCTIVE_LINKING_ADVERB_COMMA_PREMIUM)
[style] ~106-~106: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ss_planning: False ``` This setting is very important for performance. If homotopy is activat...
(EN_WEAK_ADJECTIVE)
[typographical] ~141-~141: It appears that a comma is missing.
Context: ...ing_radius: 12 ``` In an ideal case the polygon shape would be optimal for ...
(DURING_THAT_TIME_COMMA)
[typographical] ~142-~142: Consider adding two commas here.
Context: ... this planning. In testing the optimizer however failed miserable when trying this. We a...
(HOWEVER_COMMA)
[uncategorized] ~155-~155: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...full move_base interface into our code. However the complete stack relies heavily on bi...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~156-~156: A comma may be missing after the conjunctive/linking adverb ‘Also’.
Context: ...e constantly changing in our situation. Also we are only interested in the local_pla...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[typographical] ~158-~158: Use a comma after an introductory phrase.
Context: ..._planner with any other planner easily. For this we would need to integrate move_base.
(COMMA_INTRODUCTORY_WORDS_PHRASES)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (1)
doc/acting/teb_local_planner.md (1)
21-21:⚠️ Potential issueFix the broken image reference.
The image reference shows "MISSING: TEB-Example". Please ensure the image file exists at the specified path
./../assets/acting/single_teb.png.
| ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git | ||
| RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the repository to a specific version.
Consider pinning the ros_teb_planner repository to a specific commit or tag for better reproducibility.
Apply this diff to pin the repository version:
ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git
-RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner
+RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner && \
+ cd src/ros_teb_planner && \
+ git checkout <specific-commit-or-tag>Committable suggestion skipped: line range outside the PR's diff.
| # Install teb_local_planner as well as teb_planner_pa | ||
| RUN sudo apt-get install -y \ | ||
| ros-noetic-teb-local-planner | ||
| ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git | ||
| RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner | ||
|
|
There was a problem hiding this comment.
Address security concerns in package installation.
The use of sudo in Dockerfile instructions is discouraged as it can lead to unpredictable behavior.
Apply this diff to fix the security issue:
-# Install teb_local_planner as well as teb_planner_pa
-RUN sudo apt-get install -y \
+# Install teb_local_planner as well as teb_planner_pa
+USER root
+RUN apt-get install -y \
ros-noetic-teb-local-planner
ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git
-RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner
+RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner && \
+ chown -R $USERNAME:$USERNAME src/ros_teb_planner
+USER $USERNAME📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install teb_local_planner as well as teb_planner_pa | |
| RUN sudo apt-get install -y \ | |
| ros-noetic-teb-local-planner | |
| ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git | |
| RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner | |
| # Install teb_local_planner as well as teb_planner_pa | |
| USER root | |
| RUN apt-get install -y \ | |
| ros-noetic-teb-local-planner | |
| ARG TEB_PLANNER_PA_GITHUB_URL=https://github.com/TUC-ProAut/ros_teb_planner.git | |
| RUN git clone $TEB_PLANNER_PA_GITHUB_URL src/ros_teb_planner && \ | |
| chown -R $USERNAME:$USERNAME src/ros_teb_planner | |
| USER $USERNAME |
🧰 Tools
🪛 Hadolint (2.12.0)
[error] 106-106: Do not use sudo as it leads to unpredictable behavior. Use a tool like gosu to enforce root
(DL3004)
|
As we decided against TEB - Local Planner for now I will close this PR and create a new one with documentation and HOW-TOs about TEB Planner. |
Description
This PR includes the basis for all TEB functionality.
It includes the installs in the docker-files.
It is specifically designed to avoid conflicts in the future by separating this from any functional changes.
Fixes #608
Type of change
Does this PR introduce a breaking change?
No
Most important changes
Added installs to Dockerfiles
Checklist:
Summary by CodeRabbit
New Features
Documentation