Closed kdw503 closed 7 years ago
Yes, I already implemented.
From: Cody Greer notifications@github.com Sent: Thursday, December 15, 2016 4:58:33 PM To: HolyLab/Imagine Cc: Kim, Dae Woo; Author Subject: Re: [HolyLab/Imagine] Binning, Laser control, Save / Load configurations (#25)
@Cody-G commented on this pull request.
In Imagine/imagine.cpphttps://github.com/HolyLab/Imagine/pull/25#pullrequestreview-13240602:
@@ -77,7 +77,8 @@ int nUpdateImage; Imagine::Imagine(Camera cam, Positioner pos, Laser laser, Imagine mImagine, QWidget *parent, Qt::WindowFlags flags) : QMainWindow(parent, flags) {
- masterImagine = mImagine;
- m_OpenDialogLastDirectory = QDir::homePath();
Did you mean to include this commit in your pull request? As I understood the load/save configuration feature is not finished yet. Or is this ready for users?
- You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/HolyLab/Imagine/pull/25#pullrequestreview-13240602, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AV-GXnR7gqjff9hSSVbMr3_PQ3-QP80Iks5rIcYZgaJpZM4LMQAh.
Cody, I resolved all the white space issues and committed and pushed it successfully! Please check them. Thank you for the help. I really learn a lot about git today.
Great, everything looks cleaned up! I'm going to be working on the microscope today, so I will check out this branch and test it before doing the merge.
Okay I tested it out on the microscope and noticed a few issues:
Comments on ROI (Binning tab) feature:
It only increments by 160 when adjusting hor. start. Would be best to do this for hor. end too.
hor. start and hor. end both accept invalid values. For example, if you type "3" in hor. start and then click the up arrow, it will change to 163, which is invalid. (When trying to fix this you may notice that Qt is too aggressive with its corrections because it's re-evaluating the callback with every digit that you type. I've run into similar problems when trying to correct piezo parameters automatically. Maybe there's a way that we can delay the evaluation of the callback until the user changes the focus away from the entry box? Or we could just delay it until the users clicks "apply", but then we would want to warn them that we corrected the ROI parameters.)
(for future, don't need to fix for this pull request) The maximum accepted value for hor. end is 2560. This is actually leftover from the OCPI-1 camera (PCO Edge 5.5), which has a slightly larger sensor chip. The OCPI2 camera has a maximum at 2060. You can change the value by hand, but I think life will be easier when updating other microscopes if we do something more robust. Easiest may be to query the camera for the chip size and use that (see PCO SDK, and I think we already do this somewhere in our code). It's also true that the two cameras have different rules for the horizontal increment size (I think it's 120 on the Edge 5.5), so it would be good to make the code aware of the camera version somehow.
Comments on laser control feature:
Currently we can only control 4 lasers. The OCPI2 system actually has 5. We seem to be missing the 514nm laser.
Could we display the wavelength of the laser for the user instead of just the line number? I think there is a way to query the laser system for this information. Or, slightly less good would be too just enter them by hand into the ui. Again, the problem with that is that it won't work immediately on other microscopes.
Do the "open shutters" and "close shutters" buttons do anything now? They seem to be grayed-out whenever the lasers are in use.
Config file saving and loading seems to work great!
Thank you Cody. I will take care of these issues. About Open shutters/close shutters. I suggested 'laser setting mode'. We can enter 'laser setting mode' by checking 'Laser shutter and AOTF setup' check box in laser control tab. In this mode, 'Open shutters' and 'Close shutters' buttons are disabled. After setting the laser shutters and transmissions, we can exit the mode by unchecking it. Then, 'Close shutters' button is activated. And, when we close shutters, 'Open shutters' button is activated. Those laser settings are applied every time we open shutters with 'Open Shutters' button.
And about the chunk of code for the load/save configuration feature, those functions are used in void Imagine::writeSettings(QString file) void Imagine::readSettings(QString file) functions.
And, these functions are called by void Imagine::on_actionSave_Configuration_triggered() void Imagine::on_actionLoad_Configuration_triggered() , when we use 'Load configuration' and 'Save configuration' features.
I updated binning and laser control feature as you commented.
It only increments by 160 when adjusting hor. start. Would be best to do this for hor. end too.
hor. start and hor. end both accept invalid values. For example, if you type "3" in hor. start and then click the up arrow, it will change to 163, which is invalid.
(for future, don't need to fix for this pull request) The maximum accepted value for hor. end is 2560. This is actually leftover from the OCPI-1 camera (PCO Edge 5.5), which has a slightly larger sensor chip. The OCPI2 camera has a maximum at 2060. You can change the value by hand, but I think life will be easier when updating other microscopes if we do something more robust. Easiest may be to query the camera for the chip size and use that (see PCO SDK, and I think we already do this somewhere in our code).
I added these lines maxROIHSize = camera.getChipWidth(); maxROIVSize = camera.getChipHeight();
It's also true that the two cameras have different rules for the horizontal increment size (I think it's 120 on the Edge 5.5), so it would be good to make the code aware of the camera version somehow.
Could we display the wavelength of the laser for the user instead of just the line number? I think there is a way to query the laser system for this information. Or, slightly less good would be too just enter them by hand into the ui. Again, the problem with that is that it won't work immediately on other microscopes.
Looks like a great set of updates, thanks! Go ahead and merge when ready!
Also note that these changes address all of the following issues: #7, #12, #13, #23
Nice work! Go ahead and close those issues after you merge.
Great work folks! Sorry I got so far behind on my gmail I didn't even see this PR until just now. Thanks for reviewing, Cody, and of course for the great work, Dae Woo!
Change some binning control Hstart -> step size 160 Vstart -> if Vstart chaned Vend = 2049 - Vstart
Laser control
Save / Load configurations