Avoid losing completed count when capture was suspended by a guiding deviation.
Changes PlannedPublic

Authored by TallFurryMan on Sep 9 2018, 12:18 AM.

Details

Summary

This change resets the sequence job that was aborted by a guiding deviation, but retains its count of completed captures.
It also avoids resetting all job when restarting after a guiding deviation.

Test Plan

Verify a sequence job recovering from a guiding deviation doesn't reset its capture count to zero.
Without this change, sequence job restarts captures from scratch.
With this change, sequence job continues captures from the count at which it aborted because of the guiding deviation.

Note that the test is relatively difficult to make with the Simulator.

Diff Detail

Repository
R321 KStars
Branch
bugfix__job_reset_after_capture_suspends (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2616
Build 2634: arc lint + arc unit
TallFurryMan created this revision.Sep 9 2018, 12:18 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptSep 9 2018, 12:18 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
TallFurryMan requested review of this revision.Sep 9 2018, 12:18 AM

Functionally OK, but it is difficult to read. Wouldn't it be better to add a flag to resetStatus() which indicates whether the count should be kept or not?

Yes! Adding a flag to resetStatus was my first change. It worked the same, but when I looked at the diff later on for code-review, it was clear I was adding an API change for a specific problem that could be fixed with the current API, so I decided against it.

If we find other locations in the code where it is interesting to reset the status without losing the completed count, then I can reconsider. One other location I found is when manually stopping/restarting a sequence initiated by the scheduler, but that use case is already difficult to handle.

wreissenberger accepted this revision.Sep 10 2018, 1:38 PM

The pros are that (a) the change is smaller and (b) there is no need to change an API. The cons are that (a) it makes the code more difficult to read and (b) we are talking of three occurrences, only inside of capture.cpp, where two are touched by this diff.

But in the end, it's a matter of taste. Feel free...

This revision is now accepted and ready to land.Sep 10 2018, 1:38 PM
TallFurryMan planned changes to this revision.Sep 10 2018, 1:52 PM

I'll rework this then, thanks!

Any update on this? is it still applicable to the current code base?

Sorry I'm overbooked, I can't return to this yet.