685 feature remodel acc third iteration #698
Conversation
…ure-remodel-acc-third-iteration
…ure-remodel-acc-third-iteration
WalkthroughThis pull request updates configuration files and control logic across multiple components. The changes correct a route file path in the agent service YAML, refine a velocity threshold in the controller, and overhaul ACC configurations by replacing Stop & Go parameters with PI controller settings including an acceleration limiter. Furthermore, modifications in the ACC and MotionPlanning classes accommodate leading vehicle information and behavior-based speed adjustments. A legacy ACC implementation is removed, and new XML route files for bicycle and highway scenarios are added. Changes
Sequence Diagram(s)sequenceDiagram
participant ROS as "/paf/{role_name}/curr_behavior"
participant ACC as "ACC Node"
participant Config as "Dynamic Reconfigure"
ROS->>ACC: Publish behavior message
ACC->>ACC: Execute __set_curr_behavior(data)
Config->>ACC: Update acceleration_factor based on config
ACC->>ACC: update_velocity() uses __curr_behavior for speed adjustments
sequenceDiagram
participant Sensor as "Sensor Data"
participant MP as "MotionPlanning"
Sensor->>MP: Provide speeds (be_speed, acc_speed, corner_speed)
MP->>MP: Aggregate candidate speeds
MP->>MP: Determine target speed and set target_velocity_selector
Assessment against linked issues
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
code/planning/src/local_planner/ACC.py (2)
187-187: Minor docstring update recognized.
Thank you for clarifying the heading’s purpose in the docstring.
303-310: Front mask distance adjustment based on current behavior.
This conditional approach is straightforward. However, if new behavior states are introduced in the future, consider centralizing these mappings (e.g., a dictionary) for easier extension.code/planning/config/ACC.cfg (1)
14-15: Typographical correction suggested.
“accelaration” in the parameter description should be spelled “acceleration.” Otherwise, adding acceleration_factor is a valuable refinement.Apply the following diff to fix the spelling:
- gen.add("acceleration_factor", double_t, 0, "Limits the accelaration", 0.0, 0.0, 1.0) + gen.add("acceleration_factor", double_t, 0, "Limits the acceleration", 0.0, 0.0, 1.0)code/planning/config/acc_config.yaml (1)
6-6: Document the new acceleration_factor parameter.Please add a comment explaining:
- The purpose of the acceleration_factor parameter
- How it affects the ACC behavior
- Why the value 0.1 was chosen
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.vscode/settings.json(1 hunks)build/agent_service.yaml(1 hunks)code/control/src/velocity_controller.py(1 hunks)code/planning/config/ACC.cfg(1 hunks)code/planning/config/acc_config.yaml(1 hunks)code/planning/src/behavior_agent/behaviors/overtake.py(1 hunks)code/planning/src/local_planner/ACC.py(12 hunks)code/planning/src/local_planner/ACC_old.py(0 hunks)code/planning/src/local_planner/motion_planning.py(3 hunks)code/routes/routes_bicycle.xml(1 hunks)code/routes/routes_highway.xml(1 hunks)
💤 Files with no reviewable changes (1)
- code/planning/src/local_planner/ACC_old.py
✅ Files skipped from review due to trivial changes (1)
- build/agent_service.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
code/planning/config/acc_config.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (22)
code/planning/src/local_planner/ACC.py (12)
9-9: Import of String looks good.
It is necessary for subscribing to behavior messages, and does not appear to introduce any conflicts.
48-48: New attribute __curr_behavior is appropriate.
Defining the current behavior as an optional string is clear, and its private usage aligns well with the rest of the ACC’s internal state.
116-122: Behavior subscription is well handled.
This subscription properly updates the new __curr_behavior attribute whenever a message is received from the “curr_behavior” topic.
150-150: acceleration_factor attribute introduced.
Storing this in the ACC class for dynamic usage is a good design choice.
166-166: Dynamic reconfigure callback correctly updates acceleration_factor.
This makes runtime tuning seamless.
203-203: Docstring improved for __set_trajectory.
Explanations are consistent with the function’s usage.
210-217: New method __set_curr_behavior is properly scoped.
It is cleanly implemented, contains an updated docstring, and simply assigns the received data to __curr_behavior.
346-346: Initialization of marker_text is appropriate.
This addition primes the debugging information for subsequent concatenations.
358-362: Enriching marker_text for debug info.
Appending more detailed data (distance, velocity, raw ACC speed) is helpful for runtime diagnostics.
365-371: Fallback for missing speed_limit is well-handled.
Driving at 5 m/s if speed_limit is None ensures safer default behavior instead of an unbounded approach.
372-376: Additional text marker for final ACC speed.
Publishing debug markers is beneficial for visualization.
405-437: Overhaul of calculate_velocity_based_on_lead is logically sound.
• The code now checks for overtake wait states and handles minimal distances accordingly.
• The new acceleration_factor usage is applied selectively when hero_velocity is above 1 to ensure gentler acceleration.
• This logic is more flexible, but be sure to validate it thoroughly in real or simulation tests, especially for edge cases (e.g., partial overtakes, large speed differences, or minimal distances).code/control/src/velocity_controller.py (1)
131-131: Lower threshold for very low velocities.
Reducing the threshold from 0.1 to 0.01 will cause the controller to brake sooner for extremely low target velocities. Ensure this aligns with your desired crawling or rolling behavior.code/planning/src/local_planner/motion_planning.py (2)
435-438: Good improvement in error handling!The addition of the
Optional[float]return type and the null check for empty corners list enhances type safety and prevents potential index out of bounds errors.
501-527: Excellent refactoring of the target speed selection logic!The changes improve code maintainability and robustness through:
- List-based gathering of potential speeds reduces code duplication
- Explicit handling of the case when no speeds are available
- Clear selection of target velocity selector based on the chosen speed
code/planning/config/acc_config.yaml (1)
1-4: Verify the impact of control parameter changes.The modifications to control parameters could significantly affect the ACC behavior:
ct_Kpincreased from 0.0 to 0.5 (proportional gain)ct_d_minincreased from 0.6 to 0.7 (minimum distance)Please ensure these changes have been tested and validated in various scenarios.
code/routes/routes_highway.xml (1)
14-17: LGTM! Route extension with consistent coordinates.The new waypoints maintain consistent altitude (z=371.0) and provide a logical extension to the highway route.
.vscode/settings.json (1)
33-34: New VSCode Settings for Enhanced Configuration:
The new setting "cmake.ignoreCMakeListsMissing": true is introduced to allow the workspace to ignore missing CMakeLists.txt files, which can be useful if your project intentionally omits them or uses alternative build systems. Additionally, "markdown.extension.theming.decoration.renderTrailingSpace": true is maintained (now with a trailing comma to integrate into the settings object).code/routes/routes_bicycle.xml (4)
1-10: Route and Weather Configuration:
The file begins with a element enclosing a . The block is clearly defined with two entries that assign weather attributes such as cloudiness, precipitation, wind intensity, and sun angles. These values appear to be hard-coded as intended; just ensure they reflect the desired simulation conditions.
11-15: Waypoint Definitions:
The section defines three elements with explicit x, y, and z coordinates. This clear delineation of spatial waypoints supports a precise route layout.
16-216: Scenario Definitions:
The block comprehensively lists multiple simulation scenarios using elements. Each scenario contains a <trigger_point> along with additional parameters (like distance, frequency, offset, and bicycle-specific attributes) where applicable. Verify that:
• All scenario names and types align with the simulation engine’s specifications.
• The parameters (e.g., "bicycle_drive_distance", "bicycle_speed") are consistent with real-world units and intended simulation behavior.
• If a scenario does not require additional parameters, its structure remains valid.
Overall, the organization is clear and modular.
217-218: XML Structure Closure:
The closing tags for the and elements are correctly placed, ensuring proper XML document termination.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/planning/config/ACC.cfg (1)
14-14: Fix typo in parameter description.There's a typo in the description of the acceleration_factor parameter.
-gen.add("acceleration_factor", double_t, 0, "Limits the accelaration", 0.0, 0.0, 1.0) +gen.add("acceleration_factor", double_t, 0, "Limits the acceleration", 0.0, 0.0, 1.0)code/planning/src/local_planner/ACC.py (2)
299-308: Consider using an enum or constants for behavior strings.Using string literals for behavior comparison is error-prone. Consider defining these as constants or using an enum.
# At the top of the file class VehicleBehavior: OVERTAKE_WAIT_STOPPED = "ot_wait_stopped" OVERTAKE_WAIT_FREE = "ot_wait_free" OVERTAKE_APPROACH_BLOCKED = "ot_app_blocked" OVERTAKE_LEAVE = "ot_leave" # Then in the code if self.__curr_behavior in [ VehicleBehavior.OVERTAKE_WAIT_STOPPED, VehicleBehavior.OVERTAKE_WAIT_FREE, VehicleBehavior.OVERTAKE_APPROACH_BLOCKED, VehicleBehavior.OVERTAKE_LEAVE, ]:
405-411: Consider adding a configuration parameter for the overtaking distance multiplier.The
6 * d_mindistance check for overtaking is hardcoded. Consider making this multiplier configurable.+# In ACC.cfg +gen.add("overtake_distance_multiplier", double_t, 0, "Multiplier for minimum distance during overtaking", 6.0, 1.0, 10.0) +# In ACC.py class +self.overtake_distance_multiplier: float +# In dynamic_reconfigure_callback +self.overtake_distance_multiplier = config["overtake_distance_multiplier"] +# In calculate_velocity_based_on_lead -and lead_distance < 6 * d_min +and lead_distance < self.overtake_distance_multiplier * d_mincode/planning/config/acc_config.yaml (1)
6-6: Add newline at end of file and consider documenting the conservative acceleration factor.The acceleration_factor of 0.1 is very conservative, which is good for safety but might need documentation for future maintainers.
-acceleration_factor: 0.1 +acceleration_factor: 0.1 # Conservative value for smooth acceleration +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/planning/config/ACC.cfg(1 hunks)code/planning/config/acc_config.yaml(1 hunks)code/planning/src/local_planner/ACC.py(11 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
code/planning/config/acc_config.yaml
[error] 6-6: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (4)
code/planning/config/ACC.cfg (1)
10-14: LGTM! Parameter ranges look appropriate for a PI controller implementation.The ranges for controller gains (0.0-3.0), time gap (0.0-5.0s), and minimum distance (0.0-10.0m) are reasonable for an ACC system.
code/planning/src/local_planner/ACC.py (2)
117-122: LGTM! Good addition of behavior tracking.The subscriber for current behavior is well-integrated and follows ROS best practices.
429-430: Good use of acceleration factor for smoother control.The acceleration factor is appropriately applied only when speed_adjustment is positive, ensuring smoother acceleration while maintaining normal deceleration behavior.
code/planning/config/acc_config.yaml (1)
1-4: LGTM! PI controller parameters are well-tuned for stability.The conservative gains (Kp=0.5, Ki=1.2) and safe following distance (T_gap=1.9s) should provide stable control.
.vscode/settings.json
Outdated
There was a problem hiding this comment.
File shouldn't be in PR
build/agent_service.yaml
Outdated
There was a problem hiding this comment.
Is this your own work or fix from another branch (that should be than merged there..)
Same for files: routes_highway.xml and routes_bycicle.xml files
| @@ -128,7 +128,7 @@ def loop(timer_event=None): | |||
| throttle = 0 | |||
|
|
|||
| # very low target_velocities -> stand | |||
There was a problem hiding this comment.
why is the standstill speed now 0.01 m/s? Is the ACC so accurate that it handles less than 0,36 km/h (=0,1/ms) and is the car also dealing with so low speeds?
There was a problem hiding this comment.
It happens sometimes that we want to drive at a very low speed but that was not possible before. The value 0.01 was chosen experimentally.
…ure-remodel-acc-third-iteration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/planning/src/local_planner/ACC.py (1)
401-408: Define magic numbers as constants.Consider defining the magic numbers as class-level constants for better maintainability:
+ OVERTAKE_MIN_DELTA_V = 2 + OVERTAKE_MIN_DISTANCE_FACTOR = 6 + def calculate_velocity_based_on_lead( self, hero_velocity: float, lead_distance: float, delta_v: float ) -> float: # ... if ( self.__curr_behavior == "ot_wait_free" - and delta_v < 2 - and lead_distance < 6 * d_min + and delta_v < self.OVERTAKE_MIN_DELTA_V + and lead_distance < self.OVERTAKE_MIN_DISTANCE_FACTOR * d_min ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/planning/config/ACC.cfg(1 hunks)code/planning/src/local_planner/ACC.py(11 hunks)code/planning/src/local_planner/motion_planning.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/planning/config/ACC.cfg
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (7)
code/planning/src/local_planner/ACC.py (4)
9-9: Group private attributes together.The changes look good. However, consider grouping all private attributes (
__xyz) together for better code organization.Also applies to: 47-47
116-122: LGTM!The subscriber is correctly initialized and follows the established pattern.
147-147: LGTM!The acceleration factor parameter is correctly integrated with the dynamic reconfigure system.
Also applies to: 159-163
300-304: Consider using a string array for behaviors.Consider defining the behaviors as a constant array at the class level for better maintainability:
- front_mask_reduce_behaviours = ["ot_wait_free", "ot_app_blocked", "ot_leave"] + FRONT_MASK_REDUCE_BEHAVIORS = ["ot_wait_free", "ot_app_blocked", "ot_leave"] + + if self.__curr_behavior in self.FRONT_MASK_REDUCE_BEHAVIORS:code/planning/src/local_planner/motion_planning.py (3)
52-52: LGTM!The change improves consistency by using underscores in the selector values.
Also applies to: 756-756
435-437: LGTM!The changes improve type safety and prevent potential index out of range errors.
501-527: LGTM!The refactored speed selection logic is more maintainable and handles edge cases better.
Description
The ACC now includes motion data of the entities.
Fixes #685
Type of change
Does this PR introduce a breaking change?
No
Most important changes
ACC.py
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores