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

Reuniting spot_ros and this repository #25

Closed
heuristicus opened this issue Feb 16, 2023 · 12 comments
Closed

Reuniting spot_ros and this repository #25

heuristicus opened this issue Feb 16, 2023 · 12 comments

Comments

@heuristicus
Copy link
Contributor

Since Clearpath stopped their maintenance of the original spot_ros package, I've become the maintainer. Somehow I wasn't aware of this repository until now. I'd be interested in working out whether there is a sensible way to merge the two repositories and have a single place where we can track everything, and potentially have ros2 and ros1 as branches.

I think the packages have diverged quite a lot now, but since the core structure is still the same, there is overlap which means that many improvements could apply to both the ros1 and ros2 versions.

@jbarry-bdai
Copy link
Collaborator

Hi! Yes if we can figure out a sensible way to get these all under one repository roof, let's do it! I see some questions we should answer:

  • Which repo should we use?
  • Should ROS1 and ROS2 always be on two separate branches? (My opinion: it'd be nice to work towards one branch for both, not sure how feasible that is. If we want to start on two different ones I think that's fine.)

@heuristicus
Copy link
Contributor Author

heuristicus commented Feb 23, 2023

I think in the interest of preserving the history (and contributors) of the original repository, my preference would be to use the original repo. That said, perhaps it doesn't make much practical difference. I'm not sure how feasible it is to truly merge two repositories with issues and pull requests and so on, so some of that will be sacrificed either way. Merging this repo's main branch into the main branch of the ROS1 repo would preserve the commit history from both places, and it might be possible to somehow also migrate the pull requests and issues from here as well which may be easier since there are fewer.

