×

INDI Library v2.0.7 is Released (01 Apr 2024)

Bi-monthly release with minor bug fixes and improvements

Partitioning the Scheduler code

  • Posts: 1187
  • Thank you received: 370
Hi Eric,
no need to say that I appreciate separating the 7000+ lines megalith - we both struggled too often with fixes that had unexpected side effects :ohmy:

The split you propose makes absolute sense:
  • Separating a runtime from the scheduling calculations (maybe we call the latter SchedulingStrategy instead of "plan")
  • Having a separate UI controller that handles all the UI interactions - this is especially necessary for testability!
  • Maybe the Scheduler itself is the runtime, also holding the logic for observatory management
  • Whether we need a separate serializer - not sure, but does not hurt.
Here some additional ideas:
  • Would it make sense having a Schedule object as a container for the SchedulerJobs that represents the current state of the Scheduler?
  • Where do we place all the astronomical calculations like dawn/dusk etc? I think its place should be inside SchedulingStrategy.
In terms of migration strategy: I'm not sure whether it makes sense to refactor the existing Scheduler. This bears the risk that we create regression problems over several months of refactoring.

What about if we create an entirely new one in parallel - using the current Scheduler more in terms of a code/ideas quarry. This way may do bolder steps without always fearing that we might hit the KStars community.

- Wolfgang
The following user(s) said Thank You: Derek, Eric
4 years 11 months ago #38767

Please Log in or Create an account to join the conversation.

  • Posts: 456
  • Thank you received: 76
Before the refactor, would it be possible to add back the feature where the scheduler monitored the weather and triggered the shutdown on alert?
4 years 11 months ago #38832

Please Log in or Create an account to join the conversation.

  • Posts: 554
  • Thank you received: 138
I'm in two minds about a rewrite.
One one had the coding gurus seem to reckon that a major refactor will be less work.
On the other hand starting from scratch will help avoiding inheriting structural deficiencies.
On the gripping hand you are doing it so do what seems best to you.

I remember a program, a primitive word processor, written in DG assembler, which we all wanted to start again with but every time the choice was a short and painful struggle with a heap of assembler or spending months starting again. And every time the short painful struggle won, even though we knew that in the long term the rewrite would have been preferable. The problem was solved by abandoning the platform and moving to Windows where the word processor was someone else's problem.

As for the safety monitor - there's more than weather involved - I would have that as a separate thing that managed making the observatory safe and reported that it had done this to the scheduler. I wouldn't build safety critical functionality into a complex scheduler, ideally I'd run it totally independently of everything else.
4 years 11 months ago #38835

Please Log in or Create an account to join the conversation.

  • Posts: 85
  • Thank you received: 40
The length of the scheduler.cpp file alone implies it needs to be broken in pieces.
The class has 98 member functions and is 7310 lines long :pinch:

I'd like to see a sorting of each of these member functions into the proposed new classes, like this :
"scheduler.cpp", which would contain observatory startup and shutdown
 
    Scheduler::Scheduler()
    QString Scheduler::getCurrentJobName()
    void Scheduler::toggleScheduler()
    void Scheduler::stop()
    void Scheduler::start()
    void Scheduler::pause()
    void Scheduler::wakeUpScheduler()
    bool Scheduler::checkEkosState()
    bool Scheduler::isINDIConnected()
    bool Scheduler::checkINDIState()
    bool Scheduler::checkStartupState()
    bool Scheduler::checkShutdownState()
    bool Scheduler::checkParkWaitState()
    bool Scheduler::manageConnectionLoss()
    void Scheduler::disconnectINDI()
    void Scheduler::stopEkos()
    void Scheduler::parkMount()
    void Scheduler::unParkMount()
    void Scheduler::checkMountParkingStatus()
    bool Scheduler::isMountParked()
    void Scheduler::parkDome()
    void Scheduler::unParkDome()
    void Scheduler::checkDomeParkingStatus()
    bool Scheduler::isDomeParked()
    void Scheduler::parkCap()
    void Scheduler::unParkCap()
    void Scheduler::checkCapParkingStatus()
    void Scheduler::checkStartupProcedure()
    void Scheduler::runStartupProcedure()
    void Scheduler::checkShutdownProcedure()
    void Scheduler::runShutdownProcedure()
 
