feat(heater-shaker): add firmware upload capability #10307
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #10307 +/- ##
==========================================
+ Coverage 73.64% 74.74% +1.10%
==========================================
Files 2085 2052 -33
Lines 57605 55304 -2301
Branches 5741 5728 -13
==========================================
- Hits 42421 41338 -1083
+ Misses 13946 12728 -1218
Partials 1238 1238
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Adding info for testing:
|
| @@ -442,7 +442,10 @@ async def close_labware_latch(self) -> None: | |||
| await self._wait_for_labware_latch(HeaterShakerLabwareLatchStatus.IDLE_CLOSED) | |||
|
|
|||
| async def prep_for_update(self) -> str: | |||
| return "no" | |||
| await self._poller.stop_and_wait() # Check if this is really needed | |||
There was a problem hiding this comment.
I think it's a good idea to stop sending get_temp/rpm/latch_status messages (via poller) before sending the dfu gcode
There was a problem hiding this comment.
Oh absolutely. I just meant that it might not be required because the poller might be implicitly getting stopped because of the module connection closing. But we need to check that definitely happens in order to remove this.
| stdout, stderr = await proc.communicate() | ||
|
|
||
| if stdout is None and stderr is None: | ||
| # This probably needs to be refactored into just a check for empty stdout |
There was a problem hiding this comment.
shouldn't we check for stderr?
There was a problem hiding this comment.
You're right. We want to retry only if there is no output and no error.
I don't remember why I wanted to refactor this so we can tag that comment for removal if we don't run into any problems during testing.
| # which dfu device to upload firmware to, then we can specify the device serial | ||
| # (or the vid:pid) in the command. This will take care of the super rare case where | ||
| # a robot has two stm32 devices plugged in and a user initiates firmware upload for | ||
| # both of them in quick succession. It's a rare case but has happened before. |
There was a problem hiding this comment.
I think the device serial will change after entering bootloader, making it difficult to check for a specific module. I added a check for multiple bootloader devices (1 module lists 2 devices, so check is for greater than 2 devices). Since find_dfu_device checks only for devices in the bootloader state, shouldn't it be rare (or can we make it impossible) for multiple modules to update firmware concurrently?
There was a problem hiding this comment.
definitely a "make it impossible" sort of thing, yeah. could put a lock in the module manager
There was a problem hiding this comment.
@pmoegenburg yes, it's rare, but has happened before where an update was initiated on both temp and mag modules in quick succession and that resulted in the mag module getting the temp module's firmware and vice versa. It happened because they both had the same bootloader identity.
@sfoster1 will the lock prevent the firmware from going to the wrong module's bootloader? Or will Peter's check for 2+ DFUs alongwith the lock make it impossible for two modules to switch to the bootloader at the same time?
There was a problem hiding this comment.
@pmoegenburg @sfoster1 - The need to close out firmware update for our internal testing (and production build 7/25) is imminent. Do we know when this will be complete ?
There was a problem hiding this comment.
@sfoster1 what would that lock look like?
There was a problem hiding this comment.
Tested and it works!
We should still add unit tests for send_dfu_command but I'm fine leaving a TODO for it and adding it later so we can start using the upload feature. Another todo would be to test if the upload lock works by trying to update two heater-shakers at once.
PR is good to merge once my outdated inline comments are removed! (Let me know if you want me to remove them)
Updated upload_via_dfu method, which uses the dfu-util tool.
Closes #9634