labscript-suite / labscript-devices

A modular and extensible plugin architecture to control experiment hardware using the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦.
http://labscriptsuite.org
Other
5 stars 58 forks source link

Changed SpinnakerCamera StreamBufferCountMode #116

Closed jacksulli closed 1 month ago

jacksulli commented 2 months ago

Previously, the buffer count mode for the Spinnaker Camera in blacs_workers.py was set to Auto. This Auto feature has been depreciated by FLIR so I changed it to be manual in the same format as the continuous mode. In the case where bufferCount == 1, I set it to 3 because the Spinnaker SDK mentioned that 3 was the minimum buffer count.

dihm commented 2 months ago

Thank you for the bug fix @jacksulli, it is much appreciated! I have only have three questions:

  1. Do you know what version of the SDK deprecated the Auto feature?
  2. Is the change you've made backwards compatible with older SDKs? If it isn't, we'll want to put in checks to capture the SDK version and select the correct mode based on that.
  3. (edit) Why did you change the stream mode to take newest images first instead of oldest?
jacksulli commented 2 months ago

Thank you for the bug fix @jacksulli, it is much appreciated! I have only have three questions:

  1. Do you know what version of the SDK deprecated the Auto feature?
  2. Is the change you've made backwards compatible with older SDKs? If it isn't, we'll want to put in checks to capture the SDK version and select the correct mode based on that.
  3. (edit) Why did you change the stream mode to take newest images first instead of oldest?

No problem @dihm! Version 3.0.0.118 depreciated the feature, which I found here: https://www.flir.com/support-center/iis/machine-vision/knowledge-base/spinnaker-sdk-release-notes/

I think that it should still be backwards compatible, since I just used the same format for the buffer mode that the continuous acquisition mode used. As long as that still works with previous versions, I think these changes should as well.

The only reason I switched it to newest first was because I was just trying to copy the format of the continuous mode, so that might not be right. I probably should have looked into that a bit more!

dihm commented 1 month ago

@jacksulli Sorry for continuing to be slow here. I don't have an easy way to test this here and things keep piling up.

The only reason I switched it to newest first was because I was just trying to copy the format of the continuous mode, so that might not be right. I probably should have looked into that a bit more!

Yeah, this is an explicit choice the camera drivers make (when available). When streaming in continuous mode, you don't really care about frame order or dropping frames, so you always choose newest first in case the computer gets slow relative to the camera. When in buffered mode, dropped frames and order are critical considerations, so we always pull the last frame first to ensure it gets read out before the buffer overflows. We should continue to respect that distinction.

I also think we could probably modify this if-else branch since buffercount of 1 isn't allowed anymore anyway. I'll suggest some changes directly in the code itself. If you could make a few modifications, I'll go ahead and merge.

jacksulli commented 1 month ago

@jacksulli Sorry for continuing to be slow here. I don't have an easy way to test this here and things keep piling up.

The only reason I switched it to newest first was because I was just trying to copy the format of the continuous mode, so that might not be right. I probably should have looked into that a bit more!

Yeah, this is an explicit choice the camera drivers make (when available). When streaming in continuous mode, you don't really care about frame order or dropping frames, so you always choose newest first in case the computer gets slow relative to the camera. When in buffered mode, dropped frames and order are critical considerations, so we always pull the last frame first to ensure it gets read out before the buffer overflows. We should continue to respect that distinction.

I also think we could probably modify this if-else branch since buffercount of 1 isn't allowed anymore anyway. I'll suggest some changes directly in the code itself. If you could make a few modifications, I'll go ahead and merge.

Sounds good, I've made the changes you suggested. I also looked again in the Spinnaker documentation but I also couldn't find where it said the buffer count minimum should be 3 anymore, so I'm not sure where I found that. I tested it a little bit and taking a single snap with both the UI and through RunManager seemed to work fine with the buffer count set to 1.