Add field RSTM: Restore Mode

[tboegi]

Partly revert the following commit: commit 3090983c313c57efdcd7c788ba234e775d300e06 Author: Ron Sluiter rsluiter@users.noreply.github.com Date: Wed Jul 29 15:50:23 2015 +0000 Bug fix for target position (VAL/DVAL/RVAL) initialization error when the motor record is configured to do retries. And from the release notes:

6) Kevin Peterson discovered an initialization bug when the motor record is configured to do retries. When configured to do retries, the motor record issues incremental rather than absolute moves. If the motor behaves badly (e.g., a piezo stiction motor) the controller's absolute step count can be far from its' absolute readback position. The motor record initialization logic that determines how the target positions are initialized at boot-up was not taking into consideration whether or not the motor record is configured to do retries, and therefore, whether or not absolute or incremental moves were issued by the motor record. With this release, if the motor record is configured to do retries, then the controller's absolute position is ignored (because the controller's absolute position was "corrupted" by retries) and the autosave/restore position is used to set the controllers position.

Switching between retries on and off (relative vs. absolute moves)
between reboots will cause unpredictable results and is not covered
by this bug fix.

Files modified: motor_init_record_com() in MotorSrc/motordevCom.cc init_controller() in MotorSrc/devMotorAsyn.c

Commit 3090983c improves the situation for setups where autosave is used, and makes things worse for setups where autosave is not used. When autosave is not used and the motor is configured to do retries, the the DVAL field is loaded into the controller regardless. And because DVAL is 0.0 at startup, the controller position is lost.

The first issue report is found here: "Problem with R6-10 issue 6 fix - controller position can be lost on reboot" https://github.com/epics-modules/motor/issues/85

And the we have another issue report here: "IOC zeroes encoder on startup" https://github.com/motorapp/Galil-3-0/issues/13

A possible way forward is discussed here: "Add a field to the motor record to allow always restoring the autosaved position" https://github.com/epics-modules/motor/issues/151

This commit add the "RSTM" field:

• 0 Disables restoring the autosaved position
• 1 Uses the same logic as motorRecord 6.9 (or older)
• 2 Restores the autosaved position regardless if the motor uses relative movements to do retries or not

[kmpeters]

@tboegi, I have a few questions:

1. Does the RSTM field default to zero (never restore the autosaved position) because no initial value is specified in the dbd file? The default value for RSTM should be 1 to minimize user inconvenience.
2. The logic in this line that was removed from devMotorAsyn.c is missing from the motorRSTM_Conditional case:

   int use_rel = (pmr->rtry != 0 && pmr->rmod != motorRMOD_I && (pmr->ueip || pmr->urip));

   Was this omission intentional? If so, why?

[tboegi]

@kmpeters 1) This is a bug - the initialization was missing, sorry for that. Please see new commit 2) This restores the 6.9 behavior of the "load position/restore logic" It is specified in the commit message:
This commit add the "RSTM" field:

• 0 Disables restoring the autosaved position
• 1 Uses the same logic as motorRecord 6.9 (or older)
• 2 Restores the autosaved position regardless if the motor uses relative movements to do retries or not

However, do we want the 6.10 behavior as a preferred solution ? That may break thinks for people not using autosave but retries (and are updatíng from 6.9) On the other hand, restoring the 6.9 may break things for people who use autosave (and are updating from 6.10 or 7.x) I dunno.

[kmpeters]

