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

Loading config path from ROS2 parameter #103

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

sagocz
Copy link
Contributor

@sagocz sagocz commented Sep 12, 2022

Previously, you added a feature to load config file path from ROS1 parameter, but you omitted ROS2. This feature is necessary to properly load the configuration file, which is placed outside the package location in ROS2.
I have also added printing out information, from what path the configuration file is currently loaded.
I created pull request with my solution for that.

@Timple
Copy link
Contributor

Timple commented Sep 12, 2022

See PR #52, this was merged by them. But they secretly modified the parameter name without documentation 😉 so this isn't obvious for users who transition from ROS1.

@sagocz
Copy link
Contributor Author

sagocz commented Sep 12, 2022

See PR #52, this was merged by them. But they secretly modified the parameter name without documentation wink so this isn't obvious for users who transition from ROS1.

Maybe I'm blind, but what I can see, they only merged parameter from ROS1 and skipped ROS2. -> link

@Timple
Copy link
Contributor

Timple commented Sep 12, 2022

You're right!

@JWhitleyWork
Copy link

Can we PLEASE get this merged?

robertkurvits pushed a commit to DUT-Racing/rslidar_sdk that referenced this pull request Feb 14, 2023
@wienans
Copy link

wienans commented Mar 28, 2023

Would like to have this merged too.

Copy link

@rafa-martin rafa-martin left a comment

Choose a reason for hiding this comment

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

Tested in humble

@FelixHuang18 FelixHuang18 merged commit 9c33050 into RoboSense-LiDAR:dev_opt Jul 23, 2024
@FelixHuang18
Copy link
Contributor

We have merged your PR to the dev_opt branch. Thanks to everyone.

@FelixHuang18
Copy link
Contributor

close the PR.

@Timple
Copy link
Contributor

Timple commented Jul 23, 2024

What's the branching policy you are using if I may ask?

I see: main, dev, dev-opt and release. From the naming it isn't obvious which is 'the best'.

@FelixHuang18
Copy link
Contributor

@Timple
I have deleted the unnecessary branches, and now only the main and dev_opt branches remain. The dev_opt branch is for development. Once development is complete, it will be merged into the main branch.

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.

6 participants