SparkMaxRelativeEncoder::SetPosition() method is unsynchronized with GetPosition()

[mjhealy]

Description:

Calls to the SetPosition() method on a SparkMaxRelativeEncoder are not immediately reflected in the value returned by GetPosition(); instead, the encoder appears to be receiving the update asynchronously at some later point in time.

This can result in unexpected/incorrect behavior in client code.

Example steps to reproduce:

Assumptions:

• An frc::Subsystem class (discussed below) has at least one CANSparkMax motor controller and related relative encoder.
• An frc2::Command class (discussed below) uses the subsystem. Simplified example:
• Simplified example code:

  
  class DriveBase : public frc2::SubsystemBase {
  private:
  rev::CANSparkMax m_leftFront{MotorIds::SparkMax::LEFT_FRONT_DRIVE_MOTOR_ID,
                             rev::CANSparkMax::MotorType::kBrushless};
  rev::SparkMaxRelativeEncoder m_leftFrontEncoder = m_leftFront.GetEncoder();
  // Note: controllers and encoders for other motors have been elided
  public:
  DriveBase() {
  // Configuring encoder to report position in meters (based on wheel size/gearing)
  // is elided (but assumed)
  . . . .
  // Reset encoders to 0
  resetEncoders();
  std::cerr << "Left position in ctor: " << getLeftPosition() << std::endl;
  }
  void resetEncoders() {
  if (m_leftFrontEncoder.SetPosition(0) == rev::REVLibError::kOK) {
    std::cerr << "Successfully reset encoder.\n";
  } else {
    std::cerr << "Failed to reset encoder!!!\n";
  }
  }
  double getLeftPosition() {
  return m_leftFrontEncoder.GetPosition();
  }
  void drive(double percent) {
  m_leftFront.set(percent);
  }
  };

class DriveForDistance : public frc2::CommandHelper<frc2::CommandBase, DriveForDistance> { private: DriveBase driveBase; const double distance; double start; public: DriveForDistance(DriveBase base, double meters) : driveBase(base), distance(meters), start(0) { AddRequirements(driveBase); } void Initialize() { driveBase->resetEncoders(); // should print success message start = driveBase->getLeftPosition(); std::cerr << "Left position on init: " << start << std::endl; // should report 0 } void IsFinished() { double position = driveBase->getLeftPosition(); std::cerr << "Current position: " << position << std::endl; return (position >= start + distance); } void Execute() { driveBase->drive(0.3); } void End() { driveBase->drive(0); } };

1. "Do stuff" with the subsystem so that the encoder will take on a non-zero value.
   * Note: this allows the steps below to demonstrate the issue in step 2, but is not strictly required. 
2. Restart the robot code, triggering constructor for the subsystem.
   * Expected results:
      * subsystem construction should report success on resetting encoder
      * subsystem construction should report that current encoder value is 0
   * Actual results:
      * subsystem construction reports success on resetting encoder (OK)
      * subsystem construction reports that current encoder value is still non-zero (**UNEXPECTED**)
3. Trigger `DriveForDistance` command with distance of (say) 1 meter.
   * Expected results:
      * Initialize() method should report success on resetting encoder
      * Initialize() method should report that current encoder value (assigned to `start`) is 0
      * First time that IsFinished() is called, it should report that the current encoder value is 0
      * Drive base should move for total distance of 1 meter
   * Action results:
      * Initialize() method reports success on resetting encoder (OK)
      * Initialize() method reports that current encoder value is now 0 (**Desired, but unexpected given failure observed in step 2**)
      * First time that IsFinished() is called, it reports that the current encoder value is 0 (OK)
      * Drive base moves for total distance of 1 meter (OK)
3. Trigger `DriveForDistance` command again with distance of 1 meter.
   * Expected results:
      * Initialize() method should report success on resetting encoder
      * Initialize() method should report that current encoder value (assigned to `start`) is 0
      * First time that IsFinished() is called, it should report that the current encoder value is 0
      * Drive base should move for total distance of 1 meter
   * Action results:
      * Initialize() method reports success on resetting encoder (OK)
      * Initialize() method reports that current encoder value is 1 meter (**UNEXPECTED**)
      * First time that IsFinished() is called, it reports that the current encoder value is 0 (**INCONSISTENT WITH EARLIER RESULT**)
      * Drive base moves for total distance of 2 meters (**FAILURE**)

## Theory
From the behavior exhibited, we believe that the call to SetPosition() is probably either:
a. effectively _scheduling_ an update to be applied to the encoder in the future (e.g., before the command is triggered), but not applying it immediately, or
b. updating the encoder's live value, but not updating a cached value being returned from the call to `GetPosition()`.

## Additional comments
If this is the intended behavior, then we would ask that the documentation for the Spark Max encoder be updated in order to _clearly_ reflect this, particularly since it is less than fully intuitive.

[JCaporuscio]

Hi there!

I think you've filed this in the wrong place. This is the release repo for Phoenix, so CTR Electronics devices and software (Talon, Victor, Pigeon, etc.).

You'll need to reach out to Rev for support with their libraries.

[mjhealy]

(smack forehead) You're quite right. My apologies!