Skip to content

Commit

Permalink
fix(app,api,robot-server): expose flex serial (#13791)
Browse files Browse the repository at this point in the history
The flex doesn't store its serial number in /var/serial; instead, it
stores it in an onboard eeprom. While we had the code to read it, we
weren't using that code anywhere, and we should expose it to the app for
display.

However, the update server doesn't (and shouldn't) know how to talk to
the onboard eeprom. That's a problem, because it knows perfectly well
how to read a file, and thus is the current source of truth for the
serial.

So this PR
- adds the serial to the robot server health endpoint, on both ot-2 and
flex
- changes the app to try and prefer the serial from the robot server
health endpoint, falling back to the update server health endpoint if
necessary
  • Loading branch information
sfoster1 authored Oct 16, 2023
1 parent dfb7acb commit 5a8163a
Show file tree
Hide file tree
Showing 16 changed files with 85 additions and 5 deletions.
3 changes: 3 additions & 0 deletions api/src/opentrons/hardware_control/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ async def build_hardware_simulator(
def __repr__(self) -> str:
return "<{} using backend {}>".format(type(self), type(self._backend))

async def get_serial_number(self) -> Optional[str]:
return await self._backend.get_serial_number()

@property
def loop(self) -> asyncio.AbstractEventLoop:
"""The event loop used by this instance."""
Expand Down
7 changes: 7 additions & 0 deletions api/src/opentrons/hardware_control/backends/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
cast,
)
from typing_extensions import Final
from pathlib import Path

try:
import aionotify # type: ignore[import]
Expand Down Expand Up @@ -132,6 +133,12 @@ def module_controls(self) -> AttachedModulesControl:
def module_controls(self, module_controls: AttachedModulesControl) -> None:
self._module_controls = module_controls

async def get_serial_number(self) -> Optional[str]:
try:
return Path("/var/serial").read_text().strip()
except OSError:
return None

