FIX: issues relating to "queued" moves in state mover with bMoveOk #205

ZLLentz · 2023-10-06T23:07:08Z

Description

If there is a move error, cut the execute output
Require a bExecute rising edge and bAllowMoves at the same time, or else no state moves

Motivation and Context

See https://jira.slac.stanford.edu/browse/ECS-3932
There are cases where "unsafe" moves can become marked as "safe" during execution, and then immediately start as if they were waiting to become "safe" rather than cancelling the move request altogether.

How Has This Been Tested?

I created a unit test that follows the following sequence:

Ask for a move to a bMoveOk = FALSE state
Confirm no move
Set bMoveOk := TRUE
Confirm no move
Reset the error
Confirm no move
Ask for a new move
Confirm yes move

I also tested this interactively, and it works as one would expect: you need to reset the error, wait for the move to become safe, and then explicitly ask for the move again. Here's a screenshot of the new updated error message:

Where Has This Been Documented?

Release notes only

Pre-merge checklist

Code works interactively
Test suite passes locally
Code contains descriptive comments
Libraries are set to Always Newest version (Library, *)
Committed with pre-commit or ran pre-commit run --all-files

ZLLentz · 2023-10-07T00:45:39Z

I'm a bit stumped as to the failure state of my test- at the end, the motor moves as I expect it to, but the function block reports that it is not busy. I'm not sure what I'm looking at, if it's a bug related to this code or unrelated or not a bug at all and I'm calling the code wrong.

ZLLentz · 2023-10-12T18:50:03Z

It turns out that my tests were failing simply because I wasn't waiting long enough to check the results. I feel a little bit silly.
Remaining TODO: verify that the unit test fails with the pre-patch version of the code.

ZLLentz · 2023-10-12T19:02:02Z

The tests pass without the patch, meaning that I've completely failed at some part of the dev process here.
I need to go back to interactive pre-patch testing to make sure I understood the issue properly.

ZLLentz · 2023-10-12T23:14:38Z

I'm currently taking this as a learning experience to experiment with and get better at writing these sorts of unit tests. Hopefully this is a time investment that will pay off later. I have not entirely figured out what is wrong, but it has something to do with extra moves being requested on my motor that I don't intend to make.

ZLLentz · 2023-10-13T00:00:13Z

This newest version of the test consistently passes without hitting the weird errors I was seeing, largely by being less "cycle-precise" than the previous iterations. That is, we're hitting buttons more like "humans" with human delays.

I think this means that some of these function blocks have race conditions, and I think some of these race conditions arise from the interaction of the NC process with the PLC process.

In addition, the test now fails how I expected it to fail with the pre-patch code.

ZLLentz · 2023-10-13T00:16:51Z

"The usual suspects" for code review are all at a conference right now- feel free to wait until next week to take a look.

ZLLentz · 2023-10-24T20:15:10Z

@klauer @slactjohnson @ghalym I'd appreciate at least one review on this so we can close https://jira.slac.stanford.edu/browse/ECS-3932

klauer

The latching logic makes sense to me, I think - though I do want to confirm that with the few comments I have here.

klauer · 2023-10-24T20:24:10Z

lcls-twincat-motion/Library/POUs/Motion/States/FB_PositionStateMove.TcPOU

+    IF nLatchAllowErrorID <> 0 THEN
+        nErrorID := nLatchAllowErrorID;
+    ELSIF stPositionState.bValid THEN
+        nErrorID := 16#7901;


A comment on these magic numbers would be appreciated (or eventually moved to an enum as we may have previously chatted about, I think)

I thought I'd done the background work for this and forgot to apply it here, but the reality is that I only implemented enums so far for the FF types. The custom error codes are reflected only in the error message writer FB, this needs some better organization as you've said before.
https://github.com/pcdshub/lcls-twincat-motion/blob/master/lcls-twincat-motion/Library/DUTs/E_MotionFFType.TcDUT
https://github.com/pcdshub/lcls-twincat-motion/blob/master/lcls-twincat-motion/Library/POUs/Motion/Utils/F_MotionErrorCodeLookup.TcPOU

My intuition is that this calls for a new enum specifically for custom error codes- not to rehash all the built-in errors.

I think the "custom error code enum" makes a lot of sense. This certainly could be in a follow-up PR (if at all)

Handled in 1170c26

klauer · 2023-10-24T20:26:27Z

lcls-twincat-motion/Library/POUs/Motion/States/FB_PositionStateMove.TcPOU

 // Inject custom error if we can't move because of bMoveOk or bValid
-IF bExecute AND NOT bAllowMove THEN
+IF nLatchAllowErrorID <> 0 OR (bExecute AND NOT bAllowMove) THEN


In the scenario of nLatchAllowErrorID <> 0, it's necessary to redo this IF block not to re-look things up, but ensure that bError is set (no matter what fbMotionRequest does to it), is that right? Trying to follow the logic, and it seems largely a no-op beside for that.

That's exactly correct. I need to reapply my own FB's error status, including bError and nErrorID, irrespective of the internal error state of fbMotionRequest.