I have one beamline that depends on the 6.10 behavior (documented in item 6 in the comments of this commit: https://github.com/epics-modules/motor/commit/3090983c313c57efdcd7c788ba234e775d300e06). I would bet there are others that don't realize they rely on it.

The travis builds don't like the initial value:

Default value 'Conditional' is invalid for field 'RSTM'

Context: field(RSTM, DBF_SHORT) in recordtype(motor) in file '../motorRecord.dbd'

mkdir ../../../db

Default value 'Conditional' is invalid for field 'RSTM'

I assume the default needs to be 1 instead of Conditional since the field is a SHORT

[kmpeters]

That may break thinks for people not using autosave but retries (and are updatíng from 6.9)

I'm comfortable inconveniencing people in this situation. They would have already been inconvenienced in the same way if they upgraded from 6.9 to a newer version of the motor in the last few years.

[tboegi]

Hej @kmpeters ; I think I restored the old logic. Could you please take another careful review round ? Thanks

[tboegi]

@kmpeters I think that we need to fix the travis script first (otherwise all travis builds will fail) https://github.com/epics-modules/motor/pull/161

[kmpeters]

@kmpeters I think that we need to fix the travis script first (otherwise all travis builds will fail)

161

I merged #161.

[kmpeters]

I haven't tested this yet, but I will soon.

[kmpeters]

Also, I think the logic in https://github.com/epics-modules/motor/commit/3090983c313c57efdcd7c788ba234e775d300e06 needs to be improved. The cases where URIP and UEIP are YES should be handled differently. I'll create an issue for those improvements after this pull request is closed, since that will be modifying the "Conditional" logic.

[tboegi]

Do we have an agreement about "Never", "NearZero" "Condition" "Always" ? (There is a long weekend here, so I will take the next round on Monday

[kmpeters]

@tboegi, "Conditional" instead of "Condition" and yes, I agree.

I think it's also OK to make "NearZero" the default. The number of problems that have been caused by the conditional behavior is greater than the number of cases I know that have benefited from it. I'll add a big warning to the release notes for the next version of motor. Is it possible to use flashing text in github markdown?

[kmpeters]

I've begun testing this pull request.

Changing RSTM from DBF_SHORT to DBF_MENU will allow the string representation of RSTM in motorRSTM to be used:

$ git diff motorApp/MotorSrc/motorRecord.dbd
diff --git a/motorApp/MotorSrc/motorRecord.dbd b/motorApp/MotorSrc/motorRecord.dbd
index b4aad92..38d456d 100644
--- a/motorApp/MotorSrc/motorRecord.dbd
+++ b/motorApp/MotorSrc/motorRecord.dbd
@@ -805,9 +805,11 @@ recordtype(motor) {
                prompt("Ignore SET field")
                interest(2)
        }
-       field(RSTM,DBF_SHORT) {
-               initial("2")
-               prompt("restore mode")
+       field(RSTM,DBF_MENU) {
+               initial("NearZero")
+               prompt("Restore Mode")
+               promptgroup(GUI_COMMON)
                interest(2)
+               menu(motorRSTM)
        }
 }

Without this change caget and dbpr show the integer value:

$ caget kmpRSTM:m1.RSTM
kmpRSTM:m1.RSTM                2

The output with this change is more user friendly:

$ caget kmpRSTM:m1.RSTM
kmpRSTM:m1.RSTM                NearZero

[kmpeters]

There is a typo (two instances of use_rel on the left side of the assignment) in motordevCom.cc: https://github.com/epics-modules/motor/blob/c59aa1a5df36aed02a2bd55de4fb429cef6581d3/motorApp/MotorSrc/motordevCom.cc#L300

Also, in both motordevCom.cc and devMotorAsyn.c the switch statement cases should be rearranged so that motorRSTM_Conditional falls through to motorRSTM_NearZero. Currently the behavior of the Conditional and NearZero states are swapped.

[tboegi]

Thanks @kmpeters I will push a fresh version later

[kmpeters]

Thanks.

I've tested autosaving the RSTM field and it works as expected: the autosaved value of RSTM determines the restore behavior for the motor's position.

[tboegi]

@kmpeters Thanks for testing - I will push a "cleaned up" version on Moday

[kmpeters]

@tboegi, the travis builds are failing because of the extra ) at the end of this line: https://github.com/EuropeanSpallationSource/motor/blob/a2ee61718eef3011dcfe6b611213b39ba9cfcf7a/motorApp/MotorSrc/motordevCom.cc#L199

[tboegi]

@kmpeters Thanks for the patience - (and it was even worse, because the axis_query.position was not initialized. I moved the code down)

[kmpeters]

@tboegi, thanks for these changes.