search

PR2 / pr2_common

45 stars 79 forks source link

Make pr2_msgs re-usable? #286

Closed 130s closed 2 years ago

130s commented 3 years ago

Issue

Some (or all?) components in pr2_msgs are, despite the package name, independent of a specific hardware, PR2. Because of its name, however, it gives developers an impression that the pkg is specific to PR2, hence it's been hard to reuse.

Proposal

Either one of them:

Rationale

Rationale for Opt-A

Too disruptive for PR2 users. I don't think it's a good idea.

Rationale for Opt-B

I don't have a strong opinion. Since pr2_msgs at its current state (2020/09), it doesn't add any PR2 specific dependency, so despite its name this idea might be actually fine except it can give people a false impression.

Rationale for Opt-C

This might be cleanest, thus my favorite for now.

I have to admit that I've gone through only what I need, which is GPUStatus.msg. But I verified that there's no dependency on any PR2 components, and also verified the functionality after renaming the package and removing all the mentions of PR2, still functions for Nvidia 2080 GPU.

(I verified along with its consumer component pr2_computer_monitor/nvidia_temp.py, which I also removed PR2 from names and removed PR2-specific dependency.)

Proof of concept is stored on:

While pr2_msgs is maintained on a standalone repo, pr2_robot repo adds many PR2 dependency, so it's not ideal for a useful, PR2-agnostic component like pr2_computer_monitor to reside in.

Motivation

In my work GPU temp monitor feature PR2 comes with turnes out to be helpful. So I'm 70% sure that I'm happy to continue maintenance for 1+ year (and expecting for the confidence to grow).

v4hn commented 3 years ago

Hi Isaac, nice to read you here!

Opt-A is no option at all for us, as you said yourself. The PR2 stack should have gotten rid, e.g., of the pr2_controller_manager in favor of ros_control for many years now, but that never happened. Changing every single message definition everywhere can be (mostly) automated (up to the many packages we would forget...), but is also quite invasive.

I guess Opt-B amounts to adding a text to the README.md and adjusting the comments in all the messages to remove PR2 references. It's possible, but then why even keep the name?

Not sure if there's a plan to release PR2 components into noetic and ROS2.

@k-okada released at least pr2_msgs for noetic. Most other packages are not at all tested there and are not released yet. Python3 support will require users to test the code and take the time to fix it. It's unlikely the whole code base will be python3 compatible in the future.

Adapting to ROS2 will require changes to almost every single file in every single package with a lot of hardware testing. I don't know whether anyone is currently interested in that, but as the first serious mobile manipulators are tested with ROS2 only recently, it's clearly not in a state where it makes sense for a PR2 user to migrate. Migrating individual packages (and removing their PR2-references) can make sense though.

Converting generically usable PR2 components to non-PR2 can be helpful.

ros_control is the prime example. There are probably many others, yes.

I agree that Opt-C is the best way to go. Just take any component you deem worthwhile and create a new package with a more generic name for it. It would be great to see reusable parts released in generic forms and possibly migrated to ROS2 in the future! That way future PR2 users might even profit from development effort.

I don't like the approach to just copy everything by default though. The old BatteryState and BatteryServer messages have been deprecated for a decade in favor of the *2 versions. There's no reason to keep them and no reason to keep the "2" in the newer versions. PowerBoardState only makes sense to use with the exact power board setup of the PR2 (the board was custom design and afaik was actually designed from scratch again some years ago...) All the Laser* messages have nothing to do with monitoring or status information of a computer, but control a Laser Tilt unit. So in

I have to admit that I've gone through only what I need, which is GPUStatus.msg

This is particularly funny because at least the "official" clearpath server upgrade, which many groups got for ROS indigo, does not come with a GPU worth monitoring. (Really frustrating for many users)

130s commented 3 years ago

@v4hn I appreciate a thorough response (as always) from a long time PR2 contributor!

I guess Opt-B amounts to adding a text to the README.md and adjusting the comments in all the messages to remove PR2 references. It's possible, but then why even keep the name?

I guess after all these years without PR2 in the headlines, the word "PR2" has become more symbolic with broader meaning, e.g. 1st notable robot/hardware that ran on ROS. So I think it'd be great to keep the name in somewhere in the system so that it'd live on. But realistically, keeping it in the component name would continue causing a confusion. So I'm not that serious with Opt-B.

I haven't heard from anyone else so far, but taking Michael's opinion and I'm going ahead to go on Opt-C. I will probably make a rosdistro release request soon for computer_status_msgs after addressing his feedback. For computer_monitor that contains pr2_computer_monitor, I might be more interested in adding it to linux_peripheral_interfaces.

130s commented 2 years ago

With https://github.com/plusone-robotics/computer_status_msgs/issues/2 closed I think I got what I wanted done. So closing.