epics-modules / motor

APS BCDA synApps module: motor
https://epics-modules.github.io/motor/
20 stars 48 forks source link

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

Closed kmpeters closed 4 years ago

kmpeters commented 5 years ago

There is a need to override the default autosave behavior of the motor record and always restore the autosaved position, regardless of the position reported by the controller. This change was discussed in https://github.com/epics-modules/motor/issues/85

I proposed adding a new field with the following behavior:

@mp49, it seems like a reasonable solution would be to add a field to the motor record to control the autosave behavior. If this new field is zero, the behavior would be the same as use_rel=0: the autosaved value of the motor would only be restored if the motor's readback position is within the retry-deadband distance of zero. If the new field is one, the behavior would be the same as use_rel=1: always restore the autosaved position. The default value would be zero, since there are many more motors that expect the use_rel=0 behavior. Would this work for you?

Source: https://github.com/epics-modules/motor/issues/85#issuecomment-414734240

@anjohnson suggested that the values for the new field should be easy-to-understand: https://github.com/epics-modules/motor/issues/85#issuecomment-414748747

Questions to answer:

  1. What should the name of the new field be?
  2. What should the easy-to-understand names of the states be?
kmpeters commented 5 years ago

These are the first names to pop into my head: Field: RSTM (restore mode) 0-state: Conditional 1-state: Always

anjohnson commented 5 years ago

Does this setting need to be set/changed at run-time? It now sounds more like a setting that only needs to be used once when autosave is restoring values at startup... If it's the latter, it might be better to use an info tag instead of reserving space in the record for it.

Whichever you use, how would the autosave restore process know to check for this setting? You won't want to make the autosave module dependent on the motor record, so it can't look at a field value directly; info tags are record-type agnostic, so this might be generalized as part of autosave.

Just thinking out loud here, I don't claim to understand what you're really doing...

FreddieAkeroyd commented 4 years ago

The new motor field would govern how the motor record should behave on startup with regard to reloading the position on the controller, so in theory this is independent of autosave which would just restore the DVAL field and then the new field would govern whether DVAL was sent to the controller or not. Autosave may well be restoring the value of RSTM too, but it could be set via macro at boot time. I am wondering if there should be an option "2" for RSTM of "Never", this would allow you to autosave positions as a historic boot time record (for later restore maybe) but never have them applied to the controller. We prefer not to restore positions automatically on ioc boot, so previously were just not autosaving the position and the logic appropriate for "Conditional" excluded restoring controller position when DVAL was zero.

kmpeters commented 4 years ago

Does this setting need to be set/changed at run-time? It now sounds more like a setting that only needs to be used once when autosave is restoring values at startup... If it's the latter, it might be better to use an info tag instead of reserving space in the record for it.

I don't think this setting would need to be changed at run-time.

@anjohnson are there any examples of how to access information in an info tag from device support? I see that dbrestore.c does it with a call to dbGetInfo(): https://github.com/epics-modules/autosave/blob/1fb6ea5e6255d0215cf9cceec7682f78d8426f47/asApp/src/dbrestore.c#L1493

Whichever you use, how would the autosave restore process know to check for this setting? You won't want to make the autosave module dependent on the motor record, so it can't look at a field value directly; info tags are record-type agnostic, so this might be generalized as part of autosave.

The logic of whether or not to ignore the restored position is determined by the device support for the motor record.

For model-1 motor drivers this occurs in the motor_init_record_com function of motordevCom.cc: https://github.com/epics-modules/motor/blob/1fc606fa1f7e75aec6f5da0a8cab0ad1a28f1c2a/motorApp/MotorSrc/motordevCom.cc#L328 initPos is calculated here: https://github.com/epics-modules/motor/blob/1fc606fa1f7e75aec6f5da0a8cab0ad1a28f1c2a/motorApp/MotorSrc/motordevCom.cc#L291

For asyn motor drivers this occurs in the init_controller function of devMotorAsyn.c: https://github.com/epics-modules/motor/blob/1fc606fa1f7e75aec6f5da0a8cab0ad1a28f1c2a/motorApp/MotorSrc/devMotorAsyn.c#L191

kmpeters commented 4 years ago

Autosave may well be restoring the value of RSTM too, but it could be set via macro at boot time. I am wondering if there should be an option "2" for RSTM of "Never", this would allow you to autosave positions as a historic boot time record (for later restore maybe) but never have them applied to the controller.

@FreddieAkeroyd I have no objections to including a third, "Never" option, though I think a better solution might be to have a dedicated autosave file that is saved but never restored.

tboegi commented 4 years ago

I tried to address this issue here: https://github.com/epics-modules/motor/pull/160

Comments (and tests) are welcome

kmpeters commented 4 years ago

@tboegi, thanks for the pull request. I added some questions to the PR discussion.

kmpeters commented 4 years ago

Fixed by #160