search

python-microscope / microscope

Python library for control of microscope devices, supporting hardware triggers and distribution of devices over the network for performance and flexibility.
https://www.python-microscope.org
GNU General Public License v3.0
67 stars 39 forks source link

New hardware support: Olympus IX83 microscope #178

Open dstoychev opened 3 years ago

dstoychev commented 3 years ago

The basic functionality is here: https://github.com/dstoychev/microscope/tree/olympus-ix3

It was ready to be merged at the time, don't know if any of the changes from the last couple of weeks would require changes.

carandraug commented 3 years ago

I have looked into this to be merged and here's some feedback:

  1. Can the wrapping of the SDK (basically, what's defined on the header file) be limited to a module in the microscope._wrappers subpackage? This makes it easier to mock that module which is required to generate the documentation.

  2. About having to manually loading msl_pd_1394.dll is this expected behaviour or are we working around a bug? I'm guessing the later, can you report it to them?

  3. Is the name of the class because we are using TCP for the communication? I'm not sure that is important to the end user unless we plan to have other classes with other protocols.

  4. I have found it's cleaner to have a separate "connection" class that would have the send (and I guess its __init__ would be the initialise_portmanager). This will make it easier to swap it for another later.

  5. It seems that you always check the status value of send_command_blocking so maybe do inside the function itself. I guess you still need to check the actual response on the caller but it would still be less repetition.

  6. I think there's an issue on the units passed to the IX3TPC constructor. The default is a string but then it's iterated over it (which means one character at a time). Maybe it should be a list?

  7. n a couple of places you use:

    _ = some_function

    Which I think is because you don't actually want the _ argument. In that case, simply omit the return value. But in those cases, the ignored value is the status value of a function. Even if there's nothing you can do in case the function failed, I think you should at the very least log that calling the function failed somehow.

  8. You have a cyclic reference on the stage when you pass it to the axis.

  9. Instead of manually checking the error response, as in:

    elif response == "XYM !,E0B700020":
    _logger.warning("Attempted to move the stage beyond its limits.")
elif response == "XYM !,E0B700070":
    _logger.warning("Attempted to move the stage while in motion.")

    which you already have duplicated, would a dict of error codes to error messages be helpful?

  10. In the stage axis, you compute the limits each time they are requested. Can they change over time? If not, maybe they could be computed once and then just return what was computed the first time.

  11. Don't use the str.format, especially when passing it to the loggers. Use the `logger(str_fmt, *args) This means that the string is only formatted if the message is actually going to be printed.

dstoychev commented 2 years ago

Still haven't addressed the issues from the comment above, but just wanted to say that my development has moved to the following branch: https://github.com/dstoychev/microscope/tree/olympus_ix83. I hope to get a working version within a few weeks.