feat(motor): add readback tolerance check to EpicsMotor.move()

[mrlo03]

Create an EpicsMotor subclass (ReadbackEpicsMotor) that includes readback tolerance checking. This motor extends the standard EpicsMotor to ensure that a move operation does not complete until both the standard motion status (.DMOV) and the actual readback value (user_readback) are within a specified tolerance from the target position.

This change addresses an issue during scans where the IOC reports via the .DMOV field that the movement has finished, while the motor is actually still moving. Checking the .RBV (user_readback) value is used here as a reliable solution to ensure the move is truly complete.

[flowln]

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?

Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

[RafaelLyra8]

I like the idea of also watching for the readback value as a default implementation, but I think that the tolerance might be a lot more complicated to handle than just setting a default value, as different motors have different tolerances this may cause errors in the already implemented plans. Also, it would be nice to implement the tolerance as atol(absolute tolerance) and rtol(relative tolerance) as its already done in the EpicsSignal and PVPositioner implementations.

[mrlo03]

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?

Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.

With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.

But we can talk to find a better way to implement this. What do you think?

Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.

However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

[flowln]

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?
Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.

With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.

But we can talk to find a better way to implement this.

Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.

However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

I think the main issue with using the RBV value for checking movement state is that you become less robust on other issues, like if the motor gets stuck for some reason. In those cases, the DMOV flag correctly indicates that the movement is done, and an alarm or error PV indicates some failure. With the RBV check, we can no longer trust that, and the worst case becomes timing out, which is also a very unreliable thing to configure since it changes from motor to motor.

This can be seen in this piece of code from EpicsMotor itself, where the self._moving flag (relative to DMOV) is checked, before checking the alarm state and then deciding on what the user will get. With your AndStatus implementation, that will only be propagated after the RBV status has timed out, which for the inattentive user may be never.

Aside from that consideration, another important thing is that if the DMOV signal is not reliable, then in some cases the RBV check will also be unreliable. Consider a PID motor that goes through a position settling moment before finishing the movement. In this case, the DMOV should tell us when the movement has actually stopped, but if you can't do that, the next best thing is to add an arbitrary settle time to the position monitor for that ending movement to be captured correctly, but that can either add some amount of dead time, or it can be insufficient, and errors can arise in hard-to-foresee scenarios, like a race condition between the motor controller and the orchestration layer.

[mrlo03]

What is the reasoning behind adding this class instead of relying on PVPositionerIsClose or PVPositionerComparator like we usually do? In other terms, what does it provide for us that the other method does not already provide with no extra maintenance burden?
Also, does this behavior occur in a specific IOC device? Since this is clearly a bug, it'd be nice if we could have it fixed in the lower level instead of working around it here.

I couldn't find a straightforward way to use PVPositionerIsClose in this case. I ran a few tests, but using PVPositionerIsClose directly meant losing the EpicsMotor structure.
With this approach, I think we can better preserve the EpicsMotor behavior while still adding the required check. It also allows us to use it directly in our common motor devices.
But we can talk to find a better way to implement this.
Regarding the IOC, I encountered this issue with the CNB motors using DeltaTau (PB01, PB02, etc.) and with the new controller for the 4CM (CK5M01). I can confirm with Ana and the IOC team whether this bug also occurs in other contexts.
However, I still think it would be useful to keep both checks, using .RBV and .DMOV, to make the position verification more robust.

I think the main issue with using the RBV value for checking movement state is that you become less robust on other issues, like if the motor gets stuck for some reason. In those cases, the DMOV flag correctly indicates that the movement is done, and an alarm or error PV indicates some failure. With the RBV check, we can no longer trust that, and the worst case becomes timing out, which is also a very unreliable thing to configure since it changes from motor to motor.

This can be seen in this piece of code from EpicsMotor itself, where the self._moving flag (relative to DMOV) is checked, before checking the alarm state and then deciding on what the user will get. With your AndStatus implementation, that will only be propagated after the RBV status has timed out, which for the inattentive user may be never.

Aside from that consideration, another important thing is that if the DMOV signal is not reliable, then in some cases the RBV check will also be unreliable. Consider a PID motor that goes through a position settling moment before finishing the movement. In this case, the DMOV should tell us when the movement has actually stopped, but if you can't do that, the next best thing is to add an arbitrary settle time to the position monitor for that ending movement to be captured correctly, but that can either add some amount of dead time, or it can be insufficient, and errors can arise in hard-to-foresee scenarios, like a race condition between the motor controller and the orchestration layer.

That makes sense Sofia. In cases where the motor gets stuck for some reason, we would have to rely on the configured timeout, which can vary from motor to motor. What do you think about implementing a check for self._moving in this new class, similar to what EpicsMotor does, and using that information to check the alarm/error state and report the movement failure to the user, before considering the .RBV? In any case, I’ll bring this discussion to the Orchestration meeting so we can try to come up with a solution for this issue.