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

@@ -15,9 +15,7 @@ type OwnProps = Pick<ActionButtonProps, 'id'> &
     Pick<ActionButtonProps, 'keyboardShortcut'>;
 
 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 = {

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<HubMessageActionType.RuntimeStatus> & {
-    readonly newStatus: HubRuntimeStatusType;
-};
-
-export function updateStatus(
-    newStatus: HubRuntimeStatusType,
-): HubRuntimeStatusMessageAction {
-    return {
-        type: HubMessageActionType.RuntimeStatus,
-        newStatus,
-    };
-}
-
 export type HubChecksumMessageAction = Action<HubMessageActionType.Checksum> & {
     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<HubActionType.DidStartDownload>;
+
+export function didStartDownload(): HubDidStartDownloadAction {
+    return { type: HubActionType.DidStartDownload };
+}
+
+export type HubDidFinishDownloadAction = Action<HubActionType.DidFinishDownload>;
+
+export function didFinishDownload(): HubDidFinishDownloadAction {
+    return { type: HubActionType.DidFinishDownload };
+}
+
+export type HubDidFailToFinishDownloadAction = Action<HubActionType.DidFailToFinishDownload>;
+
+export function didFailToFinishDownload(): HubDidFailToFinishDownloadAction {
+    return { type: HubActionType.DidFailToFinishDownload };
+}
+
 export type HubStopAction = Action<HubActionType.Stop>;
 
 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;

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<HubRuntimeState, Action> = (
@@ -49,29 +45,19 @@ const runtime: Reducer<HubRuntimeState, Action> = (
     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;

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();
     });

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<HubChecksumMessageAction>(
         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