usdot-fhwa-stol / carma-msgs

ROS Message definitions used by the CARMA Platform
6 stars 9 forks source link

Add SetActivateRobotic service #135

Closed adamlm closed 3 years ago

adamlm commented 3 years ago

PR Details

Description

This PR introduces a new service interface for activating and deactivating a hardware interface controller module. The interface closely follows that of the SetEnableRobotic service.

Related Issue

Closes #134

Motivation and Context

Some hardware interface controller modules (such as the VESC used on the C1T) do not have an interface for activating or deactivating control. Adding this service would provide such an interface.

How Has This Been Tested?

cav_srvs package with proposed enhancement has been built successfully.

Types of changes

Checklist:

msmcconnell commented 3 years ago

Interesting idea, but I want to clarify the current implementation and then discuss if this is needed. Currently, the usage of SetEnableRobotic is that the user requests hardware to engage, then the hardware driver reports if the engagement was successful via the robot_active field in RobotEnabled.msg. So the flow is,

  1. Send commands (to avoid driver timeouts)
  2. Set enabled
  3. Driver is enabled
  4. When the commands are received (or any other required data), the driver becomes Active when automation is actively in control of the vehicle.

A manual activation seems like it defeats the purpose of that feedback, but I could be misunderstanding the objective of this change.

adamlm commented 3 years ago

Thanks for the clarification. I think I was misunderstanding the flow.

To be precise, let's define three terms: enabled, engaged, active.

Do these definitions match the current implementation? It seems to be that same as what you're saying, but this was how I interpreted it. If this is correct, then I don't think we need a SetActivateRobotic.srv.

msmcconnell commented 3 years ago

@adamlm That is correct.

adamlm commented 3 years ago

After discussion, conclusion is this feature is unnecessary.