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

WaitForTopics: there is no way to manually trigger a topic #348

Open
LastStarDust opened this issue Jan 26, 2023 · 8 comments
Open

WaitForTopics: there is no way to manually trigger a topic #348

LastStarDust opened this issue Jan 26, 2023 · 8 comments

Comments

@LastStarDust
Copy link
Contributor

LastStarDust commented Jan 26, 2023

Feature request

Feature description

The current implementation of WaitForTopics assumes that the node under test publishes messages without any external stimulus. But it might be that a node only publishes a topic if it received a certain stimulus, like receiving a message or a request.

In other words, let us image a node acting as both subscriber and publisher, with an input topic (subscriber) and an output topic (publisher). The input is received and processed and the output is emitted.

In this case, WaitForTopics would be waiting for a topic without any means to actually trigger it.

Implementation considerations

If the input topic is injected into the test node before calling WaitForTopics::wait(), the output topic might be emitted before the wait() call and it is not intercepted by wait(). On the other hand, if the input topic is injected after the call to wait(), obviously wait() cannot intercept the output.

One solution would be to inject a callback function into WaitForTopics that can take care of feeding the node under test with the needed input to produce the desired output.

Another solution is to provide a synchronization mechanism, such that the caller can create a dedicated thread to generate the input.

In any case, the input should be generated just after the subscribers have been started

    def wait(self):
        self.__ros_node.start_subscribers(self.topic_tuples)
        # <<<<<<<<< HERE >>>>>>>>>>
        return self.__ros_node.msg_event_object.wait(self.timeout)

Similarly to the issue #346 I am available to develop a solution if you think this feature is worth the effort.

@adityapande-1995
Copy link
Contributor

adityapande-1995 commented Mar 1, 2023

I have a couple of questions wrt #356 :

  1. It would be nice to name the trigger function with something other than a callback, as that term exists in ROS nodes and can be confusing. Secondly, its a method that would be triggered before the subscribers start, and hence are not callbacks exactly. They'll be acting more like triggers, or pre-set type of methods, and I'm open to using some other name for them.

  2. In the PR, it would be nice to have an example/test node that actually waits for a topic, or a service to be set using the trigger function, and have a nontrivial test case.

  3. Ideally, I can imagine such a node would wait for a service, or a parameter to be set to act as a trigger. Would adding an API here be better than setting those explicitly before calling WaitForTopics() ?

  4. An open ended method seems pretty generic to me. Would it be worth restricting the actions it can do, like making a service/action call ?

Open to suggestions @methylDragon @wjwwood

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Mar 2, 2023

    1. I agree that the name callback is too generic. What about trigger_callback or just trigger?

    2. its a method that would be triggered before the subscribers start

      maybe there was a misunderstanding but the whole point of this trigger feature is to execute an action AFTER the subscribers have started. Otherwise just calling the trigger before constructing the WaitForTopic object would suffice.

  1. I agree, however, I would like to define the scope and base implementation before writing a full-blown non-trivial sample.
  2. Yes an API would be nicer. I am open to suggestions about it.
  3. Again yes, the whole point of this trigger callback is to make sure that the input messages into the node under test are published AFTER the node subscribers have started, otherwise the messages are lost. So the main action to "allow" is the publishing of messages.

@clalancette
Copy link
Contributor

@LastStarDust Could you give us a little more concrete example of what you are trying to do here? We are just trying to figure out whether this is the right way to go, or if we should do something different like return a future from start_subscribers. Thanks.

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Mar 13, 2023

@LastStarDust Could you give us a little more concrete example of what you are trying to do here? We are just trying to figure out whether this is the right way to go, or if we should do something different like return a future from start_subscribers. Thanks.

@clalancette Yes, of course. You can check the launch_ros/launch_testing_ros/test/examples/wait_for_topic_inject_callback.py test for a more comprehensive example of how I envision this feature to be used.

@Ryanf55
Copy link

Ryanf55 commented Aug 15, 2023

Hey there, this is a great improvement. I have implemented something similar. Would you like to compare branches, take the best of both, and get this PR into rolling, iron and humble? Support for non-periodic publishers in automated testing is very important to me.

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Aug 16, 2023

@Ryanf55 Thank you for your interest in this feature.
I am the implementer, and still waiting for the review by the maintainers.
But of course, I would be happy to see my changes merged with yours.

@LastStarDust
Copy link
Contributor Author

Actually, there is one improvement that I would like to make to this PR, after the first round of review.
I would like to make the node object inside the WaitForTopics class available to the callback function.

@Ryanf55
Copy link

Ryanf55 commented Aug 16, 2023

Yea, I took quite a different approach and can post it here once I get approval (a few days). I'm glad you're still interested in getting something like this merged. Chat soon.

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

No branches or pull requests

4 participants