search

ros-industrial / motoman

ROS-Industrial Motoman support (http://wiki.ros.org/motoman)
146 stars 193 forks source link

Missing `else` condition #519

Closed ted-miller closed 1 year ago

ted-miller commented 2 years ago

A user has found a missing else condition. This leads to an improper response to an unknown message type.

https://github.com/ros-industrial/motoman/blob/7860ff545106d98ed6a3fa0121f2fb60891f69bd/motoman_driver/MotoPlus/SimpleMessage.c#L153

gavanderhoorn commented 2 years ago

Isn't the actual consequence the else will always be taken if receiveMsg->header.msgType != ROS_MSG_MOTO_SELECT_TOOL?

So https://github.com/ros-industrial/motoman/blob/7860ff545106d98ed6a3fa0121f2fb60891f69bd/motoman_driver/MotoPlus/SimpleMessage.c#L160-L162

will always be executed?

EricMarcil commented 2 years ago

Without the "else" on line 153, all the preceeding code (line 138 to 152) is pointless because whatever is set in that code gets overwritten by line 161 and 162.

gavanderhoorn commented 2 years ago

Yes. That is what I wrote.

But only if msgType != ROS_MSG_MOTO_SELECT_TOOL.

ted-miller commented 2 years ago

Isn't the actual consequence the else will always be taken if receiveMsg->header.msgType != ROS_MSG_MOTO_SELECT_TOOL?

Yes, you're correct. Now that I think about it some more... this might explain some wireshark captures that I've seen in the past that had unexpected sequence-id's in the reply. (I don't remember for certain. But I think I've seen this and just ignored it.)

gavanderhoorn commented 1 year ago

Closing as #550 was merged (replacement PR).