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

Support and abstract EPICS 7 pvAccess and pvData #212

Open
robnagler opened this issue Nov 20, 2024 · 10 comments
Open

Support and abstract EPICS 7 pvAccess and pvData #212

robnagler opened this issue Nov 20, 2024 · 10 comments

Comments

@robnagler
Copy link
Collaborator

robnagler commented Nov 20, 2024

utils.PV.caget/put should be generally named: get/put? Perhaps this is a per-device configuration, but probably shouldn't be in the interface directly.

@nneveu
Copy link
Member

nneveu commented Nov 22, 2024

@robnagler are you talking about this file? I think this is an example where things on the sc side were way ahead of the general device stuff. Maybe we can get rid of the pyepics folder and incorporate this in another place. @lisazacarias you wrote this and use this in some of your sc repos right? Maybe we can combine/move this to the device code area? Or is there a context where we would use this outside of interacting with devices?

@robnagler
Copy link
Collaborator Author

Yes, and there are a few other places (in bmad_modeling). Maybe channel access is necessary for now. It's use should be encapsulated so that the code doesn't have to change when channel access goes away.

@nneveu
Copy link
Member

nneveu commented Nov 22, 2024

Yes, definitely agree we need to merge/update all places using channel access to one location to prepare for the change. I think @williamColocho 's bmad_modeling functions will be easier to update and touch less things. Bmad and utils file changes could be separate issues.

@MattKing06
Copy link
Collaborator

I went to pick up this issue, and it looks like it's already been abstracted somewhat with the get and put functions:
https://github.com/slaclab/lcls-tools/blob/main/lcls_tools/common/controls/pyepics/utils.py#L83

To ensure that caget and caput functions aren't used, they could be underscored.

@robnagler
Copy link
Collaborator Author

You'll want to keep them around so you don't break existing code. If there isn't a lot of code, then it's not a problem, obviously. One of the things we recommend is a shift towards infinite backwards compatibility for things like this.

If you call caget and caput, you get exactly that. When the switch to PV access happens, your code won't work with the new features available. It's at that time, that you decide to switch, not when some library breaks backwards compatibility. It has a lot do with context, which makes upgrades a lot easier and more reliable.

@MattKing06
Copy link
Collaborator

MattKing06 commented Jan 30, 2025

It also looks like these specific functions in common.controls.pyepics.pv_utils.py are not referenced anywhere else inside of lcls-tools, I can't speak for other scripts/apps though. However, they do potentially mangle the namespace of epics.caget and epics.caput if imported in as specific way.

The PVSet class used by Device uses the epics.PV class to perform get/set and this should be encouraged over the above functions.

@robnagler
Copy link
Collaborator Author

However, they do potentially mangle the namespace of epics.caget and epics.caput if imported in as specific way.

Another recommendation (obviously doesn't fix any problems): don't import unqualified. Refer to epics.caget, not "caget". This not only avoids collisions today but collisions tomorrow (forward compatibility). It also makes the code readable to someone who doesn't know what "caget" is when looking at the code (is a local function or is it imported?). This is not common practice in lcls-tools (or the vast majority of scientific software), but we strongly recommend using qualified imports.

@nneveu
Copy link
Member

nneveu commented Jan 30, 2025

I think @lisazacarias uses that pyepics util in other repos. @lisazacarias can you comment on where your utils file is used? Lets try to converge and make this more general for the channel access/PVA change that is coming.

@nneveu
Copy link
Member

nneveu commented Feb 12, 2025

I chatted with @hmarts9 and @lisazacarias on this. Lisa is open to using another method vs. what's in utils. I asked Haley to make a list of where it's used in SRF code so we can see the use cases. @MattKing06 I think at this point we have three implementations, yours, Lisa's and RadiaSoft's in slicops.

@lisazacarias
Copy link
Collaborator

Yes burn it down if you want to, I don't mind using something else; I wrote my stuff in such a way that it's an easy change to make. This was mostly a stop gap for strange channel access errors I was getting that weren't being handled properly in pyepics and were randomly crashing my programs

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

No branches or pull requests

4 participants