Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Galil: changing MRES/ERES does not re-apply soft limits #7446

Closed
2 tasks
rerpha opened this issue Oct 26, 2022 · 10 comments
Closed
2 tasks

Galil: changing MRES/ERES does not re-apply soft limits #7446

rerpha opened this issue Oct 26, 2022 · 10 comments
Assignees
Labels

Comments

@rerpha
Copy link
Contributor

rerpha commented Oct 26, 2022

I noticed when setting up the mini jaws that by:
setting a soft HLM/LLM to 40 and -40 respectively, then changing the motor and encoder res, did not re-apply soft limits going by the new resolutions. this then made moves prematurely hit the old soft limits.

for info the soft limits applied in the table of motors are then scaled by the motor or encoder res and sent to the galil, this doesn't seem to work when you change the resolutions

this may be by design, but i cant think of a use case where it'd be useful as the old soft limits are effectively useless if not re-scaled to resolution?

Acceptance criteria

  • Changing motor resolution re-applies soft limits
  • changing encoder resolution if UEIP == 1 re-applies soft limits to the galil
@FreddieAkeroyd
Copy link
Member

There is a general motor record comment on this at https://epics-modules.github.io/motor/motorRecord.html#Fields_res "Currently, changes to motor-resolution fields have no effect on the values of limit fields (although they should). " however it is not clear what the correct behaviour should be e.g. should it preserve userspace (keep 40, send new raw steps for limits) or preserve raw (change userspace 40 to match raw steps on motor with new resolution). It looks like it preserves userspace and resets raw steps, i wasn't sure from the comment if the motor people were expecting the limits to adjust rather than keep the same?

@KathrynBaker KathrynBaker added the 8 label Oct 27, 2022
@github-actions github-actions bot added ready and removed proposal labels Oct 27, 2022
@KathrynBaker KathrynBaker added this to the SPRINT_2022_10_27 milestone Oct 27, 2022
@rerpha
Copy link
Contributor Author

rerpha commented Nov 2, 2022

i think the latter would make more sense (probably easier too?) - on mres change adjust our end and keep steps the same. i think that'd be immediately apparent that the soft limits then need re-adjusting

@rerpha rerpha self-assigned this Nov 3, 2022
@rerpha
Copy link
Contributor Author

rerpha commented Nov 4, 2022

i might be getting this wrong, but it looks like we scale velo in the motor record when mres is changed? i guess we only do that on the epics side for a galil, so i think it would make sense to do the same with soft lims? https://github.com/ISISComputingGroup/EPICS-motor/blob/master/motorApp/MotorSrc/motorRecord.cc#L2836-L2847

@rerpha
Copy link
Contributor Author

rerpha commented Nov 9, 2022

ok, starting to see why this probably wasn't done beforehand. currently we update some fields on mres update ie. velo and vmax. VELO can be re-worked out because it's just UREV*S, VMAX is UREV*SMAX etc. as they are derived from other fields on the record so just need to be re-calculated.

soft limits are not derived from other field values (although they are scaled TO the device when new values are sent ie. here), they are in some cases obtained from the device but not always.

To change limits properly I think we need to initially store MRES as another field (PMRES?)

then on MRES/UREV change:

  1. update MRES
  2. scale soft limits by the difference in the new vs old MRES, by doing (MRES/PMRES)*{LLM, HLM, DHLM, DLLM}
  3. store last MRES as PMRES

I think this would work, but i wanted to make sure doing this made sense before delving even further in to add the extra field. Alternatively -

it would be much easier to re-scale the soft limits and send them to the device on an mres change so that the soft limits shown to the user are re-applied - you just do the same as if the limit values had been changed (set_dial_highlimit() etc) - this means if someone put in soft limits beforehand then changed mres they would still be correct on the device

@FreddieAkeroyd
Copy link
Member

@rerpha given this is changing the motor record, i think it would be good to discuss approaches with the motor record team. Maybe create a ticket with your findings/ideas at https://github.com/epics-modules/motor ?

@rerpha
Copy link
Contributor Author

rerpha commented Nov 9, 2022

@rerpha given this is changing the motor record, i think it would be good to discuss approaches with the motor record team. Maybe create a ticket with your findings/ideas at https://github.com/epics-modules/motor ?

have created epics-modules/motor#191 - feel free to edit if you think it doesn't make sense

@rerpha
Copy link
Contributor Author

rerpha commented Nov 21, 2022

pr created upstream, awaiting feedback, so will put this in impeded

@kmpeters
Copy link

kmpeters commented May 12, 2023

pr created upstream, awaiting feedback, so will put this in impeded

Motor pull req 93 (epics-modules/motor#193) was merged today.

@rerpha
Copy link
Contributor Author

rerpha commented May 17, 2023

Cheers Kevin.
@FreddieAkeroyd I might just merge in motor/master for this ticket, unless you think there might be things that break our fork

@rerpha
Copy link
Contributor Author

rerpha commented May 25, 2023

motor (have merged latest vendor to our vendor branch): ISISComputingGroup/EPICS-motor#59
ioc: ISISComputingGroup/EPICS-ioc#789
RN: #7827

Have run all of the ioc tests I can think of, as well as just done a runioc.bat to see if any DLL fails occur and they don't seem to. To review check the original criteria and check if i've missed any motors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants