Issue with record initialization when using controllers with readbacks already in EGU

[motorapp]

Hi Kevin,

I've encountered an issue with motor record initialization when using an asyn motor. The issue is relevant to controllers that return the axes readback already in EGU (eg. Degrees, Metres, etc). The issue effects a motor driver for Kuka robots that I'm currently writing. Also, in the powerbrick driver, a custom parameter PMAC_CMotorScale appears to be a work around for the issue described here.

Problem 1. For controllers in this category the MRES value is required in the driver early. Unfortunately, this is to undo the calculation on the controller just so the motor record can repeat the calculation for readbacks. In devAsynMotor.c line 385 send MRES to driver is commented out saying it arrives at the driver too late. It is not too late for the driver, and I recommend this code be un-commented. The driver, given mres, still will be called for setEncoderRatio, and possibly setPosition. The driver can make use of valid MRES in those calls to convert the controller EGU values, back into counts for passing to the motor record. Using code similar to this //Bottom of Driver::setEncoderRatio //Undo controller calculation motorposition = position / mres; //Pass step value to motor record setDoubleParam(pC->motorPosition, motorposition);

Problem 2. In situations where the position restore won't be triggered (eg. DVAL=0), the motor record can set pmr->val = pmr->rbv (motorRecord.cc line 636) before the driver setEncoderRatio (call initiated on devAsynMotor.c line 180) is actually called. When position restore is triggered this problem doesn't occur as the driver setPosition call is queued in devMotorAsyn.c after setEncoderRatio, then devMotorAsyn init_controller waits for setPosition completion. I recommend that devMotorAsyn.c init_controller doesn't use the start_trans, build_trans, end_trans and asyn queue mechanism. Instead use the long winded way to communicate with the driver in devMotorAsyn init_controller. Using setEncoder ratio in init_controller as an example the code would be:

pasynUser->reason = pPvt->driverReasons[motorEncRatio]; pPvt->pasynFloat64->write(pPvt->asynFloat64Pvt, pasynUser, (double)(pmr->mres/pmr->eres));

The call is not queued, it's simply made, so the timing is more predictable. The code also becomes simpler. Everything to do with initEvent can be removed.

To illustrate what I'm recommending, I've taken the latest devMotorAsyn.c from github and I've applied the changes. I've attached the modified devMotorAsyn.c here for your inspection and diff tools.

Finally, what are your thoughts on the reported issue?

I've tested the modified devMotorAsyn.c with the Kuka motor driver. In this case the controller returns readbacks in EGU. Everything worked, the driver got mres early, calculated the readback in setEncoderRatio (called devMotorAsyn.c init_controller) and passed it to the motor record via callback. The motor record set initial val to rbv correctly.

I've tested also with the Galil driver using autosave position restore. The Galil returns readbacks in motor, or encoder counts, the readbacks are not in EGU. Position restore worked as normal. When not restoring position due to the motor having a non-zero real position, val was set to rbv correctly at motor record initialization.

Kind regards, Mark

devMotorAsyn.tar.gz

[kmpeters]

@motorapp,

Problem 1

The driver should not be using the MRES field to convert the controller's position (in engineering units) into counts to pass to the motor record. This would force the motor record to use the same EGU as the controller and would cause major problems if the MRES was changed.

I consider the Newport XPS driver to be the standard controller that works in engineering units. The stepSize is specified the calls to XPSCreateAxis, which defines the conversion between controller units and motor record counts: https://github.com/epics-motor/motorNewport/blob/af40ea725d6ec17237d8feb8c835e2a97b40405f/iocs/newportIOC/iocBoot/iocNewport/st.cmd.xps6#L26-L29 This allows the MRES & EGU of the motor record to be changed. A common example of this is to use microns for the motor record with a controller that uses mm as its engineering unit.

Problem 2

I'm not sure I understand this problem. What are the symptoms that the proposed code changes fix?

Are you aware of the new, restore-mode field that was recently added to the motor record? Would the new field help? https://github.com/epics-modules/motor/pull/160

[MarkRivers]

I consider the Newport XPS driver to be the standard controller that works in engineering units. The stepSize is specified the calls to XPSCreateAxis, which defines the conversion between controller units and motor record counts:

I agree. The parameter to XPSCreateAxis is actually not called stepSize, it is called stepsPerUnit.

asynStatus XPSCreateAxis(const char *XPSName,         /* specify which controller by port name */
                         int axis,                    /* axis number 0-7 */
                         const char *positionerName,  /* groupName.positionerName e.g. Diffractometer.Phi */
                         const char *stepsPerUnit)    /* steps per user unit */

Note that if stepsPerUnit = 1/MRES then the engineering units in the motor record and the XPS are the same. But as Kevin said this is not required, they could be different.

The reason that we originally passed stepsPerUnit (typically a large integer) rather than unitsPerStep (typically a small float) is that we wanted to be able to call XPSCreateAxis from the vxWorks shell, and that shell does not permit float arguments. We have subsequently changed to pass a string instead so that even on vxWorks stepsPerUnit is not limited to being an integer.

Most of the motor template files I use don't specify MRES directly, they specify SREV and UREV, and the motor record calculates MRES=UREV/SREV. In many cases I can then set SREV=stepsPerUnit from the startup script and UREV=1.

Note that for the XPS the MRES and stepsPerUnit are somewhat arbitrary. For open-loop stepper motors the XPS always does 65536 microsteps per step. That is much more resolution than most physical systems have, so I chose a stepsPerUnit (and thus MRES) that is somewhat better than the mechanical resolution of the system. For motors with encoders one would normally use the encoder resolution to set those values.

