Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(robot-server): initialize fw update status_cache so we dont hang on bootup when client queries fw updates. #15049

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Apr 30, 2024

Overview

The "instrument firmware update needed" modal was shown on bootup when a firmware update was required. This modal has a "Update Firmware" button that, when pressed, initiates the update for the attached instrument. However, since the updates have already been started internally, pressing the button does nothing until the first update is finished.

The main issue is that the /subsystem/updates/all request would hang until the first update was finished for ~2m, this happens because the start_update_process function called during bootup would hang waiting on provide_latest_progress which itself is waiting on to get a status report on self._status_queue.get() which blocks. Since the start_update_process acquires the _management_lock, once we call all_ongoing_processes from the client query we lock until an update is finished.

The problem is the "waiting to get the progress" of a specific update process from the hardware controller layer. The start_update_process function "queues" the firmware updates by creating the _UpdateProcess and then running the _update_task with TaskRunner.run in the background. This does not guarantee that the update will pass, but, as far as the robot server is concerned the update has been "queued".

So what this PR proposes is, when creating the _UpdateProcess in the _emplace method, that we initialize the status_cache: UpdateProgress with a default queued state. This way we don't block waiting on the response from the hardware controller, and the /subsystem/updates/all request can return.

Closes: RQA-2574

Test Plan

  • Make sure that firmware updates are automatically initiated when the robot boots up
  • Make sure that firmware updates are started from the ODD during the attachment flow
  • Make sure that firmware updates required modal is displayed when an instrument is attached outside the attach flow
  • Make sure that firmware update required modal is NOT shown when the robot boots up and an instrument needs an update, instead we should see the "Updating x Module Firmware..." modal.

Changelog

  • add status_cache arg to _UpdateProcess class so we can have an UpdateProgress state when creating the update

Review requests

  • Does this approach make sense?
  • Any implications I might be missing?

Risk assessment

Low, but will need additional testing as this touches firmware updates

@vegano1 vegano1 requested a review from a team as a code owner April 30, 2024 16:52
Copy link
Contributor

@smb2268 smb2268 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach makes sense to me! Thanks for all of the investigation into this one 🙏🏻

Copy link
Contributor

@caila-marashaj caila-marashaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm- this seems like it'll be really nice to have

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, nice!

@vegano1 vegano1 merged commit c163426 into edge Apr 30, 2024
13 checks passed
@vegano1 vegano1 deleted the RQA-2574-init-fw-update-state-so-we-dont-hang branch April 30, 2024 20:56
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants