search

CCNYRoboticsLab / phidgets_drivers

phidgets_drivers
6 stars 15 forks source link

Added High Speed Encoders Phidgets Drivers #23

Open geoffviola opened 8 years ago

geoffviola commented 8 years ago

Modified this code from an older driver. It fills in the header information, now. Tested it in Indigo.

muhrix commented 7 years ago

@mintar thanks for reviewing the PR!

I haven't got a phidgets encoder to test either. In fact, I don't even have my IMU (it's probably inside one of the boxes from my last move - or so I hope).

@geoffviola do you think you could make the changes suggested above? If not, just ping me and I can try to do it. Thanks for this PR by the way! :+1:

mintar commented 7 years ago

Hi @geoffviola , thanks for responding so quickly to my comments even though your PR is now already 5 months old!

I'm not familiar with the JointState Message. It might fit, but it is technically for "torque controlled joints". The encoder is not necessarily for that.

Yeah, I have no idea why the comments in JointState.msg say that. The message can be used for all kinds of joints, not just torque controlled joints. If you don't know joint velocity and effort, it's perfectly fine to leave those arrays empty and just fill the name and position arrays.

The EncoderParms message is legacy from the old package. It would require an extra parameter to convert the counts to an angle, but seems like a little bit too much work for now.

I still think this has to be changed before merging. The sensor_msgs/JointState message is the ROS standard for joint states, and by using it people can plug this node into all other kinds of ROS software. The custom EncoderParams message is useless, because it has to be converted into a JointState anyway, and that requires knowledge of how many encoder tics per radian there are and what index number corresponds to what joint name. That could easily be passed into this driver node as parameters, and the conversion be done here.

We should fix that now, because everything else can easily be fixed later, but changing the public API is always a pain. If it's too much work, tell @muhrix, he offered to help.

muhrix commented 7 years ago

Yes let me know, I'm happy to help!

mintar commented 7 years ago

This PR is now continued in ros-drivers/phidgets_drivers#15.