Draft of motor and encoder API #3
Conversation
|
Hi @Zabot , thanks for your pull request! Looks good to me and I'd be ready to merge this. Replacing the asserts with exceptions also sounds like a good idea. Is there a reason why you didn't implement the event handlers yet? Are you planning on providing ROS wrappers that use the new API calls? BTW, I'm trying to get new contributions to follow the ROS style guide. The easiest way is to let sudo apt-get install astyle
astyle include/phidgets_api/encoder.h include/phidgets_api/motor.h src/encoder.cpp src/motor.cpp -n --style=allman --indent=spaces=2 --pad-oper --unpad-paren --pad-header --convert-tabs |
|
I left the event handlers out because I wanted to make sure they were consistent with the rest of the event handlers, but since there aren't any user event handers implemented yet, I didn't implement them. They could either be direct wrappers of the C libary calls, or they could do a little more in terms of allowing for multiple listeners to be bound to one event and supporting C++ lambdas. Will do on the autoformating, and as for the ROS wrappers, that's probably out of the scope of this PR, but it shouldn't be too hard. |
The existing devices do use event handlers. For example, there's
Yes, it's out of scope, and this PR can be merged without them. I was just wondering if you had any code already written and lined up for future PRs. If I may ask: without any code calling the files in this PR, they don't do anything; so how did you come up with this PR? (Not complaining, just curious!) |
|
Must have missed that when I was going through, I'll add the listeners this weekend. I wrote the api and then used it for a robot specific |
|
Oh, that sounds elegant. Do you have a link? (Just for my own curiosity, it indeed doesn't belong into this repo). |
| * int CPhidgetEncoder_set_OnInputChange_Handler (CPhidgetEncoderHandle phid, int(*fptr)(CPhidgetEncoderHandle phid, void *userPtr, int index, int inputState), void *userPtr) | ||
| * int CPhidgetEncoder_set_OnPositionChange_Handler (CPhidgetEncoderHandle phid, int(*fptr)(CPhidgetEncoderHandle phid, void *userPtr, int index, int time, int positionChange), void *userPtr) | ||
| * int CPhidgetEncoder_set_OnIndex_Handler (CPhidgetEncoderHandle phid, int(*fptr)(CPhidgetEncoderHandle phid, void *userPtr, int index, int indexPosition), void *userPtr) | ||
| */ |
There was a problem hiding this comment.
What's the meaning of these "TO-DO"? As far as I see, those handlers are already set in the ctor, and virtual methods provided for a derived class to override them... (??)
There was a problem hiding this comment.
Yes, I've already made the changes addressing those TODOs in cbf4301.
There was a problem hiding this comment.
👍 I loved to see & use all those private/protected methods. Really useful... thanks!
I had a couple phidgets lying around that I had been using on a robot doing SLAM, and I figured I'd wrap the API for them. In theory the macros I defined for
MotorControllershould generalize to any of the phidget modules. Several of the phidget boards have common elements (digital inputs, encoder inputs, etc.) that might be worth creating interfaces for to try and minimize the amount of redundant code.I
asserted that none of thelibphidgetcalls would fail, but it might also be worth implementing aPhidgetExceptionto throw in case of library function failures.