Skip to content

Changing EpicsSignal to PytmcSignal for plc value#1318

Merged
KaushikMalapati merged 3 commits intopcdshub:masterfrom
KaushikMalapati:reset_indicator
Jan 24, 2025
Merged

Changing EpicsSignal to PytmcSignal for plc value#1318
KaushikMalapati merged 3 commits intopcdshub:masterfrom
KaushikMalapati:reset_indicator

Conversation

@KaushikMalapati
Copy link
Contributor

@KaushikMalapati KaushikMalapati commented Jan 24, 2025

Description

Made reset_cmd in TwinCATStatePositioner a PytmcSignal instead of an EpicsSignal so that the indicator uses the _RBV pv

Motivation and Context

https://jira.slac.stanford.edu/browse/ECS-7110
https://jira.slac.stanford.edu/browse/ECS-6741

How Has This Been Tested?

Not tested yet.

Where Has This Been Documented?

upcoming release notes

image

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@KaushikMalapati
Copy link
Contributor Author

I think that the fix Nicholas mentioned about making stEpicsToPLC a VAR_IN_OUT needs to be deployed to tmo-motion before I can "test" this with li2k4, although I'm pretty sure that this will work since the indicator is using the _RBV pv now. I got confused when I saw it wasn't updating until I saw that the old sequence mover is currently running.

NSLentz
NSLentz previously approved these changes Jan 24, 2025
Copy link

@NSLentz NSLentz left a comment

Choose a reason for hiding this comment

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

This looks good, however, Tong told me she already tested the change to the sequence mover and that it was working and we applied that change to the PLC so perhaps something else is happening if it still has issues updating on the display?

@ZLLentz
Copy link
Member

ZLLentz commented Jan 24, 2025

I suspect that you'll never catch this ui element updating unless there's an issue with the PLC code that gets it stuck in the TRUE state, since the PLC clears the indicator very quickly.

Showing the mouseover text with the corrected PV connected is enough of a test for the UI side IMO

@ZLLentz
Copy link
Member

ZLLentz commented Jan 24, 2025

I think this is mergeable with a pre-release docs added (no need to make it detailed, just a quick one-liner would suffice)

@KaushikMalapati
Copy link
Contributor Author

I forgot to push my docs yesterday. I was testing something on tmo-motion before I did this so I think I overwrote the VAR_IN changes that were present. I wonder if it makes sense to not show the indicator for things like this, I remember talking about it in #1303.

@ZLLentz
Copy link
Member

ZLLentz commented Jan 24, 2025

I wonder if it makes sense to not show the indicator for things like this

It's possible that the indicator is more distracting than useful. I think this tweak is better than what we had before because it will help us find PLC bugs, maybe we can follow-up by thinking more broadly about what would be most useful to show for these sorts of "set and then they get reset" sorts of signals.

If we can think of a most-correct thing to show in the readback indicator we can add it as a standard typhos variety and it will absolutely get some use in various places.

Some examples of possible variants, to give an idea of the decision space:

  • remove the circle indicator
  • have the circle indicator light up briefly if the button's caput was successful (I think this one would be neat)
  • have two circle indicators, one for setpoint and one for readback

@tangkong
Copy link
Contributor

Is "getting stuck in the True state" something that happens frequently? Aside from that one error state, it's hard to imagine the indicator providing much value.

I 100% agree that this is an obvious improvement over the status quo though, I have no objections to merging this.

@ZLLentz
Copy link
Member

ZLLentz commented Jan 24, 2025

Is "getting stuck in the True state" something that happens frequently?

It happened once in TMO as part of a tough to track down bug, otherwise I haven't seen it. I suspect it only could happen as a bug.

@KaushikMalapati KaushikMalapati merged commit a004eed into pcdshub:master Jan 24, 2025
11 checks passed
@KaushikMalapati
Copy link
Contributor Author

Going to merge this for now, but I think it would be nice for the rbv circle to momentarily light up if the caput was successful, although I'm not sure how to do this. I'm not sure if having an indicator for the setpoint would work for this. Would the setpoint pv update if the value changed for a single cycle?

@ZLLentz
Copy link
Member

ZLLentz commented Jan 24, 2025

As a rough outline I think the way I'd implement it is (in typhos) adding a special widget (rather than a base pydm indicator) that has the behavior we want, which is:

  • subscribe to both setpoint and rbv
  • when the setpoint is put to, for a period of 1s show the indicator as lit
  • after the 1s period, revert to the rbv value (and continue to update)

There are more details but that would be my first try.

Then I'd need to add a variety metadata entry for it and we'd be able to use it here.

@KaushikMalapati KaushikMalapati deleted the reset_indicator branch January 24, 2025 18:57
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.

4 participants