Switch to libphidget22

[clalancette]

This is a very large PR to switch to the entire phidgets_drivers to use libphidget22. Besides being the currently supported version, this version of the library allows us to use VINT hub devices, which we cannot do with libphidget21. With this PR, I was able to test out the following phidgets (some of them on a VINT hub):

1. Spatial
2. Analog Input
3. Digital Input
4. Digital Output
5. DCMotor
6. Temperature
7. Thermocouple

There are quite a few design decisions made in here that we need to agree on, so bear with me through each of the points below.

1. I decided that the design of the Phidget{21} base class was a bit awkward, since there is a lot of back and forth between the base class and the derived classes. Ultimately, the amount of common code between the different phidgets is relatively small, so for the libphidget22 port I just added some helper methods instead. This seems to have worked well, but let me know your thoughts about it.

2. In this PR, functions like "Digital Inputs" and "Digital Outputs" are separated into their own classes and ultimately their own ROS nodes/nodelets. This follows the design of libphidget22, and also makes things more generic. There is a big caveat with this method, though. If you have an IK Phidget, for instance, it has a set of Digital Inputs, Digital Outputs, and Analog Inputs. You cannot simultaneously access the device from multiple processes at once, so you cannot run one ROS node for each of the functions. You can access the device multiple times from the same process, so it does work to run one ROS nodelet for each function. Given this, I would suggest we actually remove the ROS nodes altogether and just have a nodelet per "function". This will reduce support problems later. Thoughts?

3. I've removed the (few) launch files that were here, mostly to keep things consistent between the different nodes. I could add some back, though this will partially depend on what we decide for the point above.

4. I've removed the use of diagnostics from the couple of nodes that used it. This is to maintain consistency between nodes and to make my life easier when porting to ROS 2 (which doesn't have diagnostics quite yet). We could revisit this decision and add it more consistently everywhere.

5. Because things are now split into functionality classes, the "IK", and "IMU" nodes make less sense. You can build their functionality by launching groups of other nodelets. I've left them in place for now for compatibility reasons, but I'd suggest we remove them.

6. All of the nodes have the concept of a publish_rate. If this value is > 0, then a ROS timer is started and data will be published at that rate regardless of whether it is changing or not. If publish_rate is <= 0, then data will only be published when it changes (i.e. when we get a callback from libphidget22). Separately, for all of the nodes except for "Digital Input", there is also a data_interval_ms, which defines how often the device acquires data and calls the callback. I think these are both useful so I don't have a specific question here, but I thought I'd point this out.

7. I've removed any use of the attachHandler and detachHandler that would allow you to start one of these nodes, then plug the device in. For my application this isn't needed, and it adds some complexity. Do you think it is something generally useful?

8. While the package.xml files here all define the license as BSD, there is no LICENSE file in the repository, and most of the files don't have a license header. I'd like to add both of those; how do you feel about it?

9. Documentation - Some of the caveats above will be good for consumers of this package to know. How would you like to do documentation? Update the README.md at the top-level?

10. Given all of the changes above, and despite some of my work for backwards compatibility, this will be a very different package when this is merged. I'd suggest creating a noetic branch on this repository and I can then re-target this PR to that branch.

Let me know your thoughts about the above points and anything else you can think of. Like on my last PR, I suggest reviewing this commit-by-commit, as each one ports/adds a single node.

Fixes #35

[clalancette]

Closing and re-opening to retrigger travis.

[clalancette]

@mintar Any thoughts here? I don't need a full review/merge at the moment, but I'd like to get some feedback on the general direction. If you like the direction, then I can go ahead and start doing the ROS 2 port. If you think there are significant structural/organizational changes to be done, I'll hold off.

Thanks!

[mintar]

I've finally had time to review the PR. First off: Thanks, and I like the general direction of the PR. I don't think any structural changes are necessary, but perhaps it's easier to finish iterating on this PR before doing the ROS 2 port, so you don't need to port any changes forward. It's up to you.

1. I decided that the design of the Phidget{21} base class was a bit awkward, since there is a lot of back and forth between the base class and the derived classes. Ultimately, the amount of common code between the different phidgets is relatively small, so for the libphidget22 port I just added some helper methods instead. This seems to have worked well, but let me know your thoughts about it.

Agreed, it looks nice.

2. In this PR, functions like "Digital Inputs" and "Digital Outputs" are separated into their own classes and ultimately their own ROS nodes/nodelets. This follows the design of libphidget22, and also makes things more generic. There is a big caveat with this method, though. If you have an IK Phidget, for instance, it has a set of Digital Inputs, Digital Outputs, and Analog Inputs. You cannot simultaneously access the device from multiple processes at once, so you cannot run one ROS node for each of the functions. You can access the device multiple times from the same process, so it does work to run one ROS nodelet for each function. Given this, I would suggest we actually remove the ROS nodes altogether and just have a nodelet per "function". This will reduce support problems later. Thoughts?

