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

Status of limit switches updated independent of movement direction. #36

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

rokvintar
Copy link
Contributor

@rokvintar rokvintar commented Aug 18, 2016

In case when move is triggered by external program (not motor record) limit switches status should also be updated.

(comment edited on 1.9.2016)
This fixes issue #35

@rokvintar rokvintar force-pushed the ls_status_update_fix branch from f7865bb to ddef3a8 Compare August 18, 2016 16:21
@tboegi
Copy link
Contributor

tboegi commented Sep 1, 2016

"Status of limit switches updated independent of movement direction."
Should that read
Status of limit switches updated dependent of movement direction. ?
?

Even then, I am not sure if I understand the patch.

When the motor has stopped after a movement e.g. to the HLS,
3 things can happen:

  • (a) HLS is active
  • (b) HLS and LLS are active
  • (c) LLS is active

For (a) and (b) nothing is changes with this patch,
(or do I miss something ?)

If we run into (c), I would consider this system broken.
It should be fixed at the hardware level, and, if that
is not possible, in the driver.
I don't see a need to fix a local problem in a global
SW module.

Beside that, the masking using cdir is not ideal.
Consider a system which is slowly creeping against
a limit switch, without any commanded movement.
Don't you want to know that ?

Beside that, a longer description in the commit message
would be appreciated:

  1. Describe the problem
  2. Shortly descr the solution

And let people 2) matches 1) and, of course,
the source code changes matches 1) and 2)

@rokvintar
Copy link
Contributor Author

This patch fixes issue #35 which is described here (this is why the description above is so short). To avoid future confusion I have also added this link to comment above.

This patch fixes exactly this problem:

Beside that, the masking using cdir is not ideal.
Consider a system which is slowly creeping against
a limit switch, without any commanded movement.
Don't you want to know that ?

I definitely agree with you that masking with cdir is far from ideal, and this is why it was removed from the part of code, where HLS, LLS, RHLS, RLLS fields defined. As you said, motor may slowly drift and hit the limit, or in our use case it can be moved by some motion programs running directly on the controller (Deltatau PPMAC in our case).

Without that fix following scenario can happen:

  1. Motor is moved with motor record in + direction.
  2. Motor is stopped and not on limit.
  3. Motor drifts or is moved by some custom motion programs (not by motor record) in - direction.
  4. Motor hits negative limit switch.
  5. LLS and RLLS aren't set to "1" (because CDIR is + from the last motor record controlled move)

Since this fix removes CDIR masking, point 5 would now be:
5. LLS and RLLS are set to "1" (and CDIR is still +, and not important any more)

Kind Regards,
Rok

P.S. If you check the commit changes, you will see that cdir checking was added to some other parts of the code (e.g. in function maybeRetry()). This was done this way to keep other functionality untouched.

@keenanlang keenanlang added the bug label Apr 26, 2017
AdrianPotter pushed a commit to ISISComputingGroup/EPICS-motor that referenced this pull request Jun 13, 2017
John-Holt-Tessella added a commit to ISISComputingGroup/EPICS-motor that referenced this pull request Jun 16, 2017
@kmpeters kmpeters requested a review from rsluiter April 27, 2018 16:00
@kmpeters kmpeters added this to the R6-11 milestone Jun 21, 2018
@kmpeters
Copy link
Member

I started testing this today, but did not finish. I expect to finish testing and merge this pull request early next week.

@kmpeters kmpeters merged commit 0dc3a09 into epics-modules:master Jun 26, 2018
rerpha pushed a commit to rerpha/motor that referenced this pull request Nov 21, 2022
…8_newport_controller

Ticket 5688: Added newport to is moving flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants