[ROS2Port]: Execution + debugging + misc. documentation update - #876
Conversation
📝 WalkthroughWalkthroughThis PR updates documentation for a ROS2 migration, adds a new parameters doc, removes legacy dynamic_reconfigure docs, adjusts Docker/Docker Compose configurations (including Python base image and docs service build args), and adds editor settings for Markdown formatting. Changes
Sequence Diagram(s)(omitted — changes are documentation, configuration and minor infra updates without new multi-component runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
doc/development/parameters.md (1)
13-13: Consider a more concise phrasing.The phrase "In order to use parameters" can be simplified to "To use parameters." This improves readability without losing clarity.
doc/development/distributed_simulation.md (1)
3-3: Consider softening the warning tone for professional documentation.The warning "Distributed Execution is currently NOT set up for ROS2 and has to be ported!!" uses multiple exclamation marks, which may come across as excessive in professional documentation. Consider revising to: "Distributed Execution is currently NOT set up for ROS2 and has to be ported." This preserves the critical message without the informal punctuation.
doc/research/overhaul25/improvements/docker.md (1)
64-64: Standardize status block capitalization for consistency.The document uses both "Status: implemented" (line 64) and "Status: Implemented" (line 82) inconsistently. Choose one style and apply it uniformly throughout the document to improve visual consistency.
Also applies to: 82-82, 113-113, 120-120, 129-129, 138-138, 149-149
doc/general/execution.md (2)
28-31: Quick Start section is clearer, but two phrasing improvements can enhance readability.The new prerequisite structure (user_setup.sh) is clearer. However, consider these optional refinements:
- Line 30: "In order to start" → "To start" (more concise)
- Line 31: "platform specific" → "platform-specific" (hyphenated when used as an adjective)
47-48: Tone: Replace informal phrasing with more professional alternatives.The phrase "pretty much all relevant parameters" (line 48) is somewhat informal. Consider replacing with "contain the majority of relevant parameters" or "contain all key parameters" for a more professional tone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
doc/mapping/generated/mapping_common/entity.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/filter.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/hero.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/index.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/map.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/markers.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/mask.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/shape.mdis excluded by!**/generated/**doc/mapping/generated/mapping_common/transform.mdis excluded by!**/generated/**doc/mapping/generated/nodes.mdis excluded by!**/generated/**
📒 Files selected for processing (14)
.vscode/settings.json(1 hunks)build/docker-compose.docs.yaml(0 hunks)build/docker/docs/Dockerfile(1 hunks)doc/development/debugging.md(3 hunks)doc/development/distributed_simulation.md(1 hunks)doc/development/parameters.md(1 hunks)doc/general/dynamic_reconfigure.md(0 hunks)doc/general/execution.md(2 hunks)doc/mapping/pydoc-markdown.yaml(3 hunks)doc/research/overhaul25/README.md(1 hunks)doc/research/overhaul25/improvements/README.md(3 hunks)doc/research/overhaul25/improvements/docker.md(8 hunks)doc/research/overhaul25/ros2_porting/README.md(5 hunks)doc/research/overhaul25/ros2_porting/python_porting.md(2 hunks)
💤 Files with no reviewable changes (2)
- doc/general/dynamic_reconfigure.md
- build/docker-compose.docs.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-01-28T15:07:09.113Z
Learnt from: SirMDA
Repo: una-auxme/paf PR: 662
File: code/control/launch/control.launch:3-14
Timestamp: 2025-01-28T15:07:09.113Z
Learning: Enabling Carla's synchronous mode (`synchronous_mode_wait_for_vehicle_control_command`) does not effectively resolve the timing issues between the vehicle controller and Carla simulator. A sleep-based approach in the vehicle controller is currently the most reliable solution.
Applied to files:
doc/research/overhaul25/improvements/README.md
📚 Learning: 2025-02-06T11:20:19.561Z
Learnt from: RoyaLxPole
Repo: una-auxme/paf PR: 687
File: doc/general/create_new_route.md:19-19
Timestamp: 2025-02-06T11:20:19.561Z
Learning: Documentation that references or quotes from the official Carla simulator should maintain exact consistency with the source, even if it contains minor grammatical or stylistic issues.
Applied to files:
doc/research/overhaul25/ros2_porting/python_porting.md
📚 Learning: 2024-11-12T13:46:38.887Z
Learnt from: Zelberor
Repo: una-auxme/paf PR: 478
File: code/debug_wrapper.py:165-165
Timestamp: 2024-11-12T13:46:38.887Z
Learning: In the Python ROS node `debug_wrapper.py` (in `code/debug_wrapper.py`), the `--debug_port` argument is optional, and the node is designed to start even when the port is not provided.
Applied to files:
doc/development/debugging.md
🪛 LanguageTool
doc/research/overhaul25/ros2_porting/README.md
[formatting] ~111-~111: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...la/tree/ue4/0.9.16) - **Chosen solution: Use the new 0.9.16 Carla rele...
(COMMA_BEFORE_BECAUSE)
[formatting] ~120-~120: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ntains the ROS2 jazzy + Python3.8 fusion and the leaderboard, scenario_runner and ros_bri...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~144-~144: Possible missing comma found.
Context: ...t uses 2 docker containers~~ ~~Merging the carla-leaderboard-ros-bridge and agent...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~149-~149: Possible missing preposition found.
Context: ...../../../build/docker/agent-ros2/Dockerfile) - Contains the Carla API and leaderb...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~298-~298: The official name of this software platform is spelled with a capital “H”.
Context: ...s - [DONE] Push new carla images to the github registry ## Risks and Mitigation ### ...
(GITHUB)
doc/research/overhaul25/README.md
[uncategorized] ~17-~17: This expression is usually spelled with a hyphen.
Context: ...al problems, etc...** can be found ➡ here. Information rela...
(BASED_HYPHEN)
[typographical] ~25-~25: Consider adding a comma here.
Context: ...g is completed and the ros2 based implementation achieves similar performance/quality to t...
(FOR_THAT_COMMA)
doc/general/execution.md
[uncategorized] ~17-~17: Possible missing comma found.
Context: ....yaml](#docker-composedeploybaseyaml) - [docker-compose.linter.yaml`](#docker-compose...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~30-~30: Consider a more concise word here.
Context: ... docker compose environment variables. In order to start the default leaderboard execution...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~31-~31: When ‘platform-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: .../../build/) folder and select the Compose up option in the right-click menu of the `docker-comp...
(SPECIFIC_HYPHEN)
[style] ~48-~48: The phrase ‘pretty much’ can be informal. To strengthen your writing, consider removing it or replacing it with an adverb.
Context: ...ut the base configuration files contain pretty much all relevant parameters for development...
(PRETTY_MUCH)
[duplication] ~58-~58: Possible typo: you repeated a word.
Context: ...hrough - Volumes: - Desktop: X11 UNIX socket + ${XDG_RUNTIME_DIR} - Custom CA...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~70-~70: “Image” is a singular noun. It appears that the verb form is incorrect.
Context: ... of the Dockerfile. - Includes: - docker-compose.carla.base.yaml - **Environm...
(PCT_SINGULAR_NOUN_PLURAL_VERB_AGREEMENT)
[uncategorized] ~108-~108: Possible missing comma found.
Context: ...ting. - Volumes: Mounts the project directory for linting files within the contai...
(AI_HYDRA_LEO_MISSING_COMMA)
doc/development/parameters.md
[style] ~12-~12: Consider a more concise word here.
Context: ...l> ## How to integrate into your Node In order to use parameters with your Node, you need...
(IN_ORDER_TO_PREMIUM)
doc/development/distributed_simulation.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2299 characters long)
Context: ...NOT set up for ROS2 and has to be ported!! Summary: This page documents the d...
(EN_EXCESSIVE_EXCLAMATION)
[grammar] ~11-~11: Possible agreement error -- use the past participle here.
Context: ... <ip-address> of the carla simulator in docker-compose distributed files](#set-the-ip...
(HAVE_PART_AGREEMENT)
doc/development/debugging.md
[formatting] ~16-~16: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...ms-of-debugging-with-vs-code) ## Debugging possibilities There are two main debugging p...
(COMMA_BEFORE_BECAUSE)
[uncategorized] ~36-~36: This expression is usually spelled with a hyphen.
Context: ...space/log/ros](/internal_workspace/log/ros). This includes the [agent.log](/internal_wor...
(BASED_HYPHEN)
[uncategorized] ~59-~59: Loose punctuation mark.
Context: ... #### Required once for the node you want to debug Inside the node.py script, ad...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (14)
build/docker/docs/Dockerfile (1)
1-1: Python 3.11 base image upgrade aligns with ROS2 Jazzy.This straightforward update supports the ROS2 porting initiative and provides access to newer Python tooling for documentation generation.
.vscode/settings.json (1)
32-35: Markdown formatter configuration improves documentation authoring workflow.The addition of the markdownlint formatter as the default for Markdown files is a good ergonomic choice given the documentation-focused nature of this PR. Configuration is syntactically correct.
doc/research/overhaul25/ros2_porting/python_porting.md (1)
20-20: Typo correction improves heading clarity.Correcting "Compatiblity" to "Compatibility" is an appropriate typo fix in our documentation structure. The learnings context applies to maintaining consistency with direct quotes from official sources, not to correcting spelling errors in our own section headings.
doc/development/debugging.md (3)
22-22: ROS2 logging patterns are correctly implemented.The migration from
rospy.logdebug/info/warn/error/fataltoself.get_logger().debug/info/warn/error/fatalis accurate and follows ROS2 conventions. The documentation correctly notes that debug messages are not published to /rosout by default in ROS2.Also applies to: 25-25, 42-42
63-84: Debugger setup examples are practical and well-structured.The code examples for
start_debuggerusage are clear, correctly demonstrate the main function integration, and align with the single-threaded executor pattern. The emphasis on avoiding multiple nodes on the same port (53000) is important and appropriately highlighted.
36-36: No action needed — directory naming is consistent.The codebase consistently uses
internal_workspace(underscore) throughout documentation and shell scripts. No instances ofinternal-workspace(hyphen) were found. The references in lines 36 and 38 ofdoc/development/debugging.md(/internal_workspace/log/rosand/internal_workspace/log/ros/agent.log) align with the environment variable${INTERNAL_WORKSPACE_DIR}used in multiple launch scripts and other documentation.doc/general/execution.md (2)
45-100: Docker Compose file documentation is comprehensive and well-organized.The detailed explanations of base vs. platform-specific configurations, inheritance patterns, and the purpose of each file (carla, dev, deploy, docs) are clear and helpful. This structure properly conveys how the configurations compose and extend each other.
129-136: ROS2 distributed execution warning is appropriately placed and consistent with related documentation.The warning that distributed execution is not yet set up for ROS2 is properly flagged and cross-references the distributed_simulation.md file. This aligns with the broader ROS2 porting narrative across the PR.
doc/development/parameters.md (1)
80-80: Theupdate_attributesutility exists and is properly documented.The function is defined in
paf_common/paf_common/parameters.pywith a complete docstring documenting its purpose, arguments, and return type. The import statementfrom paf_common.parameters import update_attributesis correct and functional.doc/research/overhaul25/improvements/README.md (1)
38-38: Status updates are clear and well-structured.The newly added status lines effectively document implementation progress and environment changes without cluttering the document. These align well with the broader PR objective of reflecting ROS2 porting completion.
Also applies to: 145-146, 203-203, 207-207
doc/research/overhaul25/README.md (2)
5-5: ROS2 port completion messaging is clear and consistent.The updates effectively communicate that the overhaul research has concluded and the ROS2 port has been successfully merged into main. The reference to PR #832 provides good traceability.
Also applies to: 13-13
23-26: Strikethrough of deprecated workflows is appropriate.Marking the temporary
ros2-devbranch organization and CI workarounds with strikethrough clearly indicates these are no longer relevant, while maintaining visibility of historical decisions for future reference.Also applies to: 32-34
doc/research/overhaul25/ros2_porting/README.md (2)
111-111: Carla 0.9.16 solution clearly marked as chosen approach.The documentation effectively communicates the pivot from the complex multi-container architecture to using Carla 0.9.16, which eliminates the Python version conflict. The strikethrough treatment of the deprecated custom solution preserves historical context while clearly indicating it is no longer the approach.
Also applies to: 113-144
274-274: ROS2 port completion is prominently marked.The "[DONE] Port the full project" milestone clearly signals that the main porting effort has been completed, aligning with the PR objective and the merge status referenced in the parent README.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/general/execution.md (1)
28-31: Minor wording improvements for clarity.Three small grammar and style tweaks to align with professional documentation standards:
- Line 30: Replace "In order to" with "To" for conciseness.
- Line 47: Compound adjective "platform specific" should be "platform-specific" (hyphenated).
- Line 48: Remove "pretty much" to strengthen the writing; replace with "contains" or "includes" directly.
✏️ Proposed wording improvements
-This sets up important docker compose environment variables. In order to start the default leaderboard execution simply navigate to the [build](../../build/) folder and select the `Compose up` option in the right-click menu of the `docker-compose.dev.<your-gpu-type>.yml` file. As `<your-gpu-type>` `cuda` should be used for the lab PCs. +This sets up important docker compose environment variables. To start the default leaderboard execution, simply navigate to the [build](../../build/) folder and select the `Compose up` option in the right-click menu of the `docker-compose.dev.<your-gpu-type>.yml` file. Use `cuda` as `<your-gpu-type>` for the lab PCs. -The base compose files (`docker-compose.*.base.yaml`) define the configurations for individual services used in the project. These files are extended in platform specific configuration files to adapt the development environment to different hosts. -For the lab, only the `docker-compose.*.cuda.yaml` configurations should be executed, but the base configuration files contain pretty much all relevant parameters for development. +The base compose files (`docker-compose.*.base.yaml`) define the configurations for individual services used in the project. These files are extended in platform-specific configuration files to adapt the development environment to different hosts. +For the lab, only the `docker-compose.*.cuda.yaml` configurations should be executed, but the base configuration files contain all relevant parameters for development.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.vscode/settings.jsondoc/general/execution.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T16:40:36.744Z
Learnt from: ll7
Repo: una-auxme/paf PR: 880
File: .vscode/tasks.json:1-62
Timestamp: 2025-12-10T16:40:36.744Z
Learning: VSCode configuration files under any .vscode directory use JSONC (JSON with Comments) syntax. They may include comments and trailing commas. Do not flag trailing commas or comments in .vscode/*.json (e.g., tasks.json, settings.json, launch.json); treat these as valid JSONCyntax and only flag non-JSONC syntax.
Applied to files:
.vscode/settings.json
🪛 LanguageTool
doc/general/execution.md
[style] ~30-~30: Consider a more concise word here.
Context: ... docker compose environment variables. In order to start the default leaderboard execution...
(IN_ORDER_TO_PREMIUM)
[grammar] ~47-~47: Use a hyphen to join words.
Context: ...ct. These files are extended in platform specific configuration files to adapt th...
(QB_NEW_EN_HYPHEN)
[style] ~48-~48: The phrase ‘pretty much’ can be informal. To strengthen your writing, consider removing it or replacing it with an adverb.
Context: ...ut the base configuration files contain pretty much all relevant parameters for development...
(PRETTY_MUCH)
🔇 Additional comments (2)
doc/general/execution.md (1)
128-137: ROS2 migration status clearly communicated.The warning about distributed execution not yet being ported to ROS2 is prominent and actionable. Users are directed to the relevant reference documentation. This is well-handled for migration documentation.
.vscode/settings.json (1)
31-34: Extension is already recommended; no changes needed.The
DavidAnson.vscode-markdownlintextension is already listed in.vscode/extensions.json(line 3), so contributors will receive the workspace recommendation. The Markdown formatter setting is correctly configured with no conflicts from other formatters like Prettier.Likely an incorrect or invalid review comment.
2 participants
Description
Updates the documentation in
Relevant issue: #851
Type of change
Does this PR introduce a breaking change?
No
Most important changes
Checklist:
Summary by CodeRabbit
Documentation
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.