-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added Assisted Teleop feature in Navigation2 #2575
Conversation
Signed-off-by: MOLOCH-dev <[email protected]>
Signed-off-by: MOLOCH-dev <[email protected]>
Signed-off-by: MOLOCH-dev <[email protected]>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good first draft, I'll have some more review comments after you fix these, but these are necessary to get into the bigger architectural patterns
std::shared_ptr<tf2_ros::Buffer> tf_; | ||
rclcpp_lifecycle::LifecyclePublisher<geometry_msgs::msg::Twist>::SharedPtr vel_pub_; | ||
rclcpp::Subscription<geometry_msgs::msg::Twist>::SharedPtr vel_sub_; | ||
std::unique_ptr<nav2_costmap_2d::CostmapSubscriber> costmap_sub_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should have access to this from the main recovery server / collision checker object. You shouldn't need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please guide me as to how I could get access to costmap_sub_ from the collision checker object? Since it is protected in the context of the CostmapTopicCollisionChecker, I'm unable to access the costmap subscriber
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the costmap sub at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unable to figure out a way to access the costmap (to use in the getResolution() method for calculating time increments ). Hence, I have retained this block of code in my most recent push. Could you please guide me as to how I can access the costmap object without creating a costmap subscriber?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add a method to get a reference to the costmap subscriber that the CostmapTopicCollisionChecker
had internally, and then query that for the resolution. Maybe even have the costmap subscriber specifically store that to pass on in a function so that you don't need to populate a full costmap just to get a piece of metadata
rclcpp::Subscription<geometry_msgs::msg::Twist>::SharedPtr vel_sub_; | ||
std::unique_ptr<nav2_costmap_2d::CostmapSubscriber> costmap_sub_; | ||
|
||
// User defined parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put all variables after all functions
Also, some of these look unnecessary (others don't have the correct naming with trailing underscores for member variables)
{ | ||
using namespace std::chrono_literals; //NOLINT | ||
void | ||
AssistedTeleop::cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reorder functions to be: configure, activate, deactivate, cleanup to be in order
Any update? |
Signed-off-by: MOLOCH-dev <[email protected]>
Sorry for being a bit inactive, I was trying to figure out a way around some problems that I have mentioned here : I am done with all but 2 of the requested changes. The ones I am stuck with are the deletion of the costmap_sub_ object I have created to get access to the getResolution method and the velocity transformation from the base frame to the odom frame. I have commented under those requested changes, |
@MOLOCH-dev, your PR has failed to build. Please check CI outputs and resolve issues. |
node->get_parameter("cmd_vel_topic", cmd_vel_topic_); | ||
node->get_parameter("input_vel_topic", input_vel_topic_); | ||
|
||
vel_sub_ = node->create_subscription<geometry_msgs::msg::Twist>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be created in execute
now? So that its made when requested to start?
|
||
protected: | ||
// Clock | ||
rclcpp::Clock steady_clock_{RCL_STEADY_TIME}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove, not used
std::string cmd_vel_topic_; | ||
std::string input_vel_topic_; | ||
|
||
geometry_msgs::msg::PoseStamped current_pose; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be a class member
geometry_msgs::msg::PoseStamped current_pose; | ||
std::string recovery_name_; | ||
std::shared_ptr<nav2_costmap_2d::Costmap2D> costmap_ros_; | ||
geometry_msgs::msg::TransformStamped transform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be a class member
geometry_msgs::msg::TransformStamped transform; | ||
|
||
// Parameters for Assisted Teleop | ||
double scaling_factor = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be a class member
On testing:
|
} | ||
|
||
void | ||
AssistedTeleop::vel_callback(const geometry_msgs::msg::Twist::SharedPtr msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
velCallback()
Signed-off-by: MOLOCH-dev <[email protected]>
This pull request is in conflict. Could you fix it @MOLOCH-dev? |
Basic Info
Signed-off-by: Anushree Sabnis [email protected]
Description of contribution in a few bullet points
N
seconds in the futureN
seconds, velocity is scaled down so the robot again remainsN
seconds away from a collisionprojection_time
,linear_velocity_threshold
,input_vel_topic
andcmd_vel_topic
projection_time
: Maximum allowed time to collision (N
)linear_velocity_threshold
: Upon collision detection withinprojection_time
, if the scaled-down velocity of the robot is below this threshold, the robot is stopped.cmd_vel_topic
: Assisted Teleop publishes velocity to this topic.input_vel_topic
: Assisted Teleop subscribes to this topic to receive velocity commandsDescription of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: