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

Replace tf_prefix by sensor_frame, lidar_frame and imu_frame parameters #96

Merged
merged 1 commit into from
Apr 28, 2023
Merged

Replace tf_prefix by sensor_frame, lidar_frame and imu_frame parameters #96

merged 1 commit into from
Apr 28, 2023

Conversation

doisyg
Copy link

@doisyg doisyg commented Apr 3, 2023 •
edited by Samahu

Related Issues & PRs

Summary of Changes

  • This PR removes tf_prefix from os_cloud node and replaces it with sensor_frame, lidar_frame and imu_frame parameters.
  • These parameters are initialized with the same default value as sensor_frame, lidar_frame and imu_frame variable were before, and at the same time they are read (not need for the double get_parameter/declare_parameter call). Hence allowing them to be optionally set by the user.

Validation

...

@Samahu Samahu self-requested a review April 3, 2023 22:55
@Aposhian
Copy link

Oops I didn't mean to duplicate this in #112. 100% agree with the declare/get in the same line. I can make my PR targeted to just the tf static broadcasting.

@Aposhian
Copy link

Aposhian commented Apr 27, 2023 •
edited

@Samahu can we push this through? It's simple enough, and it eases the transition from https://github.com/ros-drivers/ros2_ouster_drivers

@Samahu
Copy link
Contributor

Samahu commented Apr 28, 2023 •
edited

@Aposhian We can. I am working on it.

@Samahu Samahu added the enhancement New feature or request label Apr 28, 2023
@Samahu Samahu changed the base branch from ros2 to SW-4924-replace-tf-prefix-by-sensor-frame-lidar-frame-and-imu-frame-parameters April 28, 2023 19:22
Samahu
Samahu approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@Samahu Samahu left a comment

Choose a reason for hiding this comment

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

@doisyg I have reviewed your PR, but I do have some minimal changes that I would like to apply. But your fork doesn't allow changes from maintainers. So I am going to merge it to a branch of my own and then apply them. The net effect of the changes I want to apply are the same of your PR.

@Samahu Samahu merged commit acbe1fe into ouster-lidar:SW-4924-replace-tf-prefix-by-sensor-frame-lidar-frame-and-imu-frame-parameters Apr 28, 2023
@Samahu Samahu mentioned this pull request Apr 28, 2023
Merged
@Samahu
Copy link
Contributor

Samahu commented Apr 28, 2023

@doisyg @Aposhian feel free to comment on #115 if you have specific concerns before the PR gets merged.

Samahu added a commit that referenced this pull request Apr 28, 2023
@Samahu
SW-4924: Replace tf_prefix by sensor_frame lidar_frame and imu_frame …
8a9edfb 
…parameters (#115)

* deprecate tf_prefix from os_cloud (#96)
Co-authored-by: Guillaume Doisy <[email protected]>

* Squashed commit of the following:
commit 6280bfa1178bdee4fe695cb4752efd5ff15279db
Author: Ussama Naal <[email protected]>
Date:   Fri Apr 28 07:54:34 2023 -0700
    Merge branch 'deprecate_tf_prefix'

commit 35f2fd2
Author: Guillaume Doisy <[email protected]>
Date:   Mon Apr 3 18:12:44 2023 +0100
    deprecate tf_prefix from os_cloud

* Update ChangeLog and package version
* Propagate the parameters to launch files
* Add a TODO note
---------

Co-authored-by: Guillaume Doisy <[email protected]>
Samahu added a commit that referenced this pull request Apr 28, 2023
@Samahu
Squashed commit of the following:
b8216e9 
commit c94a581
Author: Ussama Naal <[email protected]>
Date:   Fri Apr 28 13:31:55 2023 -0700
-    Add a TODO note

commit 33b01cb
Author: Ussama Naal <[email protected]>
Date:   Fri Apr 28 13:20:09 2023 -0700

    Propagate the parameters to launch files

commit 2991897
Author: Ussama Naal <[email protected]>
Date:   Fri Apr 28 12:50:00 2023 -0700

    Update ChangeLog and package version

commit d58c186
Author: Ussama Naal <[email protected]>
Date:   Fri Apr 28 12:40:43 2023 -0700

    Squashed commit of the following:

    commit 6280bfa1178bdee4fe695cb4752efd5ff15279db
    Author: Ussama Naal <[email protected]>
    Date:   Fri Apr 28 07:54:34 2023 -0700

        Merge branch 'deprecate_tf_prefix'

    commit 35f2fd2
    Author: Guillaume Doisy <[email protected]>
    Date:   Mon Apr 3 18:12:44 2023 +0100

        deprecate tf_prefix from os_cloud

commit acbe1fe
Author: Guillaume Doisy <[email protected]>
Date:   Fri Apr 28 20:27:20 2023 +0100

    deprecate tf_prefix from os_cloud (#96)

    Co-authored-by: Guillaume Doisy <[email protected]>

commit 175a197
Author: Ussama Naal <[email protected]>
Date:   Wed Apr 19 12:28:13 2023 -0700

    SW-4837: replace the use of ros service to retrieve sensor metadata with latched topics (#102)

    * Working port of latched metadata topic on ros2
    * Update replay and record launch files to providing metadata file an optional parameter
    * Remove extra white space in replay record command
    * Undo changes to the metadata-qos-override
    * minor code syntax improvements
    * Add missing metadata topic when bag file isn't specified
    * Use concise syntax and formatting
    * Reverse logic for easier read
    * Apply node transition if it exists

commit 592e316
Author: Ussama Naal <[email protected]>
Date:   Tue Apr 11 18:18:24 2023 -0700

    Explicity set cxx compile standard if the env isn't (#99)

commit 7a10d2c
Author: Ussama Naal <[email protected]>
Date:   Tue Apr 4 15:42:49 2023 -0700

    SW-4747: update the ros 2 driver(s) to the 20230403 sdk release (#94)

    * Update to the latest ouster sdk
    * Forward multicast funcitonality + Other improvements and fixes
    * Add service_msgs dependency to package.xml
    * Correct sensor_mtp.launch for ros2 launch file format
    * Move to most recent SDK update
    * Declare and fill defaults for mtp paramters + fix uninitialzed compute_to_scan
    * Launch file rename and README corrections

commit fd76cc9
Author: Ussama Naal <[email protected]>
Date:   Tue Feb 28 20:07:46 2023 -0800

    Remove the duplicate sensor_info object

commit 7da1a96
Merge: 22acf88 2793304
Author: Ussama Naal <[email protected]>
Date:   Mon Feb 20 13:39:46 2023 -0800

    Merge pull request #51 from ouster-lidar/SW-4342-prototype-ros-2-driver

    ROS2 driver MVP (beta release)
@Samahu Samahu mentioned this pull request Apr 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Samahu Samahu Samahu approved these changes

Assignees

@Samahu Samahu

@doisyg doisyg

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@doisyg @Aposhian @Samahu