[motorapp]

Dear Kevin, and Mark, The raised issue is about VAL being set to a stale, or soon to be stale RBV on motor record initialization (motorRecord.cc line 636).

Problem 1. Recommended change 1. devMotorAsyn.c init_record lines 387-392

“The driver should not be using the MRES field to convert the controller's position (in engineering units) into counts to pass to the motor record. This would force the motor record to use the same EGU as the controller”

Yes, that’s what I require. Same EGU in motor records as the controller uses. For this reason, adding an axis scale parameter to the driver axis create function, and specifying effectively the same data in multiple places seems redundant. A driver axis parameter can be added later if this requirement changes. Note, I appreciate the suggestion on this point, thank you.

“and would cause major problems if the MRES was changed.”

I disagree, it’s simply a software resolution change, the motors will still move the correct distances. In other words, the motors will move to the correct position, with reduced resolution. Largely this behaviour is due to the Kuka robot having absolute encoders, closed-loop motors, and the readback/setpoints are in the required EGU for the application.

The code commented out in devAsynMotor.c (github master) lines 387-392 indicate somebody has thought MRES should be delivered early in the past, and I agree with them. Adding these few lines cannot do harm, as far as I can see. Delivering MRES early may be useful to the driver, so there is potential benefit. The comment in devAsynMotor.c line 387 is not correct, it's not too late to send MRES to the driver. The devAsynMotor.c lines 387-392 would be the first call from the device layer to the driver. The only reasons it may be too late is:

1. Poor thread control in devAsynMotor.c init_controller because set encoder ratio is always queued, but load position is queued conditionally, and the thread control (initEvent) is attached to the load position. OR
2. It’s no longer recognized that setEncoderRatio can change the readback, whilst it’s still recognized that setPosition can change the readback in devAsynMotor.c init_controller.
3. The above issues can lead to pmr->val = pmr->rbv motorRecord.cc line 636 being executed with a stale or soon to be stale rbv. This issue is described as problem 2 further below.

Problem 2. Recommended change 2. devMotorAsyn.c init_controller/asynCallback 724-726 Unfortunately, I can’t see how the new field can help with the reported issue.

The symptom is that if the driver changes the readback in setEncoderRatio, queued in devAsynMotor.c init_controller lines 179-182 during record initialization, and position load isn’t queued then pmr->val = pmr->rbv in motorRecord.cc line 636 can be executed before setEncoderRatio is called. It could be said rbv is stale in motorRecord.cc line 636, or that a race condition exists. This will result in VAL being set incorrectly to a stale RBV at motor record initialization. In motordevCom.cc line 373 indicates it was recognized in the past that setEncoderRatio during record initialization can change the readback. Has this philosophy changed?

The current code in devMotorAsyn.c init_controller acknowledges that setPosition can alter the readback. Yet, the old code in motordevCom.cc line 373 acknowledges that setEncoderRatio can alter the readback. The recommendation is the current code in devMotorAsyn.c init_controller be altered with the knowledge that both setEncoderRatio, and setPosition can alter the readback.

Why use the asyn queue in devMotorAsyn.c init_controller during record initialization anyway? It has these problems:

1. Has more verbose code than the “long winded way” (as commented in devAsynMotor.c init_record line 380). I’m not sure if I’m using the “long winded way” terminology properly, please check my reference to line 380). By “long winded way”, I mean blocking calls direct to the driver writeInt32, writeFloat64 methods. In the attached suggested devMotorAsyn.c I’ve added 6 lines and deleted 15 lines.
2. Provides no advantages over the “long winded way” (ie. Direct driver calls writeFloat64 etc)
3. Threading issues, requires associated epics event for proper control. Additional code.

I recommend exchanging these asyn queue calls in devMotorAsyn.c init_controller with direct blocking calls to the driver interfaces as shown in the attached devMotorAsyn.c. This “long winded way” appears the cleaner, shorter, less problematic way.

Final comments The suggestion(s) provided fixes rbv being stale or soon to be stale at motorRecord.cc line 636 during motor record initialization. The code changes suggested are for devMotorAsyn.c alone. There are no suggested changes for motorRecord.cc here. The two fixes suggested were tested successfully using the Kuka, and Galil motor drivers. The suggested changes should not affect the operation of other motor drivers.

Kind regards, Mark Clift

[kmpeters]

@motorapp, I created a pull request for the proposed changes to devMotorAsyn.c (problem 2) so that it is easier for others to see, test, and discuss those changes. I haven't done any testing of that branch yet.

I don't support uncommenting these lines from devMotorAsyn.c as a fix for problem 1, since there better solutions that don't encourage the misuse of the MRES field:

https://github.com/epics-modules/motor/blob/c59afdcb6031080e7172747ff29d4696577f16ce/motorApp/MotorSrc/devMotorAsyn.c#L388-L391

I am curious to see the motor driver for Kuka robots. Is it on github?

[motorapp]

Problem1 I might come back to this later, but doubtful due to low importance, and the time required. I'm happy to discuss problem 1 further if the topic is of interest to others. I don't require this change, and I've got other solutions working already. I do believe the fix suggested for problem 1 here is the most appropriate choice on the list of alternatives I have.

Problem2 Thanks Kevin for taking a look at problem 2. I've posted further comments in the related pull request.

Kuka EPICS motor driver It's in heavy development now, and will be posted on git within 2 months. If interested in robots/EPICS, I might recommend other models depending on the application requirements. I'm also happy to email you a pre-release.