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 @@ -38,6 +38,10 @@ class NodeBaseInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeBaseInterface)

RCLCPP_PUBLIC
virtual
~NodeBaseInterface() = default;

/// Return the name of the node.
/** \return The name of the node. */
RCLCPP_PUBLIC
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 @@ -31,6 +31,10 @@ class NodeClockInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeClockInterface)

RCLCPP_PUBLIC
virtual
~NodeClockInterface() = default;

/// Get a ROS clock which will be kept up to date by the node.
RCLCPP_PUBLIC
virtual
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ class NodeGraphInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeGraphInterface)

RCLCPP_PUBLIC
virtual
~NodeGraphInterface() = default;

/// Return a map of existing topic names to list of topic types.
/**
* A topic is considered to exist when at least one publisher or subscriber
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ class NodeLoggingInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeLoggingInterface)

RCLCPP_PUBLIC
virtual
~NodeLoggingInterface() = default;

/// Return the logger of the node.
/** \return The logger of the node. */
RCLCPP_PUBLIC
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class NodeParametersInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeParametersInterface)

RCLCPP_PUBLIC
virtual
~NodeParametersInterface() = default;

RCLCPP_PUBLIC
virtual
std::vector<rcl_interfaces::msg::SetParametersResult>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ class NodeServicesInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeServicesInterface)

RCLCPP_PUBLIC
virtual
~NodeServicesInterface() = default;

RCLCPP_PUBLIC
virtual
void
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 @@ -31,6 +31,10 @@ class NodeTimersInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTimersInterface)

RCLCPP_PUBLIC
virtual
~NodeTimersInterface() = default;

/// Add a timer to the node.
RCLCPP_PUBLIC
virtual
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ class NodeTopicsInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeTopicsInterface)

RCLCPP_PUBLIC
virtual
~NodeTopicsInterface() = default;

RCLCPP_PUBLIC
virtual
rclcpp::PublisherBase::SharedPtr
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class NodeWaitablesInterface
public:
RCLCPP_SMART_PTR_ALIASES_ONLY(NodeWaitablesInterface)

RCLCPP_PUBLIC
virtual
~NodeWaitablesInterface() = default;

RCLCPP_PUBLIC
virtual
void
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 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.

#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