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

Merge various fixes and improvements into the humble branch #131

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

Conversation

apojomovsky
Copy link

@apojomovsky apojomovsky commented Apr 8, 2024

This PR:

  • Brings together the great work done by Mauro and Josh
  • Replaces the irobot_events_executor with the version from upstream/iron
  • Exposes an extra argument to the irobot-benchmark CLI, so that we can choose whether to use the execute_timers_separate_thread constructor argument, consumed by the EventsExecutor.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
irobot_benchmark/package.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@alsora alsora Apr 8, 2024

Choose a reason for hiding this comment

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

profiles are in a different directory, and I think we already have one almost identical to this: scripts/rmw/rmw_fastrtps_cpp/zero-copy-shm.xml

Copy link
Author

Choose a reason for hiding this comment

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

From what I was able to see, these are indeed identical. The potential path I see here to avoid this duplication would be to make the rmw_fastrtps_cpp/setup.sh to point to the (now) installed version of the xml file (under share/profiles/*) although OTOH, these also seem to be experimental scripts, so not sure if the change is justified.

Please let me know if you consider the change worthy so I can apply the change and give these scripts a try to confirm there are no regressions.

Copy link
Author

Choose a reason for hiding this comment

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

@mauropasse / @alsora Is there any strong preference for having these profile files installed instead of what we currently have under scripts/rmw ?

I don't think I have enough experience with this to have an informed decision myself.

Copy link
Author

Choose a reason for hiding this comment

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

gentle ping

@alsora
Copy link
Collaborator

alsora commented Apr 12, 2024

@apojomovsky you should:

  • retarget this PR towards the new rolling branch.
  • include only the upstream events-executor (and not the one from the irobot library)

@apojomovsky apojomovsky changed the base branch from humble to rolling April 15, 2024 14:10
@apojomovsky apojomovsky force-pushed the apojomovsky/multi-events-executors branch from eeb585d to 6a0c1d5 Compare April 15, 2024 14:16
Fix minor leftovers from removing irobot-events-executor
@apojomovsky
Copy link
Author

Thanks for the review @mauropasse , @alsora . Your comments were addressed, PTAL

@apojomovsky apojomovsky requested a review from alsora April 15, 2024 15:22
@apojomovsky apojomovsky requested a review from alsora April 15, 2024 20:59
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.

3 participants