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.

Sign up for GitHub

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

Added new message types Trajectory and Navigation Point. #515

Merged
merged 5 commits into from
Nov 27, 2024
Merged

Added new message types Trajectory and Navigation Point. #515

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cfbd170
Added new message types Trajectory and Navigation Point.
seefelke Nov 21, 2024
a4355e7
Update code/planning/CMakeLists.txt
seefelke Nov 21, 2024
668b6d7
refactored enum into seperate file
seefelke Nov 24, 2024
63b4e0a
Merge branch '511-feature-create-new-message-type-for-trajectory-mess…
seefelke Nov 24, 2024
5c37dfc
Merge branch 'main' into 511-feature-create-new-message-type-for-traj…
THDThieleT Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions code/planning/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ project(planning)
find_package(catkin REQUIRED COMPONENTS
perception
rospy
rosout
roslaunch
std_msgs
geometry_msgs
message_generation
Comment on lines +16 to +20
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

Remove unnecessary rosout dependency

The rosout package is a core ROS functionality and doesn't need to be explicitly listed as a dependency. It's automatically available to all ROS nodes.

find_package(catkin REQUIRED COMPONENTS
  perception
  rospy
-  rosout
  roslaunch
  std_msgs
  geometry_msgs
  message_generation
)
📝 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
rosout
roslaunch
std_msgs
geometry_msgs
message_generation
perception
rospy
roslaunch
std_msgs
geometry_msgs
message_generation

)

roslaunch_add_file_check(launch)
Expand Down Expand Up @@ -48,9 +51,11 @@ catkin_python_setup()
## * add every package in MSG_DEP_SET to generate_messages(DEPENDENCIES ...)

## Generate messages in the 'msg' folder
# add_message_files(
# MinDistance.msg
# )
add_message_files(
FILES
NavigationPoint.msg
Trajectory.msg
)

## Generate services in the 'srv' folder
# add_service_files(
Expand All @@ -67,9 +72,12 @@ catkin_python_setup()
# )

## Generate added messages and services with any dependencies listed here
# generate_messages(
# perception
# )
generate_messages(
DEPENDENCIES
std_msgs
perception
geometry_msgs
)

################################################
## Declare ROS dynamic reconfigure parameters ##
Expand Down Expand Up @@ -105,7 +113,8 @@ catkin_package(
# LIBRARIES planning
# CATKIN_DEPENDS other_catkin_pkg
# DEPENDS system_lib
CATKIN_DEPENDS perception rospy
# CATKIN_DEPENDS perception rospy
CATKIN_DEPENDS message_runtime
seefelke marked this conversation as resolved.
Show resolved Hide resolved
)

###########
Expand Down
3 changes: 3 additions & 0 deletions code/planning/msg/NavigationPoint.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
geometry_msgs/Point position
float32 speed
uint8 behaviour
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Verify behaviour field constraints needs update

The behaviour field in NavigationPoint.msg is not properly constrained. The Behaviour enum in local_planner/utils.py only defines CRUISING = 0, but there's no documentation or validation of other possible values. This needs to be updated to:

  1. Document all valid behaviour values in the message definition
  2. Align with the complete set of behaviours defined in the enum
🔗 Analysis chain

Verify behaviour field constraints

The behaviour field should align with the Behaviour enum in localplanning/utils.py. Let's verify the implementation and usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and analyze the Behaviour enum definition and its usage
echo "Searching for Behaviour enum definition:"
rg -p "class Behaviour" --type py

echo -e "\nChecking for behaviour field usage:"
rg -p "behaviour.*=.*\d+" --type py

Length of output: 420

Script:

#!/bin/bash
# Let's examine the Behaviour enum definition and its values
echo "Checking Behaviour enum implementation:"
rg -A 10 "class Behaviour" code/planning/src/local_planner/utils.py

# Let's also check for any other usage of behaviour field
echo -e "\nChecking for other behaviour field references:"
rg -i "behaviour" --type py

Length of output: 44713

2 changes: 2 additions & 0 deletions code/planning/msg/Trajectory.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Header header
planning/NavigationPoint[] navigationPoints
4 changes: 4 additions & 0 deletions code/planning/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
<!-- <test_depend>gtest</test_depend> -->
<!-- Use doc_depend for packages you need only for building documentation: -->
<!-- <doc_depend>doxygen</doc_depend> -->
<build_depend>message_generation</build_depend>
<exec_depend>message_runtime</exec_depend>

<build_depend>rospy</build_depend>
<build_depend>roslaunch</build_depend>

Expand All @@ -57,6 +60,7 @@
<exec_depend>carla_msgs</exec_depend>
<exec_depend>sensor_msgs</exec_depend>
<exec_depend>std_msgs</exec_depend>
<exec_depend>geometry_msgs</exec_depend>
<exec_depend>perception</exec_depend>

<build_depend>perception</build_depend>
Expand Down
7 changes: 7 additions & 0 deletions code/planning/src/local_planner/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import math
import carla
import os
from enum import IntEnum

# import rospy

Expand All @@ -23,6 +24,12 @@
EARTH_RADIUS_EQUA = 6378137.0


# getattr(Behaviour, "name").value returns the ID
# Behaviour(id) returns Behaviour.NAME
class Behaviour(IntEnum):
CRUISING = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Additional behavior states needed for ACC functionality

The review comment is correct. The current implementation in utils.py only has a CRUISING state, while the ACC implementation in ACC.py shows more complex behavior handling including:

  • Following vehicles with safety distance calculations
  • Stopping when obstacles are too close
  • Emergency braking (when speed needs to be reduced to 0)

These behaviors are currently managed through conditional logic but should be explicitly defined as states in the Behaviour enum for better code clarity and state management.

  • Add FOLLOWING state for when following a vehicle with safety distance
  • Add STOPPING state for controlled stops
  • Add EMERGENCY_BRAKE state for critical safety stops
🔗 Analysis chain

Consider adding more behavior states for ACC functionality.

The current implementation only includes CRUISING, but ACC typically requires additional states like:

  • FOLLOWING - When following a vehicle
  • STOPPING - When coming to a stop
  • EMERGENCY_BRAKE - For emergency situations

This seems incomplete for the ACC changes mentioned in issue #490.

Let's check if these behaviors are defined elsewhere:

Consider creating a comprehensive state machine for ACC behaviors to ensure all required states are properly handled.

Let me search for more context about ACC implementation and state transitions.

Let me check the ACC implementation and its state handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other behavior/state definitions related to ACC
rg -i "class.*(behaviour|behavior|state).*enum" 
rg -i "ACC.*state"

Length of output: 729

Script:

#!/bin/bash
# Search for ACC related files and implementations
fd -e py -e cpp -e hpp -e h . | xargs rg -l "ACC|adaptive.*cruise|cruise.*control" -i

# Search for state transitions or behavior changes
rg -i "(state.*transition|behavior.*change|mode.*switch)" -A 3

Length of output: 40346

Script:

#!/bin/bash
# Check ACC implementation and state handling
cat code/planning/src/local_planner/ACC.py

# Look for any other behavior/state definitions in the codebase
ast-grep --pattern 'class $_ {
  $$$
  FOLLOWING = $_
  $$$
}'

ast-grep --pattern 'class $_ {
  $$$
  STOPPING = $_
  $$$
}'

Length of output: 10124


def get_distance(pos_1, pos_2):
"""Calculate the distance between two positions

Expand Down
Loading