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

[ros2] Add Stage property to assign a list of controllers to use when executing the planned trajectory #355

Merged
merged 2 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions capabilities/src/execute_task_solution_capability.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ bool ExecuteTaskSolutionCapability::constructMotionPlan(const moveit_task_constr
}
exec_traj.trajectory = std::make_shared<robot_trajectory::RobotTrajectory>(model, group);
exec_traj.trajectory->setRobotTrajectoryMsg(state, sub_traj.trajectory);
exec_traj.controller_name = sub_traj.execution_info.controller_names;

/* TODO add action feedback and markers */
exec_traj.effect_on_success = [this, sub_traj,
Expand Down
9 changes: 9 additions & 0 deletions core/include/moveit/task_constructor/stage.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

#pragma once

#include "trajectory_execution_info.h"
#include "utils.h"
#include <moveit/macros/class_forward.h>
#include <moveit/task_constructor/storage.h>
Expand Down Expand Up @@ -201,6 +202,14 @@ class Stage
/// marker namespace of solution markers
const std::string& markerNS() { return properties().get<std::string>("marker_ns"); }

/// Set and get info to use when executing the stage's trajectory
sjahr marked this conversation as resolved.
Show resolved Hide resolved
void setTrajectoryExecutionInfo(TrajectoryExecutionInfo trajectory_execution_info) {
setProperty("trajectory_execution_info", trajectory_execution_info);
}
TrajectoryExecutionInfo trajectoryExecutionInfo() {
return properties().get<TrajectoryExecutionInfo>("trajectory_execution_info");
}

/// forwarding of properties between interface states
void forwardProperties(const InterfaceState& source, InterfaceState& dest);
std::set<std::string> forwardedProperties() const {
Expand Down
47 changes: 47 additions & 0 deletions core/include/moveit/task_constructor/trajectory_execution_info.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2022, PickNik Inc.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of PickNik Inc. nor the names of its
* contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*********************************************************************/

/* Authors: Joe Schornak, Sebastian Jahr */

#pragma once

#include <moveit_task_constructor_msgs/msg/trajectory_execution_info.hpp>
#include <string>
#include <vector>

namespace moveit {
namespace task_constructor {
using TrajectoryExecutionInfo = moveit_task_constructor_msgs::msg::TrajectoryExecutionInfo;
} // namespace task_constructor
} // namespace moveit
5 changes: 5 additions & 0 deletions core/src/container.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,13 @@ void ContainerBase::insert(Stage::pointer&& stage, int before) {
if (!stage)
throw std::runtime_error(name() + ": received invalid stage pointer");

if (stage->trajectoryExecutionInfo().controller_names.empty()) {
stage->setTrajectoryExecutionInfo(this->trajectoryExecutionInfo());
}
Comment on lines +368 to +370
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the correct way of inheriting properties from parent to child (I think this is intended here).
Users should make this inheritance explicit in their code via:
Property::configureInitFrom(Stage::PARENT, "trajectory_execution_info");

Copy link
Contributor

@sjahr sjahr Oct 20, 2023

Choose a reason for hiding this comment

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

The intend here is to make inheriting TrajectoryExecutionInfo from the parent container default, except when the stage has explicitly configured it's own trajectory execution info. If a user has already done this via configureInitFrom, nothing happens here. Do you think that is a bad default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I do consider this a bad default, because it contradicts the philosophy of explicit property inheritance, which @v4hn requested from the very beginning. We should not deviate from that philosophy. Please revert that part.

Copy link
Contributor

Choose a reason for hiding this comment

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


StagePrivate* impl = stage->pimpl();
impl->setParent(this);

ContainerBasePrivate::const_iterator where = pimpl()->childByIndex(before, true);
ContainerBasePrivate::iterator it = pimpl()->children_.insert(where, std::move(stage));
impl->setParentPosition(it);
Expand Down
2 changes: 2 additions & 0 deletions core/src/stage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,8 @@ Stage::Stage(StagePrivate* impl) : pimpl_(impl) {
auto& p = properties();
p.declare<double>("timeout", "timeout per run (s)");
p.declare<std::string>("marker_ns", name(), "marker namespace");
p.declare<TrajectoryExecutionInfo>("trajectory_execution_info", TrajectoryExecutionInfo(),
"settings used when executing the trajectory");

p.declare<std::set<std::string>>("forwarded_properties", std::set<std::string>(),
"set of interface properties to forward");
Expand Down
4 changes: 4 additions & 0 deletions core/src/storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ void SubTrajectory::appendTo(moveit_task_constructor_msgs::msg::Solution& msg, I
moveit_task_constructor_msgs::msg::SubTrajectory& t = msg.sub_trajectory.back();
SolutionBase::fillInfo(t.info, introspection);

const auto trajectory_execution_info =
creator()->properties().get<TrajectoryExecutionInfo>("trajectory_execution_info");
t.execution_info = trajectory_execution_info;

if (trajectory())
trajectory()->getRobotTrajectoryMsg(t.trajectory);

Expand Down
1 change: 1 addition & 0 deletions msgs/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ set(msg_files
"msg/SubTrajectory.msg"
"msg/TaskDescription.msg"
"msg/TaskStatistics.msg"
"msg/TrajectoryExecutionInfo.msg"
)

set(srv_files
Expand Down
3 changes: 3 additions & 0 deletions msgs/msg/SubTrajectory.msg
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# generic solution information
SolutionInfo info

# trajectory execution information, like controller configuration
TrajectoryExecutionInfo execution_info

# trajectory
moveit_msgs/RobotTrajectory trajectory

Expand Down
2 changes: 2 additions & 0 deletions msgs/msg/TrajectoryExecutionInfo.msg
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# List of controllers to use when executing the trajectory
string[] controller_names
Loading