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: let the user inject a callaback to be executed after starting the subscribers #356

Open
wants to merge 21 commits into
base: rolling
Choose a base branch
from

Conversation

LastStarDust
Copy link
Contributor

I created this pull request as a proof of concept for the new feature discussed here: #348

@LastStarDust LastStarDust changed the title Let the user inject callables after starting subscribers WaitForTopics: let the user inject callables after starting subscribers Mar 1, 2023
Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

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

I've posted some questions worth discussing here : #348 (comment)

@LastStarDust
Copy link
Contributor Author

I've posted some questions worth discussing here : #348 (comment)

@adityapande-1995 @clalancette I have added a more comprehensive sample code in commit 5aa51d7

@LastStarDust
Copy link
Contributor Author

@adityapande-1995 @clalancette
Hello and sorry for the comment. Is there anything I can do to help with the review of this pull request?

@LastStarDust LastStarDust force-pushed the feature/inject-callables branch from 3f96cb5 to b8004ab Compare May 22, 2023 04:30
@LastStarDust
Copy link
Contributor Author

@osrf-jenkins retest this please

@LastStarDust
Copy link
Contributor Author

@adityapande-1995 @clalancette
Let me present a thought experiment that might make clearer what I am trying to achieve with this pull request.

How would you test a node that is subscribed to a topic with a single int field, that publishes the same topic where the int was incremented by one?

@LastStarDust
Copy link
Contributor Author

@osrf-jenkins retest this please

@LastStarDust
Copy link
Contributor Author

@adityapande-1995 @methylDragon
Friendly reminder about this PR.

Until now, I have been using my fork at work for our project, but it would give us more confidence if we could just use the official repository, or even better if this PR could end up in the released version on https://index.ros.org/packages/.

@LastStarDust
Copy link
Contributor Author

@osrf-jenkins retest this please

@adityapande-1995
Copy link
Contributor

@LastStarDust my apologies, taking a look now



# TODO: Test cases fail on Windows debug builds
# https://github.com/ros2/launch_ros/issues/292
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really old issue, is it still failing :O

@LastStarDust LastStarDust changed the title WaitForTopics: let the user inject callables after starting subscribers WaitForTopics: let the user inject callables to be executed after starting the subscribers Feb 1, 2024
Signed-off-by: Pintaudi Giorgio <[email protected]>
@LastStarDust
Copy link
Contributor Author

@adityapande-1995 @methylDragon I fixed some linting errors and now the checks are finally passing.

@Ryanf55
Copy link

Ryanf55 commented Feb 15, 2024

Hello, could we get a maintainer to look at this PR?

@LastStarDust LastStarDust changed the title WaitForTopics: let the user inject callables to be executed after starting the subscribers WaitForTopics: let the user inject callabacks to be executed after starting the subscribers Feb 16, 2024
@LastStarDust LastStarDust changed the title WaitForTopics: let the user inject callabacks to be executed after starting the subscribers WaitForTopics: let the user inject a callaback to be executed after starting the subscribers Feb 16, 2024
@mjcarroll
Copy link
Member

@methylDragon is OOO, but I will ping him to look when he is back.

Copy link

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

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

I tested this on a backport in humble in our own CI and it is very reliable with some minor mods.

@LastStarDust
Copy link
Contributor Author

LastStarDust commented Jul 3, 2024

@Ryanf55 Hello, I would like to resume working on this PR. Could you please take a look at the conversations you opened and resolve the ones you think are resolved?

@LastStarDust
Copy link
Contributor Author

@osrf-jenkins retest this please

@Ryanf55
Copy link

Ryanf55 commented Jul 9, 2024

@Ryanf55 Hello, I would like to resume working on this PR. Could you please take a look at the conversations you opened and resolve the ones you think are resolved?

Yep, all of the issues we found are still problems, and we never found a good workaround for doing this generically in humble. For now, we disabled our python automated testing as a requirement for PR's because this problem is so difficult to deal with, and have focused testing without the middleware. Once we can move to Jazzy, then the event handlers should make this kind of testing much more reliable because you can do a proper wait on the interfaces to come up.

Sorry, we've just spent an exorbitant amount of time on this trying to get this reliable in humble and could never figure out a way that wasn't extremely coupled to knowing exactly what was running and manual graph introspection hard-coded into our test framework. I'm optimistic that upgrading distros will make it easier, but this way of testing in python is not something we're going to pursue any further unless anyone can give us a reasonable approach that works without hard-coded timeouts/sleeps for discovery that fail on slower runners.