"scheduler_execjob.cpp", which would contain job runtime execution code
    ...
"scheduler_plan.cpp", which would contain calculations scheduling jobs
    ...
"scheduler_serialize.cpp", which would contain job and sequence loading and saving to disk
    ...
"scheduler_ui.cpp", which would contain UI manipulations
    ...
"schedulerjob.cpp"
    ...
"mosaic.cpp"
    ...
With that we could migrate piece-by-piece while making clear interfaces.
Also as sterne-jaeger said we could indeed create the new scheduler next to the existing one, users can then select one or the other during this migration.
And about 'Having a separate UI controller that handles all the UI interactions' is a good idea because that allows for other controllers (DBus anyone ? Or a nice REST API ?) as well as testing.

@dokeeffe I looked briefly at adding back the shutdown-on-bad-weather functionality, but decided controlling this from the outside was faster to achieve by writing ekos_sentinel.py which should remain useful in the future as fall-back safety mechanism for when the scheduler does have the needed features itself

@ChrisRowland: I agree with the safety monitor remark. Have a look at indilib.org/forum/ekos/5065-full-automat....html?start=12#38799 for an external process.

-- Hans
The following user(s) said Thank You: Eric
4 years 11 months ago #38836

Please Log in or Create an account to join the conversation.

  • Posts: 456
  • Thank you received: 76
A fresh start could be the best way to go alright. I'm not a cpp developer so I'm not an expert. It would be great to somehow work on the testing side from the start, enable TDD and the best design will then emerge from the TDD process. I've seen this produce better built software than big up front designs.

Could I also suggest that the 'weather' tickbox be hidden/removed from the current UI to prevent any misunderstanding? I'm really lucky it didn't rain when I discovered the function of this checkbox changed from monitoring weather to an initial check only. If not then maybe some help text next to it or something.

Derek
4 years 11 months ago #38843

Please Log in or Create an account to join the conversation.

  • Posts: 1187
  • Thank you received: 370
OK, that should be easy. I'll remove it and submit a diff.

- Wolfgang
4 years 11 months ago #38854

Please Log in or Create an account to join the conversation.

  • Posts: 1187
  • Thank you received: 370
Maybe we do not need to remove it. It seems like there is a D-Bus problem with the weather devices. I'll try to figure it out, maybe we could repair it.

- Wolfgang
The following user(s) said Thank You: Eric
4 years 11 months ago #38857

Please Log in or Create an account to join the conversation.

I don't think weather should be removed from scheduler. Hans is interested in improving the weather support, but maybe this would be part of the partitioning plan as well.

Where would this weather handling exactly resides? Well, this is up for discussion now.
The following user(s) said Thank You: Eric
4 years 11 months ago #38864

Please Log in or Create an account to join the conversation.

  • Posts: 1187
  • Thank you received: 370
Ok, absolutely fine, but it seems like the Scheduler does not connect to weather devices right now. Any idea?
4 years 11 months ago #38865

Please Log in or Create an account to join the conversation.

It does not connect directly, weather devices are handled by INDI::Weather Ekos::Weather. the latter has a signal newStatus _should_ be connected to the scheduler. I know used this to work but with all the changes to the scheduler, it might have been broken, I'll look into it.

But if Hans has plans to implement a more through implementation for the weather handling then I'll just follow up with him.
4 years 11 months ago #38866

Please Log in or Create an account to join the conversation.

  • Posts: 85
  • Thank you received: 40
I found that some weather related code blobs were commented-out, or completely missing (checkWeather() no longer exists).

That will be part of this scheduler repartitioning project. Not something we could do today like hopefully re-enabling what is there to shut down an observatory on bad weather.

-- Hans
4 years 11 months ago #38874

Please Log in or Create an account to join the conversation.

Great thanks!

I just pushed a commit that fixes the scheduler shutdown due to severe weather bug. Now I tested it and it works OK. I plan to release a maintenance KStars release (v3.2.2) this weekend and I think after that we'll start merging any scheduler partitioning work for the next release.
The following user(s) said Thank You: Wolfgang Reissenberger
4 years 11 months ago #38875

Please Log in or Create an account to join the conversation.

Time to create page: 1.324 seconds