fix(client.command.cryo_card): Catch EPICS timeout on read and retry.

[tristpinsm]

Description

Addresses issue #792

Jira Issue

N/A

Tests done on this branch

It's a simple enough fix that that testing didn't seem necessary.

Function interfaces that changed

N/A

What was the interface before the change

N/A

What will be the new interface after the change

N/A

[jlashner]

I think we may want to test this... PVs in the smurf_command module use a 10 second timeout, so a retry happens roughly every 10 sec. We may want to implement the same thing here; as things are now I believe retries will happen immediately which I don't think we want.

[tristpinsm]

OK, I've added a timeout between restarts and I can try it out on a test system at SLAC this afternoon. Just to confirm that it is retrying as expected when it can't reach the cryo card. However I'm not sure how to determine the appropriate number of retries or time in between. Currently it's set to retry 5 times, at 5s intervals.

[tristpinsm]

Alright so I've tested CryoCard.read_temperature with no cryo card connected and it's retrying as it should:

[ 2024-05-17 19:25:19 ]  CryoCard.do_read waiting 5s before retry 2 / 5.
[ 2024-05-17 19:25:24 ]  CryoCard.do_read waiting 5s before retry 3 / 5.
[ 2024-05-17 19:25:29 ]  CryoCard.do_read waiting 5s before retry 4 / 5.
[ 2024-05-17 19:25:34 ]  CryoCard.do_read waiting 5s before retry 5 / 5.

However, after those attempts it returns a nonsense value, which is just how the code is designed.

[jlashner]

I'm confused how this is working, shouldn't the continue bypass the sleep and log? Maybe this is an instance where it does not timeout but the returned address does not match.

I think instead of manually sleeping, it makes more sense to set the timeout in the epics get fn, like here:

pysmurf/python/pysmurf/client/command/smurf_command.py

Lines 208 to 211 in 1ff6632

	ret = self._pv_cache[pvname].get(count=count,
	use_monitor=use_monitor,
	timeout=timeout,
	**kwargs)

5 sec timeout with 5 retries sounds good to me as a default!

[tristpinsm]

I'm confused how this is working, shouldn't the continue bypass the sleep and log? Maybe this is an instance where it does not timeout but the returned address does not match.

yeah, that's a mistake...

I think instead of manually sleeping, it makes more sense to set the timeout in the epics get fn, like here:

pysmurf/python/pysmurf/client/command/smurf_command.py

Lines 208 to 211 in 1ff6632

	ret = self._pv_cache[pvname].get(count=count,
	use_monitor=use_monitor,
	timeout=timeout,
	**kwargs)

5 sec timeout with 5 retries sounds good to me as a default!

ok

[jlashner]

This is looking good to me... one more thing though, I think we should raise an exception on failure instead of just returning 0. Can we add that here?

[tristpinsm]

This is looking good to me... one more thing though, I think we should raise an exception on failure instead of just returning 0. Can we add that here?

Yeah, I agree that makes a lot more sense. I wonder if that change may break other code that is relying on the current behaviour though. I guess I can have look and see.

[jlashner]

Great, thanks!

[tristpinsm]

@swh76 this one should also be merged for the next release