Differential D15937

Do not alter the state of the Scheduler while evaluating
ClosedPublic
Actions

Authored by TallFurryMan on Oct 4 2018, 7:08 AM.

Details

Reviewers
mutlaqja
wreissenberger
Commits
R321:e02f376f533a: Do not alter the state of the Scheduler while evaluating
Summary

This differential does a few rewrites, removes a few logs from the console and essentially removes the shutdown decision from the evaluation algorithm.

  • Avoided multiple evaluations while loading a schedule, introduced a new Scheduler state for this purpose.
  • Prevented moving jobs in the list when option "Sort jobs by Altitude and Priority" is enabled, added tips.
  • Reduced amount of logs, fix date display, fix some logs.
  • Avoided evaluating a repeating job that just completed, shorted to completion.
  • Rewrote, clarified and added fixmes and asserts in the final part of evaluation, sequencing jobs (no change).
  • Moved decision to shutdown from the evaluation function off to the status check function.
Test Plan

Verify this differential brings no change of behavior with the all scheduler test vectors.
Observe the console reduced output as most technical logs were moved to the file journal.
Observe the console as loading a dozen schedule jobs does not result in dozens of evaluations.
Observe move buttons following the state of option "Sort jobs by Altitude and Priority".

Diff Detail

Repository
R321 KStars
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
TallFurryMan requested review of this revision.Oct 4 2018, 7:08 AM
TallFurryMan created this revision.
TallFurryMan updated this revision to Diff 42836.Oct 4 2018, 7:21 AM

Rewrote cast to double with static_cast.

TallFurryMan added inline comments.Oct 4 2018, 7:27 AM
kstars/ekos/scheduler/scheduler.cpp
1710

This sort block is really only a safeguard. No jobs are changing states while rescheduling, so the list cannot get empty and crash the next call to .first() at line 1714.
It's here to prepare that rescheduling part to properly account for fixed-time jobs and support intervals of observatory availability.

1868

This was replaced to hms because the job display format contains date information, and transit time is only hours.

1880

The cast to double avoids a C++ warning, apparently arg() is ambiguous at times.

1895

This assert is there to catch an issue in which sometimes a job is scheduled without startup time. Haven't reproduced recently.

2130

I inverted the order because most of the time, the end-user will be preparing the schedule *before* dusk.
In any case, because this order cannot be right every time, I plan to replace this log with either a text label or the same rise/transit/set as in the session planner, with all planned jobs and observatory availability summarized in.

TallFurryMan added a comment.Oct 4 2018, 7:28 AM

I also plan to reorder the jobs automatically directly in the queue table when "Sort jobs by Altitude and Priority" is enabled, to provide a clearer feedback to the end-user.

TallFurryMan added a comment.Oct 10 2018, 5:43 AM

Could someone test and merge this one? Or does it have issues applying?

mutlaqja added a comment.Oct 10 2018, 9:45 AM

I'll give it a go now.

mutlaqja accepted this revision.Oct 10 2018, 10:01 AM
This revision is now accepted and ready to land.Oct 10 2018, 10:01 AM
Closed by commit R321:e02f376f533a: Do not alter the state of the Scheduler while evaluating (authored by TallFurryMan, committed by mutlaqja). · Explain WhyOct 10 2018, 10:01 AM
This revision was automatically updated to reflect the committed changes.
wreissenberger added a comment.Oct 10 2018, 10:34 AM

Sorry for the late response, looks good. One tiny thing: when changing list positions, evaluate... is multiply called. In all other situations I tested, it’s called only once.

TallFurryMan added a comment.Oct 10 2018, 11:19 AM

Yes, when jobs are not sorted per priority/altitude, re-evaluating is necessary each time a job is moved. When that option is enabled, you can't move jobs anymore. I will push an additional differential to actually sort the visible list automatically when "Sort jobs by Altitude and Priority" is enabled for better user feedback.

TallFurryMan added inline comments.Oct 12 2018, 8:19 PM
kstars/ekos/scheduler/scheduler.cpp
692

Regression: this creates a link between job sorting and delete button being disabled.

838

Regression: this creates a link between job sorting and delete button being disabled.

895

Regression: this creates a link between job sorting and delete button being disabled.

928

Regression: this creates a link between job sorting and delete button being disabled.

Revision Contents
Changeset List

PathPackages
Mkstars/ekos/ekos.h (13 lines)
Mkstars/ekos/scheduler/scheduler.cpp (156 lines)

Diff 43285

View Options

kstars/ekos/ekos.h

Loading...
View Options

kstars/ekos/scheduler/scheduler.cpp

Loading...