bug fix #400265: meridian flip check before capturing
ClosedPublic

Authored by wreissenberger on Nov 25 2018, 8:51 PM.

Details

Summary

Meridian flips are also initiated if a sequence has only one single frame captured. This resolves bug #400265

Test Plan
  1. Create a sequence with one single light frame and select "Meridian flip if HA > xx".
  2. Create a schedule with a target close before meridian using the sequence above that repeats the sequence endlessly.
  3. Start the schedule.

Expected result: as soon as the meridian is passed, a meridian flip is executed. After a successful meridian flip, capturing is continued.

Diff Detail

Branch
EKOS/meridian_flip
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5400
Build 5418: arc lint + arc unit
wreissenberger requested review of this revision.Nov 25 2018, 8:51 PM
wreissenberger created this revision.
wreissenberger retitled this revision from Remembering current target added to bug fix #400265: meridian flip check before capturing.Nov 25 2018, 9:01 PM
wreissenberger edited the summary of this revision. (Show Details)
wreissenberger edited the test plan for this revision. (Show Details)
TallFurryMan requested changes to this revision.Nov 26 2018, 6:52 AM
TallFurryMan added inline comments.
kstars/ekos/capture/capture.cpp
3010

Is there any action that needs to be taken here? Shouldn't this abort capture? I understand the decision is pushed to caller, but the management code is missing.

4048

Agreed, this is the core of the change.

4189–4190

Missing management of bool return code.

4675

Test against nullptr?

kstars/ekos/mount/mount.cpp
835

I'm not sure this is correct : hours are reduced to [0,24[, so you need proper sign management to handle rising/setting limit. But I didn't redo the maths. Took some hair in D16429...

840

Missing management of return code.

This revision now requires changes to proceed.Nov 26 2018, 6:52 AM
wreissenberger marked 3 inline comments as done.
wreissenberger edited the test plan for this revision. (Show Details)

Exception handling added

wreissenberger added inline comments.Nov 26 2018, 7:13 PM
kstars/ekos/capture/capture.cpp
3010

I tend to simply reply "false" in case of a DBus error, i.e. meridian flip has not been executed. This is not super elegant, I know, but from a DBus error I guess there is no option of recovery. And simply issuing a shutdown seems too harsh to me.

kstars/ekos/mount/mount.cpp
835

Well, maybe, but this is the original code. At least now it is more or less properly encapsulated. I tend to leave it as is.

Commit range corrected

TallFurryMan accepted this revision.Nov 26 2018, 8:34 PM
TallFurryMan added inline comments.
kstars/ekos/capture/capture.cpp
3010

If you're not aborting, what are the consequences? Either mount is lost, and we'd better stop the procedure, or mount will continue tracking. In that situation we'd better tell the caller, be it end-user of scheduler, that something cannot be done. Scheduler is able to restart the mount connection by itself now.

This revision is now accepted and ready to land.Nov 26 2018, 8:34 PM
mutlaqja requested changes to this revision.Dec 28 2018, 4:47 AM
mutlaqja added inline comments.
kstars/ekos/capture/capture.cpp
2997

Where is mountInterface defined?

kstars/ekos/mount/mount.h
45

READ should be just currentTarget like the rest

This revision now requires changes to proceed.Dec 28 2018, 4:47 AM
wreissenberger added inline comments.Dec 28 2018, 5:08 AM
kstars/ekos/capture/capture.cpp
2997

mountInterface is defined in registerNewModule() - see line 369. The strange thing is that it is not shown as changed. Seems like the created diff does not show all changes.

kstars/ekos/mount/mount.h
45

Good point, I will change it.

TallFurryMan added inline comments.Dec 28 2018, 2:30 PM
kstars/ekos/mount/mount.h
45

currentTargetPosition?

Meridian flip check executed before capture started (Update)

Restricted Application added a project: KDE Edu. · View Herald TranscriptDec 28 2018, 7:53 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
wreissenberger marked 3 inline comments as done.Dec 28 2018, 7:58 PM
wreissenberger added inline comments.
kstars/ekos/capture/capture.cpp
2997

There was one commit missing in the diff

mutlaqja requested changes to this revision.Dec 28 2018, 9:04 PM

Tested and it worked great. Few changes required.

kstars/ekos/capture/capture.cpp
66

Why are you initializing it here and then again in registerNewModule?

kstars/skyobjects/skypoint.cpp
940

ditto

kstars/skyobjects/skypoint.h
26

DBus is not used on KStars Lite, so #ifdef should be used

653

ditto

This revision now requires changes to proceed.Dec 28 2018, 9:04 PM
wreissenberger marked an inline comment as done.Dec 28 2018, 9:27 PM

OK, I will put the wrapping #ifdef in SkyPoint.h and SkyPoint.cpp.

kstars/ekos/capture/capture.cpp
66

I am not sure what happens, if the interface is created before the mount module has registered at the DBus. Therefore I capture both situations with this construct:

  • In case the mount module is created after the capture module, it triggers a call to registerNewModule and overwrites the first value.
  • If the mount module is create before the capture module, it should be registered at the DBus. In this case registerNewModule() will not be called.
mutlaqja added inline comments.Dec 29 2018, 5:03 AM
kstars/ekos/capture/capture.cpp
66

Ok, not sure if in this instance using delete(mountInterface) in registerNewModule is required to free memory? The class is a child of "this" but not sure what happens when another "new" is used without deleting the first one.

D-Bus excluded for KStars Lite

  • Memory handling for mountInterface corrected
  • Code cleanup, D-Bus method name harmonized
wreissenberger marked 4 inline comments as done.Dec 29 2018, 12:27 PM
mutlaqja accepted this revision.Dec 29 2018, 1:19 PM
This revision is now accepted and ready to land.Dec 29 2018, 1:19 PM
This revision was automatically updated to reflect the committed changes.

Just found today that this doesn't cover the case where Slew is initiated from from the sky map or elsewhere outside Mount

wreissenberger added a comment.EditedJan 5 2019, 6:37 AM

Ah, good point, that makes sense. Nevertheless, the current implementation relies on the fact that captures take place.

I am already working on a solution that covers all cases where the mount tracks an object and crosses the meridian.