klauer · 2023-10-24T20:30:21Z

lcls-twincat-motion/Library/POUs/Motion/States/FB_PositionStateMove.TcPOU

 END_VAR
 ]]></Declaration>
    <Implementation>
      <ST><![CDATA[// Veto the move for uninitialized and unsafe states
 bAllowMove := stPositionState.bMoveOk AND stPositionState.bValid AND stPositionState.bUpdated;

+rtExec(CLK:=bExecute);
+bInnerExec S= rtExec.Q AND bAllowMove;
+bInnerExec R= NOT bExecute;


I'd naively have expected if there were a latched error we wouldn't execute fbMotionRequest at all. I'd imagine there are some details I'm missing there - mind giving a quick explanation?

I think, despite your other comment, you're correct. It may be worth doing something additional here, making the inner function blocks also protect themselves for the sake of robustness.

Handled in cc683b2

klauer · 2023-10-24T20:31:29Z

lcls-twincat-motion/Library/POUs/Motion/States/FB_StatesInputHandler.TcPOU

@@ -84,6 +86,10 @@ CASE nState OF
        END_IF
 END_CASE

+IF bMoveError THEN
+    bExecMove := FALSE;


Ah, I think this is the line that explains my above question...

This is indeed why it works out, but I think your previous point still stands

klauer

The enum additions look good and I have no further suggestions / nits

I'd say this is good to go, though it'd be nice to have another set of eyes on it for good measure

ZLLentz · 2023-11-02T18:28:37Z

I'm fairly confident in this and I'm already here, so I'm going to merge, tag, and deploy the tag. If something else is discovered between now and the PAMM there is time to address it.

ZLLentz added 3 commits October 6, 2023 16:02

FIX: ensure rising edge while move ok, block other moves if one error

0e50f24

TST: add test for move ok sequencing

b101160

FIX/TST: compilation errors ans some testing errors

051c034

TST: make the test more consistent

2bd8a35

TST: make test much more consistent and include more consistency asserts

d39582d

ZLLentz requested review from klauer, slactjohnson and ghalym October 13, 2023 00:16

ZLLentz marked this pull request as ready for review October 24, 2023 20:14

klauer previously approved these changes Oct 24, 2023

View reviewed changes

ZLLentz added 3 commits November 1, 2023 16:40

MAINT: replace custom error magic numbers with ENUM definition

1170c26

ENH: also stop execution internally if we have an error state

cc683b2

BLD: rebuild PLC project to run unit tests

02f3c64

ZLLentz dismissed klauer’s stale review via 02f3c64 November 1, 2023 23:49

ZLLentz requested a review from klauer November 2, 2023 00:18

klauer approved these changes Nov 2, 2023

View reviewed changes

ZLLentz merged commit 4898708 into pcdshub:master Nov 2, 2023
9 checks passed

ZLLentz deleted the fix_bmoveok_queue branch November 2, 2023 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: issues relating to "queued" moves in state mover with bMoveOk #205

FIX: issues relating to "queued" moves in state mover with bMoveOk #205

ZLLentz commented Oct 6, 2023 •

edited

Loading

ZLLentz commented Oct 7, 2023

ZLLentz commented Oct 12, 2023 •

edited

Loading

ZLLentz commented Oct 12, 2023

ZLLentz commented Oct 12, 2023

ZLLentz commented Oct 13, 2023

ZLLentz commented Oct 13, 2023

ZLLentz commented Oct 24, 2023

klauer left a comment

klauer Oct 24, 2023

ZLLentz Oct 24, 2023 •

edited

Loading

ZLLentz Oct 25, 2023 •

edited

Loading

klauer Oct 25, 2023

ZLLentz Nov 1, 2023

klauer Oct 24, 2023

ZLLentz Oct 25, 2023

klauer Oct 24, 2023

ZLLentz Oct 25, 2023

ZLLentz Nov 1, 2023

klauer Oct 24, 2023

ZLLentz Oct 25, 2023

klauer left a comment

ZLLentz commented Nov 2, 2023

FIX: issues relating to "queued" moves in state mover with bMoveOk #205

FIX: issues relating to "queued" moves in state mover with bMoveOk #205

Conversation

ZLLentz commented Oct 6, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

ZLLentz commented Oct 7, 2023

ZLLentz commented Oct 12, 2023 • edited Loading

ZLLentz commented Oct 12, 2023

ZLLentz commented Oct 12, 2023

ZLLentz commented Oct 13, 2023

ZLLentz commented Oct 13, 2023

ZLLentz commented Oct 24, 2023

klauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

ZLLentz Oct 25, 2023 • 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

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

klauer left a comment

Choose a reason for hiding this comment

ZLLentz commented Nov 2, 2023

ZLLentz commented Oct 6, 2023 •

edited

Loading

ZLLentz commented Oct 12, 2023 •

edited

Loading

ZLLentz Oct 24, 2023 •

edited

Loading

ZLLentz Oct 25, 2023 •

edited

Loading