Add AlliedVision camera service

[THD2-team]

Add a service for AlliedVision's camera

Fixes #134

Follow-up in #143

Todo

• figure out how to correctly submit a returned image to the image data stream
• change to using vimba.get_camera_by_id(camera_id) to open up the camera
• use contextlib.ExitStack and use a single with statement outside of the class (see Emiel's comment)
• write an acquisition loop that works correctly with the API
• create related PR on thd-controls to add new camera service to services.yml there
• fix acquisition loop vs. frame handler
• test in lab with example script form thd-controls (thd-controls/examples/take_exposure.py)
• create follow-up issue: temperature reading, pixel format, testing of attributes

[ivalaginja]

@legger01 @linarphy this is coming along nice, I left you a couple of initial comments to look at (see in-code comments below).

You have very many linting errors in your code that say you are violating the PEP8 coding convention. For a full list of errors, scroll down to the bottom of the PR and identify the failed test, it is labelled with "Linting / Flake8 (pull_request)" and has a red cross next to it since it is failing. If you click on "Details" on the right of that, it will give you a detailed list of all the linting errors.

Most of these are too many blank lines, too many spaces etc. You can follow the detailed linting report to fix them, and you can also find some information about these conventions here if you want:
https://peps.python.org/pep-0008/

I can see in the Python API manual of Vimba that they do not care the least bit about PEP8, so let's not use that as a reference 😬

[ivalaginja]

In general, we are avoiding import * statements, so please change this to explicit imports from the vimba package.

Suggested change

	from vimba import *
	from vimba import FrameStatus, PixelFormat, Vimba

[ivalaginja]

The string passed to the super init defines the name of the service type. If you look at the various entries for "service_type" in the services.yml over in thd-controls, you will notice that they are all lower case, and do not use camel case (eg, flir_camera for the FLIR camera). Please adapt that for this camera as well.

Suggested change

	super().__init__('AlliedVision_camera')
	super().__init__('allied_vision_camera')

[ivalaginja]

As discussed during our meeting, make sure to change this back to the full sensor width and height, as described in the comment in the line before. Keep in mind that the definition of "width" and "height", and/or "x" and "y" could potentially be inverted between catkit2 and vimba.

Suggested change

	self.images = self.make_data_stream('images', 'float32', [self.height, self.width],
	self.images = self.make_data_stream('images', 'float32', [self.sensor_height, self.sensor_width], self.NUM_FRAMES)

[ivalaginja]

A general question for @linarphy and @legger01: vimba uses context managers that allow you to access the camera features, and the camera, which you have implemented here. Since you are exiting the context every time you exit a service class method, have you checked that the camera parameters you have set previously don't get reinitialized?

@ehpor do you see any issues with using context managers in a service like this, since it looks like it is not possible to acquire a camera handle and then keep it outside of the context manager?

[ehpor]

@ivalaginja @legger01 @linarphy The vimba.get_instance() things are a singleton pattern. Looking at the vimba internals, it seems to do some reference counting and automatic startup/shutdown of vimba itself (it calls a c function called VmbStartup). The same is true for the camera, which opens the actual camera with reference counting. So I'd definitely avoid many with statements in the code. I'd even be surprised if it works with these repeated with-statements.

I'd use contextlib.ExitStack and use a single with statement outside of the class. Something like

import contextlib

class AlliedVisionCamera(Service):
    def __init__(self, exit_stack):
        super().__init__('allied_vision_camera')

        self.exit_stack = exit_stack
        ...

    def open(self):
        self.vimba = vimba.get_instance()
        self.exit_stack.enter(self.vimba)
        ...

        self.cam = self.vimba.get_all_cameras()[0]
        self.exit_stack.enter(self.cam)
        ...

    @property
    def exposure_time(self):
        return self.cam.ExposureTime.get()

    ...

if __name__ == '__main__':
    with contextlib.ExitStack() as exit_stack:
        service = AlliedVisionCamera(exit_stack)
        service.run()

[ehpor]

Also, I'd use vimba.get_camera_by_id(camera_id) to open up the camera, rather than just opening the first one in the list of all cameras. camera_id can then be a value in the config.

[ivalaginja]

@linarphy @legger01 does this mean that the service now works on hardware?

[linarphy]

@linarphy @legger01 does this mean that the service now works on hardware?

Yes, it works now

[ivalaginja]

@ehpor could you have a look over this please? We still want to do some tests in the lab before marking the PR as ready, but since it is working now, would be great to hear from you whether we need to change anything. I personally am not sure for example whether the is_acquiring data stream is being submitted data to in the right spots, or whether the camera is being stopped in the right place. Please advise.

[ehpor]

I don't think this should be done in the frame handler, but as the last line of the acquisition loop.

[linarphy]

I'm not sure about how everything works to be honest. I understood the allied vision api as being asynchronous with thread being started inside the start_streaming function, so for me the call is blocking and the try and catch scope should be in the handler, but we will make test to be sure. At least I remembered the service closed gracefully after our last commit.

[ehpor]

Oh, that's interesting. That's indeed how your code looks, with the start_streaming() function being blocking. Their example look like it shouldn't be blocking, and that the thread started by them will call our frame handler whenever it receives a frame. But you have the camera, and you can test that easily XD. And I only checked that one example.

The main thing I want to avoid is rapid calls to start_streaming() and end_streaming(), which might seem to work but will give inconsistent frame times and generally unreliable behaviour.

[ehpor]

I'm not sure how this is supposed to work. I'd assume that you should wait on some event in between starting and stopping streaming. As is done in https://github.com/alliedvision/VimbaPython/blob/master/Examples/asynchronous_grab_opencv.py#L24

Also, I'd wrap this in a try-except, like they do there, for exception safety.

[linarphy]

Thanks, we will check the example you gave us to add the try-except and to adapt the code.

[ivalaginja]

@ehpor what would be an appropriate event to wait for between the start and stop of the camera streaming? To me it looks like it should be waiting for a True in self.should_shut_down, but I am unaware of the exact intended behavior of that flag.

[ehpor]

I don't think this should be inside the frame handler either.

[ehpor]

Also, I just noticed that the AlliedVision is not compatible with the base camera class in #131. So I'll have to refactor that a little bit to accommodate this camera. But that's unrelated to this PR, so don't worry about that.

[ivalaginja]

I changed this back to a draft so that I can test in the lab on Monday. I find the loops and checks in here not super obvious and want to make sure it still all works after the changes.

[ivalaginja]

Tested and works @ehpor

[ehpor]

LGTM