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.

Sign up for GitHub

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

Jump to bottom

Added option to change node name for the recorder from the Python API #1180

Merged
merged 5 commits into from
Dec 10, 2022
Merged

Added option to change node name for the recorder from the Python API #1180

merged 5 commits into from
Dec 10, 2022

Conversation

ricardo-manriquez
Copy link
Contributor

@ricardo-manriquez ricardo-manriquez commented Nov 24, 2022 •
edited by emersonknapp
Loading

Fixes #1145

This is a simple proposal to add the option to select the node name of the recorder.

My motivation is the ability to run multiple recorder nodes under the same ROS2 network (ROS_DOMAIN_ID) and be able to differentiate them.

Thanks everyone for your time!

Signed-off-by: Ricardo Manriquez ricardo.manriquez+gh@gmail.com

@ricardo-manriquez ricardo-manriquez requested a review from a team as a code owner November 24, 2022 04:48
@ricardo-manriquez ricardo-manriquez requested review from gbiggs and jhdcs and removed request for a team November 24, 2022 04:48
@ricardo-manriquez ricardo-manriquez force-pushed the ricardo-manriquez/node-name-option branch 2 times, most recently from 8fb1879 to 2a64c12 Compare November 24, 2022 05:17
emersonknapp
emersonknapp requested changes Dec 4, 2022
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

I'd like to ask for this to be exposed to ros2 bag record verb (record.py) as well so that CLI users have access to it. No real blockers in the code as-is though.

rosbag2_py/src/rosbag2_py/_transport.cpp Show resolved Hide resolved
MichaelOrlov
MichaelOrlov reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@ricardo-manriquez Thanks for you contribution.
Could you please fix DCO in your latest commit https://github.com/ros2/rosbag2/pull/1180/checks?check_run_id=9944472001 ?
Other than that look good to me.

@MichaelOrlov
Copy link
Contributor

@ros-pull-request-builder retest this please

@ricardo-manriquez
Copy link
Contributor Author

Thanks to everybody for the support, I made the changes and fixed the DCO which occurred because I used the web ui instead of the CLI, but as a side effect of all the shenanigans I inadvertently brought in changes from other commits and I think I pulled a commit which is not passing all the tests, should I undo that commit or just leave at is?

Best regards
Ricardo Manríquez

ricardo-manriquez and others added 5 commits December 7, 2022 21:08
@ricardo-manriquez @MichaelOrlov
Added the option to change the node name for the recorder from the Py…
c40e0ce 
…thon API

Signed-off-by: Ricardo Manríquez <64954533+ricardo-manriquez@users.noreply.github.com>
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
@MichaelOrlov
Fixed whitespace problems
0c87f64 
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
@MichaelOrlov
Fixed uncrustify problems
f26b4b5 
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
@MichaelOrlov
Added the option to set node name parameter from CLI
c4b9081 
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
@ricardo-manriquez @emersonknapp @MichaelOrlov
Update rosbag2_py/src/rosbag2_py/_transport.cpp
26779b4 
Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
Signed-off-by: Ricardo Manríquez <ricardo.manriquez+gh@gmail.com>
@MichaelOrlov MichaelOrlov force-pushed the ricardo-manriquez/node-name-option branch from cebf820 to 26779b4 Compare December 8, 2022 05:10
@MichaelOrlov
Copy link
Contributor

@ricardo-manriquez Yes, you messed up a bit with merge on your branch.
But no worries I have fixed it.
DCO fails since I have to make commits, but this is false positive error don't worry about it.

@MichaelOrlov
Copy link
Contributor

@emersonknapp Kindly ping to reiterate on review.

emersonknapp
emersonknapp approved these changes Dec 9, 2022
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for the revision!

@MichaelOrlov
Copy link
Contributor

Gist: https://gist.githubusercontent.com/MichaelOrlov/67d0ef1c6da9ddb9e294a759e59d18ca/raw/e8e319797d5fb44e6d1bb6e081757f46ae66e99c/ros2.repos
BUILD args: --packages-above-and-dependencies ros2bag rosbag2_py rosbag2_tests
TEST args: --packages-above ros2bag rosbag2_py rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11253

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ricardo-manriquez
Copy link
Contributor Author

Thank you all for the support, Is there anything I have to do now? Should I rebase to fix the DCO or should I close the Pull Request?

Best regards
Ricardo Manríquez

@MichaelOrlov MichaelOrlov merged commit b7e6a34 into ros2:rolling Dec 10, 2022
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Dec 10, 2022 •
edited
Loading

Hi @ricardo-manriquez,
No, you are fine. I've merged your PR.
Thank you for your contribution!

@ricardo-manriquez ricardo-manriquez deleted the ricardo-manriquez/node-name-option branch December 10, 2022 10:59
@emersonknapp emersonknapp mentioned this pull request Jul 25, 2023
Open
tm99hjkl added a commit to gekidaniino001/rosbag2 that referenced this pull request Jul 10, 2024
@tm99hjkl
ros2bag, rosbag2_py: add support for specifying the node name
7465c4e 
this commit was created by patching from rolling
branch (ros2#1180)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@gbiggs gbiggs Awaiting requested review from gbiggs gbiggs is a code owner automatically assigned from ros2/tooling-reviewers

@jhdcs jhdcs Awaiting requested review from jhdcs jhdcs is a code owner automatically assigned from ros2/tooling-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the ability to change the node name of the recording node using the recorder API
3 participants
@ricardo-manriquez @MichaelOrlov @emersonknapp