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

Powermeter fast-fault edits #78

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

KaushikMalapati
Copy link
Contributor

Description

Only fast-fault if ppm is not out or moving

Motivation and Context

Power meters in rix are fast faulting even when beam energy should not be high enough to trigger them.

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

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

@KaushikMalapati
Copy link
Contributor Author

I tried adding a simple way of only triggering the fault if the energy is persistently over what's allowable, but I am completely open to doing it another way.

Comment on lines 307 to 311
IF eEnumGet <> E_PPM_States.OUT AND bOverAllowableEnergy = TRUE THEN
uOverCounter := MIN(uOverCounter + 2, 50);
ELSE
uOverCounter := MAX(uOverCounter - 1, 1);
END_IF
Copy link
Member

@ZLLentz ZLLentz Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the counter approach here because it's easy to understand and doesn't do anything overly complex. This is what we want for a robust PLC program.

25 cycles at 10ms per cycle = 250ms = quarter of a second
I'm not exactly sure what the "most optimal" count would be but this seems reasonable and will definitely cull the false positives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If bOverAllowableEnergy switches between True and False frequently, it could take up to 50 cycles or half a second to trigger the fault. (+2 * 50) + (-1 * 50) = 50, (+2 * 49) + (-1 * 51) = 47. As long as half of the cycles or more are over, it will trigger, but the higher the proportion the less time it takes, maxing at a quarter second. Like you said, these are all magic numbers that I just think seemed reasonable

ELSE
uOverCounter := MAX(uOverCounter - 1, 1);
END_IF
FF(i_xOK := eEnumGet = E_PPM_States.OUT OR uOverCounter < 50,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to get into the habit of verbosely double-checking fast faults now:

Fault is OK if:

  • PPM is out
  • OR counter is under 50

Therefore, the beam turns off when both:

  • PPM is at one of the known target or an unknown state or in motion
  • AND the counter is at least 50 (counter caps out at 50)

This is totally reasonable logic and I endorse it

Some might argue that e.g. the YAG states should also be exempt from these faults, but there is some judgement call to be made because what if the beam is actually hitting the power meter somehow in these states? And what if the state positions other than "OUT" are wrong? I think there's a philosophical discussion to be had here.

ZLLentz
ZLLentz previously approved these changes Oct 30, 2024
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to come back and re-approve, I like this

Copy link
Contributor

@ghalym ghalym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KaushikMalapati
Copy link
Contributor Author

Maggie helped me test this. We checked that the fault would not occur unless the PPM position was at the power meter and the energy was above the threshold, so I think this is good to merge

@KaushikMalapati KaushikMalapati merged commit 740f0b9 into pcdshub:master Nov 7, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants