-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Regression in rplidar_ros package. #37679
Comments
@deyouslamtec This looks like a regression as well as you're taking over maintinership(from @allenh1 ) of the package w/o a notice to the ROS community in your PR (#37606 ) it would be much better to mention that instead of just considering it a point release. @audrow I would suggest a rollback of this in the immediate term as it's only in testing right now and significantly breaks active humble users including the TurtleBot 4 community. turtlebot/turtlebot4#194 Then a migration plan can be determined for how to do this update without breaking existing users. |
Thanks for the ping @tfoote, I had no knowledge of this. This puts things in a rather awkward spot, because it clearly breaks compatibility with existing uses of the driver, and also silently swaps the codebase to a completely different one. I'm happy to surrender maintainership to SLAMTEC (it is their product, after all), but perhaps we should transfer my repository to slamtec and make changes to that codebase? It seems like it will get confusing to have two repos lingering around. |
Yes this does change things. I had assumed that there had been coordination with the existing maintainer and the handover had not been smooth. I've opened the PR to revert the breaking change and usurpation of the package. @deyouslamtec Can you please explain why you did not reach out to the existing maintainer or mention that in your pull request here: ros2-gbp/ros2-gbp-github-org#261 |
@tfoote thanks for doing that. I'm a little confused... Has this been released as part of a sync? If so, will installed binaries with this version update to the new one, or should I release a new version in the interim? |
@tfoote, I merged in your revert. Thanks for moving fast on this.
@allenh1, it doesn't look like this was included in the last sync. The last version synced was 2.1.0. Otherwise, I'd have to make another sync. |
@audrow ok, that's good. Very glad this was caught quickly! |
sorry,my misstake。It was because I didn't have a clear understanding of the contribution rules that I didn't notify the existing maintainer. @allenh1 I'm very sorry for not communicating with you in advance. Please don't mind. Actually, I am preparing to contact you soon. What do I need to do now to solve the current problem? |
Thank you for the quick fix. @deyouslamtec I will just request that if the code base is changing to the Slamtec github, try to merge in some of the new features that are available in https://github.com/allenh1/rplidar_ros beforehand. For TurtleBot 4 specifically, we are using the start/stop services as well as the auto standby mode. |
In an effort to preserve existing code, maybe the ownership of my repository should simply be transferred to @deyouslamtec, and they can operate on that codebase. I think we could make the migration a bit less painful for everyone by just having one repo instead of two. |
Since they already have the same codebase with the ROS 1 bindings I think that a "transfer" is not possible. But the ros2 branch on the Slamtec repository building on and extending the existing branch from the @allenh1 you can keep yours maybe archive it with an updated readme. |
@tfoote So, to be clear, the suggestion is that the branch on my repository will overwrite the one on Slamtech's repo, and I will change the maintainer in the I think this is a very good approach, and it will smoothe the transition considerably. @deyouslamtec does this plan work for you? Could you rename your |
@allenh1 I agree. I have renamed my ros2 branch to ros2-devel,you can push your ros2 branch to my repository now. |
@deyouslamtec great! I will update mine and archive it later today. |
@allenh1 I have already changed the maintainer to myself package.xml maintainer deyouslamtec. |
I believe that all the necessary steps have happened here and we can close this out. |
Version 2.1.2 of
rplidar_ros
was merged recently (#37606). This release came from https://github.com/Slamtec/rplidar_ros/tree/ros2, while previous releases were from this fork https://github.com/allenh1/rplidar_ros. The new release has some breaking changes such as node name changes and missing features.The text was updated successfully, but these errors were encountered: