feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. #14832

vegano1 · 2024-04-08T04:50:42Z

Overview

We need to change the splash screen in user space, this is managed by the systems service opentrons-loading.service. This pull request in oe-core (Opentrons/oe-core#143) adds the mechanism to change the splash screen via the /var/lib/opentrons-system-server/system.env file. This pull request adds the endpoint and hooks it up to the robot-server to allow the OEM Mode to be enabled/disabled when the enableOEMMode feature flag is changed via the /system/settings POST request.

Closes: PLAT-275 PLAT-276

Test Plan

Build and test oe-core and buildroot builds on the Flex and OT-2
Make sure that the robot-server and system-server startup regardless of the value of enableOEMMode
Make sure we can change the oem_mode_enabled flag in the system.env via PUT /system/oem_mode/enable.
Make sure that we change the sysem.env file when the enableOEMMode advanced flag is set via POST /system/settings.
Make sure that we return a non-200 error code if PUT /system/oem_mode/enable fails for whatever reason.
Make sure that we DONT set the enableOEMMode advanced setting if PUT /system/oem_mode/enable fails.

Changelog

add aiohttp==3.8.1 to the robot-server so we can make async HTTP requests
add /system/oem_mode/enable endpoint to enable/disable OEM Mode by changing the system.env
add oem_mode_enabled and oem_mode_splash_custom system server config flags
add oem_mode_enabled helper function to feature_flags.py
add save_settings method to system-server settings so we can save the SystemServerSettings to a system.env
use the /system/oem_mode/enable endpoint when the enableOEMMode advanced settings flag is changed.

Review requests

Does the approach make sense of having the /system/oem_mode/enable request get triggered from the robot-server.
Ideally, the set_oem_mode_request is not just some floating function, this should be an HTTPClient that encapsulates HTTP requests to local endpoints. This might need to be added at a different time.
I haven't done HTTP API's in a while, so feedback is appreciated.

Risk assessment

low, should only affect enableOEMMOde advanced setting for the Flex and OT-2

…bleOEMMode feature flag is changed.

codecov · 2024-04-08T04:55:19Z

Codecov Report

Attention: Patch coverage is 47.72727% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 67.49%. Comparing base (8f50b08) to head (b0adf34).
Report is 10 commits behind head on edge.

❗ Current head b0adf34 differs from pull request most recent head 6c068d5. Consider uploading reports for the commit 6c068d5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #14832      +/-   ##
==========================================
- Coverage   67.50%   67.49%   -0.02%     
==========================================
  Files        2521     2525       +4     
  Lines       72090    72124      +34     
  Branches     9311     9311              
==========================================
+ Hits        48666    48677      +11     
- Misses      21228    21251      +23     
  Partials     2196     2196

Flag	Coverage Δ
system-server	`91.41% <47.72%> (-4.64%)`	⬇️

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

Files	Coverage Δ
api/src/opentrons/config/feature_flags.py	`100.00% <ø> (ø)`
...er/robot_server/service/legacy/routers/settings.py	`100.00% <ø> (ø)`
system-server/system_server/settings/__init__.py	`100.00% <100.00%> (ø)`
...m-server/system_server/system/oem_mode/__init__.py	`100.00% <100.00%> (ø)`
...tem-server/system_server/system/oem_mode/models.py	`100.00% <100.00%> (ø)`
system-server/system_server/system/router.py	`100.00% <100.00%> (ø)`
...rver/system_server/system/oem_mode/dependencies.py	`0.00% <0.00%> (ø)`
...tem-server/system_server/system/oem_mode/router.py	`42.85% <42.85%> (ø)`
system-server/system_server/settings/settings.py	`63.63% <35.29%> (-31.82%)`	⬇️

CaseyBatten · 2024-04-09T14:37:56Z

api/src/opentrons/config/feature_flags.py

@@ -76,3 +76,8 @@ def enable_error_recovery_experiments() -> bool:
 return advs.get_setting_with_env_overload(
 "enableErrorRecoveryExperiments", RobotTypeEnum.FLEX
 )
+
+def oem_mode_enabled() -> bool:


Looks good but perhaps this should be passed robot type instead of assume it. We've had problems with feature flags before where we assume type in the past and had to correct it, like with enable_door_safety_switch. If this is passed an OT-2 robot type I'd expect we'd just raise an error, or do nothing at all?

this should do nothing on the OT2, since all the logic that deals with OEM mode is on the client side.

If this is unused in the current build (and is not planned to be used) should we simply removed it all together?

make sure robot type is RobotTypeEnum.FLEX when setting the enableOEMMode

load from env_file when instantiating SystemServerSettings

CaseyBatten

Overall everything looks good so far from my perspective. The justification for the odd coupling order (enabled OEM: client -> robot server -> system server, upload image: client -> system server) of the robot server, system server and client for file writing makes sense given actions occurring. However, we may want to note this decision for future divisions of labor between the system and robot servers to more cleanly separate the order of operations if any similar features are developed in the future.

CaseyBatten · 2024-04-11T19:34:58Z

robot-server/robot_server/service/legacy/routers/settings.py

 try:
+ # send request to system server if this is the enableOEMMode setting
+ if update.id == "enableOEMMode" and robot_type == RobotTypeEnum.FLEX:
+ resp = await set_oem_mode_request(update.value)
+ if resp != 200:
+ # TODO: raise correct error here
+ raise Exception(f"Something went wrong setting OEM Mode. err: {resp}")
+


As discussed, the except handlers for the raised Exception below here are probably sufficient for the error response we'd want, so this should be okay.

CaseyBatten · 2024-04-11T19:36:45Z

api/src/opentrons/config/feature_flags.py

@@ -76,3 +76,8 @@ def enable_error_recovery_experiments() -> bool:
 return advs.get_setting_with_env_overload(
 "enableErrorRecoveryExperiments", RobotTypeEnum.FLEX
 )
+
+def oem_mode_enabled() -> bool:


If this is unused in the current build (and is not planned to be used) should we simply removed it all together?

CaseyBatten · 2024-04-11T19:43:23Z

system-server/system_server/settings/settings.py

+def save_settings(settings: SystemServerSettings) -> bool:
+ """Save the settings to the dotenv file."""
+ env_path = Environment().dot_env_path
+ env_path = env_path or f"{settings.persistence_directory}/system.env"
+ prefix = settings.Config.env_prefix


As discussed the decision to have the system server responsible for this persistence file write is justified.

…via advanced settings. (#14832)

vegano1 added 2 commits April 7, 2024 23:42

feat(system-server): add endpoint to enable/disable OEM Mode.

b78bbeb

feat(robot-server): use the /system/oem_mode/enable endpoint when ena…

5af14aa

…bleOEMMode feature flag is changed.

vegano1 requested review from CaseyBatten and brenthagen April 8, 2024 04:51

vegano1 changed the title ~~feat(robot-server,)~~ feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. Apr 8, 2024

CaseyBatten reviewed Apr 9, 2024

View reviewed changes

vegano1 requested a review from SyntaxColoring April 9, 2024 18:33

vegano1 added 7 commits April 10, 2024 15:42

load system server settings from dotenv file if provided

43585eb

use RobotTypeEnum instead of str to check robot_type

2eb299d

make sure robot type is RobotTypeEnum.FLEX when setting the enableOEMMode

add aiohttp==3.8.1

131078b

Merge branch 'edge' into PLAT-276-add-oem-mode-endpoint

33244cf

format

e7014ec

load from env_file when instantiating SystemServerSettings

format

b0adf34

add tavern test

9b242ef

vegano1 requested a review from CaseyBatten April 10, 2024 22:10

vegano1 marked this pull request as ready for review April 10, 2024 22:16

vegano1 requested review from a team as code owners April 10, 2024 22:16

add show_if true for enableOEMMode

a893fe5

CaseyBatten approved these changes Apr 11, 2024

View reviewed changes

vegano1 added 2 commits April 11, 2024 16:49

leave TODO so we add an ipc mechanism in the future

49224e7

lint...

6c068d5

vegano1 merged commit 437e074 into edge Apr 12, 2024
23 checks passed

vegano1 deleted the PLAT-276-add-oem-mode-endpoint branch April 12, 2024 19:28

Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024

feat(system-server,robot-server,api): add ability to enable OEM Mode …

1077962

…via advanced settings. (#14832)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. #14832

feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. #14832

vegano1 commented Apr 8, 2024 •

edited

Loading

codecov bot commented Apr 8, 2024 •

edited

Loading

CaseyBatten Apr 9, 2024

vegano1 Apr 10, 2024

CaseyBatten Apr 11, 2024

CaseyBatten left a comment

CaseyBatten Apr 11, 2024

vegano1 Apr 11, 2024

CaseyBatten Apr 11, 2024

CaseyBatten Apr 11, 2024

feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. #14832

feat(system-server,robot-server,api): add ability to enable OEM Mode when setting enableOEMMode advanced setting. #14832

Conversation

vegano1 commented Apr 8, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Apr 8, 2024 • edited Loading

Codecov Report

CaseyBatten Apr 9, 2024

Choose a reason for hiding this comment

vegano1 Apr 10, 2024

Choose a reason for hiding this comment

CaseyBatten Apr 11, 2024

Choose a reason for hiding this comment

CaseyBatten left a comment

Choose a reason for hiding this comment

CaseyBatten Apr 11, 2024

Choose a reason for hiding this comment

vegano1 Apr 11, 2024

Choose a reason for hiding this comment

CaseyBatten Apr 11, 2024

Choose a reason for hiding this comment

CaseyBatten Apr 11, 2024

Choose a reason for hiding this comment

vegano1 commented Apr 8, 2024 •

edited

Loading

codecov bot commented Apr 8, 2024 •

edited

Loading