Fix bug with hasParam and getParamNames.#71
Fix bug with hasParam and getParamNames.#71chris-smith merged 2 commits intoRethinkRobotics-opensource:kinetic-develfrom
Conversation
The function "methodCall" was being incorrectly called on an object of type XmlrpcClient. "methodCall" is actually defined for a property of instances of XmlrpcClient, so this was a simple issue of missing indirection. XmlrpcClient is a wrapper that defines a "call" function as an interface, and delegates to an object from the xmlrpc module by calling its "methodCall". This commit changes the implementatino of hasParam and getParamNames to use "call" instead of "methodCall", adding the missing layer of indirection.
chris-smith
left a comment
chris-smith
left a comment
There was a problem hiding this comment.
Yikes! Not sure why I missed that... I'll file a ticket to create tests for these too so it doesn't inadvertently break again.
| } | ||
| else { | ||
| this._call('hasParam', data, (resp) => { | ||
| if (resp[0] !== 1) { |
There was a problem hiding this comment.
I think you can ignore this bit - XmlrpcClient should check if the response code is not 1 and automatically reject the call. You should be able to just call resolve(resp[2]) in this callback
There was a problem hiding this comment.
OK, please see the second commit!
I don't know where in the code the structure of the response array is decided, but would it make sense to define named constants for the indices like const ERROR_CODE = 0; or const REQUESTED_VALUE = 2; so you could write resolve(resp[REQUESTED_VALUE]); and wouldn't need the comment // resp[2] is whether it actually has param and presumably all anyone cares about?
There was a problem hiding this comment.
The response array follows the ROS XMLRPC Master/Slave/Parameter API spec. Generally the first element is a status code, the second element is a status message, and the third element is the actual data.
Unfortunately, this was one of the first things I wrote and so the use of different indices is scattered across a few files. We should be able to move all of that into XmlRpcClient and have it deal with any indexing into the actual response, but it would be a bit more work than just adding constants into one file.
It's been towards the back of my "nice to do" list for a while, but you're welcome to dig into it if you want.
XmlrpcClient should check if the response code is not 1 and automatically reject the call.
|
Thanks! |
2 participants
I was trying to use
hasParamin my program, but I got the following error:[TypeError: _this4._xmlrpcClient.methodCall is not a function at /home/.../node_modules/rosnodejs/dist/lib/ParamServerApiClient.js:102:30 at Promise (<anonymous>) at ParamServerApiClient.hasParam (/home/.../node_modules/rosnodejs/dist/lib/ParamServerApiClient.js:101:14) at RosNode.hasParam (/home/.../node_modules/rosnodejs/dist/lib/RosNode.js:342:35) at NodeHandle.hasParam (/home/.../node_modules/rosnodejs/dist/lib/NodeHandle.js:334:25) at fetchParams (/home/.../scripts/test_server.js:14:34) at <anonymous> at process._tickCallback (internal/process/next_tick.js:188:7)]I found that the bug was related to the indirection involving the
XmlrpcClientwrapper in rosnodejs and thexmlrpcmodule that it uses:XmlrpcClientseems to be a wrapper aroundxmlrpc: it has a_xmlrpcClientproperty, which is an object fromxmlrpc, and acall()function, which is the main interface.XmlrpcClient._tryExecuteCall()actually implements the RPC by passing_xmlrpcClienttoXmlrpcCall.call(), which then invokes itsmethodCall() function. Notice thatmethodCall()belongs to the_xmlrpcClientproperty, not to an instance ofXmlrpcClientitself.ParamServerApiClientalso has a property named_xmlrpcClient, whose type is theXmlrpcClientwrapper above. The functionsgetParamandsetParamwork fine, because they end up using thecall()interface; however,hasParamtries to usemethodCall(), which as described above belongs to a property ofParamServerApiClient._xmlrpcClientnot toParamServerApiClient._xmlrpcClientitself. I guess because the property in theXmlrpcClientwrapper, and the property in theParamServerApiClientclass that uses the wrapper have the same name, the author forgot about the level of indirection.This commit changes the implementatino of hasParam and getParamNames to use "call" instead of "methodCall", adding the missing layer of indirection.