From 7f9e6bd4db730abaa0b64be386823095b5d360af Mon Sep 17 00:00:00 2001 From: David Lechner Date: Wed, 7 Apr 2021 12:28:36 -0500 Subject: [PATCH] Fix download and run race condition It was possible for a program to start and finish running before the final checksum message was received, which resulted in a bad state (run button disabled). This fixes it by using proper didStart/didFinish/didFailToFinish actions for the download saga and updates the reducer to handle all cases. --- src/hub/ReplButton.tsx | 4 +-- src/hub/actions.ts | 56 ++++++++++++++++++++++-------------------- src/hub/reducers.ts | 32 +++++++----------------- src/hub/sagas.test.ts | 22 +++-------------- src/hub/sagas.ts | 17 +++++++------ 5 files changed, 53 insertions(+), 78 deletions(-) diff --git a/src/hub/ReplButton.tsx b/src/hub/ReplButton.tsx index e98c46fe..b4e3ff17 100644 --- a/src/hub/ReplButton.tsx +++ b/src/hub/ReplButton.tsx @@ -15,9 +15,7 @@ type OwnProps = Pick & Pick; const mapStateToProps = (state: RootState): StateProps => ({ - enabled: - state.hub.runtime === HubRuntimeState.Idle || - state.hub.runtime === HubRuntimeState.Error, + enabled: state.hub.runtime === HubRuntimeState.Idle, }); const mapDispatchToProps: DispatchProps = { diff --git a/src/hub/actions.ts b/src/hub/actions.ts index a816662e..b67dd48b 100644 --- a/src/hub/actions.ts +++ b/src/hub/actions.ts @@ -1,38 +1,15 @@ // SPDX-License-Identifier: MIT -// Copyright (c) 2020 The Pybricks Authors +// Copyright (c) 2020-2021 The Pybricks Authors import { Action } from 'redux'; -export enum HubRuntimeStatusType { - Loading = 'loading', - Loaded = 'loaded', - Error = 'error', -} - export enum HubMessageActionType { - /** - * The hub has send a message indicating the MicroPython runtime status changed. - */ - RuntimeStatus = 'hub.message.action.runtime.status', /** * The hub has sent a checksum. */ Checksum = 'hub.message.action.runtime.checksum', } -export type HubRuntimeStatusMessageAction = Action & { - readonly newStatus: HubRuntimeStatusType; -}; - -export function updateStatus( - newStatus: HubRuntimeStatusType, -): HubRuntimeStatusMessageAction { - return { - type: HubMessageActionType.RuntimeStatus, - newStatus, - }; -} - export type HubChecksumMessageAction = Action & { readonly checksum: number; }; @@ -47,13 +24,16 @@ export function checksum(checksum: number): HubChecksumMessageAction { /** * Common type for low-level hub message actions. */ -export type HubMessageAction = HubRuntimeStatusMessageAction | HubChecksumMessageAction; +export type HubMessageAction = HubChecksumMessageAction; /** * High-level hub actions. */ export enum HubActionType { DownloadAndRun = 'hub.action.downloadAndRun', + DidStartDownload = 'hub.action.didStartDownload', + DidFinishDownload = 'hub.action.didFinishDownload', + DidFailToFinishDownload = 'hub.action.didFailToFinishDownload', Stop = 'hub.action.stop', Repl = 'hub.action.repl', } @@ -64,6 +44,24 @@ export function downloadAndRun(): HubDownloadAndRunAction { return { type: HubActionType.DownloadAndRun }; } +export type HubDidStartDownloadAction = Action; + +export function didStartDownload(): HubDidStartDownloadAction { + return { type: HubActionType.DidStartDownload }; +} + +export type HubDidFinishDownloadAction = Action; + +export function didFinishDownload(): HubDidFinishDownloadAction { + return { type: HubActionType.DidFinishDownload }; +} + +export type HubDidFailToFinishDownloadAction = Action; + +export function didFailToFinishDownload(): HubDidFailToFinishDownloadAction { + return { type: HubActionType.DidFailToFinishDownload }; +} + export type HubStopAction = Action; export function stop(): HubStopAction { @@ -79,4 +77,10 @@ export function repl(): HubReplAction { /** * Common type for all high-level hub actions. */ -export type HubAction = HubDownloadAndRunAction | HubStopAction | HubReplAction; +export type HubAction = + | HubDownloadAndRunAction + | HubDidStartDownloadAction + | HubDidFinishDownloadAction + | HubDidFailToFinishDownloadAction + | HubStopAction + | HubReplAction; diff --git a/src/hub/reducers.ts b/src/hub/reducers.ts index ae8ce5ba..4628d17d 100644 --- a/src/hub/reducers.ts +++ b/src/hub/reducers.ts @@ -6,7 +6,7 @@ import { Action } from '../actions'; import { BlePybricksServiceEventActionType } from '../ble-pybricks-service/actions'; import { Status, statusToFlag } from '../ble-pybricks-service/protocol'; import { BleDeviceActionType } from '../ble/actions'; -import { HubMessageActionType, HubRuntimeStatusType } from './actions'; +import { HubActionType } from './actions'; /** * Describes the state of the MicroPython runtime on the hub. @@ -36,10 +36,6 @@ export enum HubRuntimeState { * A user program is running. */ Running = 'hub.runtime.running', - /** - * The runtime encountered an error. - */ - Error = 'hub.runtime.error', } const runtime: Reducer = ( @@ -49,29 +45,19 @@ const runtime: Reducer = ( switch (action.type) { case BleDeviceActionType.DidDisconnect: return HubRuntimeState.Disconnected; - case HubMessageActionType.RuntimeStatus: - switch (action.newStatus) { - case HubRuntimeStatusType.Loading: - return HubRuntimeState.Loading; - case HubRuntimeStatusType.Loaded: - return HubRuntimeState.Loaded; - case HubRuntimeStatusType.Error: - return HubRuntimeState.Error; - default: - console.error(`bad action/state: ${action.newStatus}`); - return state; - } + case HubActionType.DidStartDownload: + return HubRuntimeState.Loading; + case HubActionType.DidFinishDownload: + return HubRuntimeState.Loaded; + case HubActionType.DidFailToFinishDownload: + return HubRuntimeState.Idle; case BlePybricksServiceEventActionType.StatusReport: - // TODO: Status report flags need to be separated from hub runtime state. - // For now, we have this hack to ensure status updates don't interfere with - // download and run. Loading state should really be actions like didStartLoading, - // didLoad and didFailToLoad. + // The loading state is determined solely by the IDE, so we can't + // let the hub status interfere with it. if (state !== HubRuntimeState.Loading) { if (action.statusFlags & statusToFlag(Status.UserProgramRunning)) { return HubRuntimeState.Running; } - } - if (state !== HubRuntimeState.Loading && state !== HubRuntimeState.Loaded) { return HubRuntimeState.Idle; } return state; diff --git a/src/hub/sagas.test.ts b/src/hub/sagas.test.ts index 5e770896..1e79333e 100644 --- a/src/hub/sagas.test.ts +++ b/src/hub/sagas.test.ts @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// Copyright (c) 2020 The Pybricks Authors +// Copyright (c) 2020-2021 The Pybricks Authors import { Ace } from 'ace-builds'; import { mock } from 'jest-mock-extended'; @@ -12,15 +12,7 @@ import { import { BleUartActionType, BleUartWriteAction, didWrite } from '../ble-uart/actions'; import { MpyActionType, didCompile } from '../mpy/actions'; import { createCountFunc } from '../utils/iter'; -import { - HubMessageActionType, - HubRuntimeStatusMessageAction, - HubRuntimeStatusType, - checksum, - downloadAndRun, - repl, - stop, -} from './actions'; +import { HubActionType, checksum, downloadAndRun, repl, stop } from './actions'; import hub from './sagas'; jest.mock('ace-builds'); @@ -43,10 +35,7 @@ describe('downloadAndRun', () => { // then it notifies that loading has begun const loadingStatusAction = await saga.take(); - expect(loadingStatusAction.type).toBe(HubMessageActionType.RuntimeStatus); - expect((loadingStatusAction as HubRuntimeStatusMessageAction).newStatus).toBe( - HubRuntimeStatusType.Loading, - ); + expect(loadingStatusAction.type).toBe(HubActionType.DidStartDownload); // first message is the length const writeAction = await saga.take(); @@ -71,10 +60,7 @@ describe('downloadAndRun', () => { // Then a status message saying that we are done const loadedStatusAction = await saga.take(); - expect(loadedStatusAction.type).toBe(HubMessageActionType.RuntimeStatus); - expect((loadedStatusAction as HubRuntimeStatusMessageAction).newStatus).toBe( - HubRuntimeStatusType.Loaded, - ); + expect(loadedStatusAction.type).toBe(HubActionType.DidFinishDownload); await saga.end(); }); diff --git a/src/hub/sagas.ts b/src/hub/sagas.ts index e7ac1b09..55eedb29 100644 --- a/src/hub/sagas.ts +++ b/src/hub/sagas.ts @@ -41,9 +41,10 @@ import { HubDownloadAndRunAction, HubMessageActionType, HubReplAction, - HubRuntimeStatusType, HubStopAction, - updateStatus, + didFailToFinishDownload as didFailToFinishDownload, + didFinishDownload, + didStartDownload, } from './actions'; const downloadChunkSize = 100; @@ -87,7 +88,7 @@ function* downloadAndRun(_action: HubDownloadAndRunAction): Generator { defined(mpy); // let everyone know the runtime is busy loading the program - yield* put(updateStatus(HubRuntimeStatusType.Loading)); + yield* put(didStartDownload()); const checksumChannel = yield* actionChannel( HubMessageActionType.Checksum, @@ -103,7 +104,7 @@ function* downloadAndRun(_action: HubDownloadAndRunAction): Generator { const { didFailToWrite } = yield* waitForWrite(writeAction.id); if (didFailToWrite) { - yield* put(updateStatus(HubRuntimeStatusType.Error)); + yield* put(didFailToFinishDownload()); return; } @@ -112,7 +113,7 @@ function* downloadAndRun(_action: HubDownloadAndRunAction): Generator { console.error( `bad checksum ${checksumAction.checksum} vs ${0xff ^ xor8(sizeBuf)}`, ); - yield* put(updateStatus(HubRuntimeStatusType.Error)); + yield* put(didFailToFinishDownload()); return; } @@ -130,7 +131,7 @@ function* downloadAndRun(_action: HubDownloadAndRunAction): Generator { const { didFailToWrite } = yield* waitForWrite(writeAction.id); if (didFailToWrite) { - yield* put(updateStatus(HubRuntimeStatusType.Error)); + yield* put(didFailToFinishDownload()); return; } // TODO: dispatch progress @@ -140,13 +141,13 @@ function* downloadAndRun(_action: HubDownloadAndRunAction): Generator { console.error( `bad checksum ${checksumAction.checksum} vs ${0xff ^ xor8(chunk)}`, ); - yield* put(updateStatus(HubRuntimeStatusType.Error)); + yield* put(didFailToFinishDownload()); return; } } // let everyone know the runtime is done loading the program - yield* put(updateStatus(HubRuntimeStatusType.Loaded)); + yield* put(didFinishDownload()); } // SPACE, SPACE, SPACE, SPACE