search

SentioProberDev / SentioProberControl

Python bindings for controlling MPI probe stations
https://ast.mpi-corporation.com/
BSD 2-Clause "Simplified" License
5 stars 6 forks source link

added snap_image and changed step_first_die #5

Closed MStanton99 closed 1 year ago

MStanton99 commented 1 year ago

Added the vis:snap_image command to the VisionCameraCommandGroup.py.

Changed the step_first_die command to step to the first subsite in the sequence instead of subsite 0 if no subsite is provided. This matches better with the SENTIO program if the first subsite is disabled.

MStanton99 commented 1 year ago

Thanks for the pull request. I need to check this further.

The first change (snap_image) I will have to discuss internally first. Our goal is to keep the python api close to the remote command api and putting snap_image into the camera group would be a duplication of the existing logic in the vision command group. However the existence of the camera command group is already a deviation from our goal so I need to discuss this internally.

Can you tell me the motivation for changing step_to_first_die because from looking at the code I would assume that nothing really changed because the behavior of "map:step_first_die" without a parameter should be to move to site 0. The original code should already do this. Did it not? I can't currently check this because i'm not in the office right now. It also appears that the return value was removed which probably happened by accident but it would introduce a deviation to the published remote command documentation.

Apologies, I was unaware that the snap_image command was in the vision command group and not the camera command group. I can remove that from the commit, if it makes it easier on you. I did also mistakenly delete the return statement in the step_first_die command, I must have missed it when testing the code as I was doing it out of a scratch file. It should be fixed now.

As for the step_to_first_die, I think that if no subsite is provided, then the command to be sent should be "map:step_first_die", not "map:step_first_die 0". This is because if subsite 0 is disabled and the command "map:step_first_die 0" is sent, the prober goes to the home location of the first die. This is a problem for us, because our home location is not always a valid die, and I would have to specify subsite 1 as the first die to step. Now, this is not a huge problem, but it does make the code I am writing less user friendly and less general. If just the command "map:step_first_die" is sent, this is identical in behavior to pressing the "step first die" function on the prober itself, even if subsite 0 is disabled.

Currently, I have bypassed the commands all together and jury-rigged a solution where I send my own custom command:

prober.map._comm.send("map:step_first_die")
col, row, site = Response.check_resp(prober.comm.read_line()).message().split(',')

But if this was officially supported It would be cleaner and easier to work with.

beltoforion commented 1 year ago

Can you remove the snap_image from the pull request? For the time being we would like to avoid duplicate functionality since it may create confusion.

You are right about the step_first_die implementation. That is pretty much how it should look like. I tend to forget that you can actually disable the first site.

MStanton99 commented 1 year ago

Can you remove the snap_image from the pull request? For the time being we would like to avoid duplicate functionality since it may create confusion.

You are right about the step_first_die implementation. That is pretty much how it should look like. I tend to forget that you can actually disable the first site.

I have removed snap_image.