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

Regression in rplidar_ros package. #37679

Closed
roni-kreinin opened this issue Jun 15, 2023 · 17 comments
Closed

Regression in rplidar_ros package. #37679

roni-kreinin opened this issue Jun 15, 2023 · 17 comments

Comments

@roni-kreinin
Copy link
Contributor

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.

@tfoote
Copy link
Member

tfoote commented Jun 15, 2023

@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.

@allenh1
Copy link
Contributor

allenh1 commented Jun 15, 2023

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.

@tfoote
Copy link
Member

tfoote commented Jun 15, 2023

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

@allenh1
Copy link
Contributor

allenh1 commented Jun 15, 2023

I've opened the PR to revert the breaking change and usurpation of the package.

@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?

@audrow
Copy link
Contributor

audrow commented Jun 16, 2023

@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

@tfoote, I merged in your revert. Thanks for moving fast on this.

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?

@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.
https://repo.ros2.org/status_page/ros_humble_default.html?q=rplidar_ros

@allenh1
Copy link
Contributor

allenh1 commented Jun 16, 2023

@audrow ok, that's good. Very glad this was caught quickly!

@deyouslamtec
Copy link
Contributor

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

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?

@roni-kreinin
Copy link
Contributor Author

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.

@allenh1
Copy link
Contributor

allenh1 commented Jun 16, 2023

@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.

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.

@tfoote
Copy link
Member

tfoote commented Jun 16, 2023

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 ros2 branch from @allenh1 one would be best. That way the community can maintain continuity and we can transition all pointers to the repository over.

@allenh1 you can keep yours maybe archive it with an updated readme.

@allenh1
Copy link
Contributor

allenh1 commented Jun 16, 2023

@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 package.xml to @deyouslamtec? Then, at that point, I will update the readme in my repository to point to the slamtech repo, then archive my repo.

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 ros2 branch and allow me to push my branch to your repository?

@deyouslamtec
Copy link
Contributor

@allenh1 I agree. I have renamed my ros2 branch to ros2-devel,you can push your ros2 branch to my repository now.

@allenh1
Copy link
Contributor

allenh1 commented Jun 20, 2023

@allenh1 I agree. I have renamed my ros2 branch to ros2-devel,you can push your ros2 branch to my repository now.

Awesome! Could you please give me push access to your repository? Otherwise, feel free to just clone my repo and push the ros2 branch to your repo.

@deyouslamtec
Copy link
Contributor

@allenh1 OK。I have clone your ros2 branch to my repo

@allenh1
Copy link
Contributor

allenh1 commented Jun 21, 2023

@deyouslamtec great! I will update mine and archive it later today.

@deyouslamtec
Copy link
Contributor

deyouslamtec commented Jun 25, 2023

@allenh1 I have already changed the maintainer to myself package.xml maintainer deyouslamtec.
@tfoote I have been removed from the rplidar_ros team.here(ros2-gbp/ros2-gbp-github-org#285). Should I update the rplidar_ros team members myself or should it be done by @allenh1 ?

@mjcarroll
Copy link
Member

I believe that all the necessary steps have happened here and we can close this out.

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

No branches or pull requests

6 participants