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

Update ROSMOD-COMM to enable coexistence with libraries that depend on ROS #8

Open
finger563 opened this issue Apr 4, 2018 · 8 comments
Assignees

Comments

@finger563
Copy link
Member

finger563 commented Apr 4, 2018

Right now ROSMOD Actors cannot run actionlib Action Clients or Action Servers - because they require a nodehandle. With our current design we'd have to have our own version of them which uses rosmod::NodeHandle and such - this is not maintainable. We should look into how we can strip down or rewrite the rosmod-comm and rosmod-actor code to support coexistence with such library code while still providing:

  1. queue control (PFIFO, EDF)
  2. deadline control and violation monitoring
  3. Automatic trace logging (which we could mostly replicate with trace logs in the generated callbacks)
finger563 added a commit that referenced this issue Apr 4, 2018
…s. updated rosmod_actor CMakeLists to better follow package and install conventions.
@finger563
Copy link
Member Author

@pranav-srinivas-kumar since you're most familiar with the ros source code I'd like your thoughts on how reasonable this is - e.g. can we just replace the callback queue? and use an external mechanism to link the priorities and such to the entries in the queue? If I remember correctly we discussed such an approach back when we started designing rosmod-comm but I don't remember why it didn't work.

@finger563
Copy link
Member Author

finger563 commented Apr 4, 2018

if we can find a way to associate the callback with the operation then we should be good - the callback is the same for each invocation of an operation and is different between different operations. I have some test code that runs rosmod_actor with vanilla ros objects, except it uses a ros::ROSMODCallbackQueue which is a subclass of ros::CallbackQueue and overrides the

virtual void addCallback(const CallbackInterfacePtr& callback, uint64_t removal_id)

from the base class - this allows us to modify the enqueuing mechanism (and hopefully update the queue based on priorities) though right now i'm just logging the enqueue time. Dequeue / completion can be monitored from within the component itself.

@finger563
Copy link
Member Author

Scratch that - the removal_id seems to be the unique object that should be associated with each operation.

@finger563
Copy link
Member Author

finger563 commented Apr 4, 2018

And now I remember why we banged our heads against the wall when looking at ros' design

@finger563
Copy link
Member Author

It looks like removal_id is the uint64_t memory address of the pointer gotten from:

timer manager : TimerInfo
subscription : SubscriptionQueue

callback_queue->addCallback(cb, (uint64_t)info.get()); // info is boost::shared_ptr<TimerInfo> in this case

@p-ranav
Copy link
Member

p-ranav commented Apr 5, 2018

I'm trying to understand the fundamental goal of this enhancement. You have a callback queue of operations; one that ros_comm provides. We made additions to this queue to support PFIFO, EDF and deadline monitoring. What part of this is affecting lack of support for actionlib? Sorry, I'm a little lost since I haven't worked on this in years..

@finger563
Copy link
Member Author

@pranav-srinivas-kumar we have a proper callback queue that does what we want - but it requires overwriting the ros::NodeHandle into a rosmod::NodeHandle - in addition to being completely separate from the ros::CallbackQueue. This creates problems when you have libraries that

  1. internally access the node handle to perform actions (because the ros::NodeHandle will throw errors about ros::init having not been called when used in component code), or
  2. interface with a ros;:CallbackQueue - even if they support the user providing a different callbackqueue, our rosmod::CallbackQueue cannot be passed into those functions

Both of these are the case for actionlib - which uses a node handle to advertise / subscribe / do some other stuff, as well as supports providing a callback queue for the callbacks to go through.

Because of that (and after looking through other libraries that internally do similar things) there is no way to get that library code to actually execute from within rosmod components - the code can compile but will fail at run-time.

Since that's the case I'd like to reexamine the component queue and see if theres a way we can manage this by simply subclassing the queue and not having to overwrite all the other classes. If that can't be done then I think we have two options:

  1. we implement the required changes as a PR in the official ros_comm repo (either as a slight redesign offering more overridable functions or by simply adding the priority / deadline / tracing infrastructure that we wrote in rosmod_comm), or
  2. we write a version of the rosmod component and rosmod_actor which use vanilla ros and do not offer priority, deadline tracking, and a more limited trace logging (which would miss enqueue time)

Thoughts?

@finger563 finger563 changed the title Update ROSMOD_Actor to enable coexistence with libraries that depend on ROS Update ROSMOD-COMM to enable coexistence with libraries that depend on ROS Apr 7, 2018
@finger563
Copy link
Member Author

Updated the title since rosmod-actor repo has now been update to be entirely ros based instead of depending on rosmod. This issue will be relevant to the redesign of rosmod if possible or to the discussion for the fork of ros-comm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants