Add field RSTM: Restore Mode #160

tboegi · 2020-05-18T13:37:50Z

Partly revert the following commit:
commit 3090983
Author: Ron Sluiter [email protected]
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:

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 3090983 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"
#85

And the we have another issue report here:
"IOC zeroes encoder on startup"
motorapp/Galil#13

A possible way forward is discussed here:
"Add a field to the motor record to allow always restoring the
autosaved position"
#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 · 2020-05-19T16:11:44Z

@tboegi, I have a few questions:

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.
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 · 2020-05-19T16:34:36Z

@kmpeters

This is a bug - the initialization was missing, sorry for that. Please see new commit
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 · 2020-05-19T16:50:39Z

I have one beamline that depends on the 6.10 behavior (documented in item 6 in the comments of this commit: 3090983). 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 · 2020-05-19T16:55:33Z

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 · 2020-05-19T18:01:43Z

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

tboegi · 2020-05-20T07:30:02Z

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

FreddieAkeroyd · 2020-05-20T11:21:58Z

motorApp/MotorSrc/devMotorAsyn.c

+            {
+                if ((use_rel != 0) ||
+                    ((fabs(pmr->dval) > rdbd) && (pmr->mres != 0) && (fabs(position * pmr->mres) < rdbd))
+                    )


If "Conditional" includes a check on use_rel, then there is no longer a way to maintain the original (6.9) motor record behaviour - I feel this should still be available, so another RSTM option? Maybe the choices are something like "WhenZero", "Conditional", "Always", "Never" (would "WhenZeroOrRelative" be another option for "Conditional"). Using "Zero" is not strictly accurate, but I couldn't think of a better word, unless you used words like "Original" or "Motor69" etc.

Good point - I agree that both should be available.
Motor69 is so un-intuitive that people need to read the documentation.
I like that.
Then we have Motor69 and Motor610 ?

"NearZero" is more accurate than "WhenZero," but I don't love it. I think "Conditional" is better than "WhenZeroOrRelative," because it is less likely to need to be changed in the future when other corner cases arise. I'll make it clear in the documentation and the release notes what "Conditional" means.

The essence of the original behaviour was to only restore position if the motor record thought the controller had been powered on / reset, which it did by checking for counters being near zero. Do we just need to find a word that encapsulates that better? PowerOn / PowerUp / OnReset / … or could that lead to future confusion if a controller can power on and remember position /not be zero and it is better to stick to "NearZero" as that describes when it really does it as opposed to usually does? May be leaning to "NearZero" ...

"Counters cleared"?

I think "NearZero" requires the least explanation so far. I wouldn't know what "counters" are in the context of a motor controller and I would feel compelled to read the documentation.

I'm also very comfortable with the label "Conditional" now. This option leaves it up to device support do decide when to restore the position. The vague name seems appropriate for what will be increasingly complicated decision-making under the hood.

The other three options are easy to understand and probably don't require reading the documentation: "NearZero", "Always", "Never".

A user who doesn't want to read the documentation can safely pick one of the other three options and be confident the position will be restored as expected.

My question about that is what is near zero – it could mean the motor is near it's Home position, or probably several other similar things. Note that I'm just responding as a very naive user here, feel free to ignore me.

NearZero = within the retry deadband of zero.

kmpeters · 2020-05-20T15:06:02Z

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

I merged #161.

kmpeters · 2020-05-20T15:06:47Z

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

kmpeters · 2020-05-20T16:19:13Z

Also, I think the logic in 3090983 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 · 2020-05-20T16:22:29Z

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 · 2020-05-20T16:26:10Z

@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 · 2020-06-03T16:44:09Z

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 · 2020-06-04T21:39:19Z

There is a typo (two instances of use_rel on the left side of the assignment) in motordevCom.cc:

motor/motorApp/MotorSrc/motordevCom.cc

Line 300 in c59aa1a

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

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 · 2020-06-05T04:59:06Z

Thanks @kmpeters
I will push a fresh version later

kmpeters · 2020-06-05T16:55:52Z

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 · 2020-06-05T17:01:21Z

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

kmpeters · 2020-06-09T14:50:11Z

@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

Partly revert the following commit: commit 3090983 Author: Ron Sluiter <[email protected]> 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 3090983 improves the situation for setups where autosave is used, and makes things worse when 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" epics-modules#85 And the we have another issue report here: "IOC zeroes encoder on startup" motorapp/Galil#13 A possible way forward is discussed here: "Add a field to the motor record to allow always restoring the autosaved position" epics-modules#151 This commit adds the "RSTM" field: - 0 Disables restoring the autosaved position - 1 Always restores the autosaved position - 2 Uses the same logic as motorRecord 6.9 (or older) - 3 Uses the same logic as motorRecord 6.10 This numbering maps 0 and 1 somewhat to false/true, uses 2/3 for special handlings and leaves room for more choices. E.g. "use the encoder value, if valid, fall back to autosave if not. Or "use the URIP/RDBL" if possible. I am getting off-topic, those improvements should be done in later commits anyway.

tboegi · 2020-06-10T06:25:06Z

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

kmpeters · 2020-06-11T21:38:37Z

@tboegi, thanks for these changes.

tboegi mentioned this pull request May 18, 2020

Add a field to the motor record to allow always restoring the autosaved position #151

Closed

tboegi force-pushed the add-RSTM-field branch from 6490b52 to a49aa2a Compare May 18, 2020 13:46

tboegi force-pushed the add-RSTM-field branch from a49aa2a to 3a0ec9c Compare May 19, 2020 16:29

tboegi force-pushed the add-RSTM-field branch from 3a0ec9c to a784480 Compare May 19, 2020 16:43

tboegi force-pushed the add-RSTM-field branch from a784480 to a5f07ba Compare May 19, 2020 18:00

tboegi force-pushed the add-RSTM-field branch from a5f07ba to f449efa Compare May 20, 2020 06:01

tboegi force-pushed the add-RSTM-field branch 2 times, most recently from d23f014 to d2214ab Compare May 20, 2020 07:37

FreddieAkeroyd reviewed May 20, 2020

View reviewed changes

tboegi force-pushed the add-RSTM-field branch from d2214ab to 910d03d Compare May 25, 2020 12:25

tboegi force-pushed the add-RSTM-field branch from c59aa1a to a2ee617 Compare June 9, 2020 11:50

tboegi force-pushed the add-RSTM-field branch from a2ee617 to 2906f3d Compare June 10, 2020 06:21

kmpeters approved these changes Jun 11, 2020

View reviewed changes

kmpeters merged commit b7fac81 into epics-modules:master Jun 11, 2020

tboegi deleted the add-RSTM-field branch June 12, 2020 04:17

kmpeters mentioned this pull request Jun 26, 2020

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

Closed

kmpeters mentioned this pull request May 15, 2023

Problem with R6-10 issue 6 fix - controller position can be lost on reboot #85

Closed

kmpeters added this to the R7-3 milestone May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add field RSTM: Restore Mode #160

Add field RSTM: Restore Mode #160

tboegi commented May 18, 2020

kmpeters commented May 19, 2020

tboegi commented May 19, 2020 •

edited

Loading

kmpeters commented May 19, 2020

kmpeters commented May 19, 2020

tboegi commented May 19, 2020

tboegi commented May 20, 2020

FreddieAkeroyd May 20, 2020

tboegi May 20, 2020

kmpeters May 20, 2020

FreddieAkeroyd May 20, 2020 •

edited

Loading

anjohnson May 20, 2020

kmpeters May 20, 2020

kmpeters May 20, 2020

anjohnson May 20, 2020

kmpeters May 20, 2020

kmpeters commented May 20, 2020

kmpeters commented May 20, 2020 •

edited

Loading

kmpeters commented May 20, 2020 •

edited

Loading

tboegi commented May 20, 2020

kmpeters commented May 20, 2020 •

edited

Loading

kmpeters commented Jun 3, 2020

kmpeters commented Jun 4, 2020

tboegi commented Jun 5, 2020

kmpeters commented Jun 5, 2020

tboegi commented Jun 5, 2020

kmpeters commented Jun 9, 2020

tboegi commented Jun 10, 2020

kmpeters commented Jun 11, 2020

Add field RSTM: Restore Mode #160

Add field RSTM: Restore Mode #160

Conversation

tboegi commented May 18, 2020

kmpeters commented May 19, 2020

tboegi commented May 19, 2020 • edited Loading

kmpeters commented May 19, 2020

kmpeters commented May 19, 2020

tboegi commented May 19, 2020

tboegi commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FreddieAkeroyd May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmpeters commented May 20, 2020

kmpeters commented May 20, 2020 • edited Loading

kmpeters commented May 20, 2020 • edited Loading

tboegi commented May 20, 2020

kmpeters commented May 20, 2020 • edited Loading

kmpeters commented Jun 3, 2020

kmpeters commented Jun 4, 2020

tboegi commented Jun 5, 2020

kmpeters commented Jun 5, 2020

tboegi commented Jun 5, 2020

kmpeters commented Jun 9, 2020

tboegi commented Jun 10, 2020

kmpeters commented Jun 11, 2020

tboegi commented May 19, 2020 •

edited

Loading

FreddieAkeroyd May 20, 2020 •

edited

Loading

kmpeters commented May 20, 2020 •

edited

Loading

kmpeters commented May 20, 2020 •

edited

Loading

kmpeters commented May 20, 2020 •

edited

Loading