From 956b6f3c9664f1fbf547ddbee7c79160282d7e09 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Sun, 16 May 2021 16:18:34 -0500 Subject: [PATCH] firmware: add windows BLE hacks This works around some issues that have been observed on a specific Windows computer. Issue: https://github.com/pybricks/support/issues/256 --- src/firmware/sagas.test.ts | 14 +++++++------- src/firmware/sagas.ts | 14 +++++++++++++- src/lwp3-bootloader/actions.ts | 12 +++++++++--- src/lwp3-bootloader/sagas.test.ts | 13 +++++++------ src/lwp3-bootloader/sagas.ts | 8 +++++++- src/utils/os.test.ts | 13 ++++++++++++- src/utils/os.ts | 8 ++++++++ 7 files changed, 63 insertions(+), 19 deletions(-) diff --git a/src/firmware/sagas.test.ts b/src/firmware/sagas.test.ts index 6ef5f873..df3a3e94 100644 --- a/src/firmware/sagas.test.ts +++ b/src/firmware/sagas.test.ts @@ -136,7 +136,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); @@ -857,7 +857,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.Error)); @@ -958,7 +958,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); @@ -1068,7 +1068,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); @@ -1218,7 +1218,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); @@ -1370,7 +1370,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); @@ -1900,7 +1900,7 @@ describe('flashFirmware', () => { // erase first action = await saga.take(); - expect(action).toEqual(eraseRequest(1)); + expect(action).toEqual(eraseRequest(1, /* isCityHub */ false)); saga.put(didRequest(1)); saga.put(eraseResponse(Result.OK)); diff --git a/src/firmware/sagas.ts b/src/firmware/sagas.ts index d3b36f60..b523bde0 100644 --- a/src/firmware/sagas.ts +++ b/src/firmware/sagas.ts @@ -38,6 +38,7 @@ import { connect, disconnect, eraseRequest, + eraseResponse, infoRequest, initRequest, programRequest, @@ -126,6 +127,15 @@ function* waitForResponse( }); if (timedOut) { + // istanbul ignore if: this hacks around a hardware/OS issue + if (type === BootloaderResponseActionType.Erase) { + // It has been observed that sometimes this response is not received + // or gets stuck in the Bluetooth stack until another request is sent. + // So, we ignore the timeout and continue. If there really was a + // problem, then the next request should fail anyway. + console.warn('Timeout waiting for erase response, continuing anyway.'); + return eraseResponse(Result.OK) as T; + } yield* put(didFailToFinish(FailToFinishReasonType.TimedOut)); yield* disconnectAndCancel(); } @@ -335,7 +345,9 @@ function* flashFirmware(action: FlashFirmwareFlashAction): Generator { yield* put(didStart()); - const eraseAction = yield* put(eraseRequest(nextMessageId())); + const eraseAction = yield* put( + eraseRequest(nextMessageId(), deviceId === HubType.CityHub), + ); const { erase } = yield* all({ sent: waitForDidRequest(eraseAction.id), erase: waitForResponse( diff --git a/src/lwp3-bootloader/actions.ts b/src/lwp3-bootloader/actions.ts index 5ed466ef..9e1bbb01 100644 --- a/src/lwp3-bootloader/actions.ts +++ b/src/lwp3-bootloader/actions.ts @@ -251,13 +251,19 @@ type BaseBootloaderRequestAction = Action * Action that requests to erase the flash memory. */ export type BootloaderEraseRequestAction = - BaseBootloaderRequestAction; + BaseBootloaderRequestAction & { + /* City hub requires special handling due to buggy bootloader */ + isCityHub: boolean; + }; /** * Creates a request to erase the flash memory. */ -export function eraseRequest(id: number): BootloaderEraseRequestAction { - return { type: BootloaderRequestActionType.Erase, id }; +export function eraseRequest( + id: number, + isCityHub: boolean, +): BootloaderEraseRequestAction { + return { type: BootloaderRequestActionType.Erase, id, isCityHub }; } /** diff --git a/src/lwp3-bootloader/sagas.test.ts b/src/lwp3-bootloader/sagas.test.ts index e668df8c..4468888d 100644 --- a/src/lwp3-bootloader/sagas.test.ts +++ b/src/lwp3-bootloader/sagas.test.ts @@ -33,7 +33,7 @@ describe('message encoder', () => { test.each([ [ 'erase', - eraseRequest(0), + eraseRequest(0, /* isCityHub */ false), [ 0x11, // erase command ], @@ -116,6 +116,7 @@ describe('message encoder', () => { ], ])('encode %s request', async (_n, request, expected) => { const messageTypesThatShouldBeCalledWithoutResponse = [ + BootloaderRequestActionType.Erase, BootloaderRequestActionType.Program, BootloaderRequestActionType.Reboot, BootloaderRequestActionType.Disconnect, @@ -137,10 +138,10 @@ describe('message encoder', () => { const saga = new AsyncSaga(bootloader); // we send 4 requests - saga.put(eraseRequest(0)); - saga.put(eraseRequest(1)); - saga.put(eraseRequest(2)); - saga.put(eraseRequest(3)); + saga.put(checksumRequest(0)); + saga.put(checksumRequest(1)); + saga.put(checksumRequest(2)); + saga.put(checksumRequest(3)); // but only two didSend action meaning only the first two completed saga.put(didSend()); @@ -152,7 +153,7 @@ describe('message encoder', () => { const numPending = saga.numPending(); expect(numPending).toEqual(5); - const message = new Uint8Array([Command.EraseFlash]); + const message = new Uint8Array([Command.GetChecksum]); const nextId = createCountFunc(); // every other action is the "send" action diff --git a/src/lwp3-bootloader/sagas.ts b/src/lwp3-bootloader/sagas.ts index 8e08b929..7f38bf88 100644 --- a/src/lwp3-bootloader/sagas.ts +++ b/src/lwp3-bootloader/sagas.ts @@ -13,6 +13,7 @@ import { } from 'typed-redux-saga/macro'; import { Action } from '../actions'; import { hex } from '../utils'; +import { isWindows } from '../utils/os'; import { BootloaderConnectionActionType, BootloaderConnectionDidFailToSendAction, @@ -79,7 +80,12 @@ function* encodeRequest(): Generator { switch (action.type) { case BootloaderRequestActionType.Erase: - yield* put(send(createEraseFlashRequest())); + yield* put( + send( + createEraseFlashRequest(), + /* withResponse */ action.isCityHub && !isWindows(), + ), + ); break; case BootloaderRequestActionType.Program: yield* put( diff --git a/src/utils/os.test.ts b/src/utils/os.test.ts index 9051252f..6e6be02d 100644 --- a/src/utils/os.test.ts +++ b/src/utils/os.test.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // Copyright (c) 2021 The Pybricks Authors -import { isMacOS, prefersDarkMode } from './os'; +import { isMacOS, isWindows, prefersDarkMode } from './os'; describe('isMacOS', () => { test('is true', () => { @@ -14,6 +14,17 @@ describe('isMacOS', () => { }); }); +describe('isWindows', () => { + test('is true', () => { + jest.spyOn(navigator, 'platform', 'get').mockReturnValue('Win32'); + expect(isWindows()).toBeTruthy(); + }); + test('is false', () => { + jest.spyOn(navigator, 'platform', 'get').mockReturnValue('MacIntel'); + expect(isWindows()).toBeFalsy(); + }); +}); + describe('prefersDarkMode', () => { test('is true', () => { window.matchMedia = jest.fn().mockReturnValue({ diff --git a/src/utils/os.ts b/src/utils/os.ts index e32d5471..0a56d56d 100644 --- a/src/utils/os.ts +++ b/src/utils/os.ts @@ -11,6 +11,14 @@ export function isMacOS(): boolean { return /mac/i.test(navigator.platform); } +/** + * Tests if we are running on Windows. + * @returns `true` if running on Windows, otherwise `false`. + */ +export function isWindows(): boolean { + return /win/i.test(navigator.platform); +} + /** * Tests if the OS is set to dark mode. * @returns: `true` if dark mode should be preferred, otherwise `false`.