Ask Your Question
0

ros::Node as static member - any better ideas

asked 2017-01-14 09:38:21 -0500

potaahto gravatar image

updated 2017-01-15 05:12:05 -0500

Situation

I'm part of a larger team working with robots and ROS. My current project is to create easy-to-use libraries for navigation and other things from existing code in our repositories. Over time, many people in our lab have implemented the same function a bit differently and I want to unify and streamline these implementations in one single library.

Now, the library has to publish on and subscribe to multiple topics and therefore it requires a ros::NodeHandle to work with. So far my library is purely static (i.e. no classes are instantiated, static members are called directly). This has the advantage, that my library has global awareness and I can prevent many user/robot/real-time errors. Furthermore, subscribing to topics takes a bit of time and messages can be missed. This is another argument for static members, since I can have a static subscriber and initialize it when the node is created.

Problem

When I have a ros::NodeHandle as a static class member, it is initialized right on program-launch, before ros::init() can be called. This returns an error as soon as the rosnode using my library is launched - makes sense.

My Solution

I thought I could perform lazy-initialization of my static members, including subscribers, publishers and the nodehandle. In order to achieve this, all ros objects are static pointers, that I initialize right before first use. My classes all look something like this.

// Class used to actually send various commands to robot
class RobotHelper {
public:
  static int sendCommand(...);

private:
  static void initialize();
  static void callback(const my_msgs::Telemetry &msg);

  static bool initialized;
  static std::unique_ptr<ros::NodeHandle> nh_ptr_;
  static std::unique_ptr<ros::Publisher> cmd_pub_ptr_;
  static std::unique_ptr<ros::Subscriber> telemetry_sub_ptr_;
};

int RobotHelper::sendCommand(...) {
  // Lazy-initialize the RobotHelper
  initialize();

  // Uses telemetry and other things to make sure nothing dangeorus happens when
  // command is sent
  // Also uses telemetry to make sure the robot accepted the command, so
  // we don't have to worry about that later
  // ...
  // ...

  return 0;
}

void RobotHelper::initialize() {
  if (initialized) {
    // nothing to do, already initialized
    return;
  }

  // Initialize all the static ros members
  nh_ptr_ = std::unique_ptr<ros::NodeHandle>(new ros::NodeHandle);

  // create subscriber
  telemetry_sub_ptr_ = std::unique_ptr<ros::Subscriber>(new ros::Subscriber);
  *telemetry_sub_ptr_ =
      nh_ptr_->subscribe("telemetry", 10, &RobotHelper::callback);

  while (ros::ok()) {
    // Wait for crucial telemetry subscriber to receive messages
    // NOTE: Here, time is wasted waiting. I don't want to do that too often 
    // ...
  }

  // Create publisher
  some_pub_ptr_ = std::unique_ptr<ros::Publisher>(new ros::Publisher);
  *some_pub_ptr_ = nh_ptr_->advertise<std_msgs::Bool>("commands", 10);
}


class PathNavigation : public NavigationBase {
public:
  // For every navigation action I create its own object
  PathNavigation(double x, double y, double z);

  // Actually perform the navigation
  // This has to be called periodically until navigation is complete.
  // In between I could handle obstacle avoidance or perform other actions.
  // Hence this should be a non-blocking function and called periodically
  int execute(bool &navigation_completed);

private:
  static std::unique_ptr<ros::NodeHandle> nh_ptr_;
  static std::unique_ptr<ros::Publisher> some_pub_ptr_;
};

