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

Add run_name entry in app_manager to specify the node name #64

Open
wants to merge 1 commit into
base: kinetic-devel
Choose a base branch
from

Conversation

tkmtnt7000
Copy link

I added run_name entry to specify the node name when we use run entry.
Currently when we execute an app with run entry, the node name is not fixed such as node_name_<ramdom numbers>.

@v4hn
Copy link
Member

v4hn commented Jul 3, 2023

True, setting the node name is not supported in the run type. Maybe you could specify __name:=foobar as a argument though. But you are just adding more functionality here, that already exists in launch anyway as you can set the name in the launch file. Is it possible for you to migrate the app you need to launch instead? I would prefer to keep the run interface as-is.

That aside: welcome to the community and greetings to the JSK lab :-)

@tkmtnt7000
Copy link
Author

Thank you for advice.

That's true. The app can migrate to launch instead if we do not use parallel execution. I think running apps parallel is now supported only when we use run entry, so I sent this PR.
(I deeply undenstand that you would like to keep run interface as-is)

Maybe you could specify __name:=foobar as a argument though.

I have tested adding __name:=foobar to run_args, but the node name do not change into foobar.

@v4hn
Copy link
Member

v4hn commented Jul 3, 2023

@knorth55 please comment if you have a moment for your junior. ;)

I have tested adding __name:=foobar to run_args, but the node name do not change into foobar.

I guess they are overriden by the roslaunch command line then.

I think running apps parallel is now supported only when we use run entry, so I sent this PR.

True, @Affonso-Gui and @knorth55 worked on parallel support last year and explicitly excluded launch files.
They say it's a current limitation that launch is not supported here, but they don't say what the problem is. :)
also @knorth55 mentions that launch can be parallel in the future, so maybe you can solve that instead (assuming there is actually a problem aside from the conditional I link).

I really don't see why I'm a maintainer of this, afaict JSK is the only user of this module, so please get @k-okada to review JSK patches here. 🐕

@Affonso-Gui
Copy link
Contributor

Hello Michael,

Yes, currently parallel execution is only supported with the run entry.
If I recall correctly, run entries with allow_parallel set to true (default) will never lock the manager, while launch entries or run entries with allow_parallel set to false always will.

The problem was mostly a lack of time of my part.. Implementing parallel launches might be a little dull, but shouldn't be particularly challenging.

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