search

HolyLab / Imagine

A graphical interface for recording with OCPI microscopes.
1 stars 1 forks source link

Roadmap to merging the waveform branch #36

Closed Cody-G closed 5 years ago

Cody-G commented 7 years ago

Not necessary for merging, but let's do this anyway:

kdw503 commented 7 years ago

Process analog entries in the .json command file as raw values. With our DAQs this means that they should be UInt16 values (0...2^16-1). These values will then be scaled by Imagine to a 0-10V range before writing them to the analog output channels.

Now, Imagine set up analog output channels as -10 ~ 10. So, we need to change this to 0 ~ 10V. This means we only provide 0 ~ 10V range to user even non-secured AO channels also.

If we do provide different voltage range for every AO channels, we need to create every analog output task individually and even callback functions also.(this might cause another burden to control DAQ)

How about using -10V ~ 10V range for every AO channels and using raw value as int16 instead of uint16 if we need to provide minus voltage range? In this case, maximum raw value will be 2^15-1 and minimum value will be -2^15.

Improve handling of redundant GUI entries when waveform mode is enabled. We thought about graying-out these entries.

How about graying-out 'Camera', 'Stimuli' and 'Positioner' tab entirely?

waveform

camera

positioner

However I see one potential problem with this. When positioning the sample, users typically run "live" mode before doing a recording. They may also run live mode between recordings. Live mode takes its exposure time parameter from the GUI. I think we should continue to allow setting the exposure time for live mode from the GUI, and that should remain independent from the .json exposure time.

In this case, user might be confused. This seems to be decided with exposer trigger mode selection we talked yesterday.

How about this idea?

  1. Add 'exposure trigger mode' selection combobox to waveform tab. This value is used only when waveform control is enabled (checkbox located in left top corner in 'Waveform'tab). For ocpi-2, we set 'exposure trigger mode' as the value of master window when waveform control is enabled. Or, the exposure should be configurable separately for the two cameras?
  2. 'live' mode uses a exposure value in Camera tab when waveform enable check box is disabled. When this check box is enabled, it uses a exposure value in Waveform tab.
  3. When waveform is enabled, the 'exposure trigger mode' selection combobox works like this:
    • 'External start' is selected: 'Exposure time (sec)' spinbox is editable and this value is used for both 'live' mode and 'recording' mode.
    • 'External control' is selected: Exposure time (sec)' spinbox is grayed-out and recalls the original exposure value in command file. In this case, 'live' mode uses this value as a exposure value. ('recording' mode will just follow waveform file) ps. I plan to gray-out other metadata (Samples per second, Sample num, stack, frame/stack, bi-direction)
Cody-G commented 7 years ago

Now, Imagine set up analog output channels as -10 ~ 10.

Oh I didn't realize we were doing this! I support the idea of using the same voltage limits on all channels. I just thought we were already using 0-10V. The advantage of using 0-10V is that we'll get better precision in the analog output values. This article explains why. But in this case it may be insignificant since it's only cutting the precision in half. Overall I like the idea of configuring all AO channels as -10 to 10V. This will require one extra safety check: piezo voltage commands should never be negative because it could damage the device. I can enforce that requirement in Julia. Can you do it in Imagine?

Cody-G commented 7 years ago

How about this idea? Add 'exposure trigger mode' selection combobox to waveform tab. This value is used only when waveform control is enabled (checkbox located in left top corner in 'Waveform'tab). For ocpi-2, we set 'exposure trigger mode' as the value of master window when waveform control is enabled. Or, the exposure should be configurable separately for the two cameras? 'live' mode uses a exposure value in Camera tab when waveform enable check box is disabled. When this check box is enabled, it uses a exposure value in Waveform tab. When waveform is enabled, the 'exposure trigger mode' selection combobox works like this: 'External start' is selected: 'Exposure time (sec)' spinbox is editable and this value is used for both 'live' mode and 'recording' mode. 'External control' is selected: Exposure time (sec)' spinbox is grayed-out and recalls the original exposure value in command file. In this case, 'live' mode uses this value as a exposure value. ('recording' mode will just follow waveform file) ps. I plan to gray-out other metadata (Samples per second, Sample num, stack, frame/stack, bi-direction)

This is an excellent solution, better than what I proposed! :-)

Cody-G commented 7 years ago

Also note that I added another item to the todo list, and your solution basically covers that. We should still make sure we're using the "fast" setting when calling PCO_SetTriggerMode, both for external start and external control modes.

kdw503 commented 7 years ago

This will require one extra safety check: piezo voltage commands should never be negative because it could damage the device. I can enforce that requirement in Julia. Can you do it in Imagine?

Sure, no problem.

We should still make sure we're using the "fast" setting when calling PCO_SetTriggerMode, both for external start and external control modes.

You mean 'Acquisition trigger mode' as 'Auto'?

Cody-G commented 7 years ago

No this is unrelated to the acquisition trigger mode. I think it will be clear if you check the SDK manual entry for that function.

On Jun 15, 2017 5:30 PM, "Dae Woo Kim" notifications@github.com wrote:

This will require one extra safety check: piezo voltage commands should never be negative because it could damage the device. I can enforce that requirement in Julia. Can you do it in Imagine?

Sure, no problem.

