epics-modules / motor

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

user/dial limits: change when motor/encoder resolution changes #191

Closed rerpha closed 1 year ago

rerpha commented 1 year ago

Currently the motor record does nothing with dial/user limits when a motor encoder resolution changes.

this is potentially problematic if a device stores soft limits on the controller (in our case at ISIS/STFC a Galil) which are not scaled (ie in steps), as the motor record does not re-scale these values by calling set_dial_highlimit etc.

I propose that, when changing the motor/encoder resolution, either:

The former feels like it would require caching the old mres so we can compare against it when trying to rescale the limits. I think the latter would just be a case of calling setdial{high, low}limit() and then set_user_limits from motorRecord.cc

kmpeters commented 1 year ago

I prefer the second of the two proposed options, as I expect it would result in fewer potential problems.

I also think a cleaner approach would be to add raw limit fields, analogous to the raw position fields, that would allow appropriately-scaled limits to be sent to the driver without keeping track of the previous MRES.

kmpeters commented 1 year ago

I also think a cleaner approach would be to add raw limit fields, analogous to the raw position fields, that would allow appropriately-scaled limits to be sent to the driver without keeping track of the previous MRES.

Specifially, RHLM and RLLM fields could be added, with the following relationships to DHLM and DLLM:

RHLM = DHLM / MRES
RLLM = DLLM / MRES

The raw limits would update and be consistent with the dial limits. If the raw limits are changed, the dial limits would be recalulcated using the current MRES. If the MRES is changed, the dial limits would be recalculated based on the the raw limits, along with the velocities.

rerpha commented 1 year ago

yes, that seems a better solution. I think I prefer that as it follows what we currently do with scaled velo etc. I think this could potentially simplify device support as well, as no scaling will be needed outside of the motor record for setting the limits on a controller.

rerpha commented 1 year ago

as a (very) rough proof of concept this seems to work...

https://github.com/ISISComputingGroup/EPICS-motor/compare/rawlims?expand=1

as this is the first time i've looked at the motor record i'm not super confident! will test some more and make a PR when happy

tboegi commented 1 year ago

If I may join the discussion: Taking a short look: I think that updating the fields RLLM and RHLM needs review. Are we are missing the calculation of LLM and HLM here ? And what happens when MRES < 0 ? We need to make sure that LLM/HLM, DLLM/DHLM and the new RLLM/RHLM always stay synchronized, right ? And in that sense: The introduction of set_user_highlimit() and set_user_lowlimit() are a good change.

However, synching of Raw/Dial/User limits seems incomplete. It may be easier, to keep RLLM/RHLM as "read only", at least to start with. Does somebody wants to write to them ? Or are they only needed internally ?

rerpha commented 1 year ago

many thanks for having a look and giving feedback! RLLM/RHLM being read-only to the outside world is fine I think. Have just committed a change to make them read-only, hopefully it's correct. edit: think i needed to add SPC_NOMOD in the dbd as well?

MRES < 0 currently applies *-1 to both lims, not sure if this is correct or not

looks like dhlm and dllm are not affected by DIR, only llm and hlm are - i think it makes sense for RLLM and HLLM to also not be affected?

kmpeters commented 1 year ago

MRES < 0 currently applies *-1 to both lims, not sure if this is correct or not

This is correct.

looks like dhlm and dllm are not affected by DIR, only llm and hlm are - i think it makes sense for RLLM and HLLM to also not be affected?

This is also correct.

kmpeters commented 1 year ago

However, synching of Raw/Dial/User limits seems incomplete.

@tboegi, I don't understand. How would the syncing be incomplete?

tboegi commented 1 year ago

My first impression, after reading all the changes, was that RLLM and RHLM are writable. And if somebody writes to them, the synching into DLLM and LLM / DHLM HLM may be wrong because we may need to look at MRES... (But I may be wrong)

However, I wonder, if a simple set_dial_highlimit(pmr, pdset); set_dial_lowlimit(pmr, pdset); At the end of velcheckB: would be enough ?

tboegi commented 1 year ago

@rerpha I think that this patch may solve your issue ? https://github.com/tboegi/motor/pull/new/update-raw-limits-on-MRES-change

rerpha commented 1 year ago

i think so - its just whether we want to update on the controller or update values on our end instead?

tboegi commented 1 year ago

What is our end ?

rerpha commented 1 year ago

DHLM/DLLM/HLM/LLM being updated instead of new limit being pushed to controller

rerpha commented 1 year ago

I think the former now works on my branch, with raw lims read-only? just needs some tidying up

tboegi commented 1 year ago

I see: I did the wrong solution.

rerpha commented 1 year ago

have made https://github.com/epics-modules/motor/pull/193 , feedback welcomed!