- User Since
- Apr 13 2018, 9:37 PM (57 w, 5 d)
Mon, May 20
Do we create a feature branch for this, or do we use the trunk?
Fri, May 3
Thu, May 2
No more problem with reordering of jobs when the scheduler is running. The behavior is now coherent with the Capture module.
However, the end-user now has the same feedback when clicking (job characteristics are displayed) and double-clicking (job characteristics are edited).
But that's a better behavior I think (and it matches with the draft UI I'm preparing).
Wed, May 1
Oh sorry this requires my review. I'll have a look this morning.
Tue, Apr 30
I think we should merge this. I'll then separate scheduler.cpp into several file to try to clarify what functions are for. I'll also separate the interface from the implementation, so we don't have too many private stuff cluttering.
Rebased to 20190430 state.
Apr 11 2019
I'll double check this as soon as I can. Sorry for the delay.
I'm sorry my availability is really low these days. Hopefully "better" in the coming w16 as I'll be in holidays.
When a scheduler job aborts, it does not change the completed frame count. So probably an in-sequence focus failing might do the job?
Apr 10 2019
It's unclear, but maybe, yes. Could I offer my own implementation on the two fixes that are in this differential? I'd like to first fix the FindNextJob issue, then in another diff the frame counting via messages from the capture module.
Apr 7 2019
That's a good idea, but weeell I have two disagreements : first this is integer calculation and you probably need to reorder your operators, and second, if I understand correctly, you are considering the amount of captures to get equiprobable scattering over all sequence jobs.
I'll nonetheless test this asap.
Mar 27 2019
Situation is clear, thanks, and sorry for missing the point at first.
I propose we then divide SchedulerJob into SchedulerJobModel and SchedulerJob, deriving from SchedulerJobModel.
We can then move all the accessor stuff away into SchedulerJobModel, and focus on making SchedulerJob more a controller.
We should be able to tweak SchedulerJobModel to comply with the QT model interface perhaps.
I cannot disagree with this, this is what you proposed last year, and this is the approach I support!
Obviously we need to be very careful to not moving the complexity from one class to the other.
Oh no I'm sorry if I sounded too annoying to you, that wasn't my intent. Let me propose a preliminary differential too based on your work last year and with the same idea as this one. And then we both hit it until it has the shape of a sports car (well hopefully).
Mar 26 2019
I disagree with disabling the queue table. I think checking whether the scheduler is running when enabling or disabling the button that allows sorting, and duplicating the check in the function that does the actual sorting, is a better option.
I had the same reflection. Why -15 and not -90: I don't even know why it was 15 in the first place, so my change is the minus sign. It would take a high and isolated place to require a restriction under -15 degrees.
For 90 instead of 89.9 I think 90 is taboo in altaz/radec conversions because of the indefinite value of the azimuth. On the other hand, -90 is used for coherence with "no altitude restriction" in XML files. But agreed, there's no particular other justification.
Mar 25 2019
I agree with your points, but I still tend to think SchedulerJob should be able to produce a score by itself, answering "job, what's your score?".
SchedulerJob knows when it starts, knows the restrictions, and there's no added value in Scheduler for this purpose that I can see. Scheduler is just a tunnel providing global variables in most cases.
In the case of darkness, Scheduler needs the startup time of SchedulerJob. For dawn and dusk, when that will be fixed, Scheduler will still need the startup and completion time of SchedulerJob. Same again for Moon separation.
Now, I agree, altitude calculations need SchedulerJob times and non-Scheduler global variables, but also require a startup time that pertains to the previous job in the schedule. So, my opinion is obviously criticizable.
As a side note, scoring is only a can/cannot run flag in the end, and would better be completely removed at some point. That would spare us some hair.
Mar 24 2019
Agreed, this differential was here just to provide a code baseline. We've got more urgent stuff to do. Let's see if the forum thread gets traction.
I fully agree, but that means darkness and weather should go in ScheduleStrategy, and altitude score and moon separation score calculations should go to SchedulerJob.
Also, are you sure you need a separate class for now? Could you keep the code in Scheduler, but move it to a different source file?
That would prepare the codebase with zero risk of regression, and discuss what goes where. scheduler_strategy.cpp and scheduler_schedulerjob.cpp?
OK mostly, I'd rather have calculation pertaining to SchedulerJob relocated in SchedulerJob. "Strategy" would mean we have multiple ways of scheduling, which is not really the case today. We have "Restrictions" that schedules must comply with, and we have calculations leading to a score. I'm not a fan of that "score" value, but it pertains to the SchedulerJob, it should be the one owning the calculation. I think your previous work in moving stuff there was more interesting (but I can't find it anywhere...).
Let me add an additional comment later on, I'm a bit busy now.
@mutlaqja If you want to mix lights with calibration, I wonder if that is really clearer that you need to remember the progress of jobs in that case. In that situation the scheduler will be able to determine what's left to do, and not redo lights if only calibration remain.
If not remembering, the minimal granularity of the work to do is the full job, which contains in your case lights and calibration.
This may push users to run lights and calibration frames as separate jobs just to be sure on what happens.
But in both cases starting the execution will still schedule the job for capture as if there were lights. There are two sides to this: scheduling and running. This differential is lacking work on scheduling, agreed.
You get a good point. Also a scheduler job with only calibration frames should issue a certain amount of warnings: twilight, altitude, etc. The current UI is not prepared for this...
Mar 23 2019
Mar 17 2019
That was a "we" for both authors, but the incentive is of course for Jasem to merge if he accepts :)
Functionality tested OK.
This is a change of UI behavior: now clicking once on a job does sync the left fields with the properties of the selected job, as mentioned in the changeset.
However, it makes the end-user think the job can be modified in-place, while there is actually no effect. Counter-however, the change makes adding a new instance of an existing job easier.
I will say the change makes the UI slightly better in terms of usability, but the ergonomy is still flawed.
I observe that arrow buttons are always enabled even if the selected row is first or last while scheduler has this proper (but not always), but this is completely minor. The code doing that should probably be refactored somewhere.
Mar 16 2019
I think what Jasem means is that tab modules may work with slots and signals internally, but should communicate between each other with dbus. Just as if they were independent processes, possibly distributed over different machines.
Mar 10 2019
Mar 9 2019
Ah yes I forgot to comment on this. I think the feature is important because for now we cannot estimate properly how long preliminary steps will take. So that's one more of them for the altitude. It was there before, I don't remove it and I make it customizable, as I will for others. This said, I fully agree it is quite hidden for now. I want to rework the UI of the Scheduler to better manage all these parameters and I will take this into account. But before that, I will follow up on your attempts to divide the code of the Scheduler because frankly that file is horrible :)
Mar 8 2019
D19528 is needed, and is independent of D19393. D19528 does fix a guiding recovery issue, but relies on D19393 to fix a problem with aborted jobs being rescheduled and pushing following jobs away in that situation.
The current discussion on D19393 is about whether all jobs should be left in the original schedule order or completed/errored/invalid jobs should be moved at the end. The current patch implements the second item, but should still be tested. I can do the first item as a later improvement over the original 3.0 design.
I'd say D19528 and D19393 should be merged at the same time in that order. In terms of merge, however, they are independent and should not cause conflicts.
Mar 7 2019
Updated the patch to properly handle aborted jobs.
Now, we can discuss whether it makes sense to reorder completed/invalid/error jobs.
The altitude cutoff, while confusing, remains important to avoid focusing/aligning and finding the job under the restriction when starting capture.
When Scheduler assigns a startup date to the job, it considers the altitude restriction and adds the altitude cutoff, then checks if the job has time to run.
On the opposite, when Scheduler is executing the job, it only aborts it when the altitude restriction is crossed. The altitude cutoff is not considered.
Because that's confusing to the end-user, I first provided the customization of that cutoff as an option. That's not extremely clearer that way, but at least a simpler test can be run by setting the cutoff to zero.
Mar 6 2019
Thanks, I'll start from here.
It seems the situation is still better than before with your patch. I should be able to push a diff tonight, based on yours. I can rework the part where you bypass the aborted job, and eventually rebase if you change your diff. No problem.
Oh wow, so you investigated that too? I have the same observation about aborted jobs! Did you have a look at D19393? Coincidence :)
Nope. Maybe you should add me as a reviewer? :-)
That patch was unfinished, but my obs setup accepts a list of differentials to apply when updating itself so I pushed it without reviewers. I like the fact that we both found the same issue at that location :)
Mar 5 2019
Oh wow, so you investigated that too? I have the same observation about aborted jobs! Did you have a look at D19393? Coincidence :)
Mar 1 2019
Looks good except for the unrelated changes.
Feb 28 2019
Good point! Would you care to make the whole line selected too in an additional diff? Right now the selection is per cell, which is confusing.
Feb 27 2019
I upload this commit as first step to fixing the issue. Work is still needed to reschedule the aborted jobs when all runnable jobs are complete.
Feb 2 2019
Congratulations to all of you, this really is an improvement to the platform.
Jan 31 2019
D18627 requires attention. Is it compatible/complementary/unnecessary with this diff?
Jan 25 2019
Jan 24 2019
Cristal clear, thanks.
Jan 23 2019
There's something I'd like clarified.
First, if the mount is set east of the meridian at an original position, and is not tracking, and we wait long enough that if it was tracking, it would flip. What happens if we then start tracking from that eastern position? Do we get an unneeded and ineffective flip request?
Second, if a capture overlaps the meridian flip, I don't readily understand whether the capture is aborted, the flip executed, and the capture restarted, or if the capture continues and delays the flip, potentially for very long. This situation could be pre-planned by Scheduler.
I'm sorry, I couldn't test all this. But this seems really good.
Jan 16 2019
Jan 14 2019
That's great, agreed. I added a few (as usual) pedantic comments. My setup is out of order while I install the fixed obs, so I don't know when I will be able to test.
Jan 7 2019
Dec 28 2018
Dec 19 2018
Dec 7 2018
- Minor grammar typo.
- Fix regression on current job status.
Dec 3 2018
Dec 2 2018
Complete list of changes. Job selection highlight is now under control.
Nov 28 2018
OK the index concept of currentRow and the index concept of selectRow are different. I will move to a custom QModelIndex as soon as possible.
@wreissenberger Thanks your patch! I observed that when sorting jobs per altitude manually and sorting is not expected to change the order, the selection moves up by itself one row at a time. I'll check this.
Nov 26 2018
In the case of floating point, either QString("%L1").arg() or ki18n().subs() should be used, and HFR is a float (no i18n with args here).
But I didn't investigate whether, while the QString method systematically used the system locale, the ki18n could handle a locale switch in the application without system locale changes.
You should use QString("%L1").arg(variable, width, 'f', précision) to properly format floating point values using the decimal point of the locale
Nov 22 2018
OK that's clear :) thanks for taking the time to check this!
Go ahead and post, I can integrate it to this differential.
Nov 21 2018
Nov 20 2018
Issue found: when changing the start-up constraint from asap to culmination time, and there are two jobs in the list, the second job is overwritten with the first job contents.
Investigation in progress to determine the root cause.
Nov 18 2018
Nov 17 2018
This went out of my list, I'll check this. I think the main point - the dome - was merged.
Nov 15 2018
Progressing on use cases at https://github.com/TallFurryMan/kstars/wiki.
Nov 12 2018
Nov 10 2018
Differential changes are now complete.
Please start testing now while I write the changeset documentation and describe integration tests!
Nov 9 2018
Still WIP, not entirely rebased, but shows were the rewrite is at.
UI is probably prettier than it was before, but that won't help the first Ekos tab.
Altitude sorting is clarified with the UI rework, but clicking the sort button is not stable.
Row selection is still to be fixed.
All schedule consolidation issues are now fixed!
I'm nearly finished, still quite a bit of testing needed.
Nov 3 2018
Still WIP, need to rebase, fix a few logs and add altitude as a column.
- Rewrote algorithm documentation.
- Fixes from Phabricator comments.
Nov 2 2018
This is still a WIP. Posting advancements to share progress.
Oct 28 2018
I also plan to add altitude for startup time, with an icon showing whether the target is rising or setting.
This weekend I found out that most calculations in the scheduler were using the current date instead of the job date (which could be the next day, thus offset with ~15min), and that sometimes local to universal conversions were using the time zone of the system running KStars, instead of the geographic location of the observatory. But trying for instance to calculate the culmination of a target at a future date is somewhat tricky with the current code.
Thanks for this report!
- Yes, agreed, the way the QWidgetTable works for selection is implemented and documented in such a contrived way that it appears to be bugged. SelectRow() should be used, but never results in the right effect. There are apparently several follow-up signals that make a mess with the rendering...
- I agree also there, the behavior of the sort option is opaque. Well, for the altitude at least. It works well only when the targets are all asap, and all rising. I plan to change the order predicate so that prioritary targets are the one setting, not the highest ones. Moreover, there's no point in sorting both in priority and altitude probably. Except for infinitely looping jobs, priority is a weird feature now...