pradeeptyagi23 / robotsimulation Goto Github PK

Existing Bugs :

1. The SimulatedRobotWithCommunicationDelay class's set_position method creates a new thread but doesn't start it. The thread_update.start() line is missing.
2. The SimulatedRobotWithCommunicationDelay class's set_navigation_command method creates a new thread but doesn't start it. The thread_update.start() line is missing.
3. The SimulatedRobotWithCommunicationDelay class's set_navigation_command method uses the self._robot variable without locking it first, which could cause synchronization issues in a multithreaded environment.
4. The MissionController class's _poll_position method calls itself recursively without an exit condition, which could lead to a stack overflow error.
5. The MissionController class's _poll_position method doesn't use a lock when accessing the current_waypoint_idx variable, which could cause synchronization issues in a multithreaded environment.

Changelog for fixing issues and refactoring code for readability and maintainability.

To address these issues, you can add the following modifications to the code:

BUG FIXES :

1. In the SimulatedRobotWithCommunicationDelay class's set_position method, add Thread(target=self._update_position, args=(position,)).start()
2. In the SimulatedRobotWithCommunicationDelay class's set_navigation_command method, add Thread(target=self._update_position, args=(waypoint,)).start()
3. In the SimulatedRobotWithCommunicationDelay class's set_navigation_command method, add a lock before accessing the self._robot variable by defining lock = Lock() in the class constructor and using with lock: when accessing the self._robot variable.
4. In the MissionController class's _poll_position method, add a condition is_mission_completed to the recursive call by checking if the mission has been completed. That is to check if the current waypoint index has reached the last waypoint in the trajectory.
5. In the MissionController class's _poll_position method, add a lock before accessing the current_waypoint_idx variable by defining lock = Lock() in the class constructor and using with lock: when accessing the current_waypoint_idx variable.

REFACTORIZATION for readability and maintainability
mission_controller.py

1. Calling seperate method _start_polling_position to start the polling thread for better maintainability.
2. Initialize trajectory to None within constructor and set current_waypoint_idx to 0 whenever a new trajectory is set in set_trajectory method
3. Added a is_mission_completed method to encapsulate the logic for checking if the mission is completed.
4. Added a check in the _poll_position method to avoid infinite recursion in case the trajectory is not set.

SimulatedRobotWithCommunicationDelay

1. Added _update_position_with_delay and called it as the target within set_navigation_command to start a new thread and secure it with a lock

INPUT VALIDATION: Added below input validation

mission_controller.py

1. Check if robot is instance of SimulatedRobot or SimulatedRobotWithCommunicationDelay in the constructor
2. Check if trajectory is a numpy array

SimulatedRobot

1. Check if update_position_callback is callable and not None in the constructor
2. Check if waypoint, initial_position and position are a numy array

SAMPLE OUTPUT

Creating SimulatedRobot!
Creating MissionController!
Sending waypoint 0
Commanding robot to move to [2. 0.]
Sending waypoint 1
Commanding robot to move to [3. 0.]
Commanding robot to move to [2. 0.]
Commanding robot to move to [3. 0.]
Robot is now at [2. 0.]
Robot is now at [3. 0.]
Test complete