def start_gpio_door_watcher(
self,
loop: asyncio.AbstractEventLoop,
Expand Down
5 changes: 5 additions & 0 deletions api/src/opentrons/hardware_control/backends/ot3controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,11 @@ def __init__(
)
self._current_settings: Optional[OT3AxisMap[CurrentConfig]] = None

async def get_serial_number(self) -> Optional[str]:
if not self.initialized:
return None
return self.eeprom_data.serial_number

@property
def initialized(self) -> bool:
"""True when the hardware controller has initialized and is ready."""
Expand Down
3 changes: 3 additions & 0 deletions api/src/opentrons/hardware_control/backends/ot3simulator.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,9 @@ def _sanitize_attached_instrument(
self._current_settings: Optional[OT3AxisMap[CurrentConfig]] = None
self._sim_jaw_state = GripperJawState.HOMED_READY

async def get_serial_number(self) -> Optional[str]:
return "simulator"

@property
def initialized(self) -> bool:
"""True when the hardware controller has initialized and is ready."""
Expand Down
3 changes: 3 additions & 0 deletions api/src/opentrons/hardware_control/backends/simulator.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,9 @@ def _sanitize_attached_instrument(
# to the hardware api controller.
self._module_controls: Optional[AttachedModulesControl] = None

async def get_serial_number(self) -> Optional[str]:
return "simulator"

@property
def gpio_chardev(self) -> GPIODriverLike:
return self._gpio_chardev
Expand Down
3 changes: 3 additions & 0 deletions api/src/opentrons/hardware_control/ot3api.py
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ async def set_gantry_load(self, gantry_load: GantryLoad) -> None:
)
await self._backend.update_to_default_current_settings(gantry_load)

async def get_serial_number(self) -> Optional[str]:
return await self._backend.get_serial_number()

async def set_system_constraints_for_calibration(self) -> None:
self._move_manager.update_constraints(
get_system_constraints_for_calibration(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Dict
from typing import Dict, Optional
from typing_extensions import Protocol

from ..types import SubSystem, SubSystemState
Expand Down Expand Up @@ -41,3 +41,7 @@ def attached_subsystems(self) -> Dict[SubSystem, SubSystemState]:
whether or not the hardware is operating properly.
"""
...

async def get_serial_number(self) -> Optional[str]:
"""Get the robot serial number, if provisioned. If not provisioned, will be None."""
...
10 changes: 10 additions & 0 deletions app/src/redux/discovery/__fixtures__/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ export const mockHealthResponse = {
protocol_api_version: [2, 0] as [number, number],
}

export const mockHealthResponseWithSerial = {
...mockHealthResponse,
robot_serial: 'this is a robot serial from robot server',
}

export const mockUpdateServerHealthResponse = {
name: 'robot-name',
apiServerVersion: '0.0.0-mock',
Expand All @@ -48,6 +53,11 @@ export const mockDiscoveryClientRobot = {
],
}

export const mockDiscoveryClientRobotWithHealthSerial = {
...mockDiscoveryClientRobot,
health: mockHealthResponseWithSerial,
}

export const mockBaseRobot: BaseRobot = {
// NOTE(mc, 2020-11-10): it's important that name and displayName are
// different in this fixture to ensure proper test coverage
Expand Down
27 changes: 27 additions & 0 deletions app/src/redux/discovery/__tests__/selectors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -643,3 +643,30 @@ describe('discovery selectors', () => {
it(name, () => expect(selector(state as State, ...args)).toEqual(expected))
})
})

describe('getRobotSerialNumber', () => {
const SPECS: Array<{ name: string; robot: any; expected: string | null }> = [
{
name: 'returns health serial on flex',
robot: MOCK_STATE.discovery.robotsByName.fizzbuzz,
expected: 'this is a flex serial',
},
{
name: 'getRobotSerial returns health serial on ot2 if available',
robot: MOCK_STATE.discovery.robotsByName.baz,
expected: 'this is an ot2 serial',
},
{
name: 'getRobotSerial falls back to update server if necessary',
robot: MOCK_STATE.discovery.robotsByName.bar,
expected: '12345',
},
]
SPECS.forEach(spec => {
it(spec.name, () => {
expect(discovery.getRobotSerialNumber(spec.robot as any)).toEqual(
spec.expected
)
})
})
})
2 changes: 1 addition & 1 deletion app/src/redux/discovery/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ export const getDiscoverableRobotByName: (
)

export const getRobotSerialNumber = (robot: DiscoveredRobot): string | null =>
(robot.serverHealth && robot.serverHealth.serialNumber) ?? null
robot.health?.robot_serial ?? robot.serverHealth?.serialNumber ?? null

export const getRobotApiVersion = (robot: DiscoveredRobot): string | null =>
(robot.health && semver.valid(robot.health.api_version)) ??
Expand Down
4 changes: 3 additions & 1 deletion discovery-client/src/__fixtures__/health.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const mockOT3HealthResponse = {
fw_version: '4.5.6',
system_version: '7.8.9',
robot_model: 'OT-3 Standard',
robot_serial: 'this is a flex serial',
}

export const mockOT2HealthResponse = {
Expand All @@ -20,6 +21,7 @@ export const mockOT2HealthResponse = {
fw_version: '4.5.6',
system_version: '7.8.9',
robot_model: 'OT-2 Standard',
robot_serial: 'this is an ot2 serial',
}

export const mockLegacyServerHealthResponse = {
Expand All @@ -34,7 +36,7 @@ export const mockLegacyServerHealthResponse = {
export const mockOT3ServerHealthResponse = {
name: 'opentrons-dev',
apiServerVersion: '1.2.3',
serialNumber: '12345',
serialNumber: 'unknown',
updateServerVersion: '1.2.3',
smoothieVersion: '4.5.6',
systemVersion: '7.8.9',
Expand Down
1 change: 1 addition & 0 deletions discovery-client/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export interface HealthResponse {
minimum_protocol_api_version?: [number, number]
maximum_protocol_api_version?: [number, number]
robot_model?: string
robot_serial?: string | null
}

export type Capability =
Expand Down
5 changes: 5 additions & 0 deletions robot-server/robot_server/health/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ class Health(BaseResponseBody):
min_items=2,
max_items=2,
)
robot_serial: typing.Optional[str] = Field(
...,
description="The robot serial number. Should be used if not none; if none, use result of /server/update/health.",
)
links: HealthLinks

class Config:
Expand All @@ -103,5 +107,6 @@ class Config:
"apiSpec": "/openapi.json",
"systemTime": "/system/time",
},
"robot_serial": None,
}
}
1 change: 1 addition & 0 deletions robot-server/robot_server/health/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,5 @@ async def get_health(
minimum_protocol_api_version=list(minimum_protocol_api_version),
robot_model=robot_type,
links=health_links,
robot_serial=(await hardware.get_serial_number()),
)
2 changes: 2 additions & 0 deletions robot-server/tests/health/test_health_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ def test_get_health(
"""Test GET /health."""
hardware.fw_version = "FW111"
hardware.board_revision = "BR2.1"
hardware.get_serial_number.return_value = "mytestserial"
versions.return_value = ComponentVersions(
api_version="mytestapiversion", system_version="mytestsystemversion"
)
Expand All @@ -36,6 +37,7 @@ def test_get_health(
"apiSpec": "/openapi.json",
"systemTime": "/system/time",
},
"robot_serial": "mytestserial",
}

resp = api_client.get("/health")
Expand Down
8 changes: 6 additions & 2 deletions robot-server/tests/integration/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ def check_health_response(response: Response) -> None:
"systemTime": "/system/time",
"serverLog": "/logs/server.log",
},
"robot_serial": "simulator",
}
got = response.json()

assert response.json() == expected
assert got == expected, f"health response failed:\n {got}"


def check_ot3_health_response(response: Response) -> None:
Expand All @@ -59,9 +61,11 @@ def check_ot3_health_response(response: Response) -> None:
"systemTime": "/system/time",
"serverLog": "/logs/server.log",
},
"robot_serial": "simulator",
}
got = response.json()

assert response.json() == expected
assert got == expected, f"health response failed:\n {got}"


def get_module_id(response: Response, module_model: ModuleModel) -> Box:
Expand Down

0 comments on commit 5a8163a

Please sign in to comment.