-
Notifications
You must be signed in to change notification settings - Fork 38
Following server #43
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
base: main
Are you sure you want to change the base?
Following server #43
Conversation
Something with the smoothness would be great too! I think the thread in ros-navigation/navigation2#4115 covers that intention a bit on the controller side. Otherwise, I think your other descriptions make sense. I actually filed a ticket when migrating this over to Nav2 (ros-navigation/navigation2#4404) on the collision checking bits, I agree that it would also be applicable here too! This generally looks good! I think there's thing we should do to reduce code duplication across the different servers, but that's something once we have a working final prototype so we know what we need to abstract out or see if its sensible to have some common base class or something similar |
Great. I'll keep going then. Because the docking is ported to nav2, should I make the PR here, merge here and then port it to nav2 or make it directly to nav2? |
Probably just leave it here for the moment. The Nav2 version has some refactoring that we can wait to address until we're done with a POC |
@SteveMacenski I'd like to start finishing this. Due to recent changes in docking and the deprecation of this repo, do you mind if I move this PR to a new PR in the nav2 repo? |
Sure! Though, I think some of this work is blocked by the Graceful Controller's selection of the path point to be smooth ros-navigation/navigation2#4115 So, I'd argue perhaps leaving this here until we're unblocked might be a good choice. |
No problem, it was to make everything easier with the final steps, testing, etc. Although any improvements made to the Graceful Controller must also be applied here, this server only uses the control law. |
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Last month we had a convention and we tested the following, here you can see a video of the following server working: https://www.youtube.com/watch?v=g-g58J1g9Ww I think it's mostly finished, you can start reviewing it if you have time. I made a demo package with instructions for a showcase. |
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
I'm addressing all the comments you left me. I'll let you know when it's ready for a second review. |
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
opennav_following/include/opennav_following/following_exceptions.hpp
Outdated
Show resolved
Hide resolved
opennav_following/include/opennav_following/following_server.hpp
Outdated
Show resolved
Hide resolved
@ajtudela sorry for the delay, I'm steadily working through my backlog :-) Generally looks really good! I have some comments but large strokes look great! |
…ew range for error codes Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
@ajtudela re-request a review when you'd like me to take a look again :-) |
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
I think all the comments have been addressed and it's ready for a new review. |
const double dt = 1.0 / controller_frequency_; | ||
auto target_pose = object_pose; | ||
target_pose.pose.orientation = nav2_util::geometry_utils::orientationAroundZAxis( | ||
tf2::getYaw(target_pose.pose.orientation) + M_PI); |
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.
So PI makes it turn 180, but always turns one way. As such, we don't fully explore the space. Should we do:
- A full rotation?
- Do a 90 degree left and then 180 deg right (so we do search 90 degrees left and right)? Kind of a wiggle move
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 like the wiggle move. I think it looks more 'natural'.
|
||
BT::NodeStatus FollowObjectAction::on_cancelled() | ||
{ | ||
setOutput("error_code_id", ActionResult::NONE); |
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.
total_elapsed_time set as well?
return providedBasicPorts( | ||
{ | ||
BT::InputPort<std::string>( | ||
"pose_topic", "Topic to publish the pose of the object to follow"), |
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.
Maybe set a default for the BT node?
{ | ||
RCLCPP_INFO(get_logger(), "Creating %s", get_name()); | ||
|
||
declare_parameter("controller_frequency", 50.0); |
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.
Once we approve here, this is something we'll want a docs PR for in the configuration guide + migration guide to bring this to folks' mind. If you have a nice video / gif of the following (I think you already showed some) that would be a great place to use it in the migration & configuration guides. Maybe a tutorial for how to use and important features like the pose vs TF, static obstacle timeout, rotate vs wait for target when lost, etc?
return false; | ||
} | ||
|
||
geometry_msgs::msg::PoseStamped FollowingServer::getRobotPoseInFrame(const std::string & frame) |
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.
Can this be replaced using Nav2 Util's robot utils?
void FollowingServer::publishZeroVelocity() | ||
{ | ||
auto cmd_vel = std::make_unique<geometry_msgs::msg::TwistStamped>(); | ||
cmd_vel->header.stamp = now(); |
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.
Maybe add the header for good measure?
// is not set correctly or has a lot of noise. | ||
// Then, we skip the target orientation by pointing it | ||
// in the same orientation than the vector from the robot to the object. | ||
if (skip_orientation_) { |
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.
Note: nice idea, I agree :-)
} | ||
|
||
// Filter the detected pose | ||
dynamic_pose_ = filter_->update(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.
I don't think dynamic_pose_
is used anywhere (?). detected_dynamic_pose_ is from the topic, but not the frame.
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.
Ditto with a similar block above in getRefinedPose
robot_pose.pose.position.x - target_pose.pose.position.x, | ||
robot_pose.pose.position.y - target_pose.pose.position.y); | ||
bool reversing = allow_backward_ && | ||
dist < desired_distance_ && target_pose.pose.position.x < robot_pose.pose.position.x; |
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 don't quite understand this point dist < desired_distance_
. Why if we're closer only to the desired distance do we allow reversing?
I might also be misunderstanding the reversing topic. Is this a temporary reversing or is this now just executing the task in the reverse direction?
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.
Update on 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.
Yes, sorry. I wanted to push all the commits and then address the comments.
The reversing
option is a planned feature that will be executed when the robot is closer to the object than the required distance. This allows the robot to be pushed back to maintain the distance.
I never tried it in practice because the person was always faster than the robot, and the graceful controller parameters were set a bit low.
So I can remove it without any problems.
Signed-off-by: Alberto Tudela <[email protected]>
Co-authored-by: Steve Macenski <[email protected]>
…CMake test dependencies Signed-off-by: Alberto Tudela <[email protected]>
…t tracking Signed-off-by: Alberto Tudela <[email protected]>
…y for improved clarity Signed-off-by: Alberto Tudela <[email protected]>
…l::getCurrentPose Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Signed-off-by: Alberto Tudela <[email protected]>
Most of the messages have been addressed. The only thing that bothers me is that this callback is never executed, and I can't understand why. |
I've opened this draft so you can take a look at the code and open a discussion to add changes, improvemnet, etc. Later, I'll create a new PR with all the changes.
The following server is currently working, as seen in this demo video I made: https://youtu.be/YaQQm7aWb-Y
I'm working on the following new features:
As the following and docking server share a large part of the code. I'll try to extract the code into an ApproachServer class (I'm really bad with the names :/) so both server can inherit from it.