We should still make sure we're using the "fast" setting when calling PCO_SetTriggerMode, both for external start and external control modes.

You mean 'Acquisition trigger mode' as 'Auto'?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HolyLab/Imagine/issues/36#issuecomment-308883055, or mute the thread https://github.com/notifications/unsubscribe-auth/AE78hKn_1a0_wyhkGblcNpxg987w90wkks5sEbA_gaJpZM4N7maz .

kdw503 commented 7 years ago

No this is unrelated to the acquisition trigger mode. I think it will be clear if you check the SDK manual entry for that function.

Well.., I read SDK manual but I only found that

kdw503 commented 7 years ago

I had an old version of SDK manual. Now, I found what you mentioned in the latest SDK manual. And, I also realized that our Imagine code now using only above 4 modes

if (expTriggerMode == eAuto) {
    errorCode = PCO_SetTriggerMode(hCamera, 0); //0: auto, camera is in charge of timing
}
else if (expTriggerMode == eSoftwareTrigger) {
    errorCode = PCO_SetTriggerMode(hCamera, 1); //1: trigger only by a "force trigger" command, good for single images
}
else if (expTriggerMode == eExternalStart) {
    errorCode = PCO_SetTriggerMode(hCamera, 2); //2: external start via SMA input #1, uses pre-set exposure time
            //Busy status at SMA #3 determines if new trigger will be accepted
            //(HIGH signal means busy).  LOW is < 0.8V and HIGH is > 2.0V
}
else {
    errorCode = PCO_SetTriggerMode(hCamera, 3); //3: external exposure control, pulse length dictates exposure
            //HiGH means expose.  Busy status is available as above.
            //10s is maximum exposure time
}

So, you mean we need to change this mode to 'fast' one.

Cody-G commented 7 years ago

I had an old version of SDK manual. Now, I found what you mentioned in the latest SDK manual.

Yes this is confusing. It's possible that we need to update the SDK before we can use the "fast" mode. But yes, I think we may benefit from using that one (set to 5) instead of setting it to 3.

kdw503 commented 7 years ago

It's possible that we need to update the SDK before we can use the "fast" mode.

So, we need to update SDK. And, the manual said the trigger timing should be accurate in this mode.

"Fast External Exposure Control : ... This increases the frame rate, but leads to destructive images, if the trigger timing is not accurate: internal camera error correction is inactive in this mode"

We need some tests for this mode. How about applying this mode to next pull request? Before that, we just use our previous normal 'External Control' mode?

Cody-G commented 7 years ago

Yes, so I think this means that if exposure pulses are delivered too fast then the images will be corrupted. In our current mode, if the pulses are delivered too fast then they are simply ignored (so we end up with too few images). In either case we would prefer to make sure that the pulse rate isn't too high. For now I think it is okay to make that the responsibility of users (But I do have some helper functions in the Julia code to give camera-specific info).

On Jun 16, 2017 4:16 PM, "Dae Woo Kim" notifications@github.com wrote:

It's possible that we need to update the SDK before we can use the "fast" mode.

So, we need to update SDK. And, the manual said the trigger timing should be accurate in this mode.

"Fast External Exposure Control : ... This increases the frame rate, but leads to destructive images, if the trigger timing is not accurate: internal camera error correction is inactive in this mode"

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HolyLab/Imagine/issues/36#issuecomment-309136077, or mute the thread https://github.com/notifications/unsubscribe-auth/AE78hGLR3K_uFKpziPDgIuhbMIYxtBQnks5sEvCMgaJpZM4N7maz .

Cody-G commented 7 years ago

@kdw503 would you be okay with merging this soon? I'm testing it on OCPI2 this week. If I don't find any serious problems then as far as I'm concerned we can merge. If you have more changes locally please submit them so I can test the most recent version.

kdw503 commented 7 years ago

@Cody-G I just submitted my recent version.

Cody-G commented 5 years ago

As far as I'm concerned we can now merge waveform to master and delete waveform. Maybe we can also bring dev up-to-date with master and all new development can occur on dev @kdw503 it's up to you how you want to manage that. I'm happy to do the merging if it's okay with you. I think it's also time to update the README (#69) and tag a new version, "Imagine 2.0". I can also do that if you can review my PR for the README update.

kdw503 commented 5 years ago

I'm also okay with this merging. And I'll check the PR of the README update.

Cody-G commented 5 years ago

I tried to rebase, but there is a commit on master that is missing from the waveform branch:

https://github.com/HolyLab/Imagine/commit/267e7ab67247e9d99fc9ee705172ce2abd40028b

I'm not sure how that happened. I could try to carefully manage the merge conflicts so that we can have a clean PR, but that's a lot of work and a bit risky when I don't have a machine to test it on. The commit is from a long time ago (April 2017) and is related to outdated code (before we switched to hardware timing for all signals). I'm inclined to instead do a force push of the waveform branch to master. I know it's not good development practice, but it may make sense in this case. We've been using waveform as if it were master for quite some time now. @timholy is this okay with you?

Cody-G commented 5 years ago

I went ahead and force-pushed. I doubt we'll need it, but I kept the old master branch as oldmaster. We can now delete the waveform branch as far as I'm concerned. @kdw503 I don't want to mess up your workflow by deleting it right away, but delete it when you want. Glad to see this merged!