Open
Conversation
andchiind
force-pushed
the
experiment-functions
branch
9 times, most recently
from
2309926 to
f2b1050
Compare
andchiind
force-pushed
the
experiment-functions
branch
from
bc73781 to
a9e49bb
Compare
andchiind
commented
Mar 6, 2026
Comment on lines
+10
to
+19
| class FunctionThread(Thread): | ||
| def __init__( | ||
| self, handler_function: Callable[P, None], *args: P.args, **kwargs: P.kwargs | ||
| ): | ||
| self.handler_function = lambda: handler_function(*args, **kwargs) | ||
| Thread.__init__(self, name="Robot start mission thread") | ||
| self.start() | ||
|
|
||
| def run(self) -> None: | ||
| self.handler_function() |
Contributor
Author
There was a problem hiding this comment.
This class just runs whatever function it gets, with whatever arguments it gets. It's useful for when we want a function in its own thread, but we also want to type check its arguments.
andchiind
commented
Mar 6, 2026
| self.signal_thread_quitting: Event = signal_thread_quitting | ||
| self.error_message: Optional[ErrorMessage] = None | ||
| Thread.__init__(self, name="Robot pause mission thread") | ||
| def robot_pause_mission( |
Contributor
Author
There was a problem hiding this comment.
The robot service handlers are running in thier own threads, instead of the functions that the handlers call.
andchiind
force-pushed
the
experiment-functions
branch
from
453916c to
0d72cab
Compare
andchiind
force-pushed
the
experiment-functions
branch
from
ee57df1 to
a523518
Compare
andchiind
force-pushed
the
experiment-functions
branch
from
a523518 to
207ad11
Compare
andchiind
force-pushed
the
experiment-functions
branch
from
207ad11 to
cd975f4
Compare
andchiind
commented
Apr 8, 2026
Comment on lines
-175
to
-193
| def test_mission_succeeds_to_stop( | ||
| mocked_robot_service: Robot, mocker: MockerFixture | ||
| ) -> None: | ||
| r_service = mocked_robot_service | ||
| mocker.patch.object(RobotStopMissionThread, "is_alive", return_value=False) | ||
|
|
||
| r_service.stop_mission_thread = RobotStopMissionThread( | ||
| r_service.robot, r_service.signal_thread_quitting | ||
| ) | ||
|
|
||
| r_service._stop_mission_done_handler() | ||
|
|
||
| assert not r_service.robot_service_events.mission_failed_to_stop.has_event() | ||
| assert r_service.robot_service_events.mission_successfully_stopped.has_event() | ||
|
|
||
| assert not r_service.robot_service_events.mission_succeeded.has_event() | ||
| assert not r_service.robot_service_events.mission_failed.has_event() | ||
|
|
||
| assert r_service.monitor_mission_thread is None |
Contributor
Author
There was a problem hiding this comment.
This test became equivalent to the test_successful_stop test after the refactor.
andchiind
commented
Apr 8, 2026
Comment on lines
-403
to
-423
| def test_mission_stop_waits_for_mission_to_start( | ||
| mocked_robot_service: Robot, mocker: MockerFixture | ||
| ) -> None: | ||
| r_service = mocked_robot_service | ||
| mocker.patch.object(RobotStartMissionThread, "is_alive", return_value=True) | ||
|
|
||
| task_1: Task = TakeImage( | ||
| target=DummyPose.default_pose().position, robot_pose=DummyPose.default_pose() | ||
| ) | ||
| mission: Mission = Mission(name="Dummy misson", tasks=[task_1]) | ||
| r_service.start_mission_thread = RobotStartMissionThread( | ||
| r_service.robot, r_service.signal_thread_quitting, mission | ||
| ) | ||
| r_service.stop_mission_thread = None | ||
|
|
||
| r_service.state_machine_events.stop_mission.trigger_event(EmptyMessage()) | ||
|
|
||
| r_service._stop_mission_request_handler(r_service.state_machine_events.stop_mission) | ||
|
|
||
| assert r_service.state_machine_events.stop_mission.has_event() | ||
| assert r_service.stop_mission_thread is None |
Contributor
Author
There was a problem hiding this comment.
This test also became redundant after the rewrite. Only one action thread running is handled in the robot service loop, not by the individual threads.
andchiind
commented
Apr 8, 2026
Comment on lines
-426
to
-451
| def test_mission_stop_starts_when_start_is_done( | ||
| mocked_robot_service: Robot, mocker: MockerFixture | ||
| ) -> None: | ||
| r_service = mocked_robot_service | ||
| mocker.patch.object(RobotStartMissionThread, "is_alive", return_value=False) | ||
|
|
||
| mock_stop_mission_start = mocker.patch( | ||
| "isar.robot.robot.RobotStopMissionThread.start" | ||
| ) | ||
|
|
||
| task_1: Task = TakeImage( | ||
| target=DummyPose.default_pose().position, robot_pose=DummyPose.default_pose() | ||
| ) | ||
| mission: Mission = Mission(name="Dummy misson", tasks=[task_1]) | ||
| r_service.start_mission_thread = RobotStartMissionThread( | ||
| r_service.robot, r_service.signal_thread_quitting, mission | ||
| ) | ||
| r_service.stop_mission_thread = None | ||
|
|
||
| r_service.state_machine_events.stop_mission.trigger_event(EmptyMessage()) | ||
|
|
||
| r_service._stop_mission_request_handler(r_service.state_machine_events.stop_mission) | ||
|
|
||
| assert not r_service.state_machine_events.stop_mission.has_event() | ||
| assert r_service.stop_mission_thread is not None | ||
| mock_stop_mission_start.assert_called_once() |
Contributor
Author
There was a problem hiding this comment.
This test also became redundant. Missions starting is now event based, instead of relying on logic checking the status of other threads.
andchiind
commented
Apr 8, 2026
Comment on lines
-454
to
-476
| def test_mission_pause_waits_for_mission_to_start( | ||
| mocked_robot_service: Robot, mocker: MockerFixture | ||
| ) -> None: | ||
| r_service = mocked_robot_service | ||
| mocker.patch.object(RobotStartMissionThread, "is_alive", return_value=True) | ||
|
|
||
| task_1: Task = TakeImage( | ||
| target=DummyPose.default_pose().position, robot_pose=DummyPose.default_pose() | ||
| ) | ||
| mission: Mission = Mission(name="Dummy misson", tasks=[task_1]) | ||
| r_service.start_mission_thread = RobotStartMissionThread( | ||
| r_service.robot, r_service.signal_thread_quitting, mission | ||
| ) | ||
| r_service.pause_mission_thread = None | ||
|
|
||
| r_service.state_machine_events.pause_mission.trigger_event(EmptyMessage()) | ||
|
|
||
| r_service._pause_mission_request_handler( | ||
| r_service.state_machine_events.pause_mission | ||
| ) | ||
|
|
||
| assert r_service.state_machine_events.pause_mission.has_event() | ||
| assert r_service.pause_mission_thread is None |
Contributor
Author
There was a problem hiding this comment.
Same as above, the robot service handles this now.
andchiind
force-pushed
the
experiment-functions
branch
from
cd975f4 to
ca5df35
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The point of this PR is to simplify the robot interface.
Instead of having separate thread objections for each action, we create a generic action thread which runs each event handler. In the eventhandler we both run the action handler (which was previously its own thread) and handle the output, synchronously. Only one action is handled at a time. Other state machine events are ignored in the mean time.
Ready for review checklist: