Refs/heads/488 feature split up acting controller #508
Conversation
Moved pure_pursuit, stanley, vehicle and velocity controller to new control pck Changed topic names in the new control package Added a passthrough node in acting, which passes necessary topic to control Adapted acting launch file to accomodate the changes
Moved necessary documentation from acting to control. Added documentation for passthrough node. Adjusted linking in Readmes
Added new architecuraly diagrams. Added and updated linking through markdown documentation. Moved assets into correct folder and updated docs accordingly.
WalkthroughThe changes in this pull request involve significant modifications to the ROS (Robot Operating System) configuration and structure for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🧹 Outside diff range and nitpick comments (24)
doc/acting/README.md (1)
8-8: Add missing newline at end of fileThe file should end with a single newline character.
4. [How to test/tune the acting component](./acting_testing.md) +🧰 Tools
🪛 Markdownlint
8-8: null
Files should end with a single newline character(MD047, single-trailing-newline)
code/acting/launch/acting.launch (2)
6-12: Enhance the passthrough node documentationWhile the comment indicates the purpose, it would be helpful to provide more details about:
- The specific messages being passed through
- The topics being subscribed to and published
- The relationship with the control package
Consider expanding the comment like this:
- <!--Passthrough - all messages the control package needs--> + <!-- Passthrough Node + Forwards essential messages from acting to control package: + - List specific message types + - List input/output topics + - Explain interaction with control package + -->
1-21: Well-structured separation of acting and control componentsThe launch file changes effectively implement the separation of acting and control functionality through:
- Introduction of a passthrough node for clean message passing
- Removal of controller nodes (now in control package)
- Proper parameter sharing between packages
This architecture promotes better modularity and maintainability.
Consider documenting the following in the package's README:
- The new architecture overview
- The role of the passthrough node
- The interaction between acting and control packages
code/control/launch/control.launch (2)
2-3: Add documentation for launch file argumentsConsider adding XML comments to document:
- Purpose and impact of the control_loop_rate parameter
- Explanation of the role_name parameter and its possible values
- Any constraints or dependencies for these parameters
<launch> + <!-- control_loop_rate: Time interval between control commands in seconds. + Must be 0.05s to meet leaderboard requirements and prevent lag. --> + <!-- role_name: Identifier for the vehicle in the system. Default: "hero" --> <arg name="role_name" default="hero" /> <arg name="control_loop_rate" default="0.05" />
6-26: Consider refining the logging strategyAll nodes currently output to screen, which could flood the console with messages. Consider:
- Using a ROS logger with different verbosity levels
- Redirecting output to log files for non-critical nodes
- Keeping screen output only for critical messages
Consider using ROS parameter groups
The control_loop_rate and role_name parameters are duplicated across all nodes. Consider using a parameter group or YAML configuration:
<launch> <arg name="role_name" default="hero" /> <arg name="control_loop_rate" default="0.05" /> + + <!-- Load common parameters --> + <rosparam command="load" file="$(find control)/config/controller_params.yaml" />Highlight critical timing requirement
The timing requirement (0.05s) is crucial for system performance but is hidden in a comment. Consider:
- Moving this to the top of the file as a prominent warning
- Adding validation for the control_loop_rate parameter
- Documenting this requirement in the package's README
code/acting/src/acting/passthrough.py (1)
59-68: Improve QoS configuration and add error handling.
- Use descriptive QoS profile constants instead of magic number 1
- Add error handling for message publishing
+from rclpy.qos import QoSProfile, QoSReliabilityPolicy, QoSHistoryPolicy + + def _message_callback(self, msg, publisher): + try: + publisher.publish(msg) + except Exception as e: + self.get_logger().error(f"Failed to publish message: {e}") + def __init__(self): + self.qos_profile = QoSProfile( + reliability=QoSReliabilityPolicy.RELIABLE, + history=QoSHistoryPolicy.KEEP_LAST, + depth=1 + ) for topic in self.passthrough_topics: self.publishers[topic.pub_name] = self.new_publisher( - topic.topic_type, topic.pub_name, qos_profile=1 + topic.topic_type, topic.pub_name, qos_profile=self.qos_profile ) self.subscribers[topic.pub_name] = self.new_subscription( topic.topic_type, topic.sub_name, - callback=self.publishers[topic.pub_name].publish, - qos_profile=1, + callback=lambda msg, pub=self.publishers[topic.pub_name]: + self._message_callback(msg, pub), + qos_profile=self.qos_profile, )doc/control/architecture_documentation.md (3)
4-7: Improve clarity by using more concise phrasing.Consider revising the sentence structure for better readability:
-The control component applies control theory based on a local trajectory provided -by the [acting component](./../acting/README.md). It uses knowledge of the current state -of the vehicle in order to send [CarlaEgoVehicleControl](https://carla.readthedocs.io/en/0.9.8/ros_msgs/#CarlaEgoVehicleControlmsg) commands to the Simulator. This component also sends the [/carla/hero/status](https://leaderboard.carla.org/get_started/) command, +The control component applies control theory based on a local trajectory provided +by the [acting component](./../acting/README.md). It uses the vehicle's current state +to send [CarlaEgoVehicleControl](https://carla.readthedocs.io/en/0.9.8/ros_msgs/#CarlaEgoVehicleControlmsg) commands to the Simulator. This component also sends the [/carla/hero/status](https://leaderboard.carla.org/get_started/) command,🧰 Tools
🪛 LanguageTool
[style] ~6-~6: Consider a shorter alternative to avoid wordiness.
Context: ...dge of the current state of the vehicle in order to send [CarlaEgoVehicleControl](https://c...(IN_ORDER_TO_PREMIUM)
27-81: Fix markdown formatting issues.The document has several formatting inconsistencies:
- Add blank lines around headings (lines 27, 53, 66)
- Remove trailing spaces
- Add a final newline
Apply these fixes:
-### Steering controllers +### Steering controllers + -> [!TIP] +> [!TIP] > Follow this link for the [Documentation](./steering_controllers.md) on steering controllers. -### Velocity controllers +### Velocity controllers + -> [!TIP] +> [!TIP] > Follow this link for the [Documentation](./velocity_controller.md) on velocity component. -### vehicle_controller.py +### vehicle_controller.py + -> [!TIP] +> [!TIP] > Follow this link for [Documentation](./vehicle_controller.md) on vehicle controller. ... - **emergency**: Bool +🧰 Tools
🪛 LanguageTool
[style] ~67-~67: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1958 characters long)
Context: ...: Float32 ### vehicle_controller.py > [!TIP] > Follow this link for [Documentati...(EN_EXCESSIVE_EXCLAMATION)
🪛 Markdownlint
27-27: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
28-28: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
31-31: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
53-53: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
27-27: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
53-53: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
66-66: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
81-81: null
Files should end with a single newline character(MD047, single-trailing-newline)
1-81: Well-structured documentation supporting component separation.The documentation effectively describes the control component's architecture and its interaction with the acting component. The clear separation of concerns and well-defined interfaces align with the PR objective of splitting the acting and control functionality.
Consider adding:
- Error handling and edge cases for each controller
- Configuration parameters and their impact
- Performance characteristics and constraints
🧰 Tools
🪛 LanguageTool
[style] ~6-~6: Consider a shorter alternative to avoid wordiness.
Context: ...dge of the current state of the vehicle in order to send [CarlaEgoVehicleControl](https://c...(IN_ORDER_TO_PREMIUM)
[style] ~67-~67: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1958 characters long)
Context: ...: Float32 ### vehicle_controller.py > [!TIP] > Follow this link for [Documentati...(EN_EXCESSIVE_EXCLAMATION)
🪛 Markdownlint
3-3: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
22-22: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
27-27: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
28-28: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
31-31: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
53-53: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
27-27: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
53-53: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
66-66: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
81-81: null
Files should end with a single newline character(MD047, single-trailing-newline)
doc/acting/acting_testing.md (3)
11-11: Consider separating acting and control debugging functionalityGiven the architectural split between acting and control components, it would be more maintainable to have separate debug nodes for each component. This would:
- Maintain clear component boundaries
- Reduce coupling between acting and control
- Simplify maintenance
- Make testing more focused
Would you like assistance in creating a separate control debug node and documentation?
18-19: Fix warning syntax and improve clarityThe warning syntax is incorrect and the message could be clearer.
Apply this diff:
->[!WARNING]! Make sure to revert these changes when pushing your branch! +> [!WARNING] +> Make sure to revert the launch file and docker-compose changes before committing your changes!
Line range hint
1-50: Update documentation to reflect architectural changesThe documentation needs to be updated to reflect the new architecture where acting and control are separate components. Please add:
- Information about the new passthrough node and its role
- Control-specific configuration guidance
- Clear distinction between acting and control components throughout the document
Would you like me to help draft these documentation updates?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: Possible missing comma found.
Context: ...om the acting package. As mentioned above this includes everything we need. - In ...(AI_HYDRA_LEO_MISSING_COMMA)
doc/acting/architecture_documentation.md (5)
3-5: Improve summary wording for clarity and conciseness.Consider rewording to avoid wordiness:
-The acting component receives information from the [planning component](./../planning/README.md) as well -as the [perception component](./../perception/README.md). It processes this information in order to -navigate on a local basis. This information is then processed in the [control_component](./../control/README.md). +The acting component processes information from the [planning](./../planning/README.md) and +[perception](./../perception/README.md) components for local navigation. The processed information +is then handled by the [control component](./../control/README.md).🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...(IN_ORDER_TO_PREMIUM)
24-25: Fix grammar in documentation link text.Add the missing article "the":
-> For documentation on passthrough component see: [passthrough](./passthrough.md) +> For documentation on the passthrough component see: [passthrough](./passthrough.md)🧰 Tools
🪛 LanguageTool
[uncategorized] ~25-~25: You might be missing the article “the” here.
Context: ...gh.py > [!TIP] > For documentation on passthrough component see: [passthrough](./passthro...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
36-42: Improve mathematical formula formatting.Consider using proper markdown math formatting for better readability:
-rotation = - **current_heading** -position x = cos(rotation) * **current_pos**.x + sin(rotation) * **current_pos**.y -position y = sin(rotation) * **current_pos**.x + cos(rotation) * **current_pos**.y -position z = - **current_pos**.z -rot_quat = rot as quaternion -**transform** = position x/y/z, rot_quat, Timestamp(now), "global", "hero" +- rotation = -current_heading +- position_x = cos(rotation) × current_pos.x + sin(rotation) × current_pos.y +- position_y = sin(rotation) × current_pos.x + cos(rotation) × current_pos.y +- position_z = -current_pos.z +- rot_quat = rotation as quaternion +- transform = (position_x, position_y, position_z, rot_quat, Timestamp(now), "global", "hero")
76-76: Add missing newline at end of file.Add a newline character at the end of the file to comply with markdown best practices.
🧰 Tools
🪛 Markdownlint
76-76: null
Files should end with a single newline character(MD047, single-trailing-newline)
Line range hint
1-76: Documentation effectively captures architectural changes.The documentation successfully reflects the separation of acting and control components while maintaining clear explanations of the remaining functionality. The structure is logical, and the component interactions are well-defined.
Consider adding sequence diagrams to illustrate the interaction flow between the acting and control components, which would help developers better understand the new architecture.
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...(IN_ORDER_TO_PREMIUM)
[uncategorized] ~25-~25: You might be missing the article “the” here.
Context: ...gh.py > [!TIP] > For documentation on passthrough component see: [passthrough](./passthro...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint
17-17: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces(MD009, no-trailing-spaces)
44-44: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
27-27: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
code/control/src/velocity_controller.py (3)
Line range hint
63-117: Consider separating gear control logicThe
loopmethod handles both velocity control and gear selection (reverse). Consider extracting gear control into a separate component to improve modularity and align with the control/acting split objective.Consider making PID parameters configurable
The PID parameters (0.60, 0.00076, 0.63) are hardcoded. Consider moving these to a configuration file or ROS parameters for easier tuning and maintenance.
- pid_t = PID(0.60, 0.00076, 0.63) + self.kp = self.get_param("pid_kp", 0.60) + self.ki = self.get_param("pid_ki", 0.00076) + self.kd = self.get_param("pid_kd", 0.63) + pid_t = PID(self.kp, self.ki, self.kd)
Line range hint
89-93: Replace magic numbers with named constantsThe value
-3as a signal for reverse driving is a magic number. Consider defining it as a class constant for better maintainability and documentation.+ class VelocityController(CompatibleNode): + REVERSE_SIGNAL = -3 + """Signal value indicating reverse driving mode""" + def __init__(self):Then update the condition:
- if self.__target_velocity == -3: + if self.__target_velocity == self.REVERSE_SIGNAL:
Line range hint
63-117: Add velocity validation for safetyConsider adding input validation for target and current velocity values to ensure safe vehicle operation. This is particularly important for a control component.
def __get_target_velocity(self, data: Float32): + if not isinstance(data.data, (int, float)): + self.logerr("Invalid velocity type received") + return + if abs(data.data) > self.get_param("max_velocity", 50.0): + self.logerr(f"Velocity {data.data} exceeds safety limits") + return self.__target_velocity = float(data.data)doc/control/steering_controllers.md (2)
21-21: Fix grammatical issues in controller descriptions.There are two grammatical issues that should be addressed:
- Missing comma after the URL reference
- Subject-verb agreement in the Stanley Controller description
Apply these corrections:
- For more indepth information about the PurePursuit Controller, click [this link](https://de.mathworks.com/help/nav/ug/pure-pursuit-controller.html) and [this link](https://thomasfermi.github.io/Algorithms-for-Automated-Driving/Control/PurePursuit.html).At every moment + For more indepth information about the PurePursuit Controller, click [this link](https://de.mathworks.com/help/nav/ug/pure-pursuit-controller.html) and [this link](https://thomasfermi.github.io/Algorithms-for-Automated-Driving/Control/PurePursuit.html). At every moment - The [Stanley Controller's](../../code/control/src/stanley_controller.py ) main features to determine a steering-output is + The [Stanley Controller's](../../code/control/src/stanley_controller.py ) main features to determine a steering-output areAlso applies to: 41-41
🧰 Tools
🪛 LanguageTool
[typographical] ~21-~21: It appears that a comma is missing.
Context: ...ng/Control/PurePursuit.html). At every moment it checks a point of the trajectory in ...(DURING_THAT_TIME_COMMA)
Line range hint
58-67: Consider clarifying the performance differences between environments.The documentation notes significant performance differences between Dev-Launch and Leaderboard-Launch environments, with a hypothesis about synchronicity. This warrants further investigation and clearer documentation.
Consider:
- Adding specific performance metrics or examples
- Documenting the exact configuration differences between environments
- Creating a tracking issue to investigate the root cause of the performance disparity
Would you like me to help create a GitHub issue to track this investigation?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~59-~59: If “ why” starts an indirect question, you do not need to put a comma before it.
Context: ...ey alot "weaker". We do not exactly know, why the two launches are this different. (D...(NO_COMMA_BEFORE_INDIRECT_QUESTION)
code/control/src/pure_pursuit_controller.py (1)
Line range hint
61-67: Maintain consistency in topic naming conventionWhile subscription topics have been centralized under the 'acting' namespace, publication topics still use role-based naming. This inconsistency could lead to confusion and maintenance issues.
Apply this diff to maintain consistency:
self.pure_pursuit_steer_pub: Publisher = self.new_publisher( - Float32, f"/paf/{self.role_name}/pure_pursuit_steer", qos_profile=1 + Float32, "/paf/acting/pure_pursuit_steer", qos_profile=1 ) self.debug_msg_pub: Publisher = self.new_publisher( - Debug, f"/paf/{self.role_name}/pure_p_debug", qos_profile=1 + Debug, "/paf/acting/pure_p_debug", qos_profile=1 )code/control/src/stanley_controller.py (1)
Line range hint
57-62: Fix inconsistent topic naming patternThe publisher topics still use the dynamic
role_namewhile the subscribers have been updated to use the static "acting" namespace. This inconsistency should be fixed.Apply this diff to maintain consistency:
- Float32, f"/paf/{self.role_name}/stanley_steer", qos_profile=1 + Float32, "/paf/acting/stanley_steer", qos_profile=1 - StanleyDebug, f"/paf/{self.role_name}/stanley_debug", qos_profile=1 + StanleyDebug, "/paf/acting/stanley_debug", qos_profile=1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (11)
doc/assets/acting/acting_architecture.pngis excluded by!**/*.pngdoc/assets/acting/architecture_acting_legacy.pngis excluded by!**/*.pngdoc/assets/control/Steering_PurePursuit.pngis excluded by!**/*.pngdoc/assets/control/Steering_PurePursuit_Tuning.pngis excluded by!**/*.pngdoc/assets/control/Steering_Stanley.pngis excluded by!**/*.pngdoc/assets/control/Steering_Stanley_ComparedToPurePur.pngis excluded by!**/*.pngdoc/assets/control/VelContr_PID_BrakingWithThrottlePID.pngis excluded by!**/*.pngdoc/assets/control/VelContr_PID_StepResponse.pngis excluded by!**/*.pngdoc/assets/control/VelContr_PID_differentVelocities.pngis excluded by!**/*.pngdoc/assets/control/control_architecture.pngis excluded by!**/*.pngdoc/assets/control/emergency_brake_stats_graph.pngis excluded by!**/*.png
📒 Files selected for processing (18)
code/acting/launch/acting.launch(1 hunks)code/acting/src/acting/passthrough.py(1 hunks)code/control/CMakeLists.txt(1 hunks)code/control/launch/control.launch(1 hunks)code/control/package.xml(1 hunks)code/control/setup.py(1 hunks)code/control/src/pure_pursuit_controller.py(3 hunks)code/control/src/stanley_controller.py(3 hunks)code/control/src/velocity_controller.py(1 hunks)doc/acting/README.md(1 hunks)doc/acting/acting_testing.md(1 hunks)doc/acting/architecture_documentation.md(2 hunks)doc/acting/passthrough.md(1 hunks)doc/control/README.md(1 hunks)doc/control/architecture_documentation.md(1 hunks)doc/control/steering_controllers.md(4 hunks)doc/control/vehicle_controller.md(3 hunks)doc/control/velocity_controller.md(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- code/control/CMakeLists.txt
- code/control/package.xml
- code/control/setup.py
- doc/control/vehicle_controller.md
- doc/control/velocity_controller.md
🧰 Additional context used
🪛 Markdownlint
doc/acting/README.md
8-8: null
Files should end with a single newline character
(MD047, single-trailing-newline)
doc/acting/architecture_documentation.md
17-17: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
24-24: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
27-27: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
76-76: null
Files should end with a single newline character
(MD047, single-trailing-newline)
doc/acting/passthrough.md
13-13: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
19-19: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
23-23: null
Files should end with a single newline character
(MD047, single-trailing-newline)
doc/control/README.md
8-8: null
Files should end with a single newline character
(MD047, single-trailing-newline)
doc/control/architecture_documentation.md
3-3: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
22-22: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
27-27: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
28-28: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
31-31: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
53-53: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
27-27: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
53-53: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
66-66: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
81-81: null
Files should end with a single newline character
(MD047, single-trailing-newline)
🪛 LanguageTool
doc/acting/architecture_documentation.md
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~25-~25: You might be missing the article “the” here.
Context: ...gh.py > [!TIP] > For documentation on passthrough component see: [passthrough](./passthro...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
doc/acting/passthrough.md
[grammar] ~7-~7: Do not use the modal verb ‘ought’ after the verb ‘be’. Did you mean “ought”?
Context: ...ng and control. Summary: This page is ought to provide an overview of the passthrou...
(BE_MD)
[grammar] ~20-~20: Did you mean to write ‘therefore’?
Context: ...e it resided within the acting package. Therefor many global dependencies existed in the...
(THEREFORE)
[uncategorized] ~21-~21: A comma might be missing here.
Context: ...ded within the acting package. Therefor many global dependencies existed in the cont...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
doc/control/architecture_documentation.md
[style] ~6-~6: Consider a shorter alternative to avoid wordiness.
Context: ...dge of the current state of the vehicle in order to send [CarlaEgoVehicleControl](https://c...
(IN_ORDER_TO_PREMIUM)
[style] ~67-~67: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1958 characters long)
Context: ...: Float32 ### vehicle_controller.py > [!TIP] > Follow this link for [Documentati...
(EN_EXCESSIVE_EXCLAMATION)
doc/control/steering_controllers.md
[typographical] ~21-~21: It appears that a comma is missing.
Context: ...ng/Control/PurePursuit.html). At every moment it checks a point of the trajectory in ...
(DURING_THAT_TIME_COMMA)
[uncategorized] ~41-~41: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...features to determine a steering-output is the so-called cross-track-error (e_...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🔇 Additional comments (22)
doc/acting/README.md (2)
5-8: Review documentation completeness after architecture split
Given that this PR splits the "acting" functionality into separate "acting" and "control" components:
- The removal of controller-specific documentation (
velocity_controller.mdandsteering_controllers.md) suggests this content might need to be migrated to the control component's documentation. - Consider adding a reference or link to the new control component's documentation to help users understand the relationship between these components.
#!/bin/bash
# Description: Check for control component documentation
echo "Checking for control component documentation..."
# Look for potential control documentation directory
fd -t d "control" -d 2
# Check if there are any markdown files mentioning the relationship between acting and control
rg -l "acting.*control|control.*acting" -g "*.md"🧰 Tools
🪛 Markdownlint
8-8: null
Files should end with a single newline character
(MD047, single-trailing-newline)
5-8: Verify the existence of linked documentation files
The documentation structure has changed significantly. Let's verify that all referenced markdown files exist and are properly linked.
✅ Verification successful
All documentation files exist and are properly structured
All referenced markdown files in the README.md are present and contain relevant content:
- architecture_documentation.md: Contains comprehensive architecture overview
- passthrough.md: Documents the temporary passthrough solution
- main_frame_publisher.md: Details the frame publisher functionality
- acting_testing.md: Provides testing instructions
The documentation structure accurately reflects the architectural changes splitting "acting" and "control" components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all referenced markdown files exist
echo "Checking referenced documentation files..."
for file in architecture_documentation.md passthrough.md main_frame_publisher.md acting_testing.md; do
if [ -f "doc/acting/$file" ]; then
echo "✓ Found: $file"
else
echo "✗ Missing: $file"
fi
done
Length of output: 511
Script:
#!/bin/bash
# Let's check the content of the linked files to verify they exist and are properly structured
echo "Checking architecture_documentation.md..."
cat doc/acting/architecture_documentation.md 2>/dev/null || echo "File not found"
echo -e "\nChecking passthrough.md..."
cat doc/acting/passthrough.md 2>/dev/null || echo "File not found"
echo -e "\nChecking main_frame_publisher.md..."
cat doc/acting/main_frame_publisher.md 2>/dev/null || echo "File not found"
echo -e "\nChecking acting_testing.md..."
cat doc/acting/acting_testing.md 2>/dev/null || echo "File not found"
# Also check if there are any other markdown files in the acting directory
echo -e "\nListing all markdown files in doc/acting/..."
ls -1 doc/acting/*.md 2>/dev/null || echo "No markdown files found"
Length of output: 10620
🧰 Tools
🪛 Markdownlint
8-8: null
Files should end with a single newline character
(MD047, single-trailing-newline)
code/acting/launch/acting.launch (3)
18-20: Verify control.launch file setup
The inclusion of control.launch aligns with the PR objective to split acting and control functionality. However, we should verify:
- The existence of the control.launch file
- That it properly handles the control_loop_rate argument
#!/bin/bash
# Description: Verify control.launch file setup
# Check if control.launch exists and contains proper argument declaration
fd -e launch -x cat {} \; -g "control.launch"
# Verify control package is properly declared as dependency
rg "control" "package.xml"13-16: Verify MainFramePublisher's dependency on control parameters
Both the passthrough and MainFramePublisher nodes use the same control_loop_rate parameter. Please verify if MainFramePublisher actually needs to be synchronized with the control loop rate.
✅ Verification successful
MainFramePublisher requires control_loop_rate for frame transformation timing
The MainFramePublisher node uses control_loop_rate to synchronize the transformation broadcasts between the global and hero frames. The parameter is used to set the timer frequency for publishing transforms (self.new_timer(self.control_loop_rate, loop)), which is essential for proper coordinate frame transformations used by sensors and visualization.
Key findings:
- The node publishes TF transforms that convert between static main frame and moving hero frame
- Transform updates must be synchronized with the control rate for consistent sensor data and visualization
- Default value of 0.05 matches the control system's update rate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how MainFramePublisher uses the control_loop_rate parameter
# Search for control_loop_rate usage in MainFramePublisher
ast-grep --pattern 'rospy.get_param("control_loop_rate")'
rg "control_loop_rate" --type py -g "MainFramePublisher.py"
Length of output: 1427
Script:
#!/bin/bash
# Let's try to find and examine the MainFramePublisher.py file first
fd MainFramePublisher.py
# Then let's look for any rate or loop related code in it
fd MainFramePublisher.py -x cat {} ';'
# Also search for any launch files that include MainFramePublisher
fd -e launch -x grep -l "MainFramePublisher" {} ';'
Length of output: 3283
4-4: Verify the control loop rate value
The control loop rate of 0.05 (20Hz) seems quite low for a control system. This could impact the system's responsiveness and performance.
✅ Verification successful
The initial search revealed an important comment in control.launch indicating the rate is specifically chosen for leaderboard compatibility. Let's gather more context about the control system implementation and documentation to make a fully informed decision.
Control loop rate of 0.05s (20Hz) is correctly set
The rate of 0.05s is intentionally chosen and consistent across the system:
- It matches CARLA simulator's fixed delta seconds (carla_ros_bridge.launch)
- A crucial comment in control.launch states "leaderboard expects commands every 0.05 seconds OTHERWISE IT LAGS REALLY BADLY"
- This rate is consistently used across all components (perception, planning, acting, control)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any documentation or comments about the control loop rate
# and check if this value is used consistently across the codebase
# Check for documentation about control loop rate
rg -i "control.*loop.*rate|loop.*rate|frequency|hz" "doc/"
# Check for other occurrences of this value
rg "0\.05|20.*hz" --type py --type launch
Length of output: 1427
Script:
#!/bin/bash
# Let's check Python files in control package for any timing-related code or comments
fd -e py -g 'control/' -x cat {} ';'
# Also check for any documentation files
fd README.md README.txt -x cat {} ';'
# And check for any other launch files that might reference this rate
fd -e launch -x cat {} ';'
Length of output: 14113
code/control/launch/control.launch (1)
1-28: Verify node dependencies and integration
Please verify:
- Whether these nodes have any required startup order
- The integration with the acting package's passthrough node
✅ Verification successful
Let me generate additional scripts to gather more information about the integration between acting and control packages, since the previous results didn't show the complete picture.
Let me try one more search strategy to find the connections between these packages:
Based on the code analysis, I can now provide a definitive response about the node dependencies and integration:
No strict startup order required, integration through passthrough node is verified
The control nodes are well-integrated through the acting package's passthrough node with clear topic mappings:
- The passthrough node (
code/acting/src/passthrough.py) handles all required topic translations:/paf/acting/target_velocity→/paf/hero/target_velocity/paf/acting/trajectory→/paf/hero/trajectory/paf/acting/current_pos→/paf/hero/current_pos/paf/acting/current_heading→/paf/hero/current_heading
The nodes can start in any order since:
- All nodes use ROS topic subscriptions which handle asynchronous connections
- Each controller node has proper null checks and debug logging for missing data
- The vehicle_controller intelligently switches between Stanley and Pure Pursuit based on velocity (>5 m/s uses Stanley)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for node dependencies and integration
# Look for node dependencies in launch files
echo "Checking for node dependencies..."
rg --type xml "depend|require" -A 5 -B 5
# Check how these nodes are referenced in the acting package
echo "Checking integration with acting package..."
rg --type xml "control.launch" -A 10 -B 10
# Look for topic connections between acting and control packages
echo "Checking for topic connections..."
ast-grep --pattern 'rospy.Publisher($_, $_)'
ast-grep --pattern 'rospy.Subscriber($_, $_)'
Length of output: 22512
Script:
#!/bin/bash
# Check for launch file inclusions and node dependencies
echo "Checking launch file inclusions..."
rg --type xml "include.*control.launch" -A 5 -B 5
# Look for topic connections in control nodes
echo "Checking control nodes for topic connections..."
rg "rospy\.(Publisher|Subscriber)" code/control/ -A 2 -B 2
# Check acting nodes for control-related topics
echo "Checking acting nodes for control-related topics..."
rg "rospy\.(Publisher|Subscriber)" code/acting/ -A 2 -B 2
# Check for any launch file dependencies or required conditions
echo "Checking launch conditions..."
rg --type xml "required=\"true\"" -A 5 -B 5
Length of output: 582
Script:
#!/bin/bash
# Search for launch file references in all launch files
echo "Checking all launch files for control integration..."
fd -e launch -x cat {}
# Look for Python imports and topic connections in control nodes
echo "Checking control node implementations..."
fd -e py -t f . code/control/ -x cat {}
# Look for Python imports and topic connections in acting nodes
echo "Checking acting node implementations..."
fd -e py -t f . code/acting/ -x cat {}
Length of output: 79003
code/acting/src/acting/passthrough.py (4)
1-13: LGTM! Well-structured imports.
The imports are properly organized and demonstrate good practices:
- ROS compatibility layer for version independence
- Specific message type imports
- Type hints support
71-87: LGTM! Well-structured node initialization.
The main function follows ROS best practices:
- Proper node initialization
- Error handling with graceful shutdown
- Clean entry point implementation
28-28: 🛠️ Refactor suggestion
Make role_name configurable via ROS parameters.
The hard-coded role_name should be configurable to support different robot configurations.
- role_name = "hero" # Legacy will change soon
+ def __init__(self):
+ super().__init__('passthrough')
+ self.role_name = self.get_parameter_or('role_name', 'hero')Likely invalid or redundant comment.
22-26: Consider monitoring message latency.
While the passthrough approach effectively decouples the control package from global dependencies, it introduces an additional hop in the message flow. For time-critical control operations, consider:
- Adding latency monitoring
- Implementing message timestamps validation
- Adding metrics for dropped messages
doc/control/architecture_documentation.md (1)
29-29: Verify referenced documentation files exist.
Please ensure all linked documentation files are present:
#!/bin/bash
# Check if referenced documentation files exist
docs=(
"doc/control/steering_controllers.md"
"doc/control/velocity_controller.md"
"doc/control/vehicle_controller.md"
)
for doc in "${docs[@]}"; do
if [ -f "$doc" ]; then
echo "Found: $doc"
else
echo "Missing: $doc"
fi
doneAlso applies to: 55-55, 68-68
doc/acting/architecture_documentation.md (1)
7-11: LGTM! Table of contents accurately reflects the new architecture.
The restructured ToC clearly shows the separation of acting and control components, aligning with the PR objectives.
code/control/src/velocity_controller.py (1)
26-26: Verify topic naming convention consistency
While the target velocity subscription has been updated to use a fixed path /paf/acting/target_velocity, other topics (throttle, brake, reverse) still use role-based naming. This mixed approach might need to be reviewed for consistency with the acting/control split architecture.
✅ Verification successful
Topic naming pattern is intentionally split between fixed and role-based paths
The mixed approach in topic naming is actually correct and intentional as part of the acting/control split architecture:
- The
/paf/acting/*fixed paths are used for communication between acting and control nodes - Role-based paths (
/paf/{role_name}/*) are used for internal communication within each component
This is evidenced by:
- All controllers (velocity, stanley, pure pursuit) consistently subscribe to
/paf/acting/*topics for inputs - The acting/passthrough.py module explicitly maps between role-based and fixed paths
- Internal control topics (throttle, brake, reverse) correctly maintain role-based naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other potential topic references that might need updating
rg -A 2 "/paf/acting/" code/
# Check for remaining role-based topics
rg -A 2 "f\"/paf/\{self\.role_name\}" code/
Length of output: 23099
doc/control/steering_controllers.md (4)
Line range hint 27-31: LGTM: Mathematical formulas are well-documented.
The mathematical formulas for the PurePursuit controller are clearly presented with proper LaTeX formatting and variable explanations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~41-~41: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...features to determine a steering-output is the so-called cross-track-error (e_...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[typographical] ~46-~46: It appears that a comma is missing.
Context: ...control/Steering_Stanley.png) At every moment it checks the closest point of the traj...
(DURING_THAT_TIME_COMMA)
Line range hint 50-54: LGTM: Stanley controller implementation details are clear.
The explanation of the Stanley controller's two main components (trajectory-heading and cross-track-error) is thorough and well-documented with proper mathematical notation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~59-~59: If “ why” starts an indirect question, you do not need to put a comma before it.
Context: ...ey alot "weaker". We do not exactly know, why the two launches are this different. (D...
(NO_COMMA_BEFORE_INDIRECT_QUESTION)
18-18: Verify the updated path to the PurePursuit Controller implementation.
The documentation references the new path ../../code/control/src/pure_pursuit_controller.py. Let's verify this path exists after the restructuring.
✅ Verification successful
Path to PurePursuit Controller is correctly updated
The referenced path ../../code/control/src/pure_pursuit_controller.py in the documentation is verified to exist in the codebase after the restructuring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the PurePursuit controller implementation
fd "pure_pursuit_controller.py" | grep "control/src"
Length of output: 94
23-23: Verify the updated image paths in the assets directory.
The documentation references several images that have been moved from acting to control directory. Let's verify these paths exist after the restructuring.
Also applies to: 34-34, 44-44, 57-57
✅ Verification successful
Image paths in the assets directory are correctly updated
The verification confirms that all referenced steering controller images are present in the new location under doc/assets/control/:
Steering_PurePursuit.pngSteering_Stanley.png- And additional related images
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of controller images in the new location
echo "Checking for controller images in the new location..."
fd -e png . | grep "assets/control" | grep -E "Steering_(PurePursuit|Stanley)"
Length of output: 388
code/control/src/pure_pursuit_controller.py (2)
14-14: LGTM: Helper functions properly imported
The import of helper functions from the acting package is well-placed and the functions are properly utilized in the code.
Line range hint 36-57: Verify topic compatibility across the system
The subscription topics have been centralized under the 'acting' namespace, removing role-based naming. While this aligns with the restructuring goals, we should verify that:
- All publishers to these topics have been updated accordingly
- No other nodes still expect the role-based topic names
✅ Verification successful
Topic namespace changes require coordination with passthrough node
The verification reveals that the passthrough node (code/acting/src/acting/passthrough.py) is already handling the namespace transition correctly:
trajectory = passthrough(
pub_name="/paf/acting/trajectory",
sub_name=f"/paf/{role_name}/trajectory",
...
)
position = passthrough(
pub_name="/paf/acting/current_pos",
sub_name=f"/paf/{role_name}/current_pos",
...
)
heading = passthrough(
pub_name="/paf/acting/current_heading",
sub_name=f"/paf/{role_name}/current_heading",
...
)The passthrough node acts as a bridge, subscribing to role-based topics and republishing to the new centralized /paf/acting/ namespace. This ensures compatibility between:
- Publishers using role-based topics (e.g., position_heading_publisher_node.py)
- Subscribers using centralized topics (e.g., pure_pursuit_controller.py)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify topic publishers and subscribers across the system
# Check for any remaining references to role-based trajectory topics
echo "Checking for role-based trajectory topic references..."
rg -l "paf/.+/trajectory" --type py
# Check for any remaining references to role-based current_pos topics
echo "Checking for role-based current_pos topic references..."
rg -l "paf/.+/current_pos" --type py
# Check for any remaining references to role-based current_heading topics
echo "Checking for role-based current_heading topic references..."
rg -l "paf/.+/current_heading" --type py
# Check for publishers to the new centralized topics
echo "Checking for publishers to new centralized topics..."
rg -l "publish.*paf/acting/(trajectory|current_pos|current_heading)" --type py
Length of output: 2241
Script:
#!/bin/bash
# Let's examine the actual topic references in these files to understand the context better
echo "Checking trajectory topic usage..."
rg "paf/.+/trajectory|paf/acting/trajectory" -B2 -A2 --type py
echo -e "\nChecking current_pos topic usage..."
rg "paf/.+/current_pos|paf/acting/current_pos" -B2 -A2 --type py
echo -e "\nChecking current_heading topic usage..."
rg "paf/.+/current_heading|paf/acting/current_heading" -B2 -A2 --type py
# Also check for any topic publishers in the codebase
echo -e "\nChecking for topic publishers..."
ast-grep --pattern 'self.create_publisher($_, $_, "paf/acting/trajectory")'
ast-grep --pattern 'self.create_publisher($_, $_, "paf/acting/current_pos")'
ast-grep --pattern 'self.create_publisher($_, $_, "paf/acting/current_heading")'
Length of output: 19449
code/control/src/stanley_controller.py (3)
Line range hint 1-283: Implementation looks solid!
The Stanley Controller implementation is well-structured with:
- Clear separation of concerns
- Proper error handling for edge cases
- Comprehensive documentation
- Consistent coding style
The only issues are the topic naming inconsistencies noted above.
15-15: Verify the new import path accessibility
The import path has been updated to use the new module structure acting.helper_functions.
✅ Verification successful
Import path is correctly structured and accessible
The verification confirms:
- The
helper_functions.pymodule exists at the correct location:code/acting/src/acting/helper_functions.py - The import is consistently used across both controller implementations:
code/control/src/stanley_controller.pycode/control/src/pure_pursuit_controller.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import path exists and is accessible
# Test: Check if the helper_functions module exists in the acting package
fd -e py "helper_functions.py" | grep "acting"
# Test: Check for any other potential imports of these functions
rg "from.*helper_functions import (vector_angle|points_to_vector)"
Length of output: 373
31-31: Verify topic compatibility across the system
The subscription topics have been changed to use a static "acting" namespace instead of the dynamic role_name. While this aligns with the PR's restructuring goal, we need to ensure all publishers have been updated accordingly.
Also applies to: 36-38, 50-52
✅ Verification successful
Topic changes are properly handled through a passthrough node
The change to static "acting" namespace is safe because:
- The
passthrough.pynode in the acting package acts as a bridge, subscribing to the old dynamic topics (/paf/{role_name}/...) and republishing them to the new static topics (/paf/acting/...) - All publishers still use the dynamic
role_namepattern - The control nodes (stanley_controller.py, pure_pursuit_controller.py) are the only subscribers to the new static topics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify topic publishers and subscribers across the system
# Test: Search for any remaining uses of the old topic pattern
rg "/paf/\$\{.*role_name\}/(trajectory|current_pos|current_heading)"
# Test: Find all publishers to the new topics
ast-grep --pattern 'new_publisher($_, "/paf/acting/(trajectory|current_pos|current_heading)")'
Length of output: 166
Script:
#!/bin/bash
# Let's try a broader search for topic patterns and publishers
# Search for any remaining dynamic role_name patterns in topic strings
rg -g '*.py' -g '*.cpp' -g '*.hpp' "role_name.*trajectory|role_name.*current_pos|role_name.*current_heading" -A 2
# Search for the new static topic patterns
rg -g '*.py' -g '*.cpp' -g '*.hpp' "/paf/acting/(trajectory|current_pos|current_heading)" -A 2
# Look for create_publisher or Publisher patterns that might indicate topic publishing
ast-grep --pattern 'create_publisher($$$)'
ast-grep --pattern 'Publisher($$$)'
Length of output: 9744
| @dataclass | ||
| class passthrough: | ||
| pub_name: str | ||
| sub_name: str | ||
| topic_type: Type | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename class to follow Python naming conventions.
The class name should be in PascalCase as per PEP 8 conventions.
-class passthrough:
+class Passthrough:📝 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.
| @dataclass | |
| class passthrough: | |
| pub_name: str | |
| sub_name: str | |
| topic_type: Type | |
| @dataclass | |
| class Passthrough: | |
| pub_name: str | |
| sub_name: str | |
| topic_type: Type | |
|
|
||
| ## Control Architecture | ||
|
|
||
|  |
There was a problem hiding this comment.
💡 Codebase verification
Fix the image path in the architecture documentation
The control architecture diagram exists at doc/assets/control/control_architecture.png, but the documentation is using an incorrect relative path (./../assets/control/control_architecture.png). Update the image reference to use the correct relative path: ../assets/control/control_architecture.png.
🔗 Analysis chain
Add the missing control architecture diagram.
The architecture diagram is marked as missing but is crucial for understanding the system's structure. Please ensure the image file exists at ./../assets/control/control_architecture.png.
Let me verify if the image exists in any other locations or if there are similar image files in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the architecture diagram exists
if [ -f "assets/control/control_architecture.png" ]; then
echo "Image exists"
else
echo "Image missing"
fi
Length of output: 620
Script:
#!/bin/bash
# Search for control architecture image files in the repository
fd "control.*architecture.*\.png"
# Search for any PNG files in assets or doc directories containing "control"
fd ".*\.png" assets doc | grep -i "control"
Length of output: 678
|  | ||
|
|
||
| ## Summary of Acting Components | ||
| > [!NOTE] | ||
| > [Click here to go to control architecture](./../control/architecture_documentation.md) |
There was a problem hiding this comment.
💡 Codebase verification
Architecture diagram is missing and needs to be added
The architecture diagram file acting_architecture.png is missing from the expected path. Please:
- Add the architecture diagram at
assets/acting/acting_architecture.png - Ensure the diagram accurately represents the acting architecture
- Remove the "MISSING" text from the image alt once the diagram is added
🔗 Analysis chain
Verify architecture diagram and fix formatting.
- The "MISSING" text in the image alt suggests the diagram might be missing. Please verify that the diagram exists at
./../assets/acting/acting_architecture.png. - Remove trailing spaces after the note blockquote marker.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the architecture diagram
if [ -f "assets/acting/acting_architecture.png" ]; then
echo "Architecture diagram exists"
else
echo "Architecture diagram is missing"
fi
Length of output: 123
🧰 Tools
🪛 Markdownlint
17-17: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
doc/acting/passthrough.md (4)
4-5: Fix typo in the note sectionThere's a typo in the word "beeing" which should be "being".
->The passthrough component is a temporary solution while beeing in the transitioning phase of +>The passthrough component is a temporary solution while being in the transitioning phase of
7-8: Improve summary sentence structureThe current wording is grammatically incorrect.
-**Summary:** This page is ought to provide an overview of the passthrough node and -reason why it exists +**Summary:** This page provides an overview of the passthrough node and explains its purpose🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Do not use the modal verb ‘ought’ after the verb ‘be’. Did you mean “ought”?
Context: ...ng and control. Summary: This page is ought to provide an overview of the passthrou...(BE_MD)
22-25: Improve readability of the reasoning sectionThe section provides valuable context but needs grammatical improvements.
-Before the control package was outsourced and became its own package it resided within the acting package. -Therefor many global dependencies existed in the control package. As the goal of the outsourcing was -to eliminate global dependencies in control theory the passthrough node was created as a first stepping -stone towards independence. +Before the control package was outsourced and became its own package, it resided within the acting package. +Therefore, many global dependencies existed in the control package. As the goal of the outsourcing was +to eliminate global dependencies in control theory, the passthrough node was created as a first stepping +stone towards independence.🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: A comma might be missing here.
Context: ...ckage was outsourced and became its own package it resided within the acting package. T...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~22-~22: Did you mean to write ‘therefore’?
Context: ...e it resided within the acting package. Therefor many global dependencies existed in the...(THEREFORE)
[uncategorized] ~23-~23: A comma might be missing here.
Context: ...ded within the acting package. Therefor many global dependencies existed in the cont...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...liminate global dependencies in control theory the passthrough node was created as a f...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
25-25: Consider adding migration timeline informationSince this is described as a temporary solution, it would be helpful to add a section about:
- The conditions or criteria for removing this temporary component
- Any planned timeline for the complete separation of packages
- Steps needed to achieve full independence between packages
doc/control/architecture_documentation.md (2)
4-7: Minor: Consider simplifying the wordingThe summary is clear and well-documented. Consider simplifying "in order to" to "to" for more concise wording:
-of the vehicle in order to send [CarlaEgoVehicleControl] +of the vehicle to send [CarlaEgoVehicleControl]🧰 Tools
🪛 LanguageTool
[style] ~6-~6: Consider a shorter alternative to avoid wordiness.
Context: ...dge of the current state of the vehicle in order to send [CarlaEgoVehicleControl](https://c...(IN_ORDER_TO_PREMIUM)
73-81: Consider grouping related inputs in vehicle_controller.pyThe inputs could be more organized by grouping related parameters:
- Inputs: - - **emergency**: Bool - - **curr_behavior**: String - - **Speed**: CarlaSpeedometer - - **throttle**: Float32 - - **brake**: Float32 - - **pure_pursuit_steer**: Float32 - - **stanley_steer**: Float32 + Inputs: + Status: + - **emergency**: Bool + - **curr_behavior**: String + - **Speed**: CarlaSpeedometer + Control: + - **throttle**: Float32 + - **brake**: Float32 + Steering: + - **pure_pursuit_steer**: Float32 + - **stanley_steer**: Float32doc/acting/architecture_documentation.md (3)
3-5: Improve clarity in the summaryThe summary effectively explains the relationship between components, but can be more concise.
Consider this revision:
-**Summary**: The acting component receives information from the [planning component](./../planning/README.md) as well -as the [perception component](./../perception/README.md). It processes this information in order to -navigate on a local basis. This information is then processed in the [control_component](./../control/README.md). +**Summary**: The acting component receives information from the [planning component](./../planning/README.md) and +[perception component](./../perception/README.md). It processes this information for local navigation, +which is then handled by the [control_component](./../control/README.md).🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...(IN_ORDER_TO_PREMIUM)
33-33: Fix typo in input typeThere's a typo in the input type.
-- **current_pos**: PoseStampled +- **current_pos**: PoseStamped
Line range hint
45-83: Consider organizing helper functions by componentWith the split between acting and control components, consider grouping these helper functions based on their primary usage (acting vs. control) or moving control-specific functions to the control component.
Consider:
- Creating separate helper modules for acting and control
- Moving control-specific functions (e.g., those related to vehicle control calculations) to the control component
- Keeping only generic utilities in this shared helper module
Would you like assistance in analyzing function usage patterns to determine the optimal organization?
🧰 Tools
🪛 LanguageTool
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
doc/acting/README.md(1 hunks)doc/acting/architecture_documentation.md(1 hunks)doc/acting/passthrough.md(1 hunks)doc/control/README.md(1 hunks)doc/control/architecture_documentation.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- doc/acting/README.md
- doc/control/README.md
🧰 Additional context used
🪛 LanguageTool
doc/acting/architecture_documentation.md
[style] ~4-~4: Consider a shorter alternative to avoid wordiness.
Context: ...ADME.md). It processes this information in order to navigate on a local basis. This informa...
(IN_ORDER_TO_PREMIUM)
doc/acting/passthrough.md
[grammar] ~7-~7: Do not use the modal verb ‘ought’ after the verb ‘be’. Did you mean “ought”?
Context: ...ng and control. Summary: This page is ought to provide an overview of the passthrou...
(BE_MD)
[uncategorized] ~22-~22: A comma might be missing here.
Context: ...ckage was outsourced and became its own package it resided within the acting package. T...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~22-~22: Did you mean to write ‘therefore’?
Context: ...e it resided within the acting package. Therefor many global dependencies existed in the...
(THEREFORE)
[uncategorized] ~23-~23: A comma might be missing here.
Context: ...ded within the acting package. Therefor many global dependencies existed in the cont...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~24-~24: A comma might be missing here.
Context: ...liminate global dependencies in control theory the passthrough node was created as a f...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
doc/control/architecture_documentation.md
[style] ~6-~6: Consider a shorter alternative to avoid wordiness.
Context: ...dge of the current state of the vehicle in order to send [CarlaEgoVehicleControl](https://c...
(IN_ORDER_TO_PREMIUM)
[style] ~70-~70: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 1959 characters long)
Context: ... Float32 ### vehicle_controller.py > [!TIP] > Follow this link for [Documentati...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (4)
doc/acting/passthrough.md (1)
13-19: LGTM! Clear and well-referenced overview section
The overview effectively explains the node's functionality and provides a helpful reference to the architecture documentation.
doc/control/architecture_documentation.md (2)
9-16: LGTM: Well-structured table of contents
The table of contents provides a clear and logical structure for the documentation.
20-23: Fix relative paths in documentation
The following paths need to be corrected:
- Architecture diagram path
- Acting architecture documentation path
-
+
-> [Click here to go to acting architecture](./../acting/architecture_documentation.md)
+> [Click here to go to acting architecture](../acting/architecture_documentation.md)doc/acting/architecture_documentation.md (1)
37-43: Verify frame transformation calculations
The frame transformation calculations look correct but should be verified to ensure they match the implementation, especially given the architectural split between acting and control components.
✅ Verification successful
Let me gather more information about the frame transformation implementation.
Let me check the coordinate transformation implementation in perception to ensure consistency.
Frame transformation calculations are correctly implemented
The implementation in MainFramePublisher.py matches the documented transformation in architecture_documentation.md:
- The rotation calculation uses
-current_headingas specified - Position transformations use the correct trigonometric formulas:
pos[0] = cos(rot) * current_pos.pose.position.x - sin(rot) * current_pos.pose.position.y # x pos[1] = sin(rot) * current_pos.pose.position.x + cos(rot) * current_pos.pose.position.y # y pos[2] = -current_pos.pose.position.z # z
- The quaternion transformation and frame naming ("global" to "hero") are also consistent
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for frame transformation implementation
echo "Searching for frame transformation implementation..."
rg -l "TransformBroadcaster" --type py
# Search for the specific transformation calculations
echo "Searching for transformation calculations..."
ast-grep --pattern 'position_x = cos($rotation) * $current_pos + sin($rotation) * $current_pos'
Length of output: 395
Script:
#!/bin/bash
# Check the actual implementation in MainFramePublisher.py
echo "Content of MainFramePublisher.py:"
cat code/acting/src/acting/MainFramePublisher.py
# Search for any other files that might contain frame transformations
echo -e "\nSearching for files with frame/transform related names:"
fd -t f "frame|transform" --exec grep -l "rotation\|position"
Length of output: 3542
Script:
#!/bin/bash
# Check the coordinate transformation implementation
echo "Content of coordinate_transformation.py:"
cat code/perception/src/coordinate_transformation.py
# Check the coordinate transformation documentation
echo -e "\nContent of coordinate_transformation.md:"
cat doc/perception/coordinate_transformation.md
Length of output: 8710
There was a problem hiding this comment.
having a "passthrough" class in upper and lowercase is VERY confusing, but i see one is a dataclass and the other is not.
could you please explain why it is not possible to name the dataclass different ?
There was a problem hiding this comment.
Copilot reviewed 23 out of 31 changed files in this pull request and generated no suggestions.
Files not reviewed (8)
- code/acting/launch/acting.launch: Language not supported
- code/control/CMakeLists.txt: Language not supported
- code/control/launch/control.launch: Language not supported
- code/control/package.xml: Language not supported
- code/control/src/pure_pursuit_controller.py: Evaluated as low risk
- code/control/src/stanley_controller.py: Evaluated as low risk
- code/control/src/velocity_controller.py: Evaluated as low risk
- doc/acting/README.md: Evaluated as low risk
Comments skipped due to low confidence (1)
doc/acting/acting_testing.md:13
- The word 'necesarry' is misspelled. It should be 'necessary'.
There is a dedicated [acting_debug.launch](../../code/acting/launch/acting_debug.launch) file which starts the acting component **as well as the necesarry perception components** automatically.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
code/acting/src/acting/passthrough.py (3)
59-68: Add error handling and QoS documentationThe publisher/subscriber setup needs improvement in two areas:
- The hardcoded QoS profile value of 1 should be documented or made configurable
- Missing error handling for failed message delivery
Consider adding:
def __init__(self): + super().__init__('passthrough') + self.qos_profile = self.get_parameter_or('qos_profile', 1).value self.publishers: Dict[str, Publisher] = {} self.subscribers: Dict[str, Subscriber] = {} for topic in self.mapped_topics: self.publishers[topic.pub_name] = self.new_publisher( topic.topic_type, topic.pub_name, - qos_profile=1 + qos_profile=self.qos_profile ) + def safe_callback(msg): + try: + self.publishers[topic.pub_name].publish(msg) + except Exception as e: + self.get_logger().error(f"Failed to publish message: {e}") self.subscribers[topic.pub_name] = self.new_subscription( topic.topic_type, topic.sub_name, - callback=self.publishers[topic.pub_name].publish, + callback=safe_callback, - qos_profile=1, + qos_profile=self.qos_profile, )
71-84: Enhance error handling in main functionWhile the function handles KeyboardInterrupt, it should catch and log other potential exceptions for better debugging.
def main(args=None): roscomp.init("passthrough", args=args) + node = None try: node = Passthrough() node.spin() except KeyboardInterrupt: pass + except Exception as e: + import traceback + print(f"Node failed with error: {e}") + print(traceback.format_exc()) + return 1 finally: + if node: + node.destroy_node() roscomp.shutdown() + return 0
22-26: Consider documenting the architectural decisionThe passthrough node effectively facilitates the separation of acting and control packages. Consider adding more detailed documentation about:
- The architectural decision behind this separation
- The relationship between acting and control packages
- Future plans for removing legacy dependencies
Would you like me to help draft an architectural decision record (ADR) for this change?
| target_velocity = TopicMapping( | ||
| pub_name="/paf/acting/target_velocity", | ||
| sub_name=f"/paf/{role_name}/target_velocity", | ||
| topic_type=Float32, | ||
| ) | ||
| # Topics for steering controllers | ||
| trajectory = TopicMapping( | ||
| pub_name="/paf/acting/trajectory", | ||
| sub_name=f"/paf/{role_name}/trajectory", | ||
| topic_type=Path, | ||
| ) | ||
| position = TopicMapping( | ||
| pub_name="/paf/acting/current_pos", | ||
| sub_name=f"/paf/{role_name}/current_pos", | ||
| topic_type=PoseStamped, | ||
| ) | ||
| heading = TopicMapping( | ||
| pub_name="/paf/acting/current_heading", | ||
| sub_name=f"/paf/{role_name}/current_heading", | ||
| topic_type=Float32, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve topic configuration management
The topic paths are currently hardcoded with f-strings. Consider moving these configurations to a separate configuration file or ROS parameters for better maintainability.
Example approach:
def get_topic_mappings(self):
return [
TopicMapping(
pub_name=self.get_parameter_or('target_velocity_pub', '/paf/acting/target_velocity').value,
sub_name=self.get_parameter_or('target_velocity_sub', f'/paf/{self.role_name}/target_velocity').value,
topic_type=Float32
),
# ... other mappings
]| but is only dependent on the acting package. | ||
| """ | ||
|
|
||
| role_name = "hero" # Legacy will change soon |
There was a problem hiding this comment.
Remove hardcoded role_name value
The comment indicates this is legacy code that will change soon. Consider making this configurable through ROS parameters instead of hardcoding.
- role_name = "hero" # Legacy will change soon
+ def __init__(self):
+ super().__init__('passthrough')
+ self.role_name = self.get_parameter_or('role_name', 'hero').valueCommittable suggestion skipped: line range outside the PR's diff.
Description
Split up acting into acting and control. No functional changes.
Fixes # (issue)
#421
Please delete options that are not relevant.
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:
Summary by CodeRabbit
Release Notes
New Features
Documentation
acting_testing.mddocument regarding the functionality of theActing_Debug_Node.Bug Fixes