From ef8a2570607c5616b6532a49dac570e3feed2504 Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Thu, 19 Sep 2024 02:22:44 -0400 Subject: [PATCH 1/7] Use context managers for clean exit on Ctrl+C Also add more tests --- airbrakes/airbrakes.py | 12 ++++++ airbrakes/imu/imu.py | 19 ++++++++- airbrakes/logger.py | 15 +++++++ main.py | 15 ++----- pyproject.toml | 4 ++ scripts/run_imu.py | 62 ++--------------------------- scripts/run_logger.py | 10 ++--- tests/test_airbrakes.py | 87 +++++++++++++++++++++++++++++++++++++---- tests/test_imu.py | 45 +++++++++++++++++++++ tests/test_logger.py | 39 ++++++++++++++++++ 10 files changed, 223 insertions(+), 85 deletions(-) diff --git a/airbrakes/airbrakes.py b/airbrakes/airbrakes.py index 8fb133a9..a0f14086 100644 --- a/airbrakes/airbrakes.py +++ b/airbrakes/airbrakes.py @@ -44,6 +44,18 @@ def __init__(self, logger: Logger, servo: Servo, imu: IMU): # Placeholder for the current airbrake extension until they are set self.current_extension: float = 0.0 + def __enter__(self): + """This is what is run when the context manager is entered, i.e. a `with` statement.""" + self.start() + return self + + def __exit__(self, _, exc_val: object, __): + """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" + # If a KeyboardInterrupt was raised, we want to stop the airbrakes cleanly, and not raise + # the exception again + self.stop() + return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C + def start(self) -> None: """ Starts the IMU and logger processes. This is called before the main while loop starts. diff --git a/airbrakes/imu/imu.py b/airbrakes/imu/imu.py index 105ecee6..f2a9922b 100644 --- a/airbrakes/imu/imu.py +++ b/airbrakes/imu/imu.py @@ -2,13 +2,14 @@ import collections import multiprocessing +import signal import warnings try: import mscl except ImportError: warnings.warn( - "Could not import MSCL, IMU will not work. Please see installation instructions" + "Could not import MSCL, IMU will not work. Please see installation instructions " "here: https://github.com/LORD-MicroStrain/MSCL/tree/master", stacklevel=2, ) @@ -43,9 +44,21 @@ def __init__(self, port: str, frequency: int, upside_down: bool): # Starts the process that fetches data from the IMU self._data_fetch_process = multiprocessing.Process( - target=self._fetch_data_loop, args=(port, frequency, upside_down) + target=self._fetch_data_loop, args=(port, frequency, upside_down), name="IMU Data Fetch Process" ) + def __enter__(self): + """This is what is run when the context manager is entered, i.e. a `with` statement.""" + self.start() + return self + + def __exit__(self, _, exc_val: object, __): + """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" + # If a KeyboardInterrupt was raised, we want to stop the IMU cleanly, and not raise + # the exception again + self.stop() + return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C + @property def is_running(self) -> bool: """ @@ -64,6 +77,8 @@ def _fetch_data_loop(self, port: str, frequency: int, _: bool): """ This is the loop that fetches data from the IMU. It runs in parallel with the main loop. """ + # Ignore the SIGINT (Ctrl+C) signal, because we only want the main process to handle it + signal.signal(signal.SIGINT, signal.SIG_IGN) # Connect to the IMU connection = mscl.Connection.Serial(port) node = mscl.InertialNode(connection) diff --git a/airbrakes/logger.py b/airbrakes/logger.py index c7a98e1e..4d214c2a 100644 --- a/airbrakes/logger.py +++ b/airbrakes/logger.py @@ -3,6 +3,7 @@ import collections import csv import multiprocessing +import signal from pathlib import Path from airbrakes.constants import CSV_HEADERS, STOP_SIGNAL @@ -45,6 +46,18 @@ def __init__(self, log_dir: Path): # Start the logging process self._log_process = multiprocessing.Process(target=self._logging_loop, name="Logger") + def __enter__(self): + """This is what is run when the context manager is entered, i.e. a `with` statement.""" + self.start() + return self + + def __exit__(self, _, exc_val: object, __): + """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" + # If a KeyboardInterrupt was raised, we want to stop the logger cleanly, and not raise + # the exception again + self.stop() + return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C + @property def is_running(self) -> bool: """ @@ -85,6 +98,8 @@ def _logging_loop(self) -> None: """ The loop that saves data to the logs. It runs in parallel with the main loop. """ + # Ignore the SIGINT (Ctrl+C) signal, because we only want the main process to handle it + signal.signal(signal.SIGINT, signal.SIG_IGN) # Ignores the interrupt signal # Set up the csv logging in the new process with self.log_path.open(mode="a", newline="") as file_writer: writer = csv.DictWriter(file_writer, fieldnames=CSV_HEADERS) diff --git a/main.py b/main.py index 0eec0a60..e3721a0f 100644 --- a/main.py +++ b/main.py @@ -14,17 +14,10 @@ def main(): imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) # The context that will manage the airbrakes state machine - airbrakes = AirbrakesContext(logger, servo, imu) - - # Start the IMU and logger processes: - airbrakes.start() - - # This is the main loop that will run until the stop method on the airbrakes is called - while not airbrakes.shutdown_requested: - airbrakes.update() - - # Shutdown the IMU and logger processes: - airbrakes.stop() + with AirbrakesContext(logger, servo, imu) as airbrakes: + # This is the main loop that will run until we press Ctrl+C + while not airbrakes.shutdown_requested: + airbrakes.update() if __name__ == "__main__": diff --git a/pyproject.toml b/pyproject.toml index 958f89b3..34e9ade9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,3 +37,7 @@ select = ["E", "F", "I", "PL", "UP", "RUF", "PTH", "C4", "B", "PIE", "SIM", "RET [tool.ruff.lint.per-file-ignores] "tests/*.py" = ["T20", "S101", "D100", "ARG001"] + + +[tool.pytest.ini_options] +filterwarnings = "ignore:This process:DeprecationWarning" # ignore warning about fork() diff --git a/scripts/run_imu.py b/scripts/run_imu.py index d08e9dae..b1e1ddd6 100644 --- a/scripts/run_imu.py +++ b/scripts/run_imu.py @@ -11,66 +11,10 @@ from airbrakes.imu.imu_data_packet import EstimatedDataPacket, RawDataPacket -# import matplotlib.pyplot as plt -# import matplotlib.animation as animation -# from collections import deque -# import numpy as np - -# # Initialize a deque for each axis to store the last N acceleration values -# N = 100 # Number of points to display -# x_vals = deque(maxlen=N) -# y_vals = deque(maxlen=N) -# z_vals = deque(maxlen=N) -# time_vals = deque(maxlen=N) - -# # Initialize the plot -# fig, ax = plt.subplots() -# ax.set_xlim(0, N) -# ax.set_ylim(-2, 2) # Assuming accelerations range from -2 to 2 g - -# x_line, = ax.plot([], [], label='X-axis') -# y_line, = ax.plot([], [], label='Y-axis') -# z_line, = ax.plot([], [], label='Z-axis') - -# ax.legend() - -# def init(): -# x_line.set_data([], []) -# y_line.set_data([], []) -# z_line.set_data([], []) -# return x_line, y_line, z_line - -# def update(frame): -# # Simulate reading IMU data in real-time -# a = imu.get_imu_data_packet() -# if not isinstance(a, EstimatedDataPacket): -# return x_line, y_line, z_line - -# # Append new data to the deque -# time_vals.append(len(time_vals)) -# x_vals.append(a.estCompensatedAccelX) -# y_vals.append(a.estCompensatedAccelY) -# z_vals.append(a.estCompensatedAccelZ) - -# # Update the plot data -# x_line.set_data(time_vals, x_vals) -# y_line.set_data(time_vals, y_vals) -# z_line.set_data(time_vals, z_vals) - -# return x_line, y_line, z_line - -# ani = animation.FuncAnimation(fig, update, init_func=init, blit=True, interval=50) - -# plt.xlabel('Time') -# plt.ylabel('Acceleration (g)') -# plt.title('Real-time Acceleration') -# plt.show() imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) -imu.start() - logger = Logger(Path("test_logs/")) -logger.start() -while True: - print(imu.get_imu_data_packet()) +with imu, logger: + while True: + print(imu.get_imu_data_packet()) diff --git a/scripts/run_logger.py b/scripts/run_logger.py index b815fb09..7d6c7acf 100644 --- a/scripts/run_logger.py +++ b/scripts/run_logger.py @@ -12,15 +12,13 @@ def main(): # Initialize the logger logger = Logger(LOGS_PATH) - # Log for 5 seconds + # Log for 5 seconds, and automatically stops logging start_time = time.time() while time.time() - start_time < 5: # Create fake IMU data - imu_data_list = deque([RawDataPacket(int(time.time()), 1, 2, 3, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6)]) - logger.log("TEST_STATE", 0.5, imu_data_list) - - # Stop the logger after 5 seconds - logger.stop() + with logger: + imu_data_list = deque([RawDataPacket(int(time.time()), 1, 2, 3, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6)]) + logger.log("TEST_STATE", 0.5, imu_data_list) if __name__ == "__main__": diff --git a/tests/test_airbrakes.py b/tests/test_airbrakes.py index 63294367..e93e6828 100644 --- a/tests/test_airbrakes.py +++ b/tests/test_airbrakes.py @@ -1,22 +1,95 @@ +import time + import pytest from airbrakes.airbrakes import AirbrakesContext +from airbrakes.imu.data_processor import IMUDataProcessor +from airbrakes.state import StandByState @pytest.fixture -def airbrakes_context(imu, logger, servo): +def airbrakes(imu, logger, servo): return AirbrakesContext(logger, servo, imu) +@pytest.mark.filterwarnings("ignore:To reduce servo jitter") # ignore warning about servo jitter class TestAirbrakesContext: """Tests the AirbrakesContext class""" - def test_slots(self, airbrakes_context): - inst = airbrakes_context + def test_slots(self, airbrakes): + inst = airbrakes for attr in inst.__slots__: assert getattr(inst, attr, "err") != "err", f"got extra slot '{attr}'" - def test_init(self, airbrakes_context, logger, imu, servo): - assert airbrakes_context.logger == logger - assert airbrakes_context.servo == servo - assert airbrakes_context.imu == imu + def test_init(self, airbrakes, logger, imu, servo): + assert airbrakes.logger == logger + assert airbrakes.servo == servo + assert airbrakes.imu == imu + assert airbrakes.current_extension == 0.0 + assert isinstance(airbrakes.data_processor, IMUDataProcessor) + assert isinstance(airbrakes.state, StandByState) + assert not airbrakes.shutdown_requested + + def test_set_extension(self, airbrakes): + # Hardcoded calculated values, based on MIN_EXTENSION and MAX_EXTENSION in constants.py + airbrakes.set_airbrake_extension(0.5) + # TODO: airbrakes.current_extension must be set to 0.5 !! + assert airbrakes.servo.current_extension == 0.0803 + airbrakes.set_airbrake_extension(0.0) + assert airbrakes.servo.current_extension == -0.0999 + airbrakes.set_airbrake_extension(1.0) + assert airbrakes.servo.current_extension == 0.2605 + + def test_start(self, airbrakes): + airbrakes.start() + assert airbrakes.imu.is_running + assert airbrakes.logger.is_running + airbrakes.stop() + + def test_stop(self, airbrakes): + airbrakes.start() + airbrakes.stop() + assert not airbrakes.imu.is_running + assert not airbrakes.logger.is_running + assert not airbrakes.imu._running.value + assert not airbrakes.imu._data_fetch_process.is_alive() + assert not airbrakes.logger._log_process.is_alive() + assert airbrakes.servo.current_extension == -0.0999 # set to "0" + assert airbrakes.shutdown_requested + + def test_context_manager(self, airbrakes): + with airbrakes as ab: + assert airbrakes.imu.is_running + assert airbrakes.logger.is_running + assert ab is airbrakes + assert not airbrakes.imu.is_running + assert not airbrakes.logger.is_running + assert not airbrakes.imu._running.value + assert not airbrakes.imu._data_fetch_process.is_alive() + assert not airbrakes.logger._log_process.is_alive() + + def test_airbrakes_context_manager_clean_exit(self, airbrakes): + """Tests whether the Airbrakes context manager works correctly.""" + + with airbrakes: + time.sleep(0.01) + raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + + assert not airbrakes.imu.is_running + assert not airbrakes.logger.is_running + assert airbrakes.shutdown_requested + + def test_airbrakes_context_manager_exception(self, airbrakes): + """Tests whether the Airbrakes context manager propogates unknown exceptions.""" + + with pytest.raises(ValueError, match="some error") as exc_info, airbrakes: + raise ValueError("some error") + + assert not airbrakes.imu.is_running + assert not airbrakes.logger.is_running + assert airbrakes.shutdown_requested + assert "some error" in str(exc_info.value) + + def test_airbrakes_update(self, monkeypatch): + """Tests whether the Airbrakes update method works correctly.""" + # TODO: Implement this test diff --git a/tests/test_imu.py b/tests/test_imu.py index ce842b4a..6aa5e8f9 100644 --- a/tests/test_imu.py +++ b/tests/test_imu.py @@ -3,6 +3,8 @@ import time from collections import deque +import pytest + from airbrakes.constants import FREQUENCY, PORT, UPSIDE_DOWN from airbrakes.imu.imu import IMU from airbrakes.imu.imu_data_packet import EstimatedDataPacket, IMUDataPacket, RawDataPacket @@ -57,6 +59,49 @@ def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): assert not imu.is_running assert not imu._data_fetch_process.is_alive() + def test_imu_context_manager_no_exception(self, monkeypatch): + """Tests whether the IMU context manager works correctly.""" + values = multiprocessing.Queue() + + def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): + """Monkeypatched method for testing.""" + values.put((port, frequency, upside_down)) + + monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) + with IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) as imu: + assert imu._running.value + assert imu.is_running + assert imu._data_fetch_process.is_alive() + raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + + assert not imu._running.value + assert not imu.is_running + assert not imu._data_fetch_process.is_alive() + assert values.qsize() == 1 + assert values.get() == (PORT, FREQUENCY, UPSIDE_DOWN) + + def test_imu_context_manager_with_exception(self, monkeypatch): + """Tests whether the IMU context manager propogates unknown exceptions.""" + values = multiprocessing.Queue() + + def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): + """Monkeypatched method for testing.""" + values.put((port, frequency, upside_down)) + raise ValueError("some error") + + monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) + with ( + pytest.raises(ValueError, match="some error") as excinfo, + IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) as imu, + ): + imu._fetch_data_loop(PORT, FREQUENCY, UPSIDE_DOWN) + assert not imu._running.value + assert not imu.is_running + assert not imu._data_fetch_process.is_alive() + assert values.qsize() == 2 + assert values.get() == (PORT, FREQUENCY, UPSIDE_DOWN) + assert "some error" in str(excinfo.value) + def test_data_packets_fetch(self, monkeypatch): """Tests whether the data fetching loop actually adds data to the queue.""" diff --git a/tests/test_logger.py b/tests/test_logger.py index d8424ea0..a87a8f3d 100644 --- a/tests/test_logger.py +++ b/tests/test_logger.py @@ -69,6 +69,45 @@ def test_logging_loop_start_stop(self, logger): assert not logger.is_running assert logger._log_process.exitcode == 0 + def test_logger_context_manager_no_exception(self, monkeypatch): + """Tests whether the Logger context manager works correctly.""" + values = multiprocessing.Queue() + + def _logging_loop(self): + """Monkeypatched method for testing.""" + values.put("test") + + monkeypatch.setattr(Logger, "_logging_loop", _logging_loop) + + with Logger(LOG_PATH) as logger: + assert logger.is_running + raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + + assert not logger.is_running + assert not logger._log_process.is_alive() + assert values.qsize() == 1 + assert values.get() == "test" + + def test_logger_context_manager_with_exception(self, monkeypatch): + """Tests whether the Logger context manager propogates unknown exceptions.""" + values = multiprocessing.Queue() + + def _logging_loop(self): + """Monkeypatched method for testing.""" + values.put("test") + raise ValueError("some error") + + monkeypatch.setattr(Logger, "_logging_loop", _logging_loop) + + with pytest.raises(ValueError, match="some error") as excinfo, Logger(LOG_PATH) as logger: + logger._logging_loop() + + assert not logger.is_running + assert not logger._log_process.is_alive() + assert values.qsize() == 2 + assert values.get() == "test" + assert "some error" in str(excinfo.value) + def test_logging_loop_add_to_queue(self, logger): test_log = {"state": "state", "extension": "0.0", "timestamp": "4"} logger._log_queue.put(test_log) From 094fc0fc590bd54f3fd46222b3b1e983c648f216 Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Fri, 20 Sep 2024 03:15:12 -0400 Subject: [PATCH 2/7] bye context manager --- airbrakes/airbrakes.py | 12 ------------ airbrakes/imu/imu.py | 12 ------------ airbrakes/logger.py | 12 ------------ main.py | 8 +++++++- scripts/run_imu.py | 9 ++++++++- scripts/run_logger.py | 11 ++++++++--- scripts/run_main_local.py | 32 ++++++++++++++++++++++++++++++++ tests/test_airbrakes.py | 37 ++++++++++++++++--------------------- tests/test_imu.py | 38 ++++++++++++++++++++++++-------------- tests/test_logger.py | 37 ++++++++++++++++++++++++++----------- 10 files changed, 121 insertions(+), 87 deletions(-) create mode 100644 scripts/run_main_local.py diff --git a/airbrakes/airbrakes.py b/airbrakes/airbrakes.py index a0f14086..8fb133a9 100644 --- a/airbrakes/airbrakes.py +++ b/airbrakes/airbrakes.py @@ -44,18 +44,6 @@ def __init__(self, logger: Logger, servo: Servo, imu: IMU): # Placeholder for the current airbrake extension until they are set self.current_extension: float = 0.0 - def __enter__(self): - """This is what is run when the context manager is entered, i.e. a `with` statement.""" - self.start() - return self - - def __exit__(self, _, exc_val: object, __): - """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" - # If a KeyboardInterrupt was raised, we want to stop the airbrakes cleanly, and not raise - # the exception again - self.stop() - return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C - def start(self) -> None: """ Starts the IMU and logger processes. This is called before the main while loop starts. diff --git a/airbrakes/imu/imu.py b/airbrakes/imu/imu.py index f2a9922b..01ec5f03 100644 --- a/airbrakes/imu/imu.py +++ b/airbrakes/imu/imu.py @@ -47,18 +47,6 @@ def __init__(self, port: str, frequency: int, upside_down: bool): target=self._fetch_data_loop, args=(port, frequency, upside_down), name="IMU Data Fetch Process" ) - def __enter__(self): - """This is what is run when the context manager is entered, i.e. a `with` statement.""" - self.start() - return self - - def __exit__(self, _, exc_val: object, __): - """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" - # If a KeyboardInterrupt was raised, we want to stop the IMU cleanly, and not raise - # the exception again - self.stop() - return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C - @property def is_running(self) -> bool: """ diff --git a/airbrakes/logger.py b/airbrakes/logger.py index 4d214c2a..bc7c4bcc 100644 --- a/airbrakes/logger.py +++ b/airbrakes/logger.py @@ -46,18 +46,6 @@ def __init__(self, log_dir: Path): # Start the logging process self._log_process = multiprocessing.Process(target=self._logging_loop, name="Logger") - def __enter__(self): - """This is what is run when the context manager is entered, i.e. a `with` statement.""" - self.start() - return self - - def __exit__(self, _, exc_val: object, __): - """This is what is run when the context manager is exited, i.e. when the `with` block ends.""" - # If a KeyboardInterrupt was raised, we want to stop the logger cleanly, and not raise - # the exception again - self.stop() - return isinstance(exc_val, KeyboardInterrupt) # Suppress propogation only for Ctrl+C - @property def is_running(self) -> bool: """ diff --git a/main.py b/main.py index e3721a0f..10ec2be7 100644 --- a/main.py +++ b/main.py @@ -14,10 +14,16 @@ def main(): imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) # The context that will manage the airbrakes state machine - with AirbrakesContext(logger, servo, imu) as airbrakes: + airbrakes = AirbrakesContext(logger, servo, imu) + try: + airbrakes.start() # Start the IMU and logger processes # This is the main loop that will run until we press Ctrl+C while not airbrakes.shutdown_requested: airbrakes.update() + except KeyboardInterrupt: + pass + finally: + airbrakes.stop() # Stop the IMU and logger processes if __name__ == "__main__": diff --git a/scripts/run_imu.py b/scripts/run_imu.py index b1e1ddd6..44b5899c 100644 --- a/scripts/run_imu.py +++ b/scripts/run_imu.py @@ -15,6 +15,13 @@ imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) logger = Logger(Path("test_logs/")) -with imu, logger: +try: + imu.start() + logger.start() while True: print(imu.get_imu_data_packet()) +except KeyboardInterrupt: # Stop running IMU and logger if the user presses Ctrl+C + pass +finally: + imu.stop() + logger.stop() diff --git a/scripts/run_logger.py b/scripts/run_logger.py index 7d6c7acf..61331103 100644 --- a/scripts/run_logger.py +++ b/scripts/run_logger.py @@ -14,11 +14,16 @@ def main(): # Log for 5 seconds, and automatically stops logging start_time = time.time() - while time.time() - start_time < 5: - # Create fake IMU data - with logger: + try: + logger.start() + while time.time() - start_time < 5: + # Create fake IMU data imu_data_list = deque([RawDataPacket(int(time.time()), 1, 2, 3, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6)]) logger.log("TEST_STATE", 0.5, imu_data_list) + except KeyboardInterrupt: # Stop logging if the user presses Ctrl+C + pass + finally: + logger.stop() if __name__ == "__main__": diff --git a/scripts/run_main_local.py b/scripts/run_main_local.py new file mode 100644 index 00000000..69861c2b --- /dev/null +++ b/scripts/run_main_local.py @@ -0,0 +1,32 @@ +"""The mocked main file which can be run locally. To run this, make sure you're not inside scripts, +and run the following command: `python -m scripts.test_imu`""" + +from airbrakes.airbrakes import AirbrakesContext +from airbrakes.constants import FREQUENCY, LOGS_PATH, MAX_EXTENSION, MIN_EXTENSION, PORT, SERVO_PIN, UPSIDE_DOWN +from airbrakes.imu.imu import IMU +from airbrakes.logger import Logger +from airbrakes.servo import Servo + +from gpiozero.pins.mock import MockFactory, MockPWMPin + + +def main(): + logger = Logger(LOGS_PATH) + servo = Servo(SERVO_PIN, MIN_EXTENSION, MAX_EXTENSION, pin_factory=MockFactory(pin_class=MockPWMPin)) + imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) + + # The context that will manage the airbrakes state machine + airbrakes = AirbrakesContext(logger, servo, imu) + try: + airbrakes.start() # Start the IMU and logger processes + # This is the main loop that will run until we press Ctrl+C + while not airbrakes.shutdown_requested: + airbrakes.update() + except KeyboardInterrupt: # Stop running IMU and logger if the user presses Ctrl+C + pass + finally: + airbrakes.stop() # Stop the IMU and logger processes + + +if __name__ == "__main__": + main() diff --git a/tests/test_airbrakes.py b/tests/test_airbrakes.py index e93e6828..0d0260e7 100644 --- a/tests/test_airbrakes.py +++ b/tests/test_airbrakes.py @@ -57,39 +57,34 @@ def test_stop(self, airbrakes): assert airbrakes.servo.current_extension == -0.0999 # set to "0" assert airbrakes.shutdown_requested - def test_context_manager(self, airbrakes): - with airbrakes as ab: - assert airbrakes.imu.is_running - assert airbrakes.logger.is_running - assert ab is airbrakes - assert not airbrakes.imu.is_running - assert not airbrakes.logger.is_running - assert not airbrakes.imu._running.value - assert not airbrakes.imu._data_fetch_process.is_alive() - assert not airbrakes.logger._log_process.is_alive() - - def test_airbrakes_context_manager_clean_exit(self, airbrakes): - """Tests whether the Airbrakes context manager works correctly.""" + def test_airbrakes_ctrl_c_clean_exit(self, airbrakes): + """Tests whether the AirbrakesContext handles ctrl+c events correctly.""" + airbrakes.start() - with airbrakes: - time.sleep(0.01) + try: raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + except KeyboardInterrupt: + airbrakes.stop() assert not airbrakes.imu.is_running assert not airbrakes.logger.is_running assert airbrakes.shutdown_requested - def test_airbrakes_context_manager_exception(self, airbrakes): - """Tests whether the Airbrakes context manager propogates unknown exceptions.""" + def test_airbrakes_ctrl_c_exception(self, airbrakes): + """Tests whether the AirbrakesContext handles unknown exceptions.""" - with pytest.raises(ValueError, match="some error") as exc_info, airbrakes: - raise ValueError("some error") + airbrakes.start() + try: + raise ValueError("some error in main loop") + except (KeyboardInterrupt, ValueError): + pass + finally: + airbrakes.stop() assert not airbrakes.imu.is_running assert not airbrakes.logger.is_running assert airbrakes.shutdown_requested - assert "some error" in str(exc_info.value) def test_airbrakes_update(self, monkeypatch): """Tests whether the Airbrakes update method works correctly.""" - # TODO: Implement this test + # TODO: Implement this test after we get the state and apogee detection working diff --git a/tests/test_imu.py b/tests/test_imu.py index 6aa5e8f9..722b3738 100644 --- a/tests/test_imu.py +++ b/tests/test_imu.py @@ -1,5 +1,6 @@ import multiprocessing import multiprocessing.sharedctypes +import signal import time from collections import deque @@ -59,20 +60,29 @@ def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): assert not imu.is_running assert not imu._data_fetch_process.is_alive() - def test_imu_context_manager_no_exception(self, monkeypatch): - """Tests whether the IMU context manager works correctly.""" - values = multiprocessing.Queue() + def test_imu_ctrl_c_handling(self, monkeypatch): + """Tests whether the IMU's stop() handles Ctrl+C fine.""" + values = multiprocessing.Queue(100000) def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): """Monkeypatched method for testing.""" + signal.signal(signal.SIGINT, signal.SIG_IGN) + while self._running.value: + continue values.put((port, frequency, upside_down)) monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) - with IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) as imu: - assert imu._running.value - assert imu.is_running - assert imu._data_fetch_process.is_alive() - raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + imu = IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) + imu.start() + assert imu._running.value + assert imu.is_running + assert imu._data_fetch_process.is_alive() + time.sleep(0.001) # Give the process time to start and simulate the actual loop + # send a KeyboardInterrupt to test if the process stops cleanly + try: + raise KeyboardInterrupt + except KeyboardInterrupt: + imu.stop() assert not imu._running.value assert not imu.is_running @@ -80,8 +90,8 @@ def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): assert values.qsize() == 1 assert values.get() == (PORT, FREQUENCY, UPSIDE_DOWN) - def test_imu_context_manager_with_exception(self, monkeypatch): - """Tests whether the IMU context manager propogates unknown exceptions.""" + def test_imu_fetch_loop_exception(self, monkeypatch): + """Tests whether the IMU's _fetch_loop propogates unknown exceptions.""" values = multiprocessing.Queue() def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): @@ -90,11 +100,11 @@ def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): raise ValueError("some error") monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) - with ( - pytest.raises(ValueError, match="some error") as excinfo, - IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) as imu, - ): + imu = IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) + imu.start() + with pytest.raises(ValueError, match="some error") as excinfo: imu._fetch_data_loop(PORT, FREQUENCY, UPSIDE_DOWN) + imu.stop() assert not imu._running.value assert not imu.is_running assert not imu._data_fetch_process.is_alive() diff --git a/tests/test_logger.py b/tests/test_logger.py index a87a8f3d..c7ff173f 100644 --- a/tests/test_logger.py +++ b/tests/test_logger.py @@ -1,11 +1,12 @@ import csv import multiprocessing import multiprocessing.sharedctypes +import signal import time import pytest -from airbrakes.constants import CSV_HEADERS +from airbrakes.constants import CSV_HEADERS, STOP_SIGNAL from airbrakes.imu.imu_data_packet import EstimatedDataPacket, RawDataPacket from airbrakes.logger import Logger from tests.conftest import LOG_PATH @@ -69,24 +70,33 @@ def test_logging_loop_start_stop(self, logger): assert not logger.is_running assert logger._log_process.exitcode == 0 - def test_logger_context_manager_no_exception(self, monkeypatch): - """Tests whether the Logger context manager works correctly.""" + def test_logger_ctrl_c_handling(self, monkeypatch): + """Tests whether the Logger handles Ctrl+C events from main loop correctly.""" values = multiprocessing.Queue() def _logging_loop(self): """Monkeypatched method for testing.""" - values.put("test") + signal.signal(signal.SIGINT, signal.SIG_IGN) + while True: + a = self._log_queue.get() + if a == STOP_SIGNAL: + break + values.put("clean exit") monkeypatch.setattr(Logger, "_logging_loop", _logging_loop) - with Logger(LOG_PATH) as logger: - assert logger.is_running + logger = Logger(LOG_PATH) + logger.start() + assert logger.is_running + try: raise KeyboardInterrupt # send a KeyboardInterrupt to test __exit__ + except KeyboardInterrupt: + logger.stop() assert not logger.is_running assert not logger._log_process.is_alive() assert values.qsize() == 1 - assert values.get() == "test" + assert values.get() == "clean exit" def test_logger_context_manager_with_exception(self, monkeypatch): """Tests whether the Logger context manager propogates unknown exceptions.""" @@ -94,18 +104,23 @@ def test_logger_context_manager_with_exception(self, monkeypatch): def _logging_loop(self): """Monkeypatched method for testing.""" - values.put("test") - raise ValueError("some error") + signal.signal(signal.SIGINT, signal.SIG_IGN) + while True: + values.put("error in loop") + raise ValueError("some error") monkeypatch.setattr(Logger, "_logging_loop", _logging_loop) - with pytest.raises(ValueError, match="some error") as excinfo, Logger(LOG_PATH) as logger: + logger = Logger(LOG_PATH) + logger.start() + with pytest.raises(ValueError, match="some error") as excinfo: logger._logging_loop() + logger.stop() assert not logger.is_running assert not logger._log_process.is_alive() assert values.qsize() == 2 - assert values.get() == "test" + assert values.get() == "error in loop" assert "some error" in str(excinfo.value) def test_logging_loop_add_to_queue(self, logger): From a8f73b74248b9b2e184ca4a33172f65e91070bbf Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Fri, 20 Sep 2024 03:18:45 -0400 Subject: [PATCH 3/7] ruff fixes --- tests/test_airbrakes.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_airbrakes.py b/tests/test_airbrakes.py index 0d0260e7..a64ba7bc 100644 --- a/tests/test_airbrakes.py +++ b/tests/test_airbrakes.py @@ -1,5 +1,3 @@ -import time - import pytest from airbrakes.airbrakes import AirbrakesContext From bffe5d59907416b2268308609c9b18d71c53810a Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Fri, 20 Sep 2024 19:07:36 -0400 Subject: [PATCH 4/7] Fix import --- tests/test_airbrakes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_airbrakes.py b/tests/test_airbrakes.py index a64ba7bc..d7ed0f86 100644 --- a/tests/test_airbrakes.py +++ b/tests/test_airbrakes.py @@ -1,7 +1,7 @@ import pytest from airbrakes.airbrakes import AirbrakesContext -from airbrakes.imu.data_processor import IMUDataProcessor +from airbrakes.data_handling.data_processor import IMUDataProcessor from airbrakes.state import StandByState From c595f2ec31a1467dc97d5c012cf975700b3a85d1 Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Sun, 22 Sep 2024 03:30:00 -0400 Subject: [PATCH 5/7] Review: airbrakes extension, doc fixes --- airbrakes/airbrakes.py | 4 +--- scripts/run_imu.py | 2 +- scripts/run_main_local.py | 10 +++++----- scripts/run_servo.py | 2 +- tests/test_imu.py | 18 +++++++++--------- 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/airbrakes/airbrakes.py b/airbrakes/airbrakes.py index b4672e7a..5a384808 100644 --- a/airbrakes/airbrakes.py +++ b/airbrakes/airbrakes.py @@ -56,9 +56,6 @@ def update(self) -> None: do different things. It is what controls the airbrakes and chooses when to move to the next state. """ - # Gets the current extension and IMU data, the states will use these values - self.current_extension = self.servo.current_extension - # get_imu_data_packets() gets from the "first" item in the queue, i.e, the set of data # *may* not be the most recent data. But we want continous data for state, apogee, # and logging purposes, so we don't need to worry about that, as long as we're not too @@ -85,6 +82,7 @@ def set_airbrake_extension(self, extension: float) -> None: :param extension: the extension of the airbrakes, between 0 and 1 """ self.servo.set_extension(extension) + self.current_extension = extension def stop(self) -> None: """ diff --git a/scripts/run_imu.py b/scripts/run_imu.py index afed021c..ce708475 100644 --- a/scripts/run_imu.py +++ b/scripts/run_imu.py @@ -1,6 +1,6 @@ """ Make sure you are in the root directory of the project, not inside scripts, and run the following command: -`python -m scripts.test_imu` +`python -m scripts.run_imu` For the pi, you will have to use python3 """ diff --git a/scripts/run_main_local.py b/scripts/run_main_local.py index 69861c2b..c453790d 100644 --- a/scripts/run_main_local.py +++ b/scripts/run_main_local.py @@ -1,11 +1,11 @@ """The mocked main file which can be run locally. To run this, make sure you're not inside scripts, -and run the following command: `python -m scripts.test_imu`""" +and run the following command: `python -m scripts.run_main_local`""" from airbrakes.airbrakes import AirbrakesContext -from airbrakes.constants import FREQUENCY, LOGS_PATH, MAX_EXTENSION, MIN_EXTENSION, PORT, SERVO_PIN, UPSIDE_DOWN -from airbrakes.imu.imu import IMU -from airbrakes.logger import Logger -from airbrakes.servo import Servo +from constants import FREQUENCY, LOGS_PATH, MAX_EXTENSION, MIN_EXTENSION, PORT, SERVO_PIN, UPSIDE_DOWN +from airbrakes.hardware.imu import IMU +from airbrakes.data_handling.logger import Logger +from airbrakes.hardware.servo import Servo from gpiozero.pins.mock import MockFactory, MockPWMPin diff --git a/scripts/run_servo.py b/scripts/run_servo.py index 6562ca1a..70ac2ab5 100644 --- a/scripts/run_servo.py +++ b/scripts/run_servo.py @@ -1,6 +1,6 @@ """ Make sure you are in the root directory of the project, not inside scripts, and run the following command: -`python -m scripts.test_servo` +`python -m scripts.run_servo` For the pi, you will have to use python3 """ diff --git a/tests/test_imu.py b/tests/test_imu.py index 73de0818..2b290a84 100644 --- a/tests/test_imu.py +++ b/tests/test_imu.py @@ -64,15 +64,15 @@ def test_imu_ctrl_c_handling(self, monkeypatch): """Tests whether the IMU's stop() handles Ctrl+C fine.""" values = multiprocessing.Queue(100000) - def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): + def _fetch_data_loop(self, port: str, frequency: int): """Monkeypatched method for testing.""" signal.signal(signal.SIGINT, signal.SIG_IGN) while self._running.value: continue - values.put((port, frequency, upside_down)) + values.put((port, frequency)) monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) - imu = IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) + imu = IMU(port=PORT, frequency=FREQUENCY) imu.start() assert imu._running.value assert imu.is_running @@ -88,28 +88,28 @@ def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): assert not imu.is_running assert not imu._data_fetch_process.is_alive() assert values.qsize() == 1 - assert values.get() == (PORT, FREQUENCY, UPSIDE_DOWN) + assert values.get() == (PORT, FREQUENCY) def test_imu_fetch_loop_exception(self, monkeypatch): """Tests whether the IMU's _fetch_loop propogates unknown exceptions.""" values = multiprocessing.Queue() - def _fetch_data_loop(self, port: str, frequency: int, upside_down: bool): + def _fetch_data_loop(self, port: str, frequency: int): """Monkeypatched method for testing.""" - values.put((port, frequency, upside_down)) + values.put((port, frequency)) raise ValueError("some error") monkeypatch.setattr(IMU, "_fetch_data_loop", _fetch_data_loop) - imu = IMU(port=PORT, frequency=FREQUENCY, upside_down=UPSIDE_DOWN) + imu = IMU(port=PORT, frequency=FREQUENCY) imu.start() with pytest.raises(ValueError, match="some error") as excinfo: - imu._fetch_data_loop(PORT, FREQUENCY, UPSIDE_DOWN) + imu._fetch_data_loop(PORT, FREQUENCY) imu.stop() assert not imu._running.value assert not imu.is_running assert not imu._data_fetch_process.is_alive() assert values.qsize() == 2 - assert values.get() == (PORT, FREQUENCY, UPSIDE_DOWN) + assert values.get() == (PORT, FREQUENCY) assert "some error" in str(excinfo.value) def test_data_packets_fetch(self, monkeypatch): From b4f22ceaddd670e4b528c9d310c956b56e193d7b Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Sun, 22 Sep 2024 03:32:32 -0400 Subject: [PATCH 6/7] Fix tests --- tests/test_airbrakes.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_airbrakes.py b/tests/test_airbrakes.py index d7c42ece..d9d8a660 100644 --- a/tests/test_airbrakes.py +++ b/tests/test_airbrakes.py @@ -7,7 +7,7 @@ @pytest.fixture def airbrakes(imu, logger, servo, data_processor): - return AirbrakesContext(logger, servo, imu, data_processor) + return AirbrakesContext(servo, imu, logger, data_processor) @pytest.mark.filterwarnings("ignore:To reduce servo jitter") # ignore warning about servo jitter @@ -32,11 +32,13 @@ def test_init(self, airbrakes, logger, imu, servo, data_processor): def test_set_extension(self, airbrakes): # Hardcoded calculated values, based on MIN_EXTENSION and MAX_EXTENSION in constants.py airbrakes.set_airbrake_extension(0.5) - # TODO: airbrakes.current_extension must be set to 0.5 !! + assert airbrakes.current_extension == 0.5 assert airbrakes.servo.current_extension == 0.0803 airbrakes.set_airbrake_extension(0.0) + assert airbrakes.current_extension == 0.0 assert airbrakes.servo.current_extension == -0.0999 airbrakes.set_airbrake_extension(1.0) + assert airbrakes.current_extension == 1.0 assert airbrakes.servo.current_extension == 0.2605 def test_start(self, airbrakes): From 5d6f5ce5fd673375b989f8c1bb224631cd76a9f0 Mon Sep 17 00:00:00 2001 From: Sailor Date: Wed, 25 Sep 2024 00:02:03 +0100 Subject: [PATCH 7/7] Fix run_main_local Co-authored-by: Harshil --- scripts/run_main_local.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/run_main_local.py b/scripts/run_main_local.py index c453790d..bcf9af20 100644 --- a/scripts/run_main_local.py +++ b/scripts/run_main_local.py @@ -3,6 +3,7 @@ from airbrakes.airbrakes import AirbrakesContext from constants import FREQUENCY, LOGS_PATH, MAX_EXTENSION, MIN_EXTENSION, PORT, SERVO_PIN, UPSIDE_DOWN +from airbrakes.data_handling.data_processor import IMUDataProcessor from airbrakes.hardware.imu import IMU from airbrakes.data_handling.logger import Logger from airbrakes.hardware.servo import Servo @@ -13,10 +14,12 @@ def main(): logger = Logger(LOGS_PATH) servo = Servo(SERVO_PIN, MIN_EXTENSION, MAX_EXTENSION, pin_factory=MockFactory(pin_class=MockPWMPin)) - imu = IMU(PORT, FREQUENCY, UPSIDE_DOWN) + imu = IMU(PORT, FREQUENCY) + data_processor = IMUDataProcessor([], UPSIDE_DOWN) + # The context that will manage the airbrakes state machine - airbrakes = AirbrakesContext(logger, servo, imu) + airbrakes = AirbrakesContext(servo, imu, logger, data_processor) try: airbrakes.start() # Start the IMU and logger processes # This is the main loop that will run until we press Ctrl+C