I agree. Debugging nodelets is a PITA, so I prefer nodes where possible, but in this case I think it's better to remove the nodes.

3. I've removed the (few) launch files that were here, mostly to keep things consistent between the different nodes. I could add some back, though this will partially depend on what we decide for the point above.

I'd prefer to bring the launch files back (only the ones with the nodelets). They serve as examples on how to use the code for each piece of hardware. Especially if we start requiring several different nodelets to be started simultaneously for one device, it's nice to have an example launch file.

4. I've removed the use of diagnostics from the couple of nodes that used it. This is to maintain consistency between nodes and to make my life easier when porting to ROS 2 (which doesn't have diagnostics quite yet). We could revisit this decision and add it more consistently everywhere.

I'd prefer having the diagnostics topics back. It doesn't have to happen within this PR though, we could first merge this PR into noetic and add the diagnostics later.

5. Because things are now split into functionality classes, the "IK", and "IMU" nodes make less sense. You can build their functionality by launching groups of other nodelets. I've left them in place for now for compatibility reasons, but I'd suggest we remove them.

If there is a launch file that composes different nodelets to provide the same functionality, I'm open to removing them.

6. All of the nodes have the concept of a publish_rate. If this value is > 0, then a ROS timer is started and data will be published at that rate regardless of whether it is changing or not. If publish_rate is <= 0, then data will only be published when it changes (i.e. when we get a callback from libphidget22). Separately, for all of the nodes except for "Digital Input", there is also a data_interval_ms, which defines how often the device acquires data and calls the callback. I think these are both useful so I don't have a specific question here, but I thought I'd point this out.

I've only had hands-on experience with the Phidgets Spatial. There, the messages basically change continually - no two subsequent messages will be the same. In this case, I strongly prefer publishing the data as soon as we get the callback from libphidget22 - it simply makes no sense to publish stale data repeatedly, or to drop messages. We can still keep the publish_rate parameter, but I would prefer to make the default 0 here.

I don't know how the other sensors behave. Assuming a digital output only triggers a callback whenever it changes from "true" to "false" or vice versa, then it would make sense to periodically republish the current state (otherwise there could be a long time between messages on that topic).

7. I've removed any use of the attachHandler and detachHandler that would allow you to start one of these nodes, then plug the device in. For my application this isn't needed, and it adds some complexity. Do you think it is something generally useful?

For my application neither - I don't know about other users, but I assume it's okay not to support this use case, if we improve the error messages a bit. Currently the nodelet just crashes and the nodelet manager silently swallows the exception if no device is plugged in - it would be nice if the user got an error message saying that no device (with the specified serial number, if given) was found.

8. While the package.xml files here all define the license as BSD, there is no LICENSE file in the repository, and most of the files don't have a license header. I'd like to add both of those; how do you feel about it?

Yes, that sounds like a great idea, it makes the licensing situation more explicit. I'm not a lawyer, but a bit of googling sounded like it's okay to link BSD code to a LGPLv3 library (libphidget22).

9. Documentation - Some of the caveats above will be good for consumers of this package to know. How would you like to do documentation? Update the README.md at the top-level?

Definitely as markdown within this repo. It's up to you whether you want to do this all within the top-level README, or split among the packages (or in a separate doc subfolder) with links from the top-level README. Up until now, the documentation was all on the ROS wiki, but I believe it's better to move it into this repo.

10. Given all of the changes above, and despite some of my work for backwards compatibility, this will be a very different package when this is merged. I'd suggest creating a noetic branch on this repository and I can then re-target this PR to that branch.

Great idea. I've just created the noetic branch, please re-target.

I have some more in-depth comments for the Phidgets Spatial. I'll add comments about that tomorrow.

[mintar]

@jlblancoc : You have a high-speed encoder, right? If you want to test your device with the new code, or if you have any comments, now is the time. :)

[clalancette]

Perhaps it's worthwhile to only remove those nodes that would cause the support problems that we talked about earlier. For example, if somebody is using an InterfaceKit and then starts the phidgets_digital_inputs_node and the phidgets_digital_outputs_node, it won't work - they need to use the nodelets instead. On the other hand, starting the phidgets_spatial_node would be fine. What do you think?

The issue is that there are Spatial phidgets that connect to a VINT hub: https://www.phidgets.com/?tier=3&catid=10&pcid=8&prodid=975 . If you have a temperature sensor also connected to the VINT hub, then you need to use the nodelet version of both to make it work. This goes for pretty much all of the phidgets; it looks like they are moving to a world where everything is hooked to a VINT hub.

I see that there is value in supporting both nodes and nodelets; as you mentioned, the former are easier to debug. And I honestly don't care which way we go here, I'm just trying to reduce support burden. For now, I'll push a commit that removes all of them, but just let me know what you want to do and I'm happy to revert or modify that.

