From cfa537495a97abf5486e67332e168d5b28f12a6c Mon Sep 17 00:00:00 2001 From: Brian Arthur Cooper Date: Fri, 7 Feb 2020 12:24:29 -0500 Subject: [PATCH] fix(api): detect old bootloaders, fix tc enter bootloader, remove ununused paths (#4935) Handle the case of a user attempting to update a module with an unsupported bootloader. Fix regression that caused thermocyclers to fail to enter programming mode. Remove unused code supporting old module update pathways. re #4575 --- .../opentrons/drivers/thermocycler/driver.py | 1 + .../hardware_control/modules/__init__.py | 8 +- .../hardware_control/modules/magdeck.py | 4 +- .../hardware_control/modules/tempdeck.py | 11 +- .../hardware_control/modules/thermocycler.py | 6 +- .../hardware_control/modules/types.py | 4 + .../hardware_control/modules/update.py | 93 ++----------- api/src/opentrons/server/endpoints/update.py | 4 +- .../hardware_control/test_modules.py | 128 +++++++++++------ api/tests/opentrons/labware/test_modules.py | 130 ------------------ 10 files changed, 121 insertions(+), 268 deletions(-) diff --git a/api/src/opentrons/drivers/thermocycler/driver.py b/api/src/opentrons/drivers/thermocycler/driver.py index 9edc9689500..b814f6d0a43 100644 --- a/api/src/opentrons/drivers/thermocycler/driver.py +++ b/api/src/opentrons/drivers/thermocycler/driver.py @@ -459,6 +459,7 @@ async def enter_programming_mode(self): self.port, TC_BOOTLOADER_BAUDRATE, timeout=1) await asyncio.sleep(0.05) trigger_connection.close() + self.disconnect() def __del__(self): try: diff --git a/api/src/opentrons/hardware_control/modules/__init__.py b/api/src/opentrons/hardware_control/modules/__init__.py index 37be39496c2..21d2edd4f5b 100644 --- a/api/src/opentrons/hardware_control/modules/__init__.py +++ b/api/src/opentrons/hardware_control/modules/__init__.py @@ -9,7 +9,7 @@ from .mod_abc import AbstractModule # Must import tempdeck and magdeck (and other modules going forward) so they # actually create the subclasses -from . import update, tempdeck, magdeck, thermocycler # noqa(W0611) +from . import update, tempdeck, magdeck, thermocycler, types # noqa(W0611) log = logging.getLogger(__name__) @@ -79,10 +79,6 @@ def discover() -> List[ModuleAtPort]: return discovered_modules -class UpdateError(RuntimeError): - pass - - async def update_firmware( module: AbstractModule, firmware_file: str, @@ -101,4 +97,4 @@ async def update_firmware( successful, res = await cls.bootloader()(flash_port, firmware_file, kwargs) if not successful: log.info(f'Bootloader reponse: {res}') - raise UpdateError(res) + raise types.UpdateError(res) diff --git a/api/src/opentrons/hardware_control/modules/magdeck.py b/api/src/opentrons/hardware_control/modules/magdeck.py index 91157563418..acacff4da23 100644 --- a/api/src/opentrons/hardware_control/modules/magdeck.py +++ b/api/src/opentrons/hardware_control/modules/magdeck.py @@ -198,6 +198,6 @@ def __del__(self): self._disconnect() async def prep_for_update(self) -> str: - new_port = await update.enter_bootloader(self._driver, - self.device_info['model']) + self._driver.enter_programming_mode() + new_port = await update.find_bootloader_port() return new_port or self.port diff --git a/api/src/opentrons/hardware_control/modules/tempdeck.py b/api/src/opentrons/hardware_control/modules/tempdeck.py index c4511e6e634..fcb4422d404 100644 --- a/api/src/opentrons/hardware_control/modules/tempdeck.py +++ b/api/src/opentrons/hardware_control/modules/tempdeck.py @@ -3,7 +3,7 @@ from typing import Union, Optional from opentrons.drivers.temp_deck import TempDeck as TempDeckDriver from opentrons.drivers.temp_deck.driver import temp_locks -from . import update, mod_abc +from . import update, mod_abc, types TEMP_POLL_INTERVAL_SECS = 1 @@ -216,10 +216,15 @@ def __del__(self): self._poller.join() async def prep_for_update(self) -> str: + model = self._device_info and self._device_info.get('model') + if model in ('temp_deck_v1', 'temp_deck_v2'): + raise types.UpdateError("This Temperature Module can't be updated." + "Please contact Opentrons Support.") + if self._poller: self._poller.join() del self._poller self._poller = None - model = self._device_info and self._device_info.get('model') - new_port = await update.enter_bootloader(self._driver, model) + self._driver.enter_programming_mode() + new_port = await update.find_bootloader_port() return new_port or self.port diff --git a/api/src/opentrons/hardware_control/modules/thermocycler.py b/api/src/opentrons/hardware_control/modules/thermocycler.py index e068f0af6f4..d7151e24ee8 100644 --- a/api/src/opentrons/hardware_control/modules/thermocycler.py +++ b/api/src/opentrons/hardware_control/modules/thermocycler.py @@ -396,6 +396,8 @@ def port(self): return self._port async def prep_for_update(self): - new_port = await update.enter_bootloader(self._driver, - self.name()) + await self._driver.enter_programming_mode() + + new_port = await update.find_bootloader_port() + return new_port or self.port diff --git a/api/src/opentrons/hardware_control/modules/types.py b/api/src/opentrons/hardware_control/modules/types.py index 227ce2cfad6..9faac168826 100644 --- a/api/src/opentrons/hardware_control/modules/types.py +++ b/api/src/opentrons/hardware_control/modules/types.py @@ -11,3 +11,7 @@ class BundledFirmware(NamedTuple): def __repr__(self) -> str: return f'' + + +class UpdateError(RuntimeError): + pass diff --git a/api/src/opentrons/hardware_control/modules/update.py b/api/src/opentrons/hardware_control/modules/update.py index e1cfb004874..127753eeef9 100644 --- a/api/src/opentrons/hardware_control/modules/update.py +++ b/api/src/opentrons/hardware_control/modules/update.py @@ -7,90 +7,25 @@ log = logging.getLogger(__name__) -PORT_SEARCH_TIMEOUT = 5.5 - -async def enter_bootloader(driver, model): - """ - Using the driver method, enter bootloader mode of the module board. - The bootloader mode opens a new port on the uC to upload the firmware file. - The new port shows up as 'ot_module_(avrdude|samba)_bootloader' on the pi; - upload fw through it. - NOTE: Some temperature and magnetic modules have an old bootloader which - will show up as a regular module port- 'ot_module_tempdeck'/ - 'ot_module_magdeck' with the port number being either different or - same as the one that the module was originally on, so we check for - changes in ports and use the appropriate one - """ - # Required for old bootloader - ports_before_bootloader = await _discover_ports() - - if model == 'thermocycler': - await driver.enter_programming_mode() - else: - driver.enter_programming_mode() - - new_port = '' - try: - new_port = await asyncio.wait_for( - _port_poll(_has_old_bootloader(model), ports_before_bootloader), - PORT_SEARCH_TIMEOUT) - except asyncio.TimeoutError: - pass - return new_port - - -async def _port_on_mode_switch(ports_before_switch): - ports_after_switch = await _discover_ports() - new_port = '' - if ports_after_switch and \ - len(ports_after_switch) >= len(ports_before_switch) and \ - not set(ports_before_switch) == set(ports_after_switch): - new_ports = list(filter( - lambda x: x not in ports_before_switch, - ports_after_switch)) - if len(new_ports) > 1: - raise OSError('Multiple new ports found on mode switch') - new_port = new_ports[0] - return new_port - - -async def _port_poll(is_old_bootloader, ports_before_switch=None): +async def find_bootloader_port(): """ - Checks for the bootloader port + Finds the port of an Opentrons Module that has entered its bootloader. + The bootloader port shows up as 'ot_module_(avrdude|samba)_bootloader' + on the pi; return found port. """ - new_port = '' - while not new_port: - if is_old_bootloader: - new_port = await _port_on_mode_switch(ports_before_switch) - else: - ports = await _discover_ports() - if ports: - discovered_ports = list(filter( - lambda x: 'bootloader' in x, ports)) - if len(discovered_ports) == 1: - new_port = discovered_ports[0] - elif len(discovered_ports) > 1: - raise OSError('Multiple new bootloader ports' - 'found on mode switch') - - await asyncio.sleep(0.05) - return new_port - - -def _has_old_bootloader(model: str) -> bool: - return model in ('temp_deck_v1', 'temp_deck_v2') - -async def _discover_ports(): - for attempt in range(2): - # Measure for race condition where port is being switched in - # between calls to isdir() and listdir() - module_ports = glob('/dev/ot_module*') - if module_ports: - return module_ports + for attempt in range(3): + bootloader_ports = glob('/dev/ot_module_*_bootloader*') + if bootloader_ports: + if len(bootloader_ports) == 1: + log.info(f'Found bootloader at port {bootloader_ports[0]}') + return bootloader_ports[0] + elif len(bootloader_ports) > 1: + raise OSError('Multiple new bootloader ports' + 'found on mode switch') await asyncio.sleep(2) - raise Exception("No ot_modules found in /dev. Try again") + raise Exception("No ot_module bootloaders found in /dev. Try again") async def upload_via_avrdude(port: str, diff --git a/api/src/opentrons/server/endpoints/update.py b/api/src/opentrons/server/endpoints/update.py index 85e8dc1001e..77d902d9dad 100644 --- a/api/src/opentrons/server/endpoints/update.py +++ b/api/src/opentrons/server/endpoints/update.py @@ -66,8 +66,8 @@ async def update_module_firmware(request): res = (f'Bundled fw file not found for module of ' f'type: {module.name()}') status = 500 - except modules.UpdateError as e: - res = f'Bootloader error: {e}' + except modules.types.UpdateError as e: + res = f'Update error: {e}' status = 500 except asyncio.TimeoutError: res = 'Module not responding' diff --git a/api/tests/opentrons/hardware_control/test_modules.py b/api/tests/opentrons/hardware_control/test_modules.py index 9ced26aa809..dfbd7c0db9f 100644 --- a/api/tests/opentrons/hardware_control/test_modules.py +++ b/api/tests/opentrons/hardware_control/test_modules.py @@ -1,13 +1,12 @@ -import pytest import asyncio from pathlib import Path +from unittest import mock try: import aionotify except OSError: aionotify = None # type: ignore from opentrons.hardware_control.modules import ModuleAtPort from opentrons.hardware_control.modules.types import BundledFirmware -from opentrons.hardware_control import Controller async def test_get_modules_simulating(): @@ -56,49 +55,90 @@ async def test_module_caching(): assert two_magdecks[1] is not two_magdecks[0] -@pytest.mark.skip('update module endpoint is unused for now') -@pytest.mark.skipif(aionotify is None, - reason="requires inotify (linux only)") -@pytest.mark.skipif(not Controller, - reason='hardware controller not available') -async def test_module_update_integration(monkeypatch, loop, - cntrlr_mock_connect, running_on_pi): - import opentrons.hardware_control as hardware_control - api = await hardware_control.API.build_hardware_controller(loop=loop) - - def mock_attached_modules(): - return [ModuleAtPort(port='/dev/ot_module_sim_tempdeck0', - name='tempdeck') - ] - - monkeypatch.setattr(api, 'attached_modules', - mock_attached_modules) - - async def mock_build_module(port, model, callback): - return await hardware_control.modules.build(port, - model, - True, - callback) - - monkeypatch.setattr(api._backend, 'build_module', mock_build_module) - - async def mock_discover_ports(): - return ['port1'] - - monkeypatch.setattr(hardware_control.modules.update, - '_discover_ports', mock_discover_ports) - - async def mock_upload(port, firmware_file_path, upload_function, loop): - return (port, (True, 'it all worked')) - - monkeypatch.setattr(hardware_control.modules.update, - 'upload_firmware', mock_upload) +async def test_module_update_integration(monkeypatch, loop): + from opentrons.hardware_control import modules - modules = api.attached_modules - ok, msg = await api.update_module(modules[0], 'some-fake-file', loop) - assert ok - new_modules = api.attached_modules - assert new_modules[0] is not modules[0] + def async_return(result): + f = asyncio.Future() + f.set_result(result) + return f + + bootloader_kwargs = { + 'stdout': asyncio.subprocess.PIPE, + 'stderr': asyncio.subprocess.PIPE, + 'loop': loop, + } + + # test temperature module update with avrdude bootloader + + tempdeck = await modules.build('/dev/ot_module_sim_tempdeck0', + 'tempdeck', + True, + lambda x: None) + + upload_via_avrdude_mock = mock.Mock( + return_value=(async_return((True, 'avrdude bootloader worked')))) + monkeypatch.setattr(modules.update, + 'upload_via_avrdude', + upload_via_avrdude_mock) + + async def mock_find_avrdude_bootloader_port(): + return 'ot_module_avrdude_bootloader1' + + monkeypatch.setattr(modules.update, + 'find_bootloader_port', + mock_find_avrdude_bootloader_port) + + await modules.update_firmware(tempdeck, 'fake_fw_file_path', loop) + upload_via_avrdude_mock.assert_called_once_with( + 'ot_module_avrdude_bootloader1', + 'fake_fw_file_path', + bootloader_kwargs + ) + upload_via_avrdude_mock.reset_mock() + + # test magnetic module update with avrdude bootloader + + magdeck = await modules.build('/dev/ot_module_sim_magdeck0', + 'magdeck', + True, + lambda x: None) + + await modules.update_firmware(magdeck, 'fake_fw_file_path', loop) + upload_via_avrdude_mock.assert_called_once_with( + 'ot_module_avrdude_bootloader1', + 'fake_fw_file_path', + bootloader_kwargs + ) + + # test thermocycler module update with bossa bootloader + + thermocycler = await modules.build('/dev/ot_module_sim_thermocycler0', + 'thermocycler', + True, + lambda x: None) + + upload_via_bossa_mock = mock.Mock( + return_value=(async_return((True, 'bossa bootloader worked')))) + monkeypatch.setattr(modules.update, + 'upload_via_bossa', + upload_via_bossa_mock) + + async def mock_find_bossa_bootloader_port(): + return 'ot_module_bossa_bootloader1' + + monkeypatch.setattr(modules.update, + 'find_bootloader_port', + mock_find_bossa_bootloader_port) + + await modules.update_firmware(thermocycler, + 'fake_fw_file_path', + loop) + upload_via_bossa_mock.assert_called_once_with( + 'ot_module_bossa_bootloader1', + 'fake_fw_file_path', + bootloader_kwargs + ) async def test_get_bundled_fw(monkeypatch, tmpdir): diff --git a/api/tests/opentrons/labware/test_modules.py b/api/tests/opentrons/labware/test_modules.py index 0f258763388..d14aae7a1ea 100644 --- a/api/tests/opentrons/labware/test_modules.py +++ b/api/tests/opentrons/labware/test_modules.py @@ -3,12 +3,10 @@ # TODO: 'Temperature Module' or similar import pytest -import asyncio from threading import Lock from opentrons.drivers.mag_deck import MagDeck as MagDeckDriver from opentrons.drivers.temp_deck import TempDeck as TempDeckDriver from opentrons.drivers import serial_communication -from opentrons.hardware_control import modules as hw_modules @pytest.fixture @@ -116,131 +114,3 @@ def test_load_correct_engage_height(robot, modules, labware): assert test_container.magdeck_engage_height() == 18 assert md.labware.get_children_list()[1].magdeck_engage_height() == \ test_container.magdeck_engage_height() - - -@pytest.fixture -async def old_bootloader_module(): - module = await hw_modules.build( - port='/dev/ot_module_tempdeck0', - which='tempdeck', - simulating=True, - interrupt_callback=lambda x: None) - module._device_info = {'model': 'temp_deck_v1'} - module._driver = TempDeckDriver() - return module - - -@pytest.fixture -async def new_bootloader_module(): - module = await hw_modules.build( - port='/dev/ot_module_tempdeck0', - which='tempdeck', - simulating=True, - interrupt_callback=lambda x: None) - module._device_info = {'model': 'temp_deck_v1.1'} - module._driver = TempDeckDriver() - return module - - -@pytest.mark.api2_only -async def test_enter_bootloader( - new_bootloader_module, virtual_smoothie_env, monkeypatch): - - async def mock_discover_ports_before_dfu_mode(): - return '/dev/ot_module_tempdeck0', - - def mock_enter_programming_mode(self): - return 'ok\n\rok\n\r' - - async def mock_port_poll(_has_old_bootloader, ports_before_dfu_mode): - return '/dev/ot_module_avrdude_bootloader' - - monkeypatch.setattr( - TempDeckDriver, 'enter_programming_mode', mock_enter_programming_mode) - monkeypatch.setattr( - hw_modules.update, - '_discover_ports', - mock_discover_ports_before_dfu_mode) - monkeypatch.setattr(hw_modules.update, '_port_poll', mock_port_poll) - - bootloader_port = await hw_modules.update.enter_bootloader( - new_bootloader_module._driver, - new_bootloader_module.name()) - - assert bootloader_port == '/dev/ot_module_avrdude_bootloader' - - -@pytest.mark.api2_only -def test_old_bootloader_check( - old_bootloader_module, new_bootloader_module, virtual_smoothie_env, -): - assert hw_modules.update._has_old_bootloader( - old_bootloader_module._device_info['model']) - assert not hw_modules.update._has_old_bootloader( - new_bootloader_module._device_info['model']) - - -@pytest.mark.api2_only -async def test_port_poll(virtual_smoothie_env, monkeypatch): - has_old_bootloader = False - timeout = 0.1 - monkeypatch.setattr(hw_modules.update, 'PORT_SEARCH_TIMEOUT', timeout) - - # Case 1: Bootloader port is successfully opened on the module - async def mock_discover_ports1(): - return ['/dev/ot_module_magdeck0', - '/dev/ot_module_avrdude_bootloader1'] - monkeypatch.setattr(hw_modules.update, - '_discover_ports', mock_discover_ports1) - - port_found = await asyncio.wait_for( - hw_modules.update._port_poll(has_old_bootloader, None), - hw_modules.update.PORT_SEARCH_TIMEOUT) - assert port_found == '/dev/ot_module_avrdude_bootloader1' - - # Case 2: Switching to bootloader mode failed - async def mock_discover_ports2(): - return ['/dev/ot_module_magdeck0', '/dev/ot_module_tempdeck1'] - monkeypatch.setattr(hw_modules.update, - '_discover_ports', mock_discover_ports2) - - with pytest.raises(asyncio.TimeoutError): - port_found = await asyncio.wait_for( - hw_modules.update._port_poll(has_old_bootloader, None), - hw_modules.update.PORT_SEARCH_TIMEOUT) - assert not port_found - - -@pytest.mark.api2_only -async def test_old_bootloader_port_poll( - virtual_smoothie_env, monkeypatch): - - ports_before_switch = [ - '/dev/ot_module_magdeck0', '/dev/ot_module_tempdeck1'] - has_old_bootloader = True - timeout = 0.1 - monkeypatch.setattr(hw_modules.update, 'PORT_SEARCH_TIMEOUT', timeout) - - # Case 1: Bootloader is opened on same port - async def mock_discover_ports(): - return ['/dev/ot_module_magdeck0', '/dev/ot_module_tempdeck1'] - monkeypatch.setattr(hw_modules.update, - '_discover_ports', mock_discover_ports) - - with pytest.raises(asyncio.TimeoutError): - port_found = await asyncio.wait_for( - hw_modules.update._port_poll( - has_old_bootloader, ports_before_switch), - hw_modules.update.PORT_SEARCH_TIMEOUT) - assert not port_found - - # Case 2: Bootloader is opened on a different port - async def mock_discover_ports(): - return ['/dev/ot_module_magdeck2', '/dev/ot_module_tempdeck1'] - monkeypatch.setattr(hw_modules.update, - '_discover_ports', mock_discover_ports) - - port_found = await asyncio.wait_for( - hw_modules.update._port_poll(has_old_bootloader, ports_before_switch), - hw_modules.update.PORT_SEARCH_TIMEOUT) - assert port_found == '/dev/ot_module_magdeck2'