Skip to content

Comments

Split initiate mission into prepare mission and start mission#936

Open
LeikvollE wants to merge 1 commit intoequinor:mainfrom
LeikvollE:split_start_mission
Open

Split initiate mission into prepare mission and start mission#936
LeikvollE wants to merge 1 commit intoequinor:mainfrom
LeikvollE:split_start_mission

Conversation

@LeikvollE
Copy link
Contributor

@LeikvollE LeikvollE commented Oct 10, 2025 •
edited
Loading

Ready for review checklist:

  • A self-review has been performed
  • All commits run individually
  • Temporary changes have been removed, like logging, TODO, etc.
  • The PR has been tested locally
  • A test has been written
    • This change doesn't need a new test
  • Relevant issues are linked
  • Remaining work is documented in issues
    • There is no remaining work from this PR that requires new issues
  • The changes do not introduce dead code as unused imports, functions etc.

@LeikvollE LeikvollE changed the title Split start mission Split initiate mission into prepare mission and start mission Oct 10, 2025
@LeikvollE LeikvollE mentioned this pull request Oct 10, 2025
Open
andchiind
andchiind reviewed Oct 10, 2025
View reviewed changes
@oysand oysand added breaking-change A breaking change which introduces changes to the public APIs improvement Improvement to existing functionality labels Oct 13, 2025
@LeikvollE LeikvollE force-pushed the split_start_mission branch 2 times, most recently from 98928b2 to 44dbafe Compare October 13, 2025 08:15
andchiind
andchiind requested changes Nov 12, 2025
View reviewed changes
@andchiind
Copy link
Contributor

andchiind commented Nov 12, 2025

I agree with the general changes in the PR. There's a lot of comments mainly since I want to address the use of the global scoped Event variables which can be replaced by events passed by the parent thread. This is safer since this forces the parent thread to only report the output of the threads when they are done, which prevents timing problems where we are not sure if a thread has exited by the time we create another one.

@oysand
Copy link
Contributor

oysand commented Nov 12, 2025

I agree with the general changes in the PR. There's a lot of comments mainly since I want to address the use of the global scoped Event variables which can be replaced by events passed by the parent thread. This is safer since this forces the parent thread to only report the output of the threads when they are done, which prevents timing problems where we are not sure if a thread has exited by the time we create another one.

Agreed, I think that is a good change. It should be doable for all the threads except when cancelling the prepare mission thread. To achieve this we would need to be able to cancel the prepare attempt quite quickly and that is currently not implemented

@LeikvollE LeikvollE force-pushed the split_start_mission branch 2 times, most recently from 5e87451 to 762ac2a Compare November 25, 2025 11:10
andchiind
andchiind approved these changes Nov 25, 2025
View reviewed changes
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If you have tested it well that it works then it's ready to merge. Maybe make an issue on adding tests for this feature, if you aren't planning on adding tests for it in this PR

src/isar/robot/robot.py
Comment on lines +163 to +171
elif self.prepare_mission_thread.mission:
self.start_mission_thread = RobotStartMissionThread(
self.robot,
self.signal_thread_quitting,
self.prepare_mission_thread.mission,
)
self.prepare_mission_thread = None

self.start_mission_thread.start()
Copy link
Contributor

@andchiind andchiind Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe have an "else" with a log warning here

oysand
oysand approved these changes Dec 12, 2025
View reviewed changes
Copy link
Contributor

@oysand oysand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, wait with merging this until the PRs adjusting the translational packages are ready

@github-actions
Copy link

github-actions bot commented Jan 12, 2026

This pull request has been automatically marked as stale due to inactivity.

@github-actions github-actions bot added the stale This issue or pull request already exists label Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andchiind andchiind andchiind approved these changes

@oysand oysand oysand approved these changes

Assignees

No one assigned

Labels

breaking-change A breaking change which introduces changes to the public APIs improvement Improvement to existing functionality stale This issue or pull request already exists

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LeikvollE @andchiind @oysand