search

ros / rosdistro

This repo maintains a lists of repositories for each ROS distribution
Other
913 stars 2.54k forks source link

Regression in rplidar_ros package. #37679

Closed roni-kreinin closed 8 months ago

roni-kreinin commented 1 year ago

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 commented 1 year ago

@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. https://github.com/turtlebot/turtlebot4/issues/194 Then a migration plan can be determined for how to do this update without breaking existing users.

allenh1 commented 1 year ago

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 commented 1 year ago

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: https://github.com/ros2-gbp/ros2-gbp-github-org/issues/261

allenh1 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

deyouslamtec commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

allenh1 commented 1 year ago

@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 commented 1 year ago

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

allenh1 commented 1 year ago

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

deyouslamtec commented 1 year ago

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

mjcarroll commented 8 months ago

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