fix: propagate monitor errors to the frontend

Akos Kitta · Akos Kitta · commit eb2fc72d580d · 2023-03-17T18:03:21.000+01:00
Closes #1508 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
diff --git a/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts b/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts
@@ -1,4 +1,5 @@
 import {
+  ApplicationError,
   CommandRegistry,
   Disposable,
   Emitter,
@@ -167,7 +168,13 @@ export class MonitorManagerProxyClientImpl
     const { selectedBoard, selectedPort } =
       this.boardsServiceProvider.boardsConfig;
     if (!selectedBoard || !selectedBoard.fqbn || !selectedPort) return;
-    await this.server().startMonitor(selectedBoard, selectedPort, settings);
+    try {
+      await this.server().startMonitor(selectedBoard, selectedPort, settings);
+    } catch (err) {
+      this.messageService.error(
+        ApplicationError.is(err) ? err.message : String(err)
+      );
+    }
   }
 
   getCurrentSettings(board: Board, port: Port): Promise<MonitorSettings> {
diff --git a/arduino-ide-extension/src/browser/monitor-model.ts b/arduino-ide-extension/src/browser/monitor-model.ts
@@ -39,7 +39,7 @@ export class MonitorModel implements FrontendApplicationContribution {
     this._darkTheme = false;
     this._wsPort = 0;
     this._serialPort = '';
-    this._connected = true;
+    this._connected = false;
 
     this.onChangeEmitter = new Emitter<
       MonitorModel.State.Change<keyof MonitorModel.State>
diff --git a/arduino-ide-extension/src/common/protocol/monitor-service.ts b/arduino-ide-extension/src/common/protocol/monitor-service.ts
@@ -1,4 +1,4 @@
-import { Event, JsonRpcServer } from '@theia/core';
+import { ApplicationError, Event, JsonRpcServer, nls } from '@theia/core';
 import {
   PluggableMonitorSettings,
   MonitorSettings,
@@ -46,7 +46,7 @@ export interface PluggableMonitorSetting {
   readonly id: string;
   // A human-readable label of the setting (to be displayed on the GUI)
   readonly label: string;
-  // The setting type (at the moment only "enum" is avaiable)
+  // The setting type (at the moment only "enum" is available)
   readonly type: string;
   // The values allowed on "enum" types
   readonly values: string[];
@@ -72,24 +72,98 @@ export namespace Monitor {
   };
 }
 
-export interface Status {}
-export type OK = Status;
-export interface ErrorStatus extends Status {
-  readonly message: string;
+export const MonitorErrorCodes = {
+  ConnectionFailed: 6001,
+  NotConnected: 6002,
+  AlreadyConnected: 6003,
+  MissingConfiguration: 6004,
+} as const;
+
+export const ConnectionFailedError = createMonitorError(
+  MonitorErrorCodes.ConnectionFailed
+);
+export const NotConnectedError = createMonitorError(
+  MonitorErrorCodes.NotConnected
+);
+export const AlreadyConnectedError = createMonitorError(
+  MonitorErrorCodes.AlreadyConnected
+);
+export const MissingConfigurationError = createMonitorError(
+  MonitorErrorCodes.MissingConfiguration
+);
+
+export function createConnectionFailedError(
+  port: Port,
+  details?: string
+): ApplicationError<number, PortDescriptor> {
+  const { protocol, protocolLabel, address, addressLabel } = port;
+  const formattedDetails = details ? `: ${details}.` : '.';
+  return ConnectionFailedError(
+    nls.localize(
+      'arduino/monitor/connectionFailedError',
+      'Could not connect to {0} {1} port{2}',
+      addressLabel,
+      protocolLabel,
+      formattedDetails
+    ),
+    { protocol, address }
+  );
 }
-export namespace Status {
-  export function isOK(status: Status & { message?: string }): status is OK {
-    return !!status && typeof status.message !== 'string';
-  }
-  export const OK: OK = {};
-  export const NOT_CONNECTED: ErrorStatus = { message: 'Not connected.' };
-  export const ALREADY_CONNECTED: ErrorStatus = {
-    message: 'Already connected.',
-  };
-  export const CONFIG_MISSING: ErrorStatus = {
-    message: 'Serial Config missing.',
-  };
-  export const UPLOAD_IN_PROGRESS: ErrorStatus = {
-    message: 'Upload in progress.',
-  };
+export function createNotConnectedError(
+  port: Port
+): ApplicationError<number, PortDescriptor> {
+  const { protocol, protocolLabel, address, addressLabel } = port;
+  return NotConnectedError(
+    nls.localize(
+      'arduino/monitor/notConnectedError',
+      'Not connected to {0} {1} port.',
+      addressLabel,
+      protocolLabel
+    ),
+    { protocol, address }
+  );
+}
+export function createAlreadyConnectedError(
+  port: Port
+): ApplicationError<number, PortDescriptor> {
+  const { protocol, protocolLabel, address, addressLabel } = port;
+  return AlreadyConnectedError(
+    nls.localize(
+      'arduino/monitor/alreadyConnectedError',
+      'Could not connect to {0} {1} port. Already connected.',
+      addressLabel,
+      protocolLabel
+    ),
+    { protocol, address }
+  );
+}
+export function createMissingConfigurationError(
+  port: Port
+): ApplicationError<number, PortDescriptor> {
+  const { protocol, protocolLabel, address, addressLabel } = port;
+  return MissingConfigurationError(
+    nls.localize(
+      'arduino/monitor/missingConfigurationError',
+      'Could not connect to {0} {1} port. The monitor configuration is missing.',
+      addressLabel,
+      protocolLabel
+    ),
+    { protocol, address }
+  );
+}
+
+/**
+ * Bare minimum representation of a port. Supports neither UI labels nor properties.
+ */
+interface PortDescriptor {
+  readonly protocol: string;
+  readonly address: string;
+}
+function createMonitorError(
+  code: number
+): ApplicationError.Constructor<number, PortDescriptor> {
+  return ApplicationError.declare(
+    code,
+    (message: string, data: PortDescriptor) => ({ data, message })
+  );
 }
diff --git a/arduino-ide-extension/src/node/core-service-impl.ts b/arduino-ide-extension/src/node/core-service-impl.ts
@@ -23,7 +23,7 @@ import {
   UploadUsingProgrammerResponse,
 } from './cli-protocol/cc/arduino/cli/commands/v1/upload_pb';
 import { ResponseService } from '../common/protocol/response-service';
-import { OutputMessage, Port, Status } from '../common/protocol';
+import { OutputMessage, Port } from '../common/protocol';
 import { ArduinoCoreServiceClient } from './cli-protocol/cc/arduino/cli/commands/v1/commands_grpc_pb';
 import { Port as RpcPort } from './cli-protocol/cc/arduino/cli/commands/v1/port_pb';
 import { ApplicationError, CommandService, Disposable, nls } from '@theia/core';
@@ -392,7 +392,7 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService {
   }: {
     fqbn?: string | undefined;
     port?: Port | undefined;
-  }): Promise<Status> {
+  }): Promise<void> {
     this.boardDiscovery.setUploadInProgress(false);
     return this.monitorManager.notifyUploadFinished(fqbn, port);
   }
diff --git a/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts b/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts
@@ -2,7 +2,6 @@ import { inject, injectable } from '@theia/core/shared/inversify';
 import {
   MonitorManagerProxy,
   MonitorManagerProxyClient,
-  Status,
 } from '../common/protocol';
 import { Board, Port } from '../common/protocol';
 import { MonitorManager } from './monitor-manager';
@@ -41,11 +40,8 @@ export class MonitorManagerProxyImpl implements MonitorManagerProxy {
       await this.changeMonitorSettings(board, port, settings);
     }
 
-    const connectToClient = (status: Status) => {
-      if (status === Status.ALREADY_CONNECTED || status === Status.OK) {
-        // Monitor started correctly, connect it with the frontend
-        this.client.connect(this.manager.getWebsocketAddressPort(board, port));
-      }
+    const connectToClient = () => {
+      this.client.connect(this.manager.getWebsocketAddressPort(board, port));
     };
     return this.manager.startMonitor(board, port, connectToClient);
   }
diff --git a/arduino-ide-extension/src/node/monitor-manager.ts b/arduino-ide-extension/src/node/monitor-manager.ts
@@ -1,6 +1,11 @@
 import { ILogger } from '@theia/core';
 import { inject, injectable, named } from '@theia/core/shared/inversify';
-import { Board, BoardsService, Port, Status } from '../common/protocol';
+import {
+  AlreadyConnectedError,
+  Board,
+  BoardsService,
+  Port,
+} from '../common/protocol';
 import { CoreClientAware } from './core-client-provider';
 import { MonitorService } from './monitor-service';
 import { MonitorServiceFactory } from './monitor-service-factory';
@@ -36,7 +41,7 @@ export class MonitorManager extends CoreClientAware {
   private monitorServiceStartQueue: {
     monitorID: string;
     serviceStartParams: [Board, Port];
-    connectToClient: (status: Status) => void;
+    connectToClient: () => void;
   }[] = [];
 
   @inject(MonitorServiceFactory)
@@ -104,7 +109,7 @@ export class MonitorManager extends CoreClientAware {
   async startMonitor(
     board: Board,
     port: Port,
-    connectToClient: (status: Status) => void
+    connectToClient: () => void
   ): Promise<void> {
     const monitorID = this.monitorID(board.fqbn, port);
 
@@ -127,8 +132,14 @@ export class MonitorManager extends CoreClientAware {
       return;
     }
 
-    const result = await monitor.start();
-    connectToClient(result);
+    try {
+      await monitor.start();
+    } catch (err) {
+      if (!AlreadyConnectedError.is(err)) {
+        throw err;
+      }
+    }
+    await connectToClient();
   }
 
   /**
@@ -202,8 +213,7 @@ export class MonitorManager extends CoreClientAware {
   async notifyUploadFinished(
     fqbn?: string | undefined,
     port?: Port
-  ): Promise<Status> {
-    let status: Status = Status.NOT_CONNECTED;
+  ): Promise<void> {
     let portDidChangeOnUpload = false;
 
     // We have no way of knowing which monitor
@@ -214,7 +224,7 @@ export class MonitorManager extends CoreClientAware {
 
       const monitor = this.monitorServices.get(monitorID);
       if (monitor) {
-        status = await monitor.start();
+        await monitor.start();
       }
 
       // this monitorID will only be present in "disposedForUpload"
@@ -232,7 +242,6 @@ export class MonitorManager extends CoreClientAware {
     }
 
     await this.startQueuedServices(portDidChangeOnUpload);
-    return status;
   }
 
   async startQueuedServices(portDidChangeOnUpload: boolean): Promise<void> {
@@ -261,8 +270,8 @@ export class MonitorManager extends CoreClientAware {
         const monitorService = this.monitorServices.get(monitorID);
 
         if (monitorService) {
-          const result = await monitorService.start();
-          connectToClient(result);
+          await monitorService.start();
+          await connectToClient();
         }
       }
     }
diff --git a/arduino-ide-extension/src/node/monitor-service.ts b/arduino-ide-extension/src/node/monitor-service.ts
diff --git a/i18n/en.json b/i18n/en.json
