Skip to content

fix 180: prevent erroneous "auto-reconnect"(s) in board selector #1328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Aug 24, 2022
127 changes: 106 additions & 21 deletions arduino-ide-extension/src/browser/boards/boards-service-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
protected _availablePorts: Port[] = [];
protected _availableBoards: AvailableBoard[] = [];

private lastBoardsConfigOnUpload: BoardsConfig.Config | undefined;
private lastAvailablePortsOnUpload: Port[] | undefined;
private boardConfigToAutoSelect: BoardsConfig.Config | undefined;

/**
* Unlike `onAttachedBoardsChanged` this even fires when the user modifies the selected board in the IDE.\
* This even also fires, when the boards package was not available for the currently selected board,
Expand Down Expand Up @@ -111,6 +115,84 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
return this._reconciled.promise;
}

snapshotBoardDiscoveryOnUpload(): void {
this.lastBoardsConfigOnUpload = this._boardsConfig;
this.lastAvailablePortsOnUpload = this._availablePorts;
}

clearBoardDiscoverySnapshot(): void {
this.lastBoardsConfigOnUpload = undefined;
this.lastAvailablePortsOnUpload = undefined;
}

private portToAutoSelectCanBeDerived(): boolean {
return Boolean(
this.lastBoardsConfigOnUpload && this.lastAvailablePortsOnUpload
);
}

attemptPostUploadAutoSelect(): void {
setTimeout(() => {
if (this.portToAutoSelectCanBeDerived()) {
this.attemptAutoSelect({
ports: this._availablePorts,
boards: this._availableBoards,
});
}
}, 2000); // 2 second delay same as IDE 1.8
}

private attemptAutoSelect(
newState: AttachedBoardsChangeEvent['newState']
): void {
this.deriveBoardConfigToAutoSelect(newState);
this.tryReconnect();
}

private deriveBoardConfigToAutoSelect(
newState: AttachedBoardsChangeEvent['newState']
): void {
if (!this.portToAutoSelectCanBeDerived()) {
this.boardConfigToAutoSelect = undefined;
return;
}

const oldPorts = this.lastAvailablePortsOnUpload!;
const { ports: newPorts, boards: newBoards } = newState;

const appearedPorts =
oldPorts.length > 0
? newPorts.filter((newPort: Port) =>
oldPorts.every((oldPort: Port) => !Port.sameAs(newPort, oldPort))
)
: newPorts;

for (const port of appearedPorts) {
const boardOnAppearedPort = newBoards.find((board: Board) =>
Port.sameAs(board.port, port)
);

const lastBoardsConfigOnUpload = this.lastBoardsConfigOnUpload!;

if (
boardOnAppearedPort &&
lastBoardsConfigOnUpload.selectedBoard &&
Board.sameAs(
boardOnAppearedPort,
lastBoardsConfigOnUpload.selectedBoard
)
) {
this.clearBoardDiscoverySnapshot();

this.boardConfigToAutoSelect = {
selectedBoard: boardOnAppearedPort,
selectedPort: port,
};
return;
}
}
}

protected notifyAttachedBoardsChanged(
event: AttachedBoardsChangeEvent
): void {
Expand All @@ -119,10 +201,18 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
this.logger.info(AttachedBoardsChangeEvent.toString(event));
this.logger.info('------------------------------------------');
}

this._attachedBoards = event.newState.boards;
this._availablePorts = event.newState.ports;
this.onAvailablePortsChangedEmitter.fire(this._availablePorts);
this.reconcileAvailableBoards().then(() => this.tryReconnect());
this.reconcileAvailableBoards().then(() => {
const { uploadInProgress } = event;
// avoid attempting "auto-selection" while an
// upload is in progress
if (!uploadInProgress) {
this.attemptAutoSelect(event.newState);
}
});
}

protected notifyPlatformInstalled(event: { item: BoardsPackage }): void {
Expand Down Expand Up @@ -238,24 +328,12 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
return true;
}
}
// If we could not find an exact match, we compare the board FQBN-name pairs and ignore the port, as it might have changed.
// See documentation on `latestValidBoardsConfig`.
for (const board of this.availableBoards.filter(
({ state }) => state !== AvailableBoard.State.incomplete
)) {
if (
this.latestValidBoardsConfig.selectedBoard.fqbn === board.fqbn &&
this.latestValidBoardsConfig.selectedBoard.name === board.name &&
this.latestValidBoardsConfig.selectedPort.protocol ===
board.port?.protocol
) {
this.boardsConfig = {
...this.latestValidBoardsConfig,
selectedPort: board.port,
};
return true;
}
}

if (!this.boardConfigToAutoSelect) return false;

this.boardsConfig = this.boardConfigToAutoSelect;
this.boardConfigToAutoSelect = undefined;
return true;
}
return false;
}
Expand Down Expand Up @@ -458,6 +536,11 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
const board = attachedBoards.find(({ port }) =>
Port.sameAs(boardPort, port)
);
// "board" will always be falsey for
// port that was originally mapped
// to unknown board and then selected
// manually by user

const lastSelectedBoard = await this.getLastSelectedBoardOnPort(
boardPort
);
Expand All @@ -476,7 +559,9 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
availableBoard = {
...lastSelectedBoard,
state: AvailableBoard.State.guessed,
selected: BoardsConfig.Config.sameAs(boardsConfig, lastSelectedBoard),
selected:
BoardsConfig.Config.sameAs(boardsConfig, lastSelectedBoard) &&
Port.sameAs(boardPort, boardsConfig.selectedPort), // to avoid double selection
port: boardPort,
};
} else {
Expand All @@ -491,7 +576,7 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {

if (
boardsConfig.selectedBoard &&
!availableBoards.some(({ selected }) => selected)
availableBoards.every(({ selected }) => !selected)
) {
// If the selected board has the same port of an unknown board
// that is already in availableBoards we might get a duplicate port.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ export class UploadSketch extends CoreServiceContribution {
// toggle the toolbar button and menu item state.
// uploadInProgress will be set to false whether the upload fails or not
this.uploadInProgress = true;
this.boardsServiceProvider.snapshotBoardDiscoveryOnUpload();
this.onDidChangeEmitter.fire();
this.clearVisibleNotification();

Expand Down Expand Up @@ -243,6 +244,7 @@ export class UploadSketch extends CoreServiceContribution {
this.handleError(e);
} finally {
this.uploadInProgress = false;
this.boardsServiceProvider.attemptPostUploadAutoSelect();
this.onDidChangeEmitter.fire();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export namespace AvailablePorts {
export interface AttachedBoardsChangeEvent {
readonly oldState: Readonly<{ boards: Board[]; ports: Port[] }>;
readonly newState: Readonly<{ boards: Board[]; ports: Port[] }>;
readonly uploadInProgress: boolean;
}
export namespace AttachedBoardsChangeEvent {
export function isEmpty(event: AttachedBoardsChangeEvent): boolean {
Expand Down
7 changes: 7 additions & 0 deletions arduino-ide-extension/src/node/board-discovery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ export class BoardDiscovery
private readonly onStreamDidCancelEmitter = new Emitter<void>(); // when the watcher is canceled by the IDE2
private readonly toDisposeOnStopWatch = new DisposableCollection();

private uploadInProgress = false;

/**
* Keys are the `address` of the ports.
*
Expand Down Expand Up @@ -123,6 +125,10 @@ export class BoardDiscovery
});
}

setUploadInProgress(uploadAttemptInProgress: boolean): void {
this.uploadInProgress = uploadAttemptInProgress;
}

private createTimeout(
after: number,
onTimeout: (error: Error) => void
Expand Down Expand Up @@ -318,6 +324,7 @@ export class BoardDiscovery
ports: newAvailablePorts,
boards: newAttachedBoards,
},
uploadInProgress: this.uploadInProgress,
};

this._availablePorts = newState;
Expand Down
6 changes: 6 additions & 0 deletions arduino-ide-extension/src/node/core-service-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { Instance } from './cli-protocol/cc/arduino/cli/commands/v1/common_pb';
import { firstToUpperCase, notEmpty } from '../common/utils';
import { ServiceError } from './service-error';
import { ExecuteWithProgress, ProgressResponse } from './grpc-progressible';
import { BoardDiscovery } from './board-discovery';

namespace Uploadable {
export type Request = UploadRequest | UploadUsingProgrammerRequest;
Expand All @@ -51,6 +52,9 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService {
@inject(CommandService)
private readonly commandService: CommandService;

@inject(BoardDiscovery)
private readonly boardDiscovery: BoardDiscovery;

async compile(options: CoreService.Options.Compile): Promise<void> {
const coreClient = await this.coreClient;
const { client, instance } = coreClient;
Expand Down Expand Up @@ -378,6 +382,7 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService {
fqbn?: string | undefined;
port?: Port | undefined;
}): Promise<void> {
this.boardDiscovery.setUploadInProgress(true);
return this.monitorManager.notifyUploadStarted(fqbn, port);
}

Expand All @@ -388,6 +393,7 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService {
fqbn?: string | undefined;
port?: Port | undefined;
}): Promise<Status> {
this.boardDiscovery.setUploadInProgress(false);
return this.monitorManager.notifyUploadFinished(fqbn, port);
}

Expand Down