Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OMS: Synchronize motor position with encoder position before every move #93

Merged
merged 2 commits into from
Jun 21, 2018

Conversation

ltjax
Copy link

@ltjax ltjax commented Mar 13, 2018

Our motor position was often not synchronized with the encoder position, resulting in very erratic behavior once the encoder was used. This fixed it.

I'm not entirely sure this was the best place to apply this synchronization - if there's a better place, please do tell!

@kmpeters kmpeters requested a review from rsluiter April 27, 2018 15:36
Copy link
Contributor

@tboegi tboegi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines
"Version: $Revision$" do not belong to this change, do they?

@ltjax
Copy link
Author

ltjax commented Jun 21, 2018

You're right. I removed that.

@kmpeters
Copy link
Member

Hard-coding the encoderRatio to 1 causes problems for any application where the drive resolution is different than the encoder resolution.

Can you elaborate on the configuration of the motor records and the very erratic behavior you saw without the fix? It sounds like there may be a bug in the motor record itself when UEIP=Yes.

@ltjax
Copy link
Author

ltjax commented Jun 21, 2018

It's not hard-coded, just initialized as 1. The setEncoderRatio override sets it correctly. I just added that because I needed the encoder ratio to convert from encoder step position to motor position. It's not 1 in our case.

Well, very erratic was basically: does not make sense at all. You'd tweak it forward, and sometimes it would go forwards, but not stop until it ran into the stop switch, or sometimes it would go backwards for seemingly random amounts. Debugging revealed that the motor and encoder positions where not synced correctly. What exactly causes that, I do not know. I can tell you, that encoderPosition = motorPosition * encoderRatio did not hold in the hardware and this fixed it. So basically, the encoder's and motor's 0 position seems to be somewhere else.

I asked around with people who are driving this hardware with Tango instead of EPICS, and they basically had to do the same to get the motor/encoder pair to work correctly.

@mp49
Copy link
Contributor

mp49 commented Jun 21, 2018

The motor and encoder positions on the controller can get out of sync for all kinds of reasons, and I've seen similar issues if UEIP=Yes. However, if RTRY is non-zero then every move will be relative to the encoder readback, and so these problems can be avoided that way.

Syncing the motor to the encoder position in the driver seems like the right solution, and I know of other drivers that do this (but only when UEIP=Yes).

@kmpeters
Copy link
Member

kmpeters commented Jun 21, 2018

It's not hard-coded, just initialized as 1. The setEncoderRatio override sets it correctly. I just added that because I needed the encoder ratio to convert from encoder step position to motor position. It's not 1 in our case.

@ltjax I didn't notice the setEncoderRatio override before making my previous comment. Thanks for pointing it out.

The motor and encoder positions on the controller can get out of sync for all kinds of reasons, and I've seen similar issues if UEIP=Yes. However, if RTRY is non-zero then every move will be relative to the encoder readback, and so these problems can be avoided that way.

@mp49 It seems like the motor record should also move relative to the encoder readback when UEIP=Yes and RTRY is zero, but that may not completely eliminate the need for this fix.

@kmpeters kmpeters merged commit 31e0afe into epics-modules:master Jun 21, 2018
@kmpeters kmpeters added this to the R6-11 milestone Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants