Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

529 - Implement service for planning - acting communication #549

Merged
7 changes: 4 additions & 3 deletions code/planning/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,12 @@ catkin_python_setup()
)

## Generate services in the 'srv' folder
# add_service_files(
# FILES
add_service_files(
FILES
# Service1.srv
# Service2.srv
# )
RequestBehaviourChange.srv
)

## Generate actions in the 'action' folder
# add_action_files(
Expand Down
7 changes: 6 additions & 1 deletion code/planning/launch/planning.launch
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<launch>
<arg name="role_name" default="hero" />
<arg name="control_loop_rate" default="0.05" />

<node pkg="planning" type="collision_check.py" name="CollisionCheck" output="screen">
<param name="role_name" value="hero" />
<param name="control_loop_rate" value="$(arg control_loop_rate)" />
Expand Down Expand Up @@ -35,4 +35,9 @@
<node pkg="planning" type="behavior_tree.py" name="behavior_agent" output="screen"/>
<node pkg="rqt_gui" type="rqt_gui" name="rqt_gui"/>

<node pkg="planning" type="RequestBehaviourChangeService.py" name="RequestBehaviourChangeService" output="screen">
<param name="role_name" value="hero" />
<param name="control_loop_rate" value="$(arg control_loop_rate)" />
</node>

</launch>
82 changes: 82 additions & 0 deletions code/planning/src/behavior_agent/RequestBehaviourChangeService.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/usr/bin/env python
# import BehaviourEnum
import ros_compatibility as roscomp
from ros_compatibility.node import CompatibleNode
from rospy import Subscriber, Publisher
import rospy
from planning.srv import RequestBehaviourChange, RequestBehaviourChangeResponse
from std_msgs.msg import String, Int8


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.service = rospy.Service(
"RequestBehaviourChange",
RequestBehaviourChange,
self.handle_request_behaviour_change,
)
Comment on lines +20 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


self.behaviour_pub: Publisher = self.new_publisher(
Int8,
f"/paf/{self.role_name}/behaviour_request",
qos_profile=1,
)

self.curr_behavior_sub: Subscriber = self.new_subscription(
String,
f"/paf/{self.role_name}/curr_behavior",
self.__set_curr_behavior,
qos_profile=1,
)

self.behaviour_pub.publish(0)
rospy.spin()
Comment on lines +39 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.
"""
self.__curr_behavior = data.data

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)
Comment on lines +48 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)


def run(self):
"""
Control loop

:return:
"""

self.spin()
Comment on lines +60 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
def run(self):
"""
Control loop
:return:
"""
self.spin()
def run(self):
"""
Runs the node's main loop using ROS spin.
"""
self.spin()



if __name__ == "__main__":
"""
main function starts the RequestBehaviourChangeService node
:param args:
"""
roscomp.init("RequestBehaviourChangeService")
try:
node = RequestBehaviourChangeService()
node.run()
except KeyboardInterrupt:
pass
finally:
roscomp.shutdown()
3 changes: 3 additions & 0 deletions code/planning/srv/RequestBehaviourChange.srv
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
uint8 request
---
bool answer
26 changes: 26 additions & 0 deletions doc/planning/RequestBehaviourChangeService.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# 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.

Comment on lines +3 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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)

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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)

Comment on lines +1 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing sections to complete the documentation.

The documentation should include additional sections:

  1. Prerequisites/Dependencies
  2. Service Definition (fields and types)
  3. Return Values and Error Codes
  4. Configuration Parameters (if any)
  5. Examples of Common Use Cases
  6. 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)

Loading