nion-software / nionswift-instrumentation-kit

Base classes for Nion Swift STEM microscope instrumentation
GNU General Public License v3.0
1 stars 12 forks source link

IndexStack current index changing before call to acquire_sequence_prepare #159

Closed Brow71189 closed 10 months ago

Brow71189 commented 1 year ago

https://github.com/nion-software/nionswift-instrumentation-kit/blob/fb8760cb5d7fb862fc0b87bd167a0a00e85a3946/nion/instrumentation/Acquisition.py#L787

I think this line is very dangerous: It will break code that tracks the current index of index stack because it changes the current index before acquire_sequence_prepare is being called on a camera device. This means the device will expect this index to still be the previous value because you would only expect it to change with the call to acquire_sequence_prepare. I tried following this through but I'm having a hard time seeing how this index change can go through before the call to acquire_sequence_end, but it did definitely happen. In my opinion the IndexStack should be rebuilt here instead of changing existing objects in place.

The camera device implementation I'm using is something like this:

class CameraDevice:

    def acquire_sequence_prepare(self, camera_frame_parameters: CameraFrameParameters, count: int, **kwargs: typing.Any) -> None:
         self.index_stack: Acquisition.IndexDescriptionList = kwargs.get('index_stack')
         ...

    def acquire_sequence_end(self, **kwargs: typing.Any) -> None:
        # Do something that depends on self.index_stack[0].index still being whatever it was after the last call to acquire_sequence_prepare
        ...

With this implementation, self.index_stack[0].index changed in between the call to acquire_sequence_prepare and the subsequent call to acquire_sequence_end whereas I would only expect it to change after the next call to acquire_sequence_prepare.

I know there is a easy workaround, namely to do self.index_stack = copy.deepcopy(kwargs.get('index_stack')) in the example above. I just think that having this index changing unexpectedly is dangerous and leads to very hard to find bugs.