-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add services for Ocean Optics spectrometers #172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nicely coming into shape! It looks to me like you have made the decision to not have the spectrometer run a continuous acquisition like a camera, and instead to wait for a sent acquisition command like for example the PLP power source or the DM. Is that correct? In this case, I have left you a comment to help you straighten out the main()
loop. If I am interpreting this wrong though, do let me know and I will point you more in the direction of continuous acquisition loops like the cameras.
Also, could you maybe create a twin PR in https://github.com/ivalaginja/thd-controls with a new service entry for the spectrometer, so that I know which config entries to match and what that looks like?
The rest are just some minor comments that you will need to address before merging anyway and that are not critical to the service.
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good to me overall. I would suggest to simplify even more and to just take a measurement each interval of x seconds that is set in the service configuration. We do want a continuous acquisition, but I don't think it needs to be as fast as the camera - one new spectrum every 0.5 seconds or every 1 second should be enough.
This is also a good time to open a new PR on thd-controls to add the service config for the spectrometer there. You need that anyway to be able to test this catkit2 PR.
As for the tests that need to be done on hardware, since the service is very lean, there's only a couple of points:
- Make sure you can open the service and it appears correctly in the service state viewer of the GUI
- Make sure you can correctly update all properties, in this case the exposure time (set a new exposure time and then double-check it registered it by getting the exposure time right after)
- Make sure you can read the spectra from the data stream directly. You might want to turn the different lasers on and off (manually or through code once we connect them to the new machine) to confirm the spectra update correctly on the data stream.
- Once all of that works on hardware, create a documentation page for this service, fill it in (instructions at the bottom of Fill in all service doc pages #190) and don't forget to add the new doc page to
index.rst
- Create a new issue on thd-controls to create a spectrometer viewer; maybe you can already describe a bit what it should look like and what elements it should contain
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
The model of the spectrometer. | ||
pixels_number : int | ||
number of pix in the spectras | ||
wavelengths : NDArray[numpy.float_] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I am unfamiliar with this so I don't see a problem.
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
41cfadb
to
6ebbd76
Compare
2fb4dcb
to
c7754f3
Compare
3c49f76
to
076c0b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool to see this come alive, the only things left are some touch ups - and the documentation.
I totally forgot about the service docs until I went through the full PR thread again. @johanmazoyer before we can merge this, could you please add a new service doc page for the spectrometer service, fill it in (see instructions in #190), and add it to the doc index file.
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more tiny things and typos to address, mostly because I made you write new stuff in the previous review 😬 We also definitely need to add seabreeze to the env file, see comment below.
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer/oceanoptics_spectrometer.py
Outdated
Show resolved
Hide resolved
This service is a wrapper around the python seabreeze package. | ||
It provides a simple interface to control the spectrometer and acquire spectra. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to go since it does not use seabreeze. I'd just delete both lines.
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
catkit2/services/oceanoptics_spectrometer_sim/oceanoptics_spectrometer_sim.py
Outdated
Show resolved
Hide resolved
…is_saturating DataStream
a4ae9b3
to
f95d3c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go!
Official API exists in C by Ocean optics called seabreeze: https://www.oceaninsight.com/globalassets/catalog-blocks-and-images/software-downloads-installers/javadocs-api/seabreeze/html/index.html
Good python package wrapping this API seems to be python-seabreeze
https://pypi.org/project/seabreeze/
https://github.com/ap--/python-seabreeze
Address issue in ivalaginja/thd-controls#143