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

feat(app): status bar Update animation during update installation #13519

Merged

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Sep 12, 2023

Overview

This should be the last missing status bar animation. Without this PR, the robot server will start running the Update animation when it starts up after an update and starts updating the firmware images on the robot. However, we want the status bar to pulse while the robot is updating its software, which starts well before the robot has restarted.

This PR adds a couple of simple hooks in the RobotUpdateProgressModal to start the Updating animation on a Flex that is being updated. If an error occurs, the app changes to the "idle" animation.

Test Plan

  • Updated an OT-2 to make sure that the setStatusBar command fails silently instead of interfering with operation
  • Updated FlexyFace from a .zip file. The status bar pulsed as soon as the app start uploading the file, and pulsed during the restart.
  • Updated FlexyFace and then killed the Update Server during the .zip file upload. The app turned off the pulsing animation.

Changelog

  • When the RobotUpdateProgressModal is first rendered, send a command to the robot to start the Updating animation (pulsing white) on the status bar
  • When there is an error during an update, send a command to the robot to display the Idle animation (solid white) on the status bar

Review requests

  • I ended up taking basically the same approach here as with the "disco" animation during first-time setup, mostly because I don't know enough typescript to know if there's a better way to do this. If this is a silly method and/or location to add these hooks then please point me in the right direction :)

Risk assessment

Pretty low as long as my understanding of javascript catch is correct - when updating a robot with software old enough to not recognize the animation commands, they should just be ignored.

Otherwise, the biggest risk is some possibility of weird animation states on the robot if you start an update and then close your app in the middle of it. I think the only alternative here would be to manage this through the Update Server instead of the app. I'm a little wary about integrating this kind of functionality in the Update Server unless absolutely necessary, but I could be convinced.

@fsinapi fsinapi requested a review from a team September 12, 2023 14:14
@fsinapi fsinapi requested a review from a team as a code owner September 12, 2023 14:14
@fsinapi fsinapi self-assigned this Sep 12, 2023
@fsinapi fsinapi requested review from TamarZanzouri and removed request for a team September 12, 2023 14:14
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #13519 (92a5a89) into chore_release-7.0.0 (124f184) will decrease coverage by 0.01%.
Report is 5 commits behind head on chore_release-7.0.0.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13519      +/-   ##
=======================================================
- Coverage                71.36%   71.35%   -0.01%     
=======================================================
  Files                     2418     2421       +3     
  Lines                    67937    68150     +213     
  Branches                  7886     7964      +78     
=======================================================
+ Hits                     48480    48629     +149     
- Misses                   17616    17655      +39     
- Partials                  1841     1866      +25     
Flag Coverage Δ
app 68.98% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...tings/UpdateBuildroot/RobotUpdateProgressModal.tsx 87.50% <100.00%> (+3.77%) ⬆️

... and 6 files with indirect coverage changes

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

makes sense to me! thanks for adding test coverage. You can potentially split the startIdleAnimationIfFailed and startUpdatingAnimation into their own functions/files but i don't think its necessary since it seems like these commands will only get called here.

@fsinapi fsinapi merged commit 91002f0 into chore_release-7.0.0 Sep 12, 2023
@fsinapi fsinapi deleted the RCORE-776_update_animation_during_restart branch September 12, 2023 18:11
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.

2 participants