×

INDI Library v2.0.6 is Released (02 Feb 2024)

Bi-monthly release with minor bug fixes and improvements

Partitioning the Scheduler code

  • Posts: 1029
  • Thank you received: 301

Thread Summary #1

We introduce a new module named "Observatory". In terms of end-user UI, this is a new tab in Ekos managing observatory startup and safety conditions.

That new module:
- Has a dbus interface providing startup, shutdown, settings serialization and status notification.
- Runs customized shell scripts like Scheduler does now.
- Controls dome and mount unparking and parking like Scheduler does now, as well as safety-related INDI drivers.
- Manages and consolidates reports from weather and safety devices, translates them into notifications, but also decides on emergency shutdown by itself.
- Supports monitoring by external safety programs like Guide supports Linguider/PHD2.

Hans' project Ekos Sentinel is set to develop in parallel, in order to consolidate the interface of the module.
This new Observatory module requires develop unitary tests to serve as specifications and avoid regressions in the future. It lives at kstars/ekos/observatory in the code tree.

Discussion on dome parking and shutter closing will move to another thread.
Next in discussion: settings serialization for all tabs.



Hey,

With @sterne-jaeger, we want to address the complexity of the code in scheduler.cpp. We thought and discussed the thing a bit, and we started moving some code to schedulerjob.cpp.

On my side, I had a first attempt at dividing "scheduler.cpp" into five files:
  • "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
  • "scheduler.cpp", which would contain observatory startup and shutdown
We have "schedulerjob.cpp" and "mosaic.cpp" in that folder as well.

I committed this locally, and needless to say, when I pulled changes from the master branches, the resulting conflicts were catastrophic because of the code block moves :) That was fun to see git trying to merge the mess.
Moreover, I don't think the separation was proper. UI should probably remain in "scheduler.cpp", while observatory stuff should probably go to "scheduler_obs.cpp" or something.

The intent is to isolate the code as much as possible in those files, then eventually create separate classes from that and clarify data exchanges, in order to facilitate development and reduce the risk of regression.
The second intent is to have a clearer separation between the interface of the scheduler, that should get a proper API header that external clients could use, and its internal implementation in a different class on which we could work without breaking stuff.

Opinion on the intents, the divide and the names? "execjob" could read "runtime" for instance. Moving code to different files will wreck some developer trees, so we need to be cautious.
In any case, when committing a first draft, I will reorder functions inside scheduler.cpp and mark their separation cleanly. In a second step, I will move those blocks to separate files in order to simplify further merges.

-Eric
The following user(s) said Thank You: Jasem Mutlaq, Hans, Jarno Paananen, Ferrante Enriques, Craig
Last edit: 4 years 10 months ago by Eric.
4 years 10 months ago #38745

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

  • Posts: 1185
  • 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 10 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 10 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 10 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 10 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 10 months ago #38843

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

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

- Wolfgang
4 years 10 months ago #38854

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

  • Posts: 1185
  • 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 10 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 10 months ago #38864

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

  • Posts: 1185
  • 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 10 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 10 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 10 months ago #38874

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

Time to create page: 1.015 seconds