-
Notifications
You must be signed in to change notification settings - Fork 2
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
Combine sync and dpm services #6
Comments
The idea behind packaging this way is to limit how much a developer includes in their code. For example, most people aren't using sync. I think in the case of sync, it's a small amount of code but we were thinking it solves a potential future bloat problem. That being said, I'm not for pre-optimization so maybe services can be more integrated until we need to break them out. I don't know that combining them in the way you suggest solves the problem. They are different services and open separate channels. DPM talks to DPMs and Sync talks to SYNCD. It wasn't clear to me why someone would want both sync and DPM until I asked you. Our thinking is that DAQ is driven by events and that gives you the relevant information. Trying to take something away from this issue, I think a unified interface to iterate over all requested data is a nice idea that we should consider for the next major version. |
@rneswold: The idea behind packaging this way is to limit how much a developer includes in their code. Exactly. The control system has a lot of protocols which are the building blocks for a controls application. The acsys packages was a way to use what you need. I created the SYNCD service because there was a need: Wally wanted to know when a particular event occurred so he made a DPM request to read a device on the event of interest and simply threw away the device data. Along with being slightly wasteful of front-end CPU and network bandwidth, his script would break if the front-end went down. So I created the service, which runs on a pool of nodes, so he wouldn't be dependent on a front-end. But ACNET is a transport for many, many protocols. We're supporting two of them for Python, but others can be added. Maybe we could be better at hiding the Connection object. I wish Python had better built-in combinators for async generators. Maybe we'll have to add some in acsys... |
I dont have any issues with having to import sync and dpm to use both, I agree that compartmentalizing is the way to go. I have a lot of use cases to monitor the clock while sampling readings at the same time. acsys.run_client() passes a Connection object to a async method, if I want to monitor both clock and readings I need to use the same Connection object to instantiate both and then combine streams because iterating over either separately is synchronous. Im not sure what the best approach would be but I dont think creating multiple connections is the way to go (unless DPM has smarts to return to me the same Connection) so maybe allow the Connection object to be passed around and allow the user to pass it to a threadpool model so multiple things could asynchronously share the connection? |
Yes, the async def handle_clock(con):
async for ii in acsys.sync.get_events(con, ['e,8f']):
# do stuff because event fired
async def handle_dpm(con):
async with DpmContext(con) as dpm:
# set up DPM requests
# loop through replies
async for ii in dpm.replies():
# do stuff with DPM data
async def main(con):
asyncio.gather(handle_clock(con), handle_dpm(con)) which works when your handling of events has nothing to do with handling data replies. The |
@kjhazelwood I'm guessing your current testing will answer this question but is this still an issue. Does @rneswold 's solution work for you? |
@kjhazelwood: Is there a benefit to having the two services separated. It looks like they can share the same Connection object, why not for the users sake combine them? This way one doesnt have to iterate over combined sync and dpm streams to monitor clock and dpm within the same script. It seems like it would be possible to allow the drf2 event string as a valid entry in dpm.add_entry.
The text was updated successfully, but these errors were encountered: