From 5def3f674155d1f1445c39a8b63670321c9517b2 Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 25 Jan 2023 15:05:52 -0500 Subject: [PATCH 1/9] refactor(hardware): make RunUpdate a class We're going to track update progress and creating this class facilitates this. --- .../backends/ot3controller.py | 9 +- .../firmware_update/__init__.py | 5 +- .../opentrons_hardware/firmware_update/run.py | 189 +++++++++--------- .../firmware_update/types.py | 10 + .../opentrons_hardware/scripts/update_fw.py | 12 +- .../opentrons_hardware/scripts/update_fws.py | 5 +- .../firmware_integration/test_update_fw.py | 4 +- .../firmware_update/test_run.py | 8 +- 8 files changed, 132 insertions(+), 110 deletions(-) create mode 100644 hardware/opentrons_hardware/firmware_update/types.py diff --git a/api/src/opentrons/hardware_control/backends/ot3controller.py b/api/src/opentrons/hardware_control/backends/ot3controller.py index 57f1eedbec4..7b51019bc2b 100644 --- a/api/src/opentrons/hardware_control/backends/ot3controller.py +++ b/api/src/opentrons/hardware_control/backends/ot3controller.py @@ -700,16 +700,19 @@ def fw_version(self) -> Optional[str]: async def update_firmware(self, filename: str, target: OT3SubSystem) -> None: """Update the firmware.""" with open(filename, "r") as f: - await firmware_update.run_update( + update_details = { + sub_system_to_node_id(target): f, + } + updater = firmware_update.RunUpdate( messenger=self._messenger, - node_id=sub_system_to_node_id(target), - hex_file=f, + update_details=update_details, # TODO (amit, 2022-04-05): Fill in retry_count and timeout_seconds from # config values. retry_count=3, timeout_seconds=20, erase=True, ) + await updater.run_updates() def engaged_axes(self) -> OT3AxisMap[bool]: """Get engaged axes.""" diff --git a/hardware/opentrons_hardware/firmware_update/__init__.py b/hardware/opentrons_hardware/firmware_update/__init__.py index 16de70851b9..3ea76c4e813 100644 --- a/hardware/opentrons_hardware/firmware_update/__init__.py +++ b/hardware/opentrons_hardware/firmware_update/__init__.py @@ -6,7 +6,7 @@ from .downloader import FirmwareUpdateDownloader from .hex_file import from_hex_file_path, from_hex_file, HexRecordProcessor from .eraser import FirmwareUpdateEraser -from .run import run_update, run_updates +from .run import RunUpdate __all__ = [ "FirmwareUpdateDownloader", @@ -15,6 +15,5 @@ "from_hex_file_path", "from_hex_file", "HexRecordProcessor", - "run_update", - "run_updates", + "RunUpdate", ] diff --git a/hardware/opentrons_hardware/firmware_update/run.py b/hardware/opentrons_hardware/firmware_update/run.py index 40204549984..492d88d7468 100644 --- a/hardware/opentrons_hardware/firmware_update/run.py +++ b/hardware/opentrons_hardware/firmware_update/run.py @@ -1,7 +1,8 @@ """Complete FW updater.""" import logging import asyncio -from typing import Optional, TextIO, Dict +from typing import Optional, TextIO, Dict, Tuple +from .types import FirmwareUpdateStatus from opentrons_hardware.drivers.can_bus import CanMessenger from opentrons_hardware.firmware_bindings import NodeId @@ -18,98 +19,104 @@ logger = logging.getLogger(__name__) - -async def run_update( - messenger: CanMessenger, - node_id: NodeId, - hex_file: TextIO, - retry_count: int, - timeout_seconds: float, - erase: Optional[bool] = True, -) -> None: - """Perform a firmware update on a node target. - - Args: - messenger: The can messenger to use. - node_id: The node being updated. - hex_file: File containing firmware. - retry_count: Number of times to retry. - timeout_seconds: How much to wait for responses. - erase: Whether to erase flash before updating. - - Returns: - None - """ - hex_processor = HexRecordProcessor.from_file(hex_file) - - initiator = FirmwareUpdateInitiator(messenger) - downloader = FirmwareUpdateDownloader(messenger) - - target = Target(system_node=node_id) - - logger.info(f"Initiating FW Update on {target}.") - - await initiator.run( - target=target, - retry_count=retry_count, - ready_wait_time_sec=timeout_seconds, - ) - if erase: - eraser = FirmwareUpdateEraser(messenger) - logger.info(f"Erasing existing FW Update on {target}.") - await eraser.run( +UpdateDict = Dict[NodeId, TextIO] +StatusDict = Dict[NodeId, Tuple[FirmwareUpdateStatus, int]] + + +class RunUpdate: + """Class for updating robot microcontroller firmware.""" + + def __init__( + self, + messenger: CanMessenger, + update_details: UpdateDict, + retry_count: int, + timeout_seconds: float, + erase: Optional[bool] = True, + ) -> None: + """Initialize RunUpdate class. + + Args: + messenger: The can messenger to use. + update_details: Dict of nodes to be updated and their firmware files. + retry_count: Number of times to retry. + timeout_seconds: How much to wait for responses. + erase: Whether to erase flash before updating. + + Returns: + None + """ + self._messenger = messenger + self._update_details = update_details + self._retry_count = retry_count + self._timeout_seconds = timeout_seconds + self._erase = erase + self._status_dict = { + node_id: (FirmwareUpdateStatus.queued, 0) + for node_id in update_details.keys() + } + + @staticmethod + async def _run_update( + messenger: CanMessenger, + node_id: NodeId, + hex_file: TextIO, + retry_count: int, + timeout_seconds: float, + erase: Optional[bool] = True, + ) -> None: + """Perform a firmware update on a node target.""" + hex_processor = HexRecordProcessor.from_file(hex_file) + + initiator = FirmwareUpdateInitiator(messenger) + downloader = FirmwareUpdateDownloader(messenger) + + target = Target(system_node=node_id) + + logger.info(f"Initiating FW Update on {target}.") + + await initiator.run( + target=target, + retry_count=retry_count, + ready_wait_time_sec=timeout_seconds, + ) + if erase: + eraser = FirmwareUpdateEraser(messenger) + logger.info(f"Erasing existing FW Update on {target}.") + await eraser.run( + node_id=target.bootloader_node, + timeout_sec=timeout_seconds, + ) + else: + logger.info("Skipping erase step.") + + logger.info(f"Downloading FW to {target.bootloader_node}.") + await downloader.run( node_id=target.bootloader_node, - timeout_sec=timeout_seconds, + hex_processor=hex_processor, + ack_wait_seconds=timeout_seconds, ) - else: - logger.info("Skipping erase step.") - - logger.info(f"Downloading FW to {target.bootloader_node}.") - await downloader.run( - node_id=target.bootloader_node, - hex_processor=hex_processor, - ack_wait_seconds=timeout_seconds, - ) - - logger.info(f"Restarting FW on {target.system_node}.") - await messenger.send( - node_id=target.bootloader_node, - message=FirmwareUpdateStartApp(), - ) - -UpdateDict = Dict[NodeId, TextIO] - - -async def run_updates( - messenger: CanMessenger, - update_details: UpdateDict, - retry_count: int, - timeout_seconds: float, - erase: Optional[bool] = True, -) -> None: - """Perform a firmware update on multiple node targets. - - Args: - messenger: The can messenger to use. - update_details: Dict of nodes to be updated and their firmware files. - retry_count: Number of times to retry. - timeout_seconds: How much to wait for responses. - erase: Whether to erase flash before updating. - - Returns: - None - """ - tasks = [ - run_update( - messenger=messenger, - node_id=node_id, - hex_file=hex_file, - retry_count=retry_count, - timeout_seconds=timeout_seconds, - erase=erase, + logger.info(f"Restarting FW on {target.system_node}.") + await messenger.send( + node_id=target.bootloader_node, + message=FirmwareUpdateStartApp(), ) - for node_id, hex_file in update_details.items() - ] - await asyncio.gather(*tasks) + async def run_updates( + self, + ) -> None: + """Perform a firmware update on multiple node targets.""" + tasks = [ + self._run_update( + messenger=self._messenger, + node_id=node_id, + hex_file=hex_file, + retry_count=self._retry_count, + timeout_seconds=self._timeout_seconds, + erase=self._erase, + ) + for node_id, hex_file in self._update_details.items() + ] + + await asyncio.gather(*tasks) diff --git a/hardware/opentrons_hardware/firmware_update/types.py b/hardware/opentrons_hardware/firmware_update/types.py new file mode 100644 index 00000000000..e7d685a18bf --- /dev/null +++ b/hardware/opentrons_hardware/firmware_update/types.py @@ -0,0 +1,10 @@ +"""Types for firmware updates.""" +from enum import Enum, auto + + +class FirmwareUpdateStatus(Enum): + """Firmware Update Status for each Node.""" + + queued = auto() + updating = auto() + done = auto() diff --git a/hardware/opentrons_hardware/scripts/update_fw.py b/hardware/opentrons_hardware/scripts/update_fw.py index e393f7c6c13..ae1b9e4f2f9 100644 --- a/hardware/opentrons_hardware/scripts/update_fw.py +++ b/hardware/opentrons_hardware/scripts/update_fw.py @@ -9,7 +9,7 @@ from opentrons_hardware.drivers.can_bus import build from opentrons_hardware.firmware_bindings import NodeId -from opentrons_hardware.firmware_update.run import run_update +from opentrons_hardware.firmware_update.run import RunUpdate from .can_args import add_can_args, build_settings @@ -48,20 +48,22 @@ async def run(args: argparse.Namespace) -> None: """Entry point for script.""" - target = TARGETS[args.target] retry_count = args.retry_count timeout_seconds = args.timeout_seconds erase = not args.no_erase + update_details = { + TARGETS[args.target]: args.file, + } async with build.can_messenger(build_settings(args)) as messenger: - await run_update( + updater = RunUpdate( messenger=messenger, - node_id=target, - hex_file=args.file, + update_details=update_details, retry_count=retry_count, timeout_seconds=timeout_seconds, erase=erase, ) + await updater.run_updates() logger.info("Done") diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index 71032b1bf38..36cd8339939 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -9,7 +9,7 @@ from opentrons_hardware.drivers.can_bus import build from opentrons_hardware.firmware_bindings import NodeId -from opentrons_hardware.firmware_update.run import run_updates +from opentrons_hardware.firmware_update.run import RunUpdate from .can_args import add_can_args, build_settings @@ -58,13 +58,14 @@ async def run(args: argparse.Namespace) -> None: } async with build.can_messenger(build_settings(args)) as messenger: - await run_updates( + updater = RunUpdate( messenger=messenger, update_details=update_details, retry_count=retry_count, timeout_seconds=timeout_seconds, erase=erase, ) + await updater.run_updates() logger.info("Done") diff --git a/hardware/tests/firmware_integration/test_update_fw.py b/hardware/tests/firmware_integration/test_update_fw.py index bc71736603b..de3cc979d3f 100644 --- a/hardware/tests/firmware_integration/test_update_fw.py +++ b/hardware/tests/firmware_integration/test_update_fw.py @@ -6,7 +6,7 @@ from opentrons_hardware.drivers.can_bus import CanMessenger from opentrons_hardware.firmware_bindings import NodeId -from opentrons_hardware.firmware_update import run_update +from opentrons_hardware.firmware_update import RunUpdate @pytest.fixture @@ -29,7 +29,7 @@ async def test_update( ) -> None: """It should complete the download.""" with hex_file_path.open("r") as f: - await run_update( + await RunUpdate._run_update( messenger=can_messenger, node_id=system_node_id, hex_file=f, diff --git a/hardware/tests/opentrons_hardware/firmware_update/test_run.py b/hardware/tests/opentrons_hardware/firmware_update/test_run.py index 75ca2693de7..abef8efcf63 100644 --- a/hardware/tests/opentrons_hardware/firmware_update/test_run.py +++ b/hardware/tests/opentrons_hardware/firmware_update/test_run.py @@ -13,9 +13,8 @@ FirmwareUpdateInitiator, FirmwareUpdateDownloader, FirmwareUpdateEraser, - run_update, - run_updates, HexRecordProcessor, + RunUpdate, ) from opentrons_hardware.firmware_update.target import Target @@ -64,7 +63,7 @@ async def test_run_update( mock_hex_record_builder.return_value = mock_hex_record_processor target = Target(system_node=NodeId.head) - await run_update( + await RunUpdate._run_update( messenger=mock_messenger, node_id=target.system_node, hex_file=mock_hex_file, @@ -114,13 +113,14 @@ async def test_run_updates( target_1.system_node: hex_file_1, target_2.system_node: hex_file_2, } - await run_updates( + updater = RunUpdate( messenger=mock_messenger, update_details=update_details, retry_count=12, timeout_seconds=11, erase=should_erase, ) + await updater.run_updates() mock_initiator_run.assert_has_calls( [ From 4d461041a69f4c77195393c8a4442072e172a97e Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Mon, 6 Feb 2023 18:43:28 -0700 Subject: [PATCH 2/9] feat(hardware): added progress reporting object and refactored to queue updating object --- .../firmware_update/downloader.py | 12 +++-- .../opentrons_hardware/firmware_update/run.py | 46 +++++++++++++++---- .../firmware_update/types.py | 6 +++ .../opentrons_hardware/scripts/update_fw.py | 3 +- .../opentrons_hardware/scripts/update_fws.py | 3 +- .../firmware_integration/test_update_fw.py | 12 ++++- .../firmware_update/test_downloader.py | 25 ++++++++-- .../firmware_update/test_run.py | 16 ++++++- 8 files changed, 100 insertions(+), 23 deletions(-) diff --git a/hardware/opentrons_hardware/firmware_update/downloader.py b/hardware/opentrons_hardware/firmware_update/downloader.py index 969e8c042a1..54b9ffafb6f 100644 --- a/hardware/opentrons_hardware/firmware_update/downloader.py +++ b/hardware/opentrons_hardware/firmware_update/downloader.py @@ -18,6 +18,7 @@ payloads, fields, ) +from typing import AsyncIterator logger = logging.getLogger(__name__) @@ -34,23 +35,23 @@ async def run( node_id: NodeId, hex_processor: HexRecordProcessor, ack_wait_seconds: float, - ) -> None: + ) -> AsyncIterator[float]: """Download hex record chunks to node. Args: node_id: The target node id. hex_processor: The producer of hex chunks. - ack_wait_seconds: Number of seconds to wait for an ACK + ack_wait_seconds: Number of seconds to wait for an ACK. Returns: None """ + chunks = list(hex_processor.process(fields.FirmwareUpdateDataField.NUM_BYTES)) + total_chunks = len(chunks) with WaitableCallback(self._messenger) as reader: num_messages = 0 crc32 = 0 - for chunk in hex_processor.process( - fields.FirmwareUpdateDataField.NUM_BYTES - ): + for chunk in chunks: logger.debug( f"Sending chunk {num_messages} to address {chunk.address:x}." ) @@ -72,6 +73,7 @@ async def run( crc32 = binascii.crc32(data, crc32) num_messages += 1 + yield num_messages / total_chunks # Create and send firmware update complete message. complete_message = message_definitions.FirmwareUpdateComplete( diff --git a/hardware/opentrons_hardware/firmware_update/run.py b/hardware/opentrons_hardware/firmware_update/run.py index 492d88d7468..bba1138387a 100644 --- a/hardware/opentrons_hardware/firmware_update/run.py +++ b/hardware/opentrons_hardware/firmware_update/run.py @@ -1,8 +1,8 @@ """Complete FW updater.""" import logging import asyncio -from typing import Optional, TextIO, Dict, Tuple -from .types import FirmwareUpdateStatus +from typing import Optional, TextIO, Dict, Tuple, AsyncIterator +from .types import FirmwareUpdateStatus, StatusElement from opentrons_hardware.drivers.can_bus import CanMessenger from opentrons_hardware.firmware_bindings import NodeId @@ -20,7 +20,6 @@ logger = logging.getLogger(__name__) UpdateDict = Dict[NodeId, TextIO] -StatusDict = Dict[NodeId, Tuple[FirmwareUpdateStatus, int]] class RunUpdate: @@ -55,9 +54,12 @@ def __init__( node_id: (FirmwareUpdateStatus.queued, 0) for node_id in update_details.keys() } + self._status_queue: "asyncio.Queue[Tuple[NodeId,StatusElement]]" = ( + asyncio.Queue() + ) - @staticmethod async def _run_update( + self, messenger: CanMessenger, node_id: NodeId, hex_file: TextIO, @@ -74,12 +76,18 @@ async def _run_update( target = Target(system_node=node_id) logger.info(f"Initiating FW Update on {target}.") + await self._status_queue.put((node_id, (FirmwareUpdateStatus.updating, 0))) await initiator.run( target=target, retry_count=retry_count, ready_wait_time_sec=timeout_seconds, ) + download_start_progress = 0.1 + await self._status_queue.put( + (node_id, (FirmwareUpdateStatus.updating, download_start_progress)) + ) + if erase: eraser = FirmwareUpdateEraser(messenger) logger.info(f"Erasing existing FW Update on {target}.") @@ -87,25 +95,40 @@ async def _run_update( node_id=target.bootloader_node, timeout_sec=timeout_seconds, ) + download_start_progress = 0.2 + await self._status_queue.put( + (node_id, (FirmwareUpdateStatus.updating, download_start_progress)) + ) else: logger.info("Skipping erase step.") logger.info(f"Downloading FW to {target.bootloader_node}.") - await downloader.run( + async for download_progress in downloader.run( node_id=target.bootloader_node, hex_processor=hex_processor, ack_wait_seconds=timeout_seconds, - ) + ): + await self._status_queue.put( + ( + node_id, + ( + FirmwareUpdateStatus.updating, + download_start_progress + + (0.9 - download_start_progress) * download_progress, + ), + ) + ) logger.info(f"Restarting FW on {target.system_node}.") await messenger.send( node_id=target.bootloader_node, message=FirmwareUpdateStartApp(), ) + await self._status_queue.put((node_id, (FirmwareUpdateStatus.done, 1))) async def run_updates( self, - ) -> None: + ) -> AsyncIterator[Tuple[NodeId, StatusElement]]: """Perform a firmware update on multiple node targets.""" tasks = [ self._run_update( @@ -119,4 +142,11 @@ async def run_updates( for node_id, hex_file in self._update_details.items() ] - await asyncio.gather(*tasks) + task = asyncio.create_task(asyncio.gather(*tasks)) + while True: + try: + yield await asyncio.wait_for(self._status_queue.get(), 0.25) + except asyncio.TimeoutError: + pass + if task.done(): + break diff --git a/hardware/opentrons_hardware/firmware_update/types.py b/hardware/opentrons_hardware/firmware_update/types.py index e7d685a18bf..645a686a8c4 100644 --- a/hardware/opentrons_hardware/firmware_update/types.py +++ b/hardware/opentrons_hardware/firmware_update/types.py @@ -1,5 +1,7 @@ """Types for firmware updates.""" from enum import Enum, auto +from typing import Dict, Tuple +from opentrons_hardware.firmware_bindings import NodeId class FirmwareUpdateStatus(Enum): @@ -8,3 +10,7 @@ class FirmwareUpdateStatus(Enum): queued = auto() updating = auto() done = auto() + + +StatusElement = Tuple[FirmwareUpdateStatus, float] +StatusDict = Dict[NodeId, StatusElement] diff --git a/hardware/opentrons_hardware/scripts/update_fw.py b/hardware/opentrons_hardware/scripts/update_fw.py index ae1b9e4f2f9..1c1c3feaa38 100644 --- a/hardware/opentrons_hardware/scripts/update_fw.py +++ b/hardware/opentrons_hardware/scripts/update_fw.py @@ -63,7 +63,8 @@ async def run(args: argparse.Namespace) -> None: timeout_seconds=timeout_seconds, erase=erase, ) - await updater.run_updates() + async for progress in updater.run_updates(): + logger.info("%s is %s and %f done", "progress[0], progress[1], progress[2]") logger.info("Done") diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index 36cd8339939..afaff6c32b6 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -65,7 +65,8 @@ async def run(args: argparse.Namespace) -> None: timeout_seconds=timeout_seconds, erase=erase, ) - await updater.run_updates() + async for progress in updater.run_updates(): + logger.info("%s is %s and %f done", "progress[0], progress[1], progress[2]") logger.info("Done") diff --git a/hardware/tests/firmware_integration/test_update_fw.py b/hardware/tests/firmware_integration/test_update_fw.py index de3cc979d3f..9015472e357 100644 --- a/hardware/tests/firmware_integration/test_update_fw.py +++ b/hardware/tests/firmware_integration/test_update_fw.py @@ -29,7 +29,17 @@ async def test_update( ) -> None: """It should complete the download.""" with hex_file_path.open("r") as f: - await RunUpdate._run_update( + update_details = { + system_node_id: f, + } + updater = RunUpdate( + messenger=can_messenger, + update_details=update_details, + retry_count=3, + timeout_seconds=60, + erase=True, + ) + await updater._run_update( messenger=can_messenger, node_id=system_node_id, hex_file=f, diff --git a/hardware/tests/opentrons_hardware/firmware_update/test_downloader.py b/hardware/tests/opentrons_hardware/firmware_update/test_downloader.py index cc325b45a44..ef633273102 100644 --- a/hardware/tests/opentrons_hardware/firmware_update/test_downloader.py +++ b/hardware/tests/opentrons_hardware/firmware_update/test_downloader.py @@ -109,7 +109,10 @@ def responder(node_id: NodeId, message: MessageDefinition) -> None: mock_hex_processor.process.return_value = iter(chunks) - await subject.run(NodeId.gantry_y_bootloader, mock_hex_processor, 10) + async for progress in subject.run( + NodeId.gantry_y_bootloader, mock_hex_processor, 10 + ): + pass # When the downloader runs it creates unique message indecies for each # message, this is a little hack to get the next index it would use @@ -179,7 +182,10 @@ def responder(node_id: NodeId, message: MessageDefinition) -> None: mock_hex_processor.process.return_value = iter(chunks) with pytest.raises(ErrorResponse): - await subject.run(NodeId.gantry_y_bootloader, mock_hex_processor, 10) + async for progress in subject.run( + NodeId.gantry_y_bootloader, mock_hex_processor, 10 + ): + pass async def test_messaging_complete_error_response( @@ -232,7 +238,10 @@ def responder(node_id: NodeId, message: MessageDefinition) -> None: mock_hex_processor.process.return_value = iter(chunks) with pytest.raises(ErrorResponse): - await subject.run(NodeId.gantry_y_bootloader, mock_hex_processor, 10) + async for progress in subject.run( + NodeId.gantry_y_bootloader, mock_hex_processor, 10 + ): + pass async def test_messaging_data_no_response( @@ -246,7 +255,10 @@ async def test_messaging_data_no_response( mock_hex_processor.process.return_value = iter(chunks) with pytest.raises(TimeoutResponse): - await subject.run(NodeId.gantry_y_bootloader, mock_hex_processor, 0.5) + async for progress in subject.run( + NodeId.gantry_y_bootloader, mock_hex_processor, 0.5 + ): + pass async def test_messaging_complete_no_response( @@ -283,4 +295,7 @@ def responder(node_id: NodeId, message: MessageDefinition) -> None: mock_hex_processor.process.return_value = iter(chunks) with pytest.raises(TimeoutResponse): - await subject.run(NodeId.gantry_y_bootloader, mock_hex_processor, 0.5) + async for progress in subject.run( + NodeId.gantry_y_bootloader, mock_hex_processor, 0.5 + ): + pass diff --git a/hardware/tests/opentrons_hardware/firmware_update/test_run.py b/hardware/tests/opentrons_hardware/firmware_update/test_run.py index abef8efcf63..39b5d3394e1 100644 --- a/hardware/tests/opentrons_hardware/firmware_update/test_run.py +++ b/hardware/tests/opentrons_hardware/firmware_update/test_run.py @@ -59,11 +59,22 @@ async def test_run_update( mock_messenger = AsyncMock() mock_hex_file = MagicMock() + hex_file = cast(TextIO, mock_hex_file) mock_hex_record_processor = MagicMock() mock_hex_record_builder.return_value = mock_hex_record_processor target = Target(system_node=NodeId.head) - await RunUpdate._run_update( + update_details = { + target.system_node: hex_file, + } + updater = RunUpdate( + messenger=mock_messenger, + update_details=update_details, + retry_count=12, + timeout_seconds=11, + erase=should_erase, + ) + await updater._run_update( messenger=mock_messenger, node_id=target.system_node, hex_file=mock_hex_file, @@ -120,7 +131,8 @@ async def test_run_updates( timeout_seconds=11, erase=should_erase, ) - await updater.run_updates() + async for progress in updater.run_updates(): + pass mock_initiator_run.assert_has_calls( [ From bc5d463450087a36721593565e88c85f31865a6d Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Tue, 7 Feb 2023 09:20:59 -0700 Subject: [PATCH 3/9] fix(hardware): fixed task aggregation and test --- hardware/opentrons_hardware/firmware_update/run.py | 2 +- hardware/tests/opentrons_hardware/firmware_update/test_run.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/hardware/opentrons_hardware/firmware_update/run.py b/hardware/opentrons_hardware/firmware_update/run.py index bba1138387a..4dd9b4fc610 100644 --- a/hardware/opentrons_hardware/firmware_update/run.py +++ b/hardware/opentrons_hardware/firmware_update/run.py @@ -142,7 +142,7 @@ async def run_updates( for node_id, hex_file in self._update_details.items() ] - task = asyncio.create_task(asyncio.gather(*tasks)) + task = asyncio.gather(*tasks) while True: try: yield await asyncio.wait_for(self._status_queue.get(), 0.25) diff --git a/hardware/tests/opentrons_hardware/firmware_update/test_run.py b/hardware/tests/opentrons_hardware/firmware_update/test_run.py index 39b5d3394e1..5f63cb2347c 100644 --- a/hardware/tests/opentrons_hardware/firmware_update/test_run.py +++ b/hardware/tests/opentrons_hardware/firmware_update/test_run.py @@ -158,11 +158,13 @@ async def test_run_updates( hex_processor=mock_hex_record_processor, ack_wait_seconds=11, ), + mock.call().__aiter__(), mock.call( node_id=target_2.bootloader_node, hex_processor=mock_hex_record_processor, ack_wait_seconds=11, ), + mock.call().__aiter__(), ] ) From 9ab01f5345e3a9fd4cc97859582c0e8de5080dac Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 8 Feb 2023 10:11:42 -0700 Subject: [PATCH 4/9] fix(hardware, api): properly work with AsyncIterator method output and fix api test --- api/src/opentrons/hardware_control/backends/ot3controller.py | 4 +++- .../hardware_control/backends/test_ot3_controller.py | 2 +- hardware/opentrons_hardware/scripts/update_fw.py | 2 +- hardware/opentrons_hardware/scripts/update_fws.py | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/hardware_control/backends/ot3controller.py b/api/src/opentrons/hardware_control/backends/ot3controller.py index d08abae10ed..f4930cd6b8a 100644 --- a/api/src/opentrons/hardware_control/backends/ot3controller.py +++ b/api/src/opentrons/hardware_control/backends/ot3controller.py @@ -209,7 +209,9 @@ async def update_firmware(self, filename: str, target: OT3SubSystem) -> None: timeout_seconds=20, erase=True, ) - await updater.run_updates() + async for progress in updater.run_updates(): + pass + # TODO (peter, 2023-02-08): Pass this progress data up the stack for reporting async def update_to_default_current_settings(self, gantry_load: GantryLoad) -> None: self._current_settings = get_current_settings( diff --git a/api/tests/opentrons/hardware_control/backends/test_ot3_controller.py b/api/tests/opentrons/hardware_control/backends/test_ot3_controller.py index 2783e882386..fe549b48504 100644 --- a/api/tests/opentrons/hardware_control/backends/test_ot3_controller.py +++ b/api/tests/opentrons/hardware_control/backends/test_ot3_controller.py @@ -771,7 +771,7 @@ async def fake_umpe( "opentrons.hardware_control.backends.ot3controller.update_motor_position_estimation", fake_umpe, ), patch( - "opentrons.hardware_control.backends.ot3controller.firmware_update.run_update" + "opentrons.hardware_control.backends.ot3controller.firmware_update.RunUpdate.run_updates" ), patch( "builtins.open", mock_open() ): diff --git a/hardware/opentrons_hardware/scripts/update_fw.py b/hardware/opentrons_hardware/scripts/update_fw.py index 1c1c3feaa38..ff14e9ef7d4 100644 --- a/hardware/opentrons_hardware/scripts/update_fw.py +++ b/hardware/opentrons_hardware/scripts/update_fw.py @@ -64,7 +64,7 @@ async def run(args: argparse.Namespace) -> None: erase=erase, ) async for progress in updater.run_updates(): - logger.info("%s is %s and %f done", "progress[0], progress[1], progress[2]") + logger.info(f"{progress[0]} is {progress[1][0]} and {progress[1][1]} done") logger.info("Done") diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index afaff6c32b6..a5a7b961893 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -66,7 +66,7 @@ async def run(args: argparse.Namespace) -> None: erase=erase, ) async for progress in updater.run_updates(): - logger.info("%s is %s and %f done", "progress[0], progress[1], progress[2]") + logger.info(f"{progress[0]} is {progress[1][0]} and {progress[1][1]} done") logger.info("Done") From a4e4326ef2bababfb9d4d185309f92779e0aa784 Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 8 Feb 2023 13:14:53 -0700 Subject: [PATCH 5/9] feat(hardware): updated update_fws to take a json dict of nodes and files to be updated --- .../opentrons_hardware/scripts/update_fws.py | 42 +++---------------- 1 file changed, 5 insertions(+), 37 deletions(-) diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index a5a7b961893..284f9ad3063 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -3,9 +3,7 @@ import asyncio import logging from logging.config import dictConfig -from typing import Dict, Any - -from typing_extensions import Final +from typing import Dict, Any, TextIO from opentrons_hardware.drivers.can_bus import build from opentrons_hardware.firmware_bindings import NodeId @@ -36,14 +34,7 @@ }, } -TARGETS: Final = { - "head": NodeId.head, - "gantry-x": NodeId.gantry_x, - "gantry-y": NodeId.gantry_y, - "pipette-left": NodeId.pipette_left, - "pipette-right": NodeId.pipette_right, - "gripper": NodeId.gripper, -} +UpdateDict = Dict[NodeId, TextIO] async def run(args: argparse.Namespace) -> None: @@ -52,10 +43,7 @@ async def run(args: argparse.Namespace) -> None: timeout_seconds = args.timeout_seconds erase = not args.no_erase - update_details = { - TARGETS[args.target1]: args.file1, - TARGETS[args.target2]: args.file2, - } + update_details = {node_id: hex_file for node_id, hex_file in args.dict.items()} async with build.can_messenger(build_settings(args)) as messenger: updater = RunUpdate( @@ -77,28 +65,8 @@ def main() -> None: add_can_args(parser) parser.add_argument( - "--target1", - help="The first FW subsystem to be updated.", - type=str, - required=True, - choices=TARGETS.keys(), - ) - parser.add_argument( - "--target2", - help="The second FW subsystem to be updated.", - type=str, - required=True, - choices=TARGETS.keys(), - ) - parser.add_argument( - "--file1", - help="Path to hex file containing the first FW executable.", - type=argparse.FileType("r"), - required=True, - ) - parser.add_argument( - "--file2", - help="Path to hex file containing the second FW executable.", + "--dict", + help="Path to json file containing the dictionary of node ids and hex files to be updated.", type=argparse.FileType("r"), required=True, ) From 01cf39854c646bd9b6e8a91d385a8f3598108ebe Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 8 Feb 2023 13:21:01 -0700 Subject: [PATCH 6/9] fix(hardware): properly load json file into dict for unpacking --- hardware/opentrons_hardware/scripts/update_fws.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index 284f9ad3063..c21b69a80eb 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -2,6 +2,7 @@ import argparse import asyncio import logging +import json from logging.config import dictConfig from typing import Dict, Any, TextIO @@ -43,7 +44,8 @@ async def run(args: argparse.Namespace) -> None: timeout_seconds = args.timeout_seconds erase = not args.no_erase - update_details = {node_id: hex_file for node_id, hex_file in args.dict.items()} + update_dict = json.load(args.dict) + update_details = {node_id: hex_file for node_id, hex_file in update_dict.items()} async with build.can_messenger(build_settings(args)) as messenger: updater = RunUpdate( From 289228332ca4f39f0e9202926ce70d312e931ea5 Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 8 Feb 2023 14:25:15 -0700 Subject: [PATCH 7/9] fix(hardware): properly assign node_id from json to NodeId element --- hardware/opentrons_hardware/scripts/update_fws.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index c21b69a80eb..4163c96ef8e 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -45,7 +45,7 @@ async def run(args: argparse.Namespace) -> None: erase = not args.no_erase update_dict = json.load(args.dict) - update_details = {node_id: hex_file for node_id, hex_file in update_dict.items()} + update_details = {NodeId[node_id]: hex_file for node_id, hex_file in update_dict.items()} async with build.can_messenger(build_settings(args)) as messenger: updater = RunUpdate( From da978ede88072bc4660f05b9c2c3dacfb81930d3 Mon Sep 17 00:00:00 2001 From: pmoegenburg Date: Wed, 8 Feb 2023 14:43:54 -0700 Subject: [PATCH 8/9] fix(hardware): linted --- hardware/opentrons_hardware/scripts/update_fws.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index 4163c96ef8e..de0a5e86a73 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -45,7 +45,9 @@ async def run(args: argparse.Namespace) -> None: erase = not args.no_erase update_dict = json.load(args.dict) - update_details = {NodeId[node_id]: hex_file for node_id, hex_file in update_dict.items()} + update_details = { + NodeId[node_id]: hex_file for node_id, hex_file in update_dict.items() + } async with build.can_messenger(build_settings(args)) as messenger: updater = RunUpdate( From 4de20d88580271af86a03cce31e995bd92782458 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Thu, 9 Feb 2023 10:08:44 -0500 Subject: [PATCH 9/9] script takes ios not paths --- hardware/opentrons_hardware/scripts/update_fws.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hardware/opentrons_hardware/scripts/update_fws.py b/hardware/opentrons_hardware/scripts/update_fws.py index de0a5e86a73..4798977698a 100644 --- a/hardware/opentrons_hardware/scripts/update_fws.py +++ b/hardware/opentrons_hardware/scripts/update_fws.py @@ -46,7 +46,8 @@ async def run(args: argparse.Namespace) -> None: update_dict = json.load(args.dict) update_details = { - NodeId[node_id]: hex_file for node_id, hex_file in update_dict.items() + NodeId[node_id]: open(hex_file, "r") + for node_id, hex_file in update_dict.items() } async with build.can_messenger(build_settings(args)) as messenger: