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

Resolve startup race condition for sim time #608

Merged
merged 12 commits into from
Dec 12, 2018
Merged
1 change: 1 addition & 0 deletions rclcpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ set(${PROJECT_NAME}_SRCS
src/rclcpp/node_interfaces/node_logging.cpp
src/rclcpp/node_interfaces/node_parameters.cpp
src/rclcpp/node_interfaces/node_services.cpp
src/rclcpp/node_interfaces/node_time_source.cpp
src/rclcpp/node_interfaces/node_timers.cpp
src/rclcpp/node_interfaces/node_topics.cpp
src/rclcpp/node_interfaces/node_waitables.cpp
Expand Down
7 changes: 7 additions & 0 deletions rclcpp/include/rclcpp/node.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include "rclcpp/node_interfaces/node_logging_interface.hpp"
#include "rclcpp/node_interfaces/node_parameters_interface.hpp"
#include "rclcpp/node_interfaces/node_services_interface.hpp"
#include "rclcpp/node_interfaces/node_time_source_interface.hpp"
#include "rclcpp/node_interfaces/node_timers_interface.hpp"
#include "rclcpp/node_interfaces/node_topics_interface.hpp"
#include "rclcpp/node_interfaces/node_waitables_interface.hpp"
Expand Down Expand Up @@ -458,6 +459,11 @@ class Node : public std::enable_shared_from_this<Node>
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr
get_node_parameters_interface();

/// Return the Node's internal NodeParametersInterface implementation.
RCLCPP_PUBLIC
rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr
get_node_time_source_interface();

private:
RCLCPP_DISABLE_COPY(Node)

Expand All @@ -473,6 +479,7 @@ class Node : public std::enable_shared_from_this<Node>
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services_;
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock_;
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters_;
rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr node_time_source_;
rclcpp::node_interfaces::NodeWaitablesInterface::SharedPtr node_waitables_;

bool use_intra_process_comms_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ class NodeBaseInterface
virtual
std::unique_lock<std::recursive_mutex>
acquire_notify_guard_condition_lock() const = 0;

RCLCPP_PUBLIC
virtual
~NodeBaseInterface() = default;
Copy link
Member

Choose a reason for hiding this comment

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

The destructor is conventionally at the top of the class just after any constructors.

};

} // namespace node_interfaces
Expand Down
1 change: 0 additions & 1 deletion rclcpp/include/rclcpp/node_interfaces/node_clock.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ class NodeClock : public NodeClockInterface
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_;

rclcpp::Clock::SharedPtr ros_clock_;
rclcpp::TimeSource time_source_;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class NodeClockInterface
virtual
rclcpp::Clock::SharedPtr
get_clock() = 0;

RCLCPP_PUBLIC
virtual
~NodeClockInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ class NodeGraphInterface
virtual
size_t
count_graph_users() = 0;

RCLCPP_PUBLIC
virtual
~NodeGraphInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ class NodeLoggingInterface
virtual
const char *
get_logger_name() const = 0;

RCLCPP_PUBLIC
virtual
~NodeLoggingInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ class NodeParametersInterface
virtual
void
register_param_change_callback(ParametersCallbackFunction callback) = 0;

RCLCPP_PUBLIC
virtual
~NodeParametersInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class NodeServicesInterface
add_service(
rclcpp::ServiceBase::SharedPtr service_base_ptr,
rclcpp::callback_group::CallbackGroup::SharedPtr group) = 0;

RCLCPP_PUBLIC
virtual
~NodeServicesInterface() = default;
};

} // namespace node_interfaces
Expand Down
72 changes: 72 additions & 0 deletions rclcpp/include/rclcpp/node_interfaces/node_time_source.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright 2018 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_HPP_
#define RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_HPP_

#include "rclcpp/callback_group.hpp"
#include "rclcpp/clock.hpp"
#include "rclcpp/macros.hpp"
#include "rclcpp/node_interfaces/node_base_interface.hpp"
#include "rclcpp/node_interfaces/node_clock_interface.hpp"
#include "rclcpp/node_interfaces/node_parameters_interface.hpp"
#include "rclcpp/node_interfaces/node_time_source_interface.hpp"
#include "rclcpp/node_interfaces/node_topics_interface.hpp"
#include "rclcpp/time_source.hpp"
#include "rclcpp/visibility_control.hpp"

namespace rclcpp
{
namespace node_interfaces
{

/// Implementation of the NodeTimeSource part of the Node API.
class NodeTimeSource : public NodeTimeSourceInterface
{
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTimeSource)

RCLCPP_PUBLIC
explicit NodeTimeSource(
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock,
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters
);

RCLCPP_PUBLIC
virtual
~NodeTimeSource();

private:
RCLCPP_DISABLE_COPY(NodeTimeSource)

rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base_;
rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics_;
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_;
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services_;
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_;
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock_;
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters_;

rclcpp::TimeSource time_source_;
};

} // namespace node_interfaces
} // namespace rclcpp

#endif // RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_HPP_
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright 2018 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_INTERFACE_HPP_
#define RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_INTERFACE_HPP_

#include "rclcpp/callback_group.hpp"
#include "rclcpp/clock.hpp"
#include "rclcpp/macros.hpp"
#include "rclcpp/visibility_control.hpp"

