Actuator optional limits on actual position

d-loret commented 2 years ago

Summary: Adds optional parameters to specify limits on the actuator's actual position. If the limits are violated, an all-device fault is issued and motion is interrupted as a result. The actuator can be driven back to within limits through a profile position command with a target position that is within limits. These limits are useful as a safeguard for non-position commands which did not do checks on position, and as a replacement of Elmo-level limits on position feedback which get disabled if the modulo functionality is used (applicable to certain absolute encoders).

Test Plan: Tested on local network with a motor. Commanded the motor outside the limits and verified the motor is stopped. Verified the motor could be returned to within limits with a profile position command.

Reviewers: alex-brinkman, JosephBowkett

alex-brinkman commented 2 years ago

Alex and Daniel reviewd this in person. I provided feedback to reevaluate the new "actual" limit parameter since that introduction now adds a third type of limit parameters:

low_pos_cal_limit_eu - Calibration limit when performing hard-stop or limit-switch calibration
low_pos_cmd_limit_eu - The maximum value that can be commanded in position mode (similar to a SW Soft-stop)
low_pos_actual_limit_eu - Checks the actual value to ensure feedback is always within some valid range

I think I'd prefer to only have 2 parameters but cmd semantics doesn't fully capture the actual value check so let's decide something together.

For some Context from Slack:

Daniel:
I just realized we don't have a global position limit protection in the Actuator driver. 
I think the position limits are only checked with position-based commands. Velocity commands do not undergo that check, 
which would damage actuators with hard limits like EELS if a velocity command is sent by accident, for example. 
We probably want to add a global position limit to avoid accidentally damaging an actuator.

The underlying JSD driver gives access to HL[3] and LL[3] in the drive, which control position protection. The Egd Fastcat driver also allows access to them. But the Actuator driver explicitly disables them:
https://github.com/nasa-jpl/fastcat/blob/master/src/jsd/actuator.cc#L210-L211
[actuator.cc](https://github.com/nasa-jpl/fastcat/blob/master/src/jsd/actuator.cc)

  jsd_slave_config_.egd.low_position_limit      = 0;  // disable
  jsd_slave_config_.egd.high_position_limit     = 0;  // disable

Alex:

I don't think we want to use the EGD level of protection of this 
(unless you have an absolute encoder of course which is historically not been the case) 
At startup, the current ticks are read and then offset to get the expected output position 
(retrieved from the saved YAML) file so this drive-reported encoder reading won't be useful for most applications.
Currently, We have position checks only on position commands as you've correctly discovered.
If you want to add position checks to velocity and torque commands, I recommend implementing a 
signed motion check and some stateful-ness for when you start outside the position limits. 
The elements of this fault protection would be:

    A state variable that tracks if a motion command starts outside the limits (call it start_out_of_limits) 

    If starting outside limits, mark the flag true
    Once the actuator position comes within the limits for some persistence counts, mark the flag false
    If starting within limits, mark the flag false

    Pos Cmds: 

    Only permit motion commands that end within the position limits
    If starting outside the limits, still honor the command. (happens more than you might think during hard stop calibration)
    if start_out_of_limits = false and actual position reading is outside the limits -> Fault

    Vel/Cur Cmds:

    if start_out_of_limits = true 

    Permit Negative-signed effort if above the upper limit
    Permit Positive-signed effort if below the lower limit

    if start_out_of_limits = false

    If the actual position exceeds high_pos_cmd_limit_eu -> fault
    if the actual position is below low_pos_cmd_limit_eu -> fault

    If performing a Fastcat::Actuator profiled velocity command, check that result of the trap does not exceed a limit before starting motion, otherwise reject motion. Cannot be performed on CSV/CST commands.

d-loret commented 1 year ago

@alex-brinkman, can we revisit this. I originally wrote it for EELS, but now it has also been requested by STS. Dane needs to do some tests with the torque loop so he will need actual position limit checking to keep the actuator safe.

alex-brinkman commented 1 year ago

I'd like to hear a proposal for 2-parameter limits, instead of the third parameter this MR introduces. And review them with an internal SME to make sure the behavior and parameterization are no surprise to other applications.

nasa-jpl / fastcat

Actuator optional limits on actual position #48