fix(api): detect old bootloaders, fix tc enter bootloader, remove unu…

…nused 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

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:

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)

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

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

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

api/src/opentrons/hardware_control/modules/types.py

@@ -11,3 +11,7 @@ class BundledFirmware(NamedTuple):
 
     def __repr__(self) -> str:
         return f'<BundledFirmware {self.version}, path={self.path}>'
+
+
+class UpdateError(RuntimeError):
+    pass

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,

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'

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):