Add Phidgets high-speed encoder ROS node

[jlblancoc]

This PR is based on previous code by Geoff Viola (@geoffviola), which has been extended to:

• Publish one topic per encoder channel.
• On-the-fly determination of number of encoder channels.
• Two topic message types:
  • (a) Tick count + instantaneous speed.
  • (b) decimated, smooth velocity.
• Publication at a programmable rate, independent of arrival of encoder ticks.

I would also suggest removing all branch names and working from now on on master (which will imply changing rosdistro as well, of course), since packages for all recent ROS distros can be built from exactly the same sources. This PR is against kinetic just because it's the default branch, but it should be shared with lunar, etc.

[mintar]

This should be updated to #include <libphidget21/phidget21.h> in order to fix compilation on the ROS buildfarm CI. The Travis CI build failure is unrelated, I'm investigating that.

[mintar]

Hi @jlblancoc , thanks for taking up the work on this. I'll do a thorough review of this PR in the coming days.

[jlblancoc]

Sure, no rush! I was also thinking of investigating the travis error, let me know if you find it first. Also, we had dependency problems of the node wrt the phidget lib, will investigate it....

[jlblancoc]

@mintar dependencies are now fixed at the catkin/cmake levels after the last commit.

Travis still fails, but I'm not really sure what's going on there... (!!)

PS: ROS Wiki http://wiki.ros.org/phidgets_drivers is also updated (added new section no. 3)

[mintar]

Travis still fails, but I'm not really sure what's going on there... (!!)

Travis also fails on all other branches, so it's not related to this pull request. It has to be a change in some upstream dependency (because it started failing for the same commits that it was succeeding at before), but I haven't been able to track it down or reproduce locally yet. Just ignore it for now.

[jlblancoc]

Just a minor update: above I said a new section was added to the wiki page.

I later removed it for consistency with the other packages/nodes and moved the docs to an independent page. It's still not listed on the top of the main phidgets_drivers page, because changes obviously need to be merged first for the docs bot to catch them!

I also just realized that the new package needs to be added to the metapackage. Will add a commit for it.

Cheers.

[mintar]

I would also suggest removing all branch names and working from now on on master (which will imply changing rosdistro as well, of course), since packages for all recent ROS distros can be built from exactly the same sources. This PR is against kinetic just because it's the default branch, but it should be shared with lunar, etc.

Thanks for the suggestion, but I prefer keeping it the way it is. One reason is that on Indigo, we still use the libphidgets from cob_extern. From Jade onwards, we moved that library to this repo. I don't want to release two different libphidgets libraries into Indigo. Once this PR is merged, I'll also sync it to the Jade and Lunar branches (because as you say, Jade/Kinetic/Lunar all work the same). It's a bit more work for me (manually keeping the branches in sync), but I have more flexibility if at a later point we have to introduce breaking changes into say lunar.

[mintar]

So now on to the main point. Sorry it took me so long to review this PR. In my opinion there are two major changes that would make this PR much better:

1. Use sensor_msgs/JointState instead of EncoderParams and EncoderDecimatedSpeed.
2. Reuse the code from phidgets_api: encoder.h and encoder.cpp.

1. Using JointState

This point is more important than the other, since it defines the public API; the second point could still be changed behind the scenes later. In my opinion, it's best to use standard ROS messages in a case like this instead of defining one's own messages for the following reasons:

• One can use standard ROS nodes like robot_state_publisher, the existing RViz visualizations etc. out of the box.
• The Encoder* messages don't contain enough information to convert them into JointStates: the number of ticks in one radian is missing, and the joint name (from the URDF) is missing too.

The following things would have to be changed:

• the encoder node needs to know two things (probably provided via params): the joint names and ticks per radian for each channel
• also, it would be best to publish all channels via a single joint state topic (called joint_states) instead of on different topics per channel
• the decimated speed could still be published on a separate topic (like joint_states_decim_speed).

2. Re-using phidgets_api

I've recently merged support for the high speed encoder in #3 , but it's still missing a ROS wrapper (i.e., exactly what this PR does). It would be nice if that code was reused, so that we avoid code duplication. The phidgets_imu node follows the same pattern, and it would keep things consistent. It shouldn't be too hard to do the same thing in the encoder node. BTW: the phidgets_imu node does some more things, like compiling both as a node and a nodelet. I think that's unnecessary, a node should be totally sufficient for the data rates that we're dealing with. Just saying, in case you look at the phidgets_imu code, there is no need to replicate the node/nodelet part.

---

Thanks again for your contribution. I'm sorry that I'm proposing such major changes to the PR, but in my opinion the code would become much better. What do you think?

[jlblancoc]

Sure, your proposals make sense. Will give them a go...

[jlblancoc]

I'll also sync it to the Jade and Lunar branches (because as you say, Jade/Kinetic/Lunar all work the same). It's a bit more work for me (manually keeping the branches in sync), but I have more flexibility if at a later point we have to introduce breaking changes into say lunar.

Ok, as you prefer! One never knows... a bit more of manual work but as you say, you gain room for reacting to unexpected conflicts 👍

[jlblancoc]

I've rewritten the node based on the phidgets::Encoder C++ wrapper... it's a delight to use! 👍
Everything was where one would expect to be... good API.

Only remains to port to the new msg type...

[mintar]

Thanks for all the work so far! Let me know when you've ported it to the JointState message, or when you need anything else from me.

[jlblancoc]

Still alive! ;-)
Just being focused on (too many) other projects... will come back to this PR soon. Sorry for the delay!

[mintar]

All good, I'll wait until you come back. :)

[jlblancoc]

Done with the commit above. (was about time!)
Please, take a look at the code just in case you have further suggestions.

In the end, I followed your idea of publishing all channels together in one message of type JointState; however, decimated speeds are still published in separate topics, one per channel, since they are computed in an asynchronous way for each channel (i.e. as "edge events" arrive).

Note that this code is tested to compile, but has not been tested in a real device. It should work since the changes are only in the part of publish data format; but we'll try to test it next Monday and will come back here with the feedback.

Cheers.

[jlblancoc]

It works!

For me, it's now ready to be merged.

[mintar]

Thanks! I'll look into it in the coming days (I'm on vacation ATM). Just one quick thing I've noticed: The effort array is always all zeros, right? In that case it should be empty ([]) instead. From the JointState.msg comments:

# This message consists of a multiple arrays, one for each part of the joint state. 
# The goal is to make each of the fields optional. When e.g. your joints have no
# effort associated with them, you can leave the effort array empty. 
#
# All arrays in this message should have the same size, or be empty.
# This is the only way to uniquely associate the joint name with the correct
# states.

From your code it seems that the velocity array isn't always zeros, but just coincidentally in your screenshot, right?

[mintar]

Wow, that was quick! :)

[jlblancoc]

Great! I missed that part on effort. Done.
This part was a bit confusing but I think I now got it:

All arrays in this message should have the same size, or be empty.

On velocity: yes, they work, only that they are zero in the picture.
Enjoy the vacations!

[mintar]

I've rebased this onto kinetic, updated the README, applied some code formatting filters and merged via 7394959. I'm releasing it right now into kinetic and lunar. Many thanks @jlblancoc and @geoffviola for your contribution! 🎉 🎉 🎉

[jlblancoc]

👍
You're welcome!