529 - Implement service for planning - acting communication #549
Conversation
WalkthroughThe pull request introduces a new service for behavior change requests in the Changes
Possibly related PRs
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (5)
code/planning/srv/RequestBehaviourChange.srv (1)
1-3: Add field documentation to improve service definition clarity.The service definition would benefit from comments explaining:
- Valid ranges/values for the
requestfield- Meaning of the
answerfield values (true/false)- Any enum values that correspond to specific behaviors
Example improvement:
+# Request a behavior change +# request: Behavior ID (enum) +# 0: FOLLOW_LANE +# 1: CHANGE_LANE_LEFT +# ... (add other valid behaviors) uint8 request --- +# Response indicating if behavior change was granted +# answer: true if granted, false if rejected bool answerdoc/planning/RequestBehaviourChangeService.md (2)
9-13: Add language specifiers to code blocks and improve example.-``` +```python rospy.wait_for_service('RequestBehaviourChange') name = rospy.ServiceProxy('RequestBehaviourChange', RequestBehaviourChange)<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 9-9: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `17-22`: **Enhance error handling documentation.** The example should include proper error handling and logging. ```diff -``` +```python try: response = name(input) except rospy.ServiceException as e: - # handle exception + rospy.logerr(f"Service call failed: {e}") + # Consider appropriate error recovery actions<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 17-17: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>code/planning/src/behavior_agent/RequestBehaviourChangeService.py (2)</summary><blockquote> `2-2`: **Remove or implement the commented import** The commented import of `BehaviourEnum` suggests that behavior values might be better represented as an enum rather than strings. This would provide type safety and prevent typos in behavior names. --- `11-19`: **Add type hints for class variables** Consider adding type hints to improve code clarity and enable better IDE support: ```diff class RequestBehaviourChangeService(CompatibleNode): def __init__(self): super(RequestBehaviourChangeService, self).__init__( "RequestBehaviourChangeService" ) - self.role_name = self.get_param("role_name", "hero") - self.control_loop_rate = self.get_param("control_loop_rate", 1) - self.__curr_behavior = None + self.role_name: str = self.get_param("role_name", "hero") + self.control_loop_rate: int = self.get_param("control_loop_rate", 1) + self.__curr_behavior: str | None = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
code/planning/CMakeLists.txt(1 hunks)code/planning/launch/planning.launch(2 hunks)code/planning/src/behavior_agent/RequestBehaviourChangeService.py(1 hunks)code/planning/srv/RequestBehaviourChange.srv(1 hunks)doc/planning/RequestBehaviourChangeService.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/planning/RequestBehaviourChangeService.md
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...` For communication with the behaviour tree this node publishes a granted request t...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.35.0)
doc/planning/RequestBehaviourChangeService.md
9-9: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1-1: null
First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (2)
code/planning/launch/planning.launch (1)
38-41: LGTM! Node configuration follows existing patterns.
The new service node is properly configured with consistent parameters and follows the established pattern in the launch file.
Let's verify the node's package and script existence:
code/planning/CMakeLists.txt (1)
61-66: LGTM! Service file properly configured
The service file is correctly added to the build system and follows ROS CMake conventions.
| This service is hosted in the node RequestBehaviourChangeService. | ||
|
|
||
| Calling it requires a behaviour ID integer (based on the corresponding enum) and returns a bool depending on whether the request is granted. | ||
|
|
||
| To use it, import RequestBehaviourChange from planning.srv. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance service documentation with behavior ID details.
The documentation should specify:
- List of available behavior IDs and their meanings
- Valid range of behavior IDs
- Any preconditions for requesting specific behaviors
🧰 Tools
🪛 Markdownlint (0.35.0)
1-1: null
First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
| # handle exception | ||
| ``` | ||
|
|
||
| For communication with the behaviour tree this node publishes a granted request to a topic behaviour_request. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add details about behaviour_request topic.
The documentation should specify:
- Message type used for the behaviour_request topic
- Format/fields of the published messages
- Frequency of publications
- Any subscribers that need to handle these messages
🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...` For communication with the behaviour tree this node publishes a granted request t...
(AI_HYDRA_LEO_MISSING_COMMA)
| self.behaviour_pub.publish(0) | ||
| rospy.spin() |
There was a problem hiding this comment.
Document initial behavior request and remove blocking spin()
The initial behavior request of 0 needs documentation explaining its purpose. Also, rospy.spin() in __init__ will block initialization and makes the run() method redundant.
+ # Initialize with default cruise behavior (0)
self.behaviour_pub.publish(0)
- rospy.spin()📝 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.
| self.behaviour_pub.publish(0) | |
| rospy.spin() | |
| # Initialize with default cruise behavior (0) | |
| self.behaviour_pub.publish(0) |
| def __set_curr_behavior(self, data: String): | ||
| """ | ||
| Sets the received current behavior of the vehicle. | ||
| If the behavior is an overtake behavior, a trajectory change is triggered. | ||
| """ | ||
| self.__curr_behavior = data.data |
There was a problem hiding this comment.
Fix incorrect docstring
The docstring mentions triggering a trajectory change, but this functionality is not implemented in the method.
def __set_curr_behavior(self, data: String):
"""
Sets the received current behavior of the vehicle.
- If the behavior is an overtake behavior, a trajectory change is triggered.
"""
self.__curr_behavior = data.data📝 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.
| def __set_curr_behavior(self, data: String): | |
| """ | |
| Sets the received current behavior of the vehicle. | |
| If the behavior is an overtake behavior, a trajectory change is triggered. | |
| """ | |
| self.__curr_behavior = data.data | |
| def __set_curr_behavior(self, data: String): | |
| """ | |
| Sets the received current behavior of the vehicle. | |
| """ | |
| self.__curr_behavior = data.data |
| self.service = rospy.Service( | ||
| "RequestBehaviourChange", | ||
| RequestBehaviourChange, | ||
| self.handle_request_behaviour_change, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include role_name in service name for multi-robot scenarios
The service name should be namespaced with role_name to support multiple robots:
self.service = rospy.Service(
- "RequestBehaviourChange",
+ f"/paf/{self.role_name}/RequestBehaviourChange",
RequestBehaviourChange,
self.handle_request_behaviour_change,
)Committable suggestion skipped: line range outside the PR's diff.
| def run(self): | ||
| """ | ||
| Control loop | ||
|
|
||
| :return: | ||
| """ | ||
|
|
||
| self.spin() |
There was a problem hiding this comment.
Implement control loop in run method
The run method's docstring mentions a control loop but doesn't implement one. Either implement the loop or update the docstring.
def run(self):
"""
- Control loop
-
- :return:
+ Runs the node's main loop using ROS spin.
"""
self.spin()📝 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.
| def run(self): | |
| """ | |
| Control loop | |
| :return: | |
| """ | |
| self.spin() | |
| def run(self): | |
| """ | |
| Runs the node's main loop using ROS spin. | |
| """ | |
| self.spin() |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
doc/planning/RequestBehaviourChangeService.md (1)
11-24: Improve code examples with error handling and type hints.The code examples should be enhanced to be more production-ready:
```python +from planning.srv import RequestBehaviourChange +from typing import Union + rospy.wait_for_service('RequestBehaviourChange') -name = rospy.ServiceProxy('RequestBehaviourChange', RequestBehaviourChange) +service_proxy = rospy.ServiceProxy('RequestBehaviourChange', RequestBehaviourChange)try: - response = name(input) + response = service_proxy(behavior_id) # behavior_id: int + if response.answer: # Assuming 'answer' is the response field + rospy.loginfo(f"Behavior change request {behavior_id} granted") + else: + rospy.logwarn(f"Behavior change request {behavior_id} denied") except rospy.ServiceException as e: - # handle exception + rospy.logerr(f"Service call failed: {e}")</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b807fea91b55fcf56ce0d85697638f047cbf9501 and 391a113dbb2e661b4f63d7cee9c07f51ea5aca1a. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `doc/planning/RequestBehaviourChangeService.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>doc/planning/RequestBehaviourChangeService.md</summary> [uncategorized] ~26-~26: Possible missing comma found. Context: ...` For communication with the behaviour tree this node publishes a granted request t... (AI_HYDRA_LEO_MISSING_COMMA) </details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary> <details> <summary>doc/planning/RequestBehaviourChangeService.md (2)</summary> `1-6`: **Enhance service documentation with behavior ID details.** The documentation should specify: - List of available behavior IDs and their meanings - Valid range of behavior IDs - Any preconditions for requesting specific behaviors --- `26-26`: _:hammer_and_wrench: Refactor suggestion_ **Add details about behaviour_request topic and fix grammar.** The documentation needs improvement in two areas: 1. Missing comma after "behaviour tree" 2. Insufficient details about the behaviour_request topic ```diff -For communication with the behaviour tree this node publishes a granted request to a topic behaviour_request. +For communication with the behaviour tree, this node publishes a granted request to the topic `behaviour_request`. The topic: +- Message Type: <specify_message_type> +- Published Fields: <list_fields_and_types> +- Publication Frequency: <specify_frequency> +- Subscribers: <list_subscribers>🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...` For communication with the behaviour tree this node publishes a granted request t...(AI_HYDRA_LEO_MISSING_COMMA)
| # Request Behaviour Change Service | ||
|
|
||
| This service is hosted in the node RequestBehaviourChangeService. | ||
|
|
||
| Calling it requires a behaviour ID integer (based on the corresponding enum) and returns a bool depending on whether the request is granted. | ||
|
|
||
| To use it, import RequestBehaviourChange from planning.srv. | ||
|
|
||
| To call it, create a callable instance with: | ||
|
|
||
| ```python | ||
| rospy.wait_for_service('RequestBehaviourChange') | ||
|
|
||
| name = rospy.ServiceProxy('RequestBehaviourChange', RequestBehaviourChange) | ||
| ``` | ||
|
|
||
| Then, you can just use this instance in a try setup: | ||
|
|
||
| ```python | ||
| try: | ||
| response = name(input) | ||
| except rospy.ServiceException as e: | ||
| # handle exception | ||
| ``` | ||
|
|
||
| For communication with the behaviour tree this node publishes a granted request to a topic behaviour_request. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing sections to complete the documentation.
The documentation should include additional sections:
- Prerequisites/Dependencies
- Service Definition (fields and types)
- Return Values and Error Codes
- Configuration Parameters (if any)
- Examples of Common Use Cases
- Troubleshooting Guide
Would you like me to help generate content for these sections?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Possible missing comma found.
Context: ...` For communication with the behaviour tree this node publishes a granted request t...
(AI_HYDRA_LEO_MISSING_COMMA)
small comment fix
…cting-communication
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
code/planning/src/behavior_agent/RequestBehaviourChangeService.py (2)
2-2: Remove the commented-out import statementThe import statement
BehaviourEnumis commented out. If it's not required, consider removing it to keep the code clean.
39-39: Document the purpose of the initial behavior requestAdding documentation for the initial behavior request
self.behaviour_pub.publish(0)will help others understand its purpose in the initialization process.Apply this diff to add a comment:
self.behaviour_pub.publish(0) +# Publishing initial behavior request of 0 to set default state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
code/planning/src/behavior_agent/RequestBehaviourChangeService.py(1 hunks)
🔇 Additional comments (4)
code/planning/src/behavior_agent/RequestBehaviourChangeService.py (4)
20-24: Include role_name in the service name for multi-robot scenarios
To support multiple robots, the service name should be namespaced with self.role_name.
Apply this diff to update the service name:
self.service = rospy.Service(
- "RequestBehaviourChange",
+ f"/paf/{self.role_name}/RequestBehaviourChange",
RequestBehaviourChange,
self.handle_request_behaviour_change,
)39-40: Avoid blocking initialization with rospy.spin() in __init__
Calling rospy.spin() inside the __init__ method blocks the initialization and renders the run() method redundant. Consider moving rospy.spin() to the run() method.
Apply this diff to adjust the code:
def __init__(self):
...
self.behaviour_pub.publish(0)
- rospy.spin() def run(self):
"""
Control loop
"""
+ rospy.spin()48-58: Improve behavior handling by using constants and adding validation
Enhance the robustness of behavior handling by:
- Using constants or enums for behavior strings to avoid typos and improve maintainability.
- Validating request values to ensure they are within the expected range.
- Adding logging to monitor behavior changes and aid in debugging.
Apply this diff to implement the improvements:
+from enum import Enum
+
+# Define an enum for behaviors
+class Behavior(Enum):
+ US_UNSTUCK = "us_unstuck"
+ US_STOP = "us_stop"
+ US_OVERTAKE = "us_overtake"
+ CRUISE = "Cruise"
def handle_request_behaviour_change(self, req):
+ if self.__curr_behavior is None:
+ rospy.logwarn("Current behavior is not set yet.")
+ return RequestBehaviourChangeResponse(False)
+
+ # Validate request value
+ if not isinstance(req.request, int) or req.request < 0:
+ rospy.logwarn(f"Invalid behavior request value: {req.request}")
+ return RequestBehaviourChangeResponse(False)
+
if (
- self.__curr_behavior == "us_unstuck"
- or self.__curr_behavior == "us_stop"
- or self.__curr_behavior == "us_overtake"
- or self.__curr_behavior == "Cruise"
+ self.__curr_behavior == Behavior.US_UNSTUCK.value
+ or self.__curr_behavior == Behavior.US_STOP.value
+ or self.__curr_behavior == Behavior.US_OVERTAKE.value
+ or self.__curr_behavior == Behavior.CRUISE.value
):
+ rospy.loginfo(f"Changing behavior to: {req.request}")
self.behaviour_pub.publish(req.request)
return RequestBehaviourChangeResponse(True)
else:
+ rospy.loginfo(f"Behavior change rejected. Current behavior: {self.__curr_behavior}")
return RequestBehaviourChangeResponse(False)60-67: Update the run() method or its docstring to reflect functionality
The run() method's docstring mentions a control loop, but the method currently only calls self.spin(). Either implement the control loop logic or update the docstring to accurately describe the method's behavior.
Apply this diff to adjust the docstring:
def run(self):
"""
- Control loop
+ Runs the node's event loop
"""
self.spin()| def handle_request_behaviour_change(self, req): | ||
| if ( | ||
| self.__curr_behavior == "us_unstuck" | ||
| or self.__curr_behavior == "us_stop" | ||
| or self.__curr_behavior == "us_overtake" | ||
| or self.__curr_behavior == "Cruise" | ||
| ): | ||
| self.behaviour_pub.publish(req.request) | ||
| return RequestBehaviourChangeResponse(True) | ||
| else: | ||
| return RequestBehaviourChangeResponse(False) |
There was a problem hiding this comment.
Handle the case when __curr_behavior is None
If self.__curr_behavior is None, the comparisons with behavior strings will fail. Add a check to handle this scenario to prevent potential errors.
Apply this diff to incorporate the check:
def handle_request_behaviour_change(self, req):
+ if self.__curr_behavior is None:
+ rospy.logwarn("Current behavior is not set yet.")
+ return RequestBehaviourChangeResponse(False)
if (
self.__curr_behavior == "us_unstuck"
or self.__curr_behavior == "us_stop"
or self.__curr_behavior == "us_overtake"
or self.__curr_behavior == "Cruise"
):
self.behaviour_pub.publish(req.request)
return RequestBehaviourChangeResponse(True)
else:
return RequestBehaviourChangeResponse(False)📝 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.
| def handle_request_behaviour_change(self, req): | |
| if ( | |
| self.__curr_behavior == "us_unstuck" | |
| or self.__curr_behavior == "us_stop" | |
| or self.__curr_behavior == "us_overtake" | |
| or self.__curr_behavior == "Cruise" | |
| ): | |
| self.behaviour_pub.publish(req.request) | |
| return RequestBehaviourChangeResponse(True) | |
| else: | |
| return RequestBehaviourChangeResponse(False) | |
| def handle_request_behaviour_change(self, req): | |
| if self.__curr_behavior is None: | |
| rospy.logwarn("Current behavior is not set yet.") | |
| return RequestBehaviourChangeResponse(False) | |
| if ( | |
| self.__curr_behavior == "us_unstuck" | |
| or self.__curr_behavior == "us_stop" | |
| or self.__curr_behavior == "us_overtake" | |
| or self.__curr_behavior == "Cruise" | |
| ): | |
| self.behaviour_pub.publish(req.request) | |
| return RequestBehaviourChangeResponse(True) | |
| else: | |
| return RequestBehaviourChangeResponse(False) |
…cting-communication
…cting-communication
seefelke
deleted the
529-feature-implement-service-for-planning-acting-communication
branch
Description
Implemented service "RequestBehaviourChange" for Acting - Planning communication.
This allows Acting to request a specific behaviour. The node will then decide whether to grant that request.
Handling a requested behaviour is not yet implemented in the behaviour tree.
For usage check the new documentation file.
The logic in handle_request_behaviour_change whether to grant the request is temporary and will be updated once we refurbish the behaviour tree.
Fixes # 529
Type of change
Does this PR introduce a breaking change?
No
Most important changes
RequestBehaviourChangeService.py
RequestBehaviourChange.srv
Checklist:
Summary by CodeRabbit
New Features
Documentation
RequestBehaviourChangeService, including usage instructions and examples.Bug Fixes