Bug 397650

Thanks for reporting this. First, do you think could seqTotalCount be dropped completely and replaced with activeJob->getCount() ? Variable "seqTotalCount" is a shortcut counter that is difficult to track.

If the frame type is FLAT, the code should probably be self-contained in terms of calibration. It should prevent the increase of the capture count if the ADU tolerance is not met, and only that. Instead it seems to rely on isPreview which has multiple meanings and can be set outside of the calibration feature.

Besides, I also see that the ADU check is done at two locations, when the ADU value is saturated, and when the new exposure is calculated.

There are also a few forum threads raising issues with the stability of the ADU calculation. I believe that the ADU polyfit algorithm should limit the number of samples it is working with to the N latest ones. This is even more true when running dawn/dusk flats, for which older ADUs quickly become irrelevant. But that is another topic perhaps.

I would also prefer replacing seqTotalCount by activeJob->getCount(), since caching the value always bears the risk that the value is not up to date. By the way, seqTotalCount is no longer used, only present in capture.h.

The algorithm and its state for shooting flats is currently quite spread over the entire class, which is already a huge one. As a first step I would recommend encapsulating it inside capture.cpp. In a future step, it might make more sense having a class FlatSequenceJob derived from SequenceJob holding all the specifics for the different types of images.

As a hotfix, I would replace 
  if (activeJob->isPreview()) 
by
  if (activeJob->getCount() <= 0)

I agree with the hotfix.

Could you just move the ADU-related code to separate functions? Do not change the structure of the algorithm, only refactor those snippets into several functions so that we may move those away if needed. Agreed, that will separate code blocks called from only one location, temporarily.

Although I just said "could you", I'll take and remove this one from Jasem's list. If you want to proceed with changes, just take over, no issues :)

I could take over this point, sure. Just give me a hint whether you started already. It does not get better when it is done twice :-)

I did nothing more than analyze the code and provide my opinion, you can go ahead.

OK, I'll take over.

The hot fix is submitted as #D14977.

Further refactoring is not so easy :( 

There are in essence one where flats handling is extensively intertwined: processPreCaptureCalibrationStage()

But I do not dare to modify since I do not have the equipment to test (dust cap, dome, light cap etc).

Any thoughts?

Understood, the hotfix is enough for now I suppose.
I need more time to test, I was delayed by the robustness tests.

The hotfix based on getCount() solved the problem, but now the preview mode is cycling infinitely. We need a different approach to separate a preview initiated by pressing the "Preview" button from previews created for calibration.

Next attempt will be using the calibrationStage attribute.

As a hotfix OK, but we need to separate the flats calibration procedure in a better way. Currently, the attribute calibrationStage is also set outside a calibration process. This makes it difficult to understand the code. But it is not so easy...

The entire control flow of Capture is very tricky. Here some examples:

- Preparing CCD temperature, filter, rotator, gain, ... is controlled by SequenceJob.
- Setting up the equipment - dust cap, flat field source, parking mount and dome, ... is controlled by Capture.
- CalibrationStage contains both equipment setup states as well as informations about the calibration process.
- The calibration process is implicitly contained in the capturing - especially in setCaptureComplete() and processPostCaptureCalibrationStage().

As a first restructuring step, I want to extract the calibration process into dedicated methods. Does that make sense?

Capture controls parking mount and dome? Wow.

Your restructuring steps make sense to me, but I don't understand whether you found a fix for the flat field calibration or you still see the looping preview.

Yepp, parking mount and dome can be controlled through the calibration dialog.

Flat field calibration can be fixed with D14977. The only problem is that my posted diff contains more than my correction of capture.cpp.

Btw.: is there a calibration for darks and bias frames? Currently, the button is active for all frame types except for light frames. From my perspective, this does not make much sense.

I am still searching for the best way to restructure it. Maybe a very helpful first step is simply changing the method names to be more meaningful:

processPreCaptureCalibrationStage() --> prepareEquipment()
This method prepares the equipment for capturing the frames. For lights, it opens the dust cap and turns off the light. For others, it closes the dust cap, parks the scope etc.

processPostCaptureCalibrationStage() --> verifyCalibration()
This method more or less checks for flats, whether the ADU of the current image is in the expected range. If yes, it changes turns the preview mode off and starts the next capture. Otherwise, it determines a new exposure time and generates a new preview.

Does this make sense?

Git commit 85bf46276f5c10afcc18cbfe2356a22bf3e18eca by Jasem Mutlaq, on behalf of Wolfgang Reissenberger.
Committed on 28/08/2018 at 07:50.
Pushed by mutlaqja into branch 'master'.

Bugfix for #397650 flat creation failed

Summary:
After rework of the capture module creation flats creation works again.

Test Plan:
Create a sequence with flats and set the calibration to a certain ADU value
to force calibration. Flats should be created with the defined ADU value
within the given range.
A new test for testing flats creation has been aded to Tests/scheduler. Run
this test and check whether three new flats have been created.

Reviewers: TallFurryMan, mutlaqja

Reviewed By: TallFurryMan, mutlaqja

Subscribers: kde-edu

Tags: #kde_edu

Differential Revision: https://phabricator.kde.org/D14977

A  +50   -0    Tests/scheduler/create_flats.esq
M  +4    -3    kstars/ekos/capture/capture.cpp

https://commits.kde.org/kstars/85bf46276f5c10afcc18cbfe2356a22bf3e18eca