Therefore I believe the libphidget22 package is LGPL and the others are BSD (exactly as the package.xml states), and it would be totally fine to add BSD license headers to all of our source files. According to this answer it would even be okay to add a LICENSE file with only the BSD text in the root folder, and then maybe a separate LICENSE file with the LGPLv3 text in the libphidget22 subfolder or something like that.

Ah, good point. I've added the BSD license headers everywhere and I've added both a LICENSE.lgplv3 and LICENSE.bsd at the top level. I'll push that commit shortly as well.

Just a note that I'm on vacation next week (Aug 19 - 23), so for anything I don't get to I'll pick up the following week.

[clalancette]

All right, I've done an initial pass over your first round of comments. What I haven't accomplished yet:

1. Reinstating the launch files.
2. Rewriting the IK node as a launch file with nodelets.
3. Writing the "migration guide"
4. Figuring out the use_imu_time (I'll need input from you there)

I'll be working on those remaining items, but it may have to wait until I get back. Thanks again for all of the reviews.

[mintar]

Thanks again for yet another round of commits! I've added two more (typo) comments. Apart from those, only the 4 points you've mentioned remain from my point. I'll also be on vacation next week. You're not by any chance visiting the chaos communication camp, are you? :)

Regarding the ROS2 port: I think it would make most sense to develop it in a separate branch in this repository. I haven't had any time to work with ROS2 yet, so it would probably make most sense if I gave you write access to this repo for working on ROS2 (I couldn't provide meaningful reviews anyway). What do you think?

[clalancette]

All right, I've addressed 3 of the 4 points. I don't have my phidgets spatial with me, so I can't test out my ideas for use_imu_time; I'll do that when I get back from vacation. Other than that, everything else should be done, barring more comments.

Thanks again for yet another round of commits! I've added two more (typo) comments. Apart from those, only the 4 points you've mentioned remain from my point. I'll also be on vacation next week. You're not by any chance visiting the chaos communication camp, are you? :)

No, family vacation. But it sounds like fun. Will you be at ROSCon?

Regarding the ROS2 port: I think it would make most sense to develop it in a separate branch in this repository. I haven't had any time to work with ROS2 yet, so it would probably make most sense if I gave you write access to this repo for working on ROS2 (I couldn't provide meaningful reviews anyway). What do you think?

Yeah, that makes perfect sense to me. What I'll probably do is create a dashing branch, then open PRs against the dashing branch as I work. That gives you visibility into what I'm doing, and gives you a chance to comment if you'd like.

[mintar]

All right, I've addressed 3 of the 4 points. I don't have my phidgets spatial with me, so I can't test out my ideas for use_imu_time; I'll do that when I get back from vacation. Other than that, everything else should be done, barring more comments.

Perfect, just comment after your vacation when it's done. I have no other comments, so we can merge then.

No, family vacation. But it sounds like fun. Will you be at ROSCon?

Unfortunately my IROS paper was rejected, so I also won't be able to attend ROSCon. It's a shame, I was really looking forward to it.

Yeah, that makes perfect sense to me. What I'll probably do is create a dashing branch, then open PRs against the dashing branch as I work. That gives you visibility into what I'm doing, and gives you a chance to comment if you'd like.

Sounds good! Ticketed as #45 .

[clalancette]

I am back from vacation, but I've been quiet. Sorry about that.

I have been working on the time resynchronization problem. I ran into some snags, but I think I finally have it mostly figured out. There is still some cleanup to do on it before I push it here, but unfortunately that will have to wait until next week. I'll ping again when I've finished.

[mintar]

OK, no worries!

[clalancette]

OK, no worries!

All right, I've finally gotten the resynchronization code done, commented, tested, and documented. It's a rather large commit, but it is probably best to just look at the changes to spatial_ros_i.cpp and in particular the comment about why it looks like it does. Let me know what you think!

[mintar]

Many thanks! It's been a real pleasure working with you. Thanks for going the extra mile and implementing a proper time synchronization logic! I'm going to forward-port #41 and #46 to noetic within the next few days. Do you mind if I add you to the maintainers tag in the package.xmls?

[clalancette]

Many thanks! It's been a real pleasure working with you.

Likewise, thanks for being a helpful and responsive maintainer.

I'm going to forward-port #41 and #46 to noetic within the next few days.

Perfect, thanks.

Do you mind if I add you to the maintainers tag in the package.xmls?

That is totally fine with me.

[arunavanag591]

Will there be an update to this wiki as the noetic branch is using libphidget22?

[clalancette]

Will there be an update to this wiki as the noetic branch is using libphidget22?

Yep, as soon as we release this package for noetic, the buildfarm will pick up the README.md in here and generate the documentation for it.

[arunavanag591]

Lastly, I see the motor is presently for DC motor, any future plans to include stepper motors

[clalancette]

Lastly, I see the motor is presently for DC motor, any future plans to include stepper motors

The open PR #56 implements this, but it needs to be cleaned up and I don't have time to work on it. If you'd like to clean up that PR and submit a new one getting the support in, I'd be happy to review it.