From ac6c7981486a4089e07f94cca9e894041f9a6cf7 Mon Sep 17 00:00:00 2001
From: David Lechner <david@pybricks.com>
Date: Fri, 22 May 2020 15:20:11 -0500
Subject: [PATCH] Fix city hub bootloader compatibility

The city hub requires that certain messages are write with response and
others are write without response, otherwise it will cause BlueZ to
disconnect because it does not follow proper BLE protocol.
---
 src/actions/bootloader.ts    |  2 +-
 src/sagas/bootloader.test.ts |  5 +++--
 src/sagas/bootloader.ts      | 26 +++++++++++++++++++++++++-
 src/services/bootloader.ts   |  4 ++++
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/actions/bootloader.ts b/src/actions/bootloader.ts
index 206aa134..cfda654e 100644
--- a/src/actions/bootloader.ts
+++ b/src/actions/bootloader.ts
@@ -84,7 +84,7 @@ export interface BootloaderConnectionSendAction
 
 export function send(
     data: ArrayBuffer,
-    withResponse = false,
+    withResponse = true,
 ): BootloaderConnectionSendAction {
     return { type: BootloaderConnectionActionType.Send, data, withResponse };
 }
diff --git a/src/sagas/bootloader.test.ts b/src/sagas/bootloader.test.ts
index e9ee5f01..9bc982f5 100644
--- a/src/sagas/bootloader.test.ts
+++ b/src/sagas/bootloader.test.ts
@@ -146,7 +146,8 @@ describe('message encoder', () => {
         expect(dispatched[0]).toEqual({
             type: BootloaderConnectionActionType.Send,
             data: message,
-            withResponse: false,
+            // Program is write without response, all others are write with response
+            withResponse: request.type !== BootloaderRequestActionType.Program,
         });
     });
 
@@ -185,7 +186,7 @@ describe('message encoder', () => {
             expect(dispatched[i]).toEqual({
                 type: BootloaderConnectionActionType.Send,
                 data: message,
-                withResponse: false,
+                withResponse: true,
             });
         }
 
diff --git a/src/sagas/bootloader.ts b/src/sagas/bootloader.ts
index 3f0a637e..d8b46a6d 100644
--- a/src/sagas/bootloader.ts
+++ b/src/sagas/bootloader.ts
@@ -38,6 +38,7 @@ import {
     checksumResponse,
     connect,
     didRequest,
+    disconnectRequest,
     eraseRequest,
     eraseResponse,
     errorResponse,
@@ -53,6 +54,7 @@ import {
     stateResponse,
 } from '../actions/bootloader';
 import { MpyCompiledAction, compile } from '../actions/mpy';
+import * as notification from '../actions/notification';
 import {
     Command,
     ErrorBytecode,
@@ -92,13 +94,23 @@ function* encodeRequest(): Generator {
     while (true) {
         const action = (yield take(chan)) as BootloaderRequestAction;
 
+        // NB: Commands other than program on city hub will cause BlueZ to
+        // disconnect because they will send a response even if we write without
+        // response, so we always write with response on those commands. The
+        // program command needs to be write without response for performance
+        // reasons (and also the city hub will disconnect if write with response
+        // is used on this command).
+
         switch (action.type) {
             case BootloaderRequestActionType.Erase:
                 yield put(send(createEraseFlashRequest()));
                 break;
             case BootloaderRequestActionType.Program:
                 yield put(
-                    send(createProgramFlashRequest(action.address, action.payload)),
+                    send(
+                        createProgramFlashRequest(action.address, action.payload),
+                        /* withResponse */ false,
+                    ),
                 );
                 break;
             case BootloaderRequestActionType.Reboot:
@@ -294,6 +306,18 @@ function* flashFirmware(action: BootloaderFlashFirmwareAction): Generator {
         );
     }
 
+    // City hub bootloader is buggy. See note in encodeRequest().
+    if (info[0].hubType === HubType.CityHub && !didConnect.canWriteWithoutResponse) {
+        yield put(
+            notification.add(
+                'error',
+                'City Hub is not compatible with this web browser.',
+            ),
+        );
+        yield put(disconnectRequest());
+        return;
+    }
+
     yield put(eraseRequest());
     const erase = (yield wait(
         BootloaderResponseActionType.Erase,
diff --git a/src/services/bootloader.ts b/src/services/bootloader.ts
index 2889a628..c5974369 100644
--- a/src/services/bootloader.ts
+++ b/src/services/bootloader.ts
@@ -86,6 +86,10 @@ async function connect(action: Action, dispatch: Dispatch): Promise<void> {
                 !char.writeValueWithoutResponse &&
                 !/Android/i.test(navigator.userAgent)
             ) {
+                // TODO: this needs to be an error if connected to city hub
+                // however it is not currently possible to get mfg-specific
+                // advertising data, so we don't know what type of hub it is
+                // until after we connect
                 dispatch(
                     notification.add(
                         'warning',