namespace rclcpp
{
namespace node_interfaces
{

/// Pure virtual interface class for the NodeTimeSource part of the Node API.
class NodeTimeSourceInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might need a virtual destructor to make sure ~NodeTimeSource() gets called when the std::shared_ptr<NodeTimeSourceInterface> on the node is destructed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is probably true as well for all the other interface classes too. I went over it with @wjwwood and it looks like virtual default constructors are the best approch. We're clearly not using it right now but in the future not having them will cause warnings or errors.

{
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTimeSourceInterface)

RCLCPP_PUBLIC
virtual
~NodeTimeSourceInterface() = default;
};

} // namespace node_interfaces
} // namespace rclcpp

#endif // RCLCPP__NODE_INTERFACES__NODE_TIME_SOURCE_INTERFACE_HPP_
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class NodeTimersInterface
add_timer(
rclcpp::TimerBase::SharedPtr timer,
rclcpp::callback_group::CallbackGroup::SharedPtr callback_group) = 0;

RCLCPP_PUBLIC
virtual
~NodeTimersInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ class NodeTopicsInterface
add_subscription(
rclcpp::SubscriptionBase::SharedPtr subscription,
rclcpp::callback_group::CallbackGroup::SharedPtr callback_group) = 0;

RCLCPP_PUBLIC
virtual
~NodeTopicsInterface() = default;
};

} // namespace node_interfaces
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class NodeWaitablesInterface
remove_waitable(
rclcpp::Waitable::SharedPtr waitable_ptr,
rclcpp::callback_group::CallbackGroup::SharedPtr group) noexcept = 0;

RCLCPP_PUBLIC
virtual
~NodeWaitablesInterface() = default;
};

} // namespace node_interfaces
Expand Down
6 changes: 5 additions & 1 deletion rclcpp/include/rclcpp/time_source.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ class TimeSource
rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics_interface,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_interface,
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services_interface,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface);
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_interface,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock_interface,
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters_interface);

RCLCPP_PUBLIC
void detachNode();
Expand All @@ -76,6 +78,8 @@ class TimeSource
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph_;
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services_;
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging_;
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock_;
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters_;

// Store (and update on node attach) logger for logging.
Logger logger_;
Expand Down
16 changes: 16 additions & 0 deletions rclcpp/src/rclcpp/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "rclcpp/node_interfaces/node_logging.hpp"
#include "rclcpp/node_interfaces/node_parameters.hpp"
#include "rclcpp/node_interfaces/node_services.hpp"
#include "rclcpp/node_interfaces/node_time_source.hpp"
#include "rclcpp/node_interfaces/node_timers.hpp"
#include "rclcpp/node_interfaces/node_topics.hpp"
#include "rclcpp/node_interfaces/node_waitables.hpp"
Expand Down Expand Up @@ -83,6 +84,15 @@ Node::Node(
use_intra_process_comms,
start_parameter_services
)),
node_time_source_(new rclcpp::node_interfaces::NodeTimeSource(
node_base_,
node_topics_,
node_graph_,
node_services_,
node_logging_,
node_clock_,
node_parameters_
)),
node_waitables_(new rclcpp::node_interfaces::NodeWaitables(node_base_.get())),
use_intra_process_comms_(use_intra_process_comms)
{
Expand Down Expand Up @@ -263,6 +273,12 @@ Node::get_node_logging_interface()
return node_logging_;
}

rclcpp::node_interfaces::NodeTimeSourceInterface::SharedPtr
Node::get_node_time_source_interface()
{
return node_time_source_;
}

rclcpp::node_interfaces::NodeTimersInterface::SharedPtr
Node::get_node_timers_interface()
{
Expand Down
10 changes: 1 addition & 9 deletions rclcpp/src/rclcpp/node_interfaces/node_clock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,7 @@ NodeClock::NodeClock(
node_services_(node_services),
node_logging_(node_logging),
ros_clock_(std::make_shared<rclcpp::Clock>(RCL_ROS_TIME))
{
time_source_.attachNode(
node_base_,
node_topics_,
node_graph_,
node_services_,
node_logging_);
time_source_.attachClock(ros_clock_);
}
{}

NodeClock::~NodeClock()
{}
Expand Down
50 changes: 50 additions & 0 deletions rclcpp/src/rclcpp/node_interfaces/node_time_source.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Copyright 2017 Open Source Robotics Foundation, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2018?

Copy link
Contributor

Choose a reason for hiding this comment

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

this could probably still be fixed

//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "rclcpp/node_interfaces/node_time_source.hpp"

#include <memory>
#include <string>

using rclcpp::node_interfaces::NodeTimeSource;

NodeTimeSource::NodeTimeSource(
rclcpp::node_interfaces::NodeBaseInterface::SharedPtr node_base,
rclcpp::node_interfaces::NodeTopicsInterface::SharedPtr node_topics,
rclcpp::node_interfaces::NodeGraphInterface::SharedPtr node_graph,
rclcpp::node_interfaces::NodeServicesInterface::SharedPtr node_services,
rclcpp::node_interfaces::NodeLoggingInterface::SharedPtr node_logging,
rclcpp::node_interfaces::NodeClockInterface::SharedPtr node_clock,
rclcpp::node_interfaces::NodeParametersInterface::SharedPtr node_parameters)
: node_base_(node_base),
node_topics_(node_topics),
node_graph_(node_graph),
node_services_(node_services),
node_logging_(node_logging),
node_clock_(node_clock),
node_parameters_(node_parameters)
{
time_source_.attachNode(
node_base_,
node_topics_,
node_graph_,
node_services_,
node_logging_,
node_clock_,
node_parameters_);
time_source_.attachClock(node_clock_->get_clock());
}

NodeTimeSource::~NodeTimeSource()
{}
Loading