New service: allow retrieving information about active alarms and/or errors #182
Conversation
|
Note: CI builds here will fail until we release M+ |
gavanderhoorn
force-pushed
the
get_active_alarm_info
branch
from
fcf4d4c to
d564025
Compare
gavanderhoorn
force-pushed
the
get_active_alarm_info
branch
3 times, most recently
from
3e87aeb to
c2243c7
Compare
|
I've marked this as no-longer-a-draft as I tested it again yesterday and it (still) works. Not all fields of the response are filled (such as the timestamp), but that's by-design right now, as they are reserved for future use and will require some more work on the MotoROS2 side. We don't need to merge this into But @ted-miller or @yai-rosejo or @jimmy-mcelwain: if you'd have some time to review, please do. Edit: I only tested on YRC1 btw. It should work on YRC1u and DX2 as well. FS1 is not supported right now. |
|
|
||
| static micro_ros_utilities_memory_rule_t mem_rules_response_[] = | ||
| { | ||
| {"result_message", 50}, |
There was a problem hiding this comment.
Where'd the 50 come from?
There was a problem hiding this comment.
Tbh I don't remember (it's been a while).
Easy to change of course, so if we can come up with a more appropriate max length, we'll use that.
There was a problem hiding this comment.
@ted-miller: any preference for a default/initial length of the result_message field?
Longest result_message is 47 chars (here), so I suggest we set this to 64.
Default value for max string capacity is 20, so that would not be sufficient.
There was a problem hiding this comment.
I have no preference. 64 is fine with me
| // TODO: use proper error codes + strings | ||
| rosidl_runtime_c__String__assign(&response->result_message, "M+ API error"); | ||
| response->result_code = -1; |
There was a problem hiding this comment.
I think the -1 is ok (maybe replace with ERROR), but I do think this string should be more descriptive
There was a problem hiding this comment.
The TODO was mostly a reminder we need to come up with some nice(r) way to deal with return codes. Both here and in other cases.
I could make them part of the .srv definition, but wasn't sure that was the best way to go about it.
There was a problem hiding this comment.
And I'd probably like to define more meaningful error codes, to facilitate automated handling of errors (otherwise clients just see -1 and would have to interpret the result_message).
There was a problem hiding this comment.
I've defined a couple of result codes and messages (here). Still need to test them.
I opted to make them part of the response message instead of creating a separate .msg file for them as they are completely tied to this service implementation.
There was a problem hiding this comment.
coming back to this:
I opted to make them part of the response message
this is not a good idea, as it means the .srv definition would have to be updated to change or add result codes and strings. This changes the msg hash on Iron and later, which would cause compatibility issues even if none of the actual message fields change.
I'll create a dedicated result codes .msg, similar to what we have for other services.
|
My testing was performed on DX200 |
gavanderhoorn
force-pushed
the
get_active_alarm_info
branch
from
c2243c7 to
ab4d6cb
Compare
Allows clients to retrieve information about active alarms and errors (code, subcode, msg, subcode msg, etc).
Supports retrieving alarm & error information.
Use micro_ros_utilities and a memory config for the response.
Retrieved using PlatformLib functionality.
gavanderhoorn
force-pushed
the
get_active_alarm_info
branch
from
ab4d6cb to
8f307ac
Compare
As per title really.
Should fix #14 -- or at least for a large part.
Interface definitions are discussed in Yaskawa-Global/motoros2_interfaces#9. Some of the fields in that service/message definition are not populated/set by the implementation in this PR. I wanted to get a basic implementation reviewed + merged before potentially working on some of the more involved parts (such as retrieving cause-remedy pairs from PP data files).
The current implementation works, at least as tested on my YRC1. There is a single issue to iron out, which is that the underlying infrastructure (from M+) has trouble reporting multiple concurrently active user alarms correctly. Alarm and error codes + subcodes will be correct, but the message will not be. This is a known issue and is solved in newer releases of the controller OS.
This doesn't necessarily need to go into
0.1.2, so reviews can be done later.I just wanted to open the PR to avoid having it lying around any longer -- and avoid going in a direction which would not be acceptable later.