In terms of where the repository lives in the end, I have no attachment to it being in any particular location, as long as it's accessible to the community. I had meant to transfer the original repository to my institute's dynamic robot systems github organisation (https://github.com/ori-drs), but perhaps it belongs here, since you have stronger ties to BD (though as far as I can tell they have no direct input on this ROS work).

I don't have a lot of experience with ROS2 so I'm not sure about the specific nuances of how it has to be set up. I agree that in the ideal case we'd have a single branch for both which can automatically switch. Thinking about it theoretically this should be possible by further abstracting the ROS interaction in spot_ros.py. My initial thought was some kind of object in which we implement functions for the common ROS interactions in both versions, and then all functions in spot_ros can call into that. The possible issue with that is that the handler functions appear to be slightly different between 1 and 2. For example, service handlers take the request and response in 2, but only the request in 1. There might also be some difficulty with message types between the two versions, but it should be possible to work around that as well.

The main and significant advantage of a single branch is that structural changes and additions when BD update the SDK don't have to be done twice. This would be a very good thing in my view. Practically speaking, having two separate repositories doesn't seem too different to having two branches, ignoring the issue and PR tracking, since everything has to be duplicated.

There are a couple of packages which might be useful: https://github.com/suurjaak/rosros, and https://github.com/dheera/rospy2 (there may also be others). I've not used them, but they may make using the two versions in one branch less painful. Or they may not be appropriate for our needs. I think the first step for this is to see if those packages would be useful by trying to convert a small part of spot_ros to see how they would work. Needs to test:

  1. Node startup
  2. Parameter reading
  3. Pub/sub
  4. Services
  5. Actionservers

Once we've worked out how we want to unify ROS interaction, both packages can make the switch to that. After that, at least one type of interaction in the driver will be at parity. Then, we will have to look at how best to merge the changes we've made in both repositories since the initial commit here in March 2022. I can't tell from that commit what version of the original repo was used. The changes added to the spot_ros repo since late 2021 are:

  • Interface for using the arm
  • Rviz interface
  • Safety features to disallow specific types of actions during driver unless a parameter flag is set at startup
  • A topic which can be used to disallow all motion of the robot, acting as a pseudo-freeze command
  • Services and parameters to allow the velocity of the robot to be limited
  • Fixes to some bugs which meant trajectory commands would occasionally fail
  • Changes to the body pose interaction so that it doesn't use the mobility command version, which has limits on the RPY values
  • Merging of Clearpath documentation repository
  • Autonomous docking interaction
  • Roll over interaction

These don't represent any significant structural changes, though it has been on my mind to make everything much more modular, as in my prototype spot_cam interface (https://github.com/ori-drs/spot_ros/blob/feature/spot-cam/spot_cam/src/spot_cam/spot_cam_ros.py). I would very much like to have the two versions merged before making such an overhaul, though.

@skpawar1305
Copy link
Collaborator

skpawar1305 commented Mar 2, 2023

maybe we can do that to preserve your history, but ros1 support will only be for 2 more years, so I think we can drop it

@jbarry-bdai
Copy link
Collaborator

Would you be open to transferring the ROS1 repo ownership to BDAI? (That's what we did for the spot_ros2 repo.) We don't have special ties to BD but we are happy to set up CI and stuff for everything (and in fact would prefer to).

Perhaps we could merge the repos but just have ros1 vs ros2 in packages to start. Probably we'll always need to keep the ros interface layer separate but we can likely combine the robot-facing layer?

@heuristicus
Copy link
Contributor Author

I'd be happy to transfer the repository here and continue to help with maintenance.

Combining the robot facing layer is definitely a good starting point. If the two repositories are still separate, perhaps this would work best by creating a separate repository for the wrapper and including it as a submodule, or requiring separate installation of it. It feels a bit odd to do this for a single file though.

@jbarry-bdai
Copy link
Collaborator

OK I think how we transferred the spot_ros2 one was to make me an admin on that repo and then I did the transfer. (We tried in many different ways to have MASKOR initiate the transfer and failed but happy to go down that route again if you'd prefer!)

I wonder if we can combine the two repos by simply putting ros1 code and ros2 code in completely different places to start and then at least they can see each other? And then we can start pulling common functions out of the underlying spot wrapper until it's gone?

@heuristicus
Copy link
Contributor Author

In terms of transfer, apparently it's necessary for the initiator of the transfer to have the ability to create repositories in the organisation that the repository is being transferred to. It probably doesn't make sense for me to have that ability in your organisation, so I will add you to spot_ros so you can initiate the transfer. I think once that is done you'll need to add me back to the repository as a collaborator or something similar so I can keep working with PRs and so on.

I'd say I'm leaning towards keeping the two separate at least until we sort out the wrapper, which is where we can definitely avoid conflicts. In theory we could shift everything into two directories spot_ros and spot_ros2 in the same repository but since there's active changes happening in both repositories things would likely get messy quickly. I think I'll try and create a repo with ros and ros2 branches with cherry-picked commits from both repositories to preserve history, and use that as a starting point for the merge process, at least to see more clearly what differences there are.

@heuristicus
Copy link
Contributor Author

Here's my initial very crude merge of the bare minimum files needed for the wrapper: https://github.com/heuristicus/spot_wrapper, along with notes about what needs further input at heuristicus/spot_wrapper#1.

I made the repo by taking the ros1 version as the base, deleting everything not relevant to the wrapper, and rearranging for a standalone python package. I did the same thing for the ros2 version by importing it as a branch. Then, I did a merge with --allow-unrelated-histories.

The difference is thankfully fairly minor. I'll try and get past the crude merge and make sure the thing actually runs.

After that, it probably makes sense to make a testing branch for the ros1 and ros2 repos which adds the separate wrapper repo as a submodule, so we can test integration.

That's all assuming that what I've presented above is acceptable to people.

@heuristicus
Copy link
Contributor Author

I've now made a pull request in this repository and the ROS1 repository with what I think are the required changes to split out the wrapper from the rest of the code.

I've tested the basic commands in the spot_ros repository with the wrapper split out, and it seems to work. Will need to do more testing myself and hopefully others will also be able to do some tests to verify that all the functionality is as it should be.

@jbarry-bdai
Copy link
Collaborator

Wow! Nice work.

I think we'd really like to own the spot wrapper repo if that's OK. Can you make me an admin on that repo and I'll transfer it over here? Once that's done I'll get it tested out with our stuff.

@jbarry-bdai
Copy link
Collaborator

jbarry-bdai commented Mar 27, 2023

Got it thanks! It's now at https://github.com/bdaiinstitute/spot_wrapper. I've made the same set of people admin/owners as are for spot_ros2 plus @heuristicus as an admin and put in the protections on main. (It's a public repo so anyone can pull it.)

I'll find some time to review the shared wrapper and test it out over here. @METEORITENMAX do you want to do the same for MASKOR?

@jiuguangw fyi

@jbarry-bdai
Copy link
Collaborator

We have a shared wrapper now! I think putting them in the same repo is actually pretty hard (based on some of our internal attempts to make ROS1 and ROS2 play together) so I'm going to close this.

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

3 participants