Closed gavanderhoorn closed 3 years ago
Instead of checking on the ROS side, it would perhaps also be an idea to check on the MotoROS side.
There is some checking right now, but MotoROS does not return a very informative status code. It basically just says "didn't work".
@ted-miller: would it perhaps be possible / desirable to make MotoROS return specific causes for different failure modes?
That would remove the need to extend io_relay
with this, and -- more importantly -- would allow the io_relay
to setup generic error handling code with MotoROS being responsible for checking controller-specific cases. That would also help with #360: instead of having to tell io_relay
which ranges are acceptable, MotoROS would handle this and return something like "invalid address: out of range".
I would imagine perhaps MotoPlus return codes could be mapped?
@marip8 @steviedale: thoughts?
I'm fine with adding additional return codes. I'm assuming that you want this in the 'resultCode' field, and not in the header info. https://github.com/ros-industrial/motoman/blob/4b0a030de3917aa13ab1407f2dcf131d4f0ffd27/motoman_driver/MotoPlus/SimpleMessage.h#L285-L290
Yes, exactly. The header should just indicate general failure. The resultCode
could then encode the specific failure.
Would you already have some result codes in mind? We could start with failures reported by MotoPlus' read/write functions.
Edit: doesn't look like mpWriteIO(..)
returns anything but 0
or -1
.
Possible failures:
123
for digital out)-1
" (despite having checked bullets 1 and 2)anything else?
@ted-miller @EricMarcil @marip8 @steviedale: feel free to edit this comment (drop down in the top right of this comment).
Edit: as a table:
Constant | Value | Description |
---|---|---|
READ_ADDRESS_INVALID | 1001 | Address cannot be read on this controller |
WRITE_ADDRESS_INVALID | 1002 | Address cannot be written to on this controller |
WRITE_VALUE_INVALID | 1003 | The value supplied is not a valid value for the addressed IO element |
READ_MP_API_ERROR | 1004 | MotoPlus mpReadIO(..) returned -1 |
WRITE_MP_API_ERROR | 1005 | MotoPlus mpWriteIO(..) returned -1 |
note: values are just suggestions. I just made up some numbers.
@ted-miller: would you see an opportunity to add checking on the MotoROS side? Would you want someone else to contribute a PR to add this?
Sorry for the delay. I'll try to get to it this afternoon. If not, then I'll do it first in the morning.
Do you want the change in this repo, the new one, or both?
This one. I'll migrate the complete history to the other one -- it's a script.
@ted-miller @EricMarcil @steviedale @marip8: could you all take a look at https://github.com/ros-industrial/motoman/issues/367#issuecomment-721162583 and see whether there is a failure mode missing? Would be good to cover the expected failure modes with result codes in MotoROS.
Seeing as the discussion has shifted from filtering in io_relay
to extending MotoROS with checks, I've updated the title.
@gavanderhoorn those failure modes look good to me. The only addition I can think of is having a different error code for addresses that don't exist and addresses that do exist but are not readable/writable. Not sure if this is worth adding another error code for or not, just a thought.
Ok. So 10001
would be an illegal address in all cases but 70010
is valid, but just not writable (from https://github.com/ros-industrial/motoman/issues/360#issuecomment-707246441, DX200).
Is that what you're suggesting?
Exactly. It depends on whether you guys think this is overkill. I can see both an argument for and against.
Is there an easily accessible lookup table for valid addresses and read/write availability of these? If it is easy to find, then maybe this is in fact overkill, as they can simply lookup the address to see why it's invalid.
The documentation is pretty clear imo. See https://github.com/ros-industrial/motoman/issues/360#issuecomment-707246441 for a screenshot of the table from the DX200 manual.
Gotcha. In that case, I think the [READ/WRITE]_INVALID_ADDRESS
error codes suffice.
PR #369
Similar to #360, the
io_relay
could check for invalid values being passed with write requests.For single digital IOs, anything but
0
or1
is illegal.For group IO, there is a specific maximum value that could be written.
We could consider checking for illegal values in the
io_relay
and not pass those requests on to MotoROS. This would also allow us to return meaningful error messages to users.