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

API: remove many unused PVs that scale with motor/state counts #192

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Aug 24, 2023

Description

https://docs.google.com/spreadsheets/d/1xfSB1mJwVu4tXvxBT85TEYVi29Z7VoQ4w5CXw13rV8A/edit#gid=0

Motivation and Context

Due to issues with PV count and long boot times in the TMO plc motion IOC, I'm planning to remove a bunch of PVs from the twincat motion library. This should improve the boot time for all the other beckhoff motion IOCs too

How Has This Been Tested?

N/a, hopefully the pragma lint CI passes

Where Has This Been Documented?

Google doc and release notes

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

Copy link
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

I think this matches what's in the spreadsheet - and think what was culled there is reasonable.

Though as you've noted/requested, more opinions would be great to ensure we're not missing something.

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

I'm going to merge on Friday if no more thoughts roll in

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

One thing I noticed today that might feed back into this:
It seems like the limit indicators on the motor record are actually the "final" "anything has said I can't go forward/backward" indicators.
So, if a device has restrictions placed by EPS, gantry, etc., it all gets combined into those LLS/HLS values.
So it might be good to bring back the specific enable PVs for limits, gantry, etc. since they may get use in UIs later.

@slactjohnson
Copy link

I don't have any issues with the removals.

However, one thought I had is that if we're just trying to cut overall PV counts, maybe some of these boolean status PVs could be put together into a bit vector PV, kind of like the motor record MSTA field?

Copy link

@slactjohnson slactjohnson left a comment

Choose a reason for hiding this comment

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

No issues from me with the removals. LGTM.

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

You're 100% right Tyler, we could make a second MSTA and/or we could batch stuff like forward enables, reverse enables, etc. together

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 24, 2023

I think there some significant value in bitmask PVs specifically for these directional and power enables to help disambiguate why LLS/HLS are hit. If there is some agreement I can implement those pretty easily.

Maybe this can come along with the long-awaited "just remove bPowerSelf" idea and related to clearing #188 and #185

@klauer
Copy link
Contributor

klauer commented Aug 25, 2023

I think it's a very reasonable addition 👍

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 25, 2023

I'm going to create an issue that proposes these bitmasks and suggest bundling it with the above work.
I'm going to merge this on Monday and make a "low PV count" tag, and work on the bitmask idea later.

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 28, 2023

@divyak1910 had a good point on the google doc RE: PVs that will be useful for configuring user patch panel motors, such as the homing mode (e.g. home to lls vs hls vs etc.)

I think what I want to do for these is to create a new ST_MotionStageUser that extends ST_MotionStage and adds some pytmc control points. I feel comfortable doing this because there's still a lot of other library changes needed to make user stages viable, including control points for NC and CoE parameters that don't belong on beamline motors.

To that end I think the removals here are all still correct- aside from bringing back some as bitmasks in a follow-up PR.

@ZLLentz
Copy link
Member Author

ZLLentz commented Aug 28, 2023

I am going to merge and tag this so we can set it loose on TMO motion and cut into that 22k PV count a little bit.

@ZLLentz ZLLentz merged commit 6be2039 into pcdshub:master Aug 28, 2023
@ZLLentz ZLLentz deleted the api_cull_pvs branch August 28, 2023 20:31
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