feat(api): make modules aware of available fw updates in file system #4856

b-cooper · 2020-01-24T21:57:19Z

overview

Creates a generalized approach for determining whether an attached module is capable of being
updated, based on the module firmware artifacts bundled in the filesystem. In addition to adding
this functionality, it includes a couple of refactors to dedupe common module subclass
responsibilities. In debugging this branch, I also included some loop level error handling so
exceptions in asyncio tasks (e.g. module instance creation) don't get swallowed.

re #4575

changelog

review requests

Creates a generalized approach for determining whether an attached module is capable of being updated, based on the module firmware artifacts bundled in the filesystem. In addition to adding this functionality, it includes a couple of refactors to dedupe common module subclass responsibilities. In debugging this branch, I also included some loop level error handling so exceptions in asyncio tasks (e.g. module instance creation) don't get swallowed. re #4575

codecov · 2020-01-24T22:23:01Z

Codecov Report

Merging #4856 into edge will decrease coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #4856      +/-   ##
==========================================
- Coverage   57.97%   57.78%   -0.19%     
==========================================
  Files         960      962       +2     
  Lines       26311    26639     +328     
==========================================
+ Hits        15253    15393     +140     
- Misses      11058    11246     +188

Impacted Files	Coverage Δ
opentrons/hardware_control/modules/types.py	`87.5% <0%> (-12.5%)`	⬇️
opentrons/main.py	`34.61% <0%> (-0.63%)`	⬇️
opentrons/hardware_control/modules/tempdeck.py	`80.31% <0%> (-0.61%)`	⬇️
opentrons/hardware_control/modules/magdeck.py	`87.5% <0%> (-0.47%)`	⬇️
opentrons/hardware_control/modules/thermocycler.py	`90.04% <0%> (-0.19%)`	⬇️
opentrons/config/__init__.py	`58.11% <0%> (-0.09%)`	⬇️
...er/src/components/modals/EditModulesModal/index.js	`0% <0%> (ø)`	⬆️
shared-data/js/constants.js	`100% <0%> (ø)`	⬆️
...list/formLevel/getNextDefaultMagnetAction/index.js	`100% <0%> (ø)`	⬆️
...ol-designer/src/components/modules/CrashInfoBox.js	`0% <0%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b388c4e...e648312. Read the comment docs.

sfoster1

Early thoughts on the draft. Structurally it looks ok but there's some inline comments

sfoster1 · 2020-01-27T15:33:02Z

api/src/opentrons/hardware_control/__init__.py

+def use_or_initialize_loop(loop: Optional[asyncio.AbstractEventLoop]):
+    checked_loop = loop or asyncio.get_event_loop()
+
+    # TODO: BC 2020-01-24 use loop.add_signal_handler for proper cleanup


i give this a big maybe - signal handlers have to be added from the main thread every time, and some signals can't be handled anyway.

sfoster1 · 2020-01-27T15:34:20Z

api/src/opentrons/hardware_control/__init__.py

@@ -44,6 +44,19 @@ def _log_call_inner(*args, **kwargs):
    return _log_call_inner


+def handle_loop_exception(loop: asyncio.AbstractEventLoop, context):
+    msg = context.get("exception", context["message"])


Bit of a nitpick but we should really log both the exception and the message. Per the docs, neither is optional so we shouldn't have to use get here.

sfoster1 · 2020-01-27T15:34:47Z

api/src/opentrons/hardware_control/__init__.py

@@ -44,6 +44,19 @@ def _log_call_inner(*args, **kwargs):
    return _log_call_inner


+def handle_loop_exception(loop: asyncio.AbstractEventLoop, context):


context could probably be typed as Dict[str, Any]

sfoster1 · 2020-01-27T15:35:03Z

api/src/opentrons/hardware_control/__init__.py

+    mod_log.error(f"Caught exception: {msg}")
+
+
+def use_or_initialize_loop(loop: Optional[asyncio.AbstractEventLoop]):


Annotate return type as asyncio.AbstractEventLoop

sfoster1 · 2020-01-27T15:35:54Z

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

+                 simulating: bool = False,
+                 loop: asyncio.AbstractEventLoop = None) -> None:
+        self._port = port
+        if None is loop:


can use that helper function here

sfoster1 · 2020-01-27T15:39:28Z

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

+        fw_dir = CONFIG['robot_firmware_dir']
+        fw_resources = [fw_dir / item for item in os.listdir(fw_dir)]
+        for fw_resource in fw_resources:
+            matches = MODULE_FW_RE.search(str(fw_resource))


We could and probably should do a match on fw_resource.name

sfoster1 · 2020-01-27T15:40:45Z

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

+    def has_available_update(self) -> bool:
+        """ Return whether a newer firmware file is available """
+        if self._device_info is not None:
+            raw_device_version = self._device_info.get('version', None)


In what conditions do we expect device_info to be valid but not have a version? Also we can just do .get('version'), returning None on key miss is implicit

sfoster1 · 2020-01-27T15:42:21Z

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

+        else:
+            self._loop = loop
+        self._device_info = None
+        self._available_update_version: Optional[str] = None


IMO we should bundle these two into a named tuple or something and have a function that returns Optional[ModuleFirmware] so we can't possibly be in a one-but-not-the-other situation

sfoster1 · 2020-01-27T15:43:46Z

api/src/opentrons/main.py

-        resources.extend([ROBOT_FIRMWARE_DIR / item
-                          for item in os.listdir(ROBOT_FIRMWARE_DIR)])
+        fw_dir = CONFIG['robot_firmware_dir']
+        resources.extend([fw_dir / item for item in os.listdir(fw_dir)])


can be resources.extend(fw_dir.iterdir()) (or resources.extend(list(fw_dir.iterdir())) if extend doesn't iterate)

sfoster1 · 2020-01-27T15:44:27Z

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

@@ -402,6 +395,10 @@ def set_loop(self, newLoop):
    def port(self):
        return self._port

+    @property
+    def available_update_path(self):


needs a return value annotation

Co-Authored-By: Seth Foster <[email protected]>

sfoster1

Looking really good!

b-cooper added 9 commits January 23, 2020 11:20

skeleton of version discoverer

31151dd

available version skeleton additions

74f46dd

investigative logging

f78de7b

abstract out shared module init logic

10a4822

move shared version getters to mod abc

8c2c286

fix file prefix map spelling

4152255

remove unused has_available_updates in subclasses

a53a589

move robot fw dir to proper config

e8ad8bc

b-cooper force-pushed the api_mod-fw-update-from-local-file branch from aebf2db to 5f370a2 Compare January 24, 2020 22:03

b-cooper changed the title ~~Api mod fw update from local file~~ feat(api): make modules aware of available fw updates in file system Jan 24, 2020

b-cooper requested review from a team January 24, 2020 22:11

b-cooper added api Affects the `api` project feature Ticket is a feature request / PR introduces a feature labels Jan 24, 2020

linting and typing

77b3e26

sfoster1 requested changes Jan 27, 2020

View reviewed changes

b-cooper and others added 6 commits January 27, 2020 11:33

Update api/src/opentrons/hardware_control/modules/mod_abc.py

f76b359

Co-Authored-By: Seth Foster <[email protected]>

address seth draft comments

7a494d0

update config

511b0ba

fix module unit test

a356588

failing on config test stub

a89daca

add some test coverage for mod abc

e648312

sfoster1 approved these changes Jan 28, 2020

View reviewed changes

b-cooper marked this pull request as ready for review January 28, 2020 19:54

b-cooper merged commit 4ede522 into edge Jan 28, 2020

b-cooper deleted the api_mod-fw-update-from-local-file branch January 28, 2020 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): make modules aware of available fw updates in file system #4856

feat(api): make modules aware of available fw updates in file system #4856

b-cooper commented Jan 24, 2020 •

edited

Loading

codecov bot commented Jan 24, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 Jan 27, 2020

sfoster1 left a comment

		@@ -44,6 +44,19 @@ def _log_call_inner(args, *kwargs):
		return _log_call_inner


		def handle_loop_exception(loop: asyncio.AbstractEventLoop, context):

		mod_log.error(f"Caught exception: {msg}")


		def use_or_initialize_loop(loop: Optional[asyncio.AbstractEventLoop]):

feat(api): make modules aware of available fw updates in file system #4856

feat(api): make modules aware of available fw updates in file system #4856

Conversation

b-cooper commented Jan 24, 2020 • edited Loading

overview

changelog

review requests

codecov bot commented Jan 24, 2020 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

b-cooper commented Jan 24, 2020 •

edited

Loading

codecov bot commented Jan 24, 2020 •

edited

Loading