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 slash for node name #179

Merged
merged 3 commits into from
Dec 13, 2018
Merged

add slash for node name #179

merged 3 commits into from
Dec 13, 2018

Conversation

Karsten1987
Copy link
Contributor

same fix as for get: #174

@Karsten1987 Karsten1987 added the in review Waiting for review (Kanban column) label Dec 13, 2018
@Karsten1987 Karsten1987 self-assigned this Dec 13, 2018
@Karsten1987
Copy link
Contributor Author

the latest commit (be9e65a) should also address #178

@nuclearsandwich
Copy link
Member

It seems a bit odd to make this same transformation in every different verb. Is there a reason not to create a normalize_node_name or somesuch function?

@Karsten1987
Copy link
Contributor Author

Karsten1987 commented Dec 13, 2018

that is correct. I think we should actually have a function for this in rcl expanding the node name in a similar fashion as we do with topics (see https://github.com/ros2/rcl/blob/master/rcl/include/rcl/expand_topic_name.h) But given the time, I opted for this solution.

Nevermind, I just found out that we have this already in ros2node. https://github.com/ros2/ros2cli/blob/master/ros2node/ros2node/api/__init__.py#L26-L35

@wjwwood
Copy link
Member

wjwwood commented Dec 13, 2018

There could be a function to do this in rcl and it could be exposed via rclpy, like the expand topic name function:

https://github.com/ros2/rclpy/blob/91f7782619b1ccd9c812012ff85f6336186baab9/rclpy/rclpy/expand_topic_name.py#L18-L33

@Karsten1987
Copy link
Contributor Author

So my referenced function does actually not really help. It does pretty much the opposite. I guess I stick with my reasoning then that the rcl solution would be preferred but given the time I think it's not feasible.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I would prefer to see at least deduplication within the ros2cli repo or within the ros2param package, but this would work for now (a ticket could be created with help wanted on it).

@Karsten1987
Copy link
Contributor Author

I just introduced a new small function in ros2node called get_absolute_node_name. It's really just checking for the leading slash because I think we can all agree that a real node name expansion function should be there at one point (I'll create a ticket for it).

@wjwwood @jacobperron please iterate one more time over this. Also feel free to directly push on this ticket if you see room for improvement :)
thanks!

@Karsten1987 Karsten1987 merged commit f816aa0 into master Dec 13, 2018
@Karsten1987 Karsten1987 deleted the fix_lifecycle_set branch December 13, 2018 06:20
@Karsten1987 Karsten1987 removed the in review Waiting for review (Kanban column) label Dec 13, 2018
sloretz added a commit to ros2/ros2_documentation that referenced this pull request Dec 13, 2018
sloretz added a commit to ros2/ros2_documentation that referenced this pull request Dec 13, 2018
* [Node Arguments] Python nodes support parameters

Python nodes in Crystal support parameters
ros2 param list requires fully qualified node name (will file bug on ros2cli)

* Update Node-arguments.rst

* Separate block for bouncy

* Note about parameters in bouncy in documentation

* Revert changes to ros2 param list command

crystal now accepts leading slash ros2/ros2cli#179

* Whitespace removal
@nuclearsandwich
Copy link
Member

nuclearsandwich commented Dec 14, 2018

Was the ros2 node info verb intentionally omitted from this feature?

https://github.com/ros2/ros2cli/blob/aa832722af1c6295eb4b55951b5cd6a50c25108e/ros2node/ros2node/verb/info.py

#181

ferranm99 added a commit to ferranm99/ferran-ros that referenced this pull request May 20, 2022
* [Node Arguments] Python nodes support parameters

Python nodes in Crystal support parameters
ros2 param list requires fully qualified node name (will file bug on ros2cli)

* Update Node-arguments.rst

* Separate block for bouncy

* Note about parameters in bouncy in documentation

* Revert changes to ros2 param list command

crystal now accepts leading slash ros2/ros2cli#179

* Whitespace removal
esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
* Support required nodes

Currently, the only way to specify that a given node is required for a
given `LaunchDescription` is to register an `OnProcessExit` handler for
the exit event of the required node that then emits a `Shutdown` event.
Instead of requiring this boilerplate for a common use-case, add an
`on_exit` parameter to `ExecuteProcess` that can take actions, and add a
new action called `Shutdown` that immediately causes the launched system
to shut down. This allows for the concept of a required node to be
expressed simply with:

    launch_ros.actions.Node(
        # ...
        on_exit=Shutdown()
    )

Resolve ros2#174

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Inherit from EmitEvent

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Properly align type

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

* Expose reason

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
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.

None yet

5 participants