PathNavigation::PathNavigation(double x, double y, double z) {
  // In the constructor I ...
(more)
edit retag flag offensive close merge delete

Comments

@gvdhoom :There are two reasons for the statics. First one is as you suspected, that I don't want to miss messages. So I initialize the subscribers once and wait to receive messages. If I do that for every new navigation action, I'm wasting time waiting for the new subscriber every time.

potaahto gravatar imagepotaahto ( 2017-01-15 02:12:20 -0500 )edit

Global awareness might be the wrong term. Essentially I wanted to always know whether my subscribers are receiving messages after they have been initialized once, statically. In other words, I'm only using static to eliminate the time it would take to re-initialize my publishers and subscribers.

potaahto gravatar imagepotaahto ( 2017-01-15 02:15:43 -0500 )edit

What did you mean by "exposing as action"? This sounds exactly like what I need. Would that be having a separate Node running for these actions and triggering them via service calls for example?

potaahto gravatar imagepotaahto ( 2017-01-15 02:19:49 -0500 )edit

1 Answer

Sort by ยป oldest newest most voted
1

answered 2017-01-15 03:57:27 -0500

gvdhoorn gravatar image

updated 2017-01-15 04:19:53 -0500

The overuse of static here is a red flag that this is not a good way to approach this. Can you elaborate a bit on how static makes things better? If not missing msgs is really important, an async pub/sub pattern might not be suitable here.

int PathNavigation::goTo(double x, double y, double z): if this is a function that (ultimately) actually makes your platform move to x, y, z, then it might make sense to expose that as an Action, instead of a subscriber. That would allow your client & server to synchronise their control flows, making it immediately clear if/when a msg is lost. Additionally, the action server could provide feedback while performing the action (fi: distance travelled or remaining).

This has the advantage, that my library has global awareness and I can prevent many user/robot/real-time errors

Can you also elaborate on this? 'global awareness' is typically not seen as an advantage, as it results in tightly coupled code (low locality of change, implicit assumptions about control flow, no access control, etc).


Edit:

There are two reasons for the statics. First one is as you suspected, that I don't want to miss messages. So I initialize the subscribers once and wait to receive messages. If I do that for every new navigation action, I'm wasting time waiting for the new subscriber every time.

Global awareness might be the wrong term. Essentially I wanted to always know whether my subscribers are receiving messages after they have been initialized once, statically. In other words, I'm only using static to eliminate the time it would take to re-initialize my publishers and subscribers.

I'm slightly confused: why would you want to reinitialise your publishers and subscribers?

You have an object, with a lifetime probably longer than that of a single request. Make use of that by initialising your publishers and subscribers only once (initialise member variables in the ctor fi), and use those instances when handling future requests.

What did you mean by "exposing as action"? This sounds exactly like what I need. Would that be having a separate Node running for these actions and triggering them via service calls for example?

In short: periodic dataflow where a single msg lost is not the end of the world: topics. Short (almost instantaneous) synchronous invocations: services. For long running (a)synchronous invocations that you also would like to monitor and potentially preempt: actions.

For a discussion on the different interaction patterns, please see ROS/Patterns/Communication. The actionlib library implements support for the Action pattern (essentially async services with a future or promise).

Can you give us an example of what a typical flow of control and data is for the functionality that you are wrapping? It sounds a bit like various responsibilites are mixed and that your current mapping of what you have and how you could/would use that in a ROS application are suboptimal. From your PathNavigation example fi I get the ... (more)

edit flag offensive delete link more

Comments

Sorry, I expanded my pseudo code now and hopefully it will make more sense. Basically I am using a class for navigations, and for every new navigation I instantiate a new object. That's why I would recreate subscribers and publishers every time.

potaahto gravatar imagepotaahto ( 2017-01-15 05:14:05 -0500 )edit

You are completely right, I do feel I am doing something wrong/unnecessary. That's why I am asking for best practices here. Or maybe example libraries doing something similar (basic navigation).

potaahto gravatar imagepotaahto ( 2017-01-15 05:14:58 -0500 )edit

For one 'example library' that deals with navigation, you could look at move_base. That seems to do at least what you show in your extended example, and a bit more even, but without all the blocking calls, and with a bit more separation of concerns.

gvdhoorn gravatar imagegvdhoorn ( 2017-01-15 05:26:46 -0500 )edit

As to your current design: you could still initialise your pubs & subs once, if you change your PathNavigation class to be more of an 'executor', which stays alive, and just receives new commands (goals in move_base terminology), instead of being recreated for every waypoint.

gvdhoorn gravatar imagegvdhoorn ( 2017-01-15 06:18:56 -0500 )edit

So instead of pushing PathNavigation instances, you'd create a single instance, which is your "robot controller" so to say, introduce a data structure to encapsulate just the waypoint info and give your PathNavigation instance a list of waypoints to navigate to.

gvdhoorn gravatar imagegvdhoorn ( 2017-01-15 06:20:53 -0500 )edit

But that is basically what move_base does.

gvdhoorn gravatar imagegvdhoorn ( 2017-01-15 06:21:04 -0500 )edit

Thank you very much. I will look into all that you have written and rethink the entire thing.

potaahto gravatar imagepotaahto ( 2017-01-15 10:19:29 -0500 )edit

No problem. Let us know about your progress.

gvdhoorn gravatar imagegvdhoorn ( 2017-01-15 10:42:36 -0500 )edit

Your Answer

Please start posting anonymously - your entry will be published after you log in or create a new account.

Add Answer

Question Tools

1 follower

Stats

Asked: 2017-01-14 09:34:14 -0500

Seen: 419 times

Last updated: Jan 15 '17