search

slaclab / pysmurf

Other
2 stars 9 forks source link

Don't start streaming after setting up pysmurf #693

Closed agustiner closed 2 years ago

agustiner commented 2 years ago

Issue

652

Description

Jack brought up the fact that pysmurf setup() turns on streaming with set_stream_enable(1). After poking around, I believe the correct way to start streaming data from pysmurf is with stream_data_on(), not set_stream_enable(1). So it feels like a bug that set_stream_enable(1) is in there. After poking through sodetlib and pysmurf it seems that everywhere disables streaming immediately, hinting that it isn't actually desired to start streaming after setup.

P.S. Why is there set_stream_enable(1) and stream_data_on and not just stream_data_on?

Does this PR break any interface?

Which interface changed?

NA

What was the interface before the change

NA

What will be the new interface after the change

NA

swh76 commented 2 years ago

@aer7 does this address an outstanding issue? I'm worried changes like this, if we don't fully understand them, could break things in subtle ways we don't anticipate.

agustiner commented 2 years ago

As far as I know enabling and disabling streaming works as expected for our test scripts, sodetlib, and smurf-streamer. The reason this change seems logical is that most places disable streaming after setup() anyway. See,

https://github.com/simonsobs/sodetlib/blob/1ff24c68ef43c055a71948c9358b7e40991839d8/scripts/setup_pysmurf.py#L10

https://github.com/slaclab/pysmurf/search?p=1&q=disable_streaming

If the risk to this is high we don't need to make this change.

jlashner commented 2 years ago

I appreciate the response to the issue, but I would also be hesitant to remove this line. Especially since disabling the stream after Setup is different then never enabling the stream at all. There must've been a reason it was put there in the first place... like maybe the server expects some data to be sent through at setup for initialization purposes. Really hard to know without asking Ed or Jesus why this was necessary.

I think if you test it well it would probably be fine to merge, but we have already figured out how to work around the issue so I don't think it's all that important for us.