@LastStarDust
Copy link
Contributor Author

@Ryanf55 Thank you for sharing the situation. I am sorry we could not figure out how to make this work reliably in Humble.
Anyway, can I resolve the discussions opened by you in this PR, since it applies to rolling?

@Ryanf55
Copy link

Ryanf55 commented Jul 11, 2024

@Ryanf55 Thank you for sharing the situation. I am sorry we could not figure out how to make this work reliably in Humble. Anyway, can I resolve the discussions opened by you in this PR, since it applies to rolling?

Looks like I don't have authority to resolve threads, but consider the comments OBE. I'd be happy for this to be merged as-is, and then a follow up to go in that adds the event support.

Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Pintaudi Giorgio <[email protected]>
Signed-off-by: Pintaudi Giorgio <[email protected]>
@LastStarDust
Copy link
Contributor Author

LastStarDust commented Jul 31, 2024

@adityapande-1995 @methylDragon @mjcarroll
It took me a while to figure it out, but the unit tests are failing because in the CI environment the ros2 command does not support the topic argument. However, until some months ago the tests were passing (as they are passing on my machine) so something might have changed in the CI environment.

Here is the offending code:

def trigger_callback():
    command = 'ros2 topic pub --once --max-wait-time-secs 10 --keep-alive 1 \
        /input std_msgs/msg/String "data: Hello World"'
    p = Popen(command, shell=True, stdout=PIPE, stderr=PIPE)
    stdout, stderr = p.communicate()
    print("stdout: ", stdout)
    print("stderr: ", stderr)
    print('Callback triggered')

and here is the CI output:

15:06:12 stderr:  b"usage: ros2 [-h] [--use-python-default-buffering]\n            Call `ros2 <command> -h` for more detailed usage. ...\nros2: error: argument Call `ros2 <command> -h` for more detailed usage.: invalid choice: 'topic' (choose from 'daemon', 'extension_points', 'extensions', 'pkg')\n"

Please let me know how you want me to proceed. If the CI environment should be fixed or I should avoid using the ros2 topic command.

Signed-off-by: Pintaudi Giorgio <[email protected]>
@clalancette
Copy link
Contributor

It took me a while to figure it out, but the unit tests are failing because in the CI environment the ros2 command does not support the topic argument.

That's because there is no test_depend on ros2topic, so it is not available in the environment.

That said, I don't think we can introduce that dependency. In particular, I'm worried about a circular dependency here, since ros2topic already depends on launch_ros. If you are only trying to publish a message, then I'd say it is easier to use the rclpy APIs directly.

@LastStarDust LastStarDust force-pushed the feature/inject-callables branch from 21e7cf9 to e92eb67 Compare August 1, 2024 02:16
@LastStarDust
Copy link
Contributor Author

@clalancette
Thank you so much for the suggestion.
@adityapande-1995 @methylDragon @mjcarroll @clalancette
Now all tests are passing. You can resume the review.

@LastStarDust LastStarDust force-pushed the feature/inject-callables branch from d6066c3 to 57277d2 Compare August 1, 2024 06:38
@MichaelOrlov
Copy link

@mjcarroll Friendly ping for review.

@LastStarDust
Copy link
Contributor Author

@mjcarroll Friendly ping for review.

@LastStarDust LastStarDust force-pushed the feature/inject-callables branch from 8936dd9 to f2ee2fc Compare December 12, 2024 08:45
@LastStarDust
Copy link
Contributor Author

LastStarDust commented Dec 12, 2024

@adityapande-1995 @methylDragon @mjcarroll @MichaelOrlov @Ryanf55

I've revised how I pass callback arguments, which I found unsatisfactory before. Additionally, I've enhanced it by including the node itself as the first argument in the callback function, significantly boosting its versatility and power. I believe this PR is nearly ready for production from my side. Please share your thoughts.

@Ryanf55, I urgently needed this feature to be reliable, so I took the initiative and upgraded our company's ROS2 distribution to ROS2 Jazzy. It wasn't an easy decision, but you might want to consider doing the same.

I've signed off on the last commit, but I'm still encountering the DCO error. Could you please advise on what I might be missing?

@gavanderhoorn
Copy link
Contributor

I signed off last commit but I still get the DCO error. What am I doing wrong?

DCO check summary:

image

note the first name <-> last name reversal.

@LastStarDust LastStarDust force-pushed the feature/inject-callables branch from f2ee2fc to e5b92a3 Compare December 12, 2024 10:31
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

Successfully merging this pull request may close these issues.

8 participants