More accurate/descriptive error messages for start_traj_mode and start_point_queue_mode services #265
Conversation
…k upon a failed Ros_MotionControl_StartMotionMode rather than just a boolean stating whether it failed or not, so the correct error message can be in the response.
…es improved, includes string associated with ErrNo, not just the (incorrect) MotionNotReadyCode error message.
…), checks more problem cases within Ros_MotionControl_StartMotionMode and does more to send back a valuable error message.
… cycle check to return proper error codes
…er error return strings
|
Dependent upon Yaskawa-Global/motoros2_interfaces#17 |
|
High-level comment (about the proposed approach): the current implementation seems to use a different approach: error reporting uses the result of The current approach is slightly racy (as in: noticing something was wrong and reporting about what was wrong are done at different times -- but so far that hasn't really been problematic), but the proposed approach starts mixing responsibilities. It also duplicates quite a bit of code. Perhaps we should try and find a middle-ground? |
|
And another (but only slightly related to this PR): seems the new error / pattern matcher in combination with the updated I'll look into this and fix it. |
|
@gavanderhoorn This is sort of an awkward responsibility-mixing design. I was trying to solve the issue without any sort of big redesign of how I do think that the current way that errors are reported, by noticing that there is an issue, and then trying to identify that issue later based on the state of the controller, seems likely to cause to problems/makes it difficult to get useful error messages. I think that a more direct error-reporting approach may be more reliable/less 'racy', but yeah there would be more code duplication. More significant changes would need to be made to get rid of the responsibility-mixing aspect, or I could move closer to the original approach. but I don't know what the best option is. I pushed some minor changes that make it a bit cleaner, but didn't change the approach. |
There was a problem hiding this comment.
I'm also not a big fan of the repeated code.
What if in Ros_MotionControl_StartMotionMode, the first thing you do is call Ros_Controller_GetNotReadySubcode? If it returns a code that cannot be resolved by the actions of this function (like estop or play-mode), then return that code immediately. That should remove some of the duplication.
I expect that such an approach would also need to ensure that things are checked in a particular order within Ros_Controller_GetNotReadySubcode. (Unresolvable issues checked first)
| @@ -361,3 +361,4 @@ libmicroros_dx200_foxy/ | |||
|
|
|||
| # M+ build output | |||
| *.out | |||
| /libmicroros_yrc1000_iron | |||
There was a problem hiding this comment.
if you're updating the gitignore, we'll need the dx200 and micro in here too. (Though I'm guessing ganvanderhoorn would want that in its own PR.)
| { | ||
| int ret; | ||
| MP_STD_RSP_DATA rData; | ||
| MP_START_JOB_SEND_DATA sStartData; | ||
| int checkCount; | ||
| int grpNo; | ||
| MotionNotReadyCode motion_readiness_code = MOTION_NOT_READY_UNSPECIFIED; | ||
| char output[256] = { 0 }; |
There was a problem hiding this comment.
The 256 needs to be a named constant. It is used in multiple places below.
Additionally, I'm not sure that the output buffer is really needed. If things are checked in the correct order, then Ros_ErrorHandling_MotionNotReadyCode_ToString should contain sufficient information to the user (I think). In that case, you shouldn't need to pass in the pointer to responseMessage.
Do you have some example strings from testing that you can post?
In reference to #37 and #192. Now the error message and error codes match the reasons that the motion start failed.