Serial communication from the CPX to the extension by jonathanwangg · Pull Request #97 · microsoft/vscode-python-devicesimulator

Jonathan Wang (jonathanwangg) · 2019-08-06T18:14:41Z

Description:

### IMPORTANT NOTE: A lot of the changes are probably from the vendor/node-usb-native folder. It is from the vscode-arduino extension and I have not modified it in any way. We probably don't have to review that portion since I'm treating it as third party library code. You will also need a physical CPX to be able to test this PR.

In this PR, we're including a serial monitor to our extension. What this means is that any print statements in the user's code will be printed to an OutputChannel under the Output panel in VS Code. I am mostly reusing code from Microsoft's vscode-arduino extension (https://github.com/Microsoft/vscode-arduino) and removing code that is specific to the Arduino that we won't need.

Aside from the Command Palette commands, we also have StatusBarItems that can be used instead of the Command Palette commands:

Select Serial Port
Open Serial Monitor
Change Baud Rate

The core files that I have taken code from are:

src/serialmonitor/serialMonitor.ts
- This file contains most of the logic of the serial monitor in regards to the command palette commands as well as the creation of the StatusBarItems (found at the bottom right of VS Code)
src/serialmonitor/serialportctrl.ts
- This file contains logic regarding the control of a serial port as well as the configuration of a serial port. The "on data" event is what writes to the OutputChannel.
src/serialmonitor/usbDetector.ts
- Uses the node-usb-native library to listen for changes and initialize the SerialMonitor
src/deviceContext.ts
- Contains the code to update cpx.json which is a JSON object that describes the configuration of the CPX using a FileSystemWatcher .
src/extension.ts
- Addition of new Command Palette commands:
  - Select Serial Port
  - Open Serial Monitor
  - Change Baud Rate
  - Close Serial Monitor
vendor/node-usb-native
- Used to detect USB changes, listing of serial ports, opening / closing serial ports, sending and receiving messages from serial ports.

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Limitations:

The vscode-arduino extension is in Preview mode and since we're relying on their code, there might be some bugs they missed that will also be in our code.
Unplugging the CPX doesn't close the serial monitor (the same thing happens if you try it with the vscode-arduino extensinon)
We still want to inquire from users the need for changing line endings. If it isn't needed or rarely used, we could remove it.
In tsconfig.json, I've changed strict: true to alwaysStrict: true instead. I am not sure if this is recommended or if I should go back to using strict: true and find another way to solve type issues.

Testing:

I ran the extension with my CPX plugged in. I use the the "Open Serial Monitor" command to select the serial port and open the serial monitor. I had code on the CPX with print statements. The print statements showed in the serial monitor OutputChannel.

Try to do the same as what I did above.
Try to change the baud rate, use a different COM port for the CPX, as well as closing the serial monitor. Make sure there are no errors.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

FMounz · 2019-08-10T09:29:42Z

Jonathan Wang (@jonathanwangg) there are conflicts here

FMounz · 2019-08-10T11:35:26Z

+  BOARD: 60,
+  ENDING: 70,
+  SKETCH: 80,
+  PROGRAMMER: 90,


PROGRAMMER is not used anywhere. Is there a reason?

Good catch, they were relevant for the Arduino, but not for us. I've removed this and the other constants.

FMounz · 2019-08-10T11:38:22Z

+  BAUD_RATE: 40,
+  BOARD: 60,
+  ENDING: 70,
+  SKETCH: 80,


SKETCH is not used anywhere.

FMounz · 2019-08-10T11:39:45Z

+  PORT: 20,
+  OPEN_PORT: 30,
+  BAUD_RATE: 40,
+  BOARD: 60,


BOARD is not used anywhere

FMounz · 2019-08-11T04:52:23Z

+                this._currentSerialPort.on("open", () => {
+                    this._currentSerialPort.write("msft", "Both NL & CR", (err: any) => {
+                        if (err && !(err.message.indexOf("Writing to COM port (GetOverlappedResult): Unknown error code 121") >= 0)) {
+                            this._outputChannel.appendLine(`[Error] Failed to open the serial port - ${this._currentPort}`);


Should'nt localized constants be used for the error and the info here since it is going in the output channel?

Converted all of them into constants.

Luke Slevinsky (LukeSlev)

Hmm some comments, also is this just me? on trying to open the serial monitor

Luke Slevinsky (LukeSlev) · 2019-08-12T18:28:37Z

+                        this._port = cpxConfigJson.port;
+                        this._onDidChange.fire();
+                    } else {
+                        // Logger.notifyUserError("arduinoFileError", new Error(constants.messages.ARDUINO_FILE_ERROR));


Commented code here. Should we conole.error or something ?

Yes, thanks for all the forgotten code. Will be changing to console.error.

Luke Slevinsky (LukeSlev) · 2019-08-12T18:29:10Z

+                return this;
+            }, (reason) => {
+                // Workaround for change in API.
+                // vscode.workspace.findFiles() for some reason now throws an error ehn path does not exist


typo in the comment -> ... error WHEN path ...

Luke Slevinsky (LukeSlev) · 2019-08-12T18:30:18Z

+                // vscode.window.showErrorMessage(reason.toString());
+                // Logger.notifyUserError("arduinoFileUnhandleError", new Error(reason.toString()));
+
+                 // Workaround for change in API, populate required props for arduino.json


arduino comments, should update

Will delete irrelevant comments & code.

Luke Slevinsky (LukeSlev) · 2019-08-12T18:30:38Z

+                // Workaround for change in API.
+                // vscode.workspace.findFiles() for some reason now throws an error ehn path does not exist
+                // vscode.window.showErrorMessage(reason.toString());
+                // Logger.notifyUserError("arduinoFileUnhandleError", new Error(reason.toString()));


should we replace or get rid of this?

I'll get rid of it, it's also commented out in their code.

Luke Slevinsky (LukeSlev) · 2019-08-12T18:53:49Z

  telemetryAI = new TelemetryAI(context);
  let currentPanel: vscode.WebviewPanel | undefined;
-  let outChannel: vscode.OutputChannel | undefined;
+  // let outChannel: vscode.OutputChannel | undefined;


don't need this do we

Luke Slevinsky (LukeSlev) · 2019-08-12T19:27:48Z

+            if (this._currentPort !== this._serialPortControl.currentPort) {
+                await this._serialPortControl.changePort(this._currentPort);
+            } else if (this._serialPortControl.isActive) {
+                vscode.window.showWarningMessage(`Serial Monitor is already opened for ${this._currentPort}`);


Constant here maybe?

Yes, will do.

Luke Slevinsky (LukeSlev) · 2019-08-12T19:28:39Z

+            this._portsStatusBar.text = "<Select Serial Port>";
+        }
+    }
+}


line ending

Luke Slevinsky (LukeSlev) · 2019-08-12T21:12:21Z

+    }
+
+    public open(): Promise<any> {
+        this._outputChannel.appendLine(`[Starting] Opening the serial port - ${this._currentPort}`); 


There a reason we don't wanna use the utils method for outputting to the channel here?

Using appendLine instead of append because the logToOutputChannel function uses append rather than appendLine. I'll also make the string a constant!

Luke Slevinsky (LukeSlev) · 2019-08-12T21:17:12Z

    "rootDir": "src",
    "jsx": "react",
-    "strict": true /* enable all strict type-checking options */
+    // "strict": true, 


if we need to change this we can probably just remove it instead of commenting

Luke Slevinsky (LukeSlev) · 2019-08-12T21:19:26Z

+        const workspaceConfig = vscode.workspace.getConfiguration();
+        const enableUSBDetection = workspaceConfig.get(CONFIG_KEYS.ENABLE_USB_DETECTION);
+
+        if (os.platform() === "linux" || !enableUSBDetection) {


we can't detect usbs on linux hey? we should probably list that lack of functionality somewhere

Yeah, this is something that their library doesn't seem to support yet. We'll have to note this down in a "Known Limitations" section of our README.md.

FMounz

Seems like some changes are needed

Luke Slevinsky (LukeSlev) · 2019-08-14T17:03:15Z

@@ -362,7 +368,7 @@ export async function activate(context: vscode.ExtensionContext) {
      childProcess.stderr.on("data", data => {


This needs to be prefixed with utils sir - https://github.com/microsoft/vscode-python-embedded/pull/97/files#diff-45327f86d4438556066de133327f4ca2R347

Luke Slevinsky (LukeSlev) · 2019-08-14T17:22:24Z

 }

+export const CPX_CONFIG_FILE = path.join(".vscode", "cpx.json");
+export const SERIAL_MONITOR_NAME = "CPX Serial Monitor";


I would name this Pacifica rather than CPX because the serial monitor is specific to our app not the board

Also localize it pls

Luke Slevinsky (LukeSlev)

seems to work for me now so after these updates good to go

Luke Slevinsky (LukeSlev) · 2019-08-14T17:50:50Z

+
+    public async openSerialMonitor() {
+        if (!this._currentPort) {
+            const ans = await vscode.window.showInformationMessage("No serial port was selected, please select a serial port first", "Yes", "No");


make yes and no constants pls and localize the message

Luke Slevinsky (LukeSlev) · 2019-08-14T17:52:28Z

+                this.updatePortListStatus(foundPort.comName);
+            }
+        } else {
+            const chosen = await vscode.window.showQuickPick(<vscode.QuickPickItem[]>lists.map((port: ISerialPortDetail): vscode.QuickPickItem => {


use as to cast instead of <> pls

Luke Slevinsky (LukeSlev)

build

Luke Slevinsky (LukeSlev) · 2019-08-15T18:36:16Z

      "https://learn.adafruit.com/circuitpython-made-easy-on-circuit-playground-express/circuit-playground-express-library"
  },
+  MISC: {
+    NO: localize(


maybe move to dialog responses as a message item? that's what we've been doing for our other dialog responses 🤷‍♂️

Luke Slevinsky (LukeSlev) · 2019-08-15T18:37:40Z

Jonathan Wang (@jonathanwangg) conflicts

…icts

Luke Slevinsky (LukeSlev)

LGTM

Jonathan Wang (jonathanwangg) added 6 commits July 29, 2019 14:48

Add 'Open Serial Monitor' command stub

edcfb77

Serial monitor at a working state with hardcoded COM port 6

a3fc9a6

Fix problem where missing cpx.json prevents opening of OutputChannel

e58e2da

Additional type fixes

76ddc98

Remove send message to serial monitor functionality

31cd938

Add Copyright statement and credit to vscode-arduino extension

b73bd9b

Jonathan Wang (jonathanwangg) requested review from Christellah, FMounz, Luke Slevinsky (LukeSlev), Ahmed Mahdy (abmahdy), Andrew Clements (adclements), Mark Schramm (markAtMicrosoft) and smmatte August 6, 2019 18:14

Jonathan Wang (jonathanwangg) added 2 commits August 6, 2019 15:37

Add user message logging

59a3c7f

Change USB detection from always true to getting config value

0afe1cd

Jonathan Wang (jonathanwangg) marked this pull request as ready for review August 6, 2019 23:02

Jonathan Wang (jonathanwangg) added 3 commits August 6, 2019 16:04

Merge dev into users/t-jowang/reuse-serial-monitor

2af1c2a

Move utils.ts into extension_utils folder

647a351

Merge dev into users/t-jowang/reuse-serial-monitor to resolve conflicts

f33609f

FMounz suggested changes Aug 10, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) added 3 commits August 10, 2019 14:17

Remove unused code from PR comments

806fcd4

Merge dev into users/t-jowang/reuse-serial-monitor to fix conflicts

43fc125

Remove line ending from serial port

5528a04

FMounz reviewed Aug 12, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) added 3 commits August 12, 2019 10:11

Remove sendMessageToSerialPort function

2618310

Change constant key to uppercase

06721c7

Change info and error messages into constants

f0351be

FMounz approved these changes Aug 12, 2019

View reviewed changes

Luke Slevinsky (LukeSlev) suggested changes Aug 12, 2019

View reviewed changes

FMounz reviewed Aug 12, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) added 2 commits August 13, 2019 15:51

Address PR suggestions

b81c9e7

Merge dev into users/t-jowang/reuse-serial-monitor

92b108e

Luke Slevinsky (LukeSlev) reviewed Aug 14, 2019

View reviewed changes

Luke Slevinsky (LukeSlev) approved these changes Aug 15, 2019

View reviewed changes

Luke Slevinsky (LukeSlev) suggested changes Aug 15, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) added 2 commits August 15, 2019 10:08

Address more PR comments

1c67b00

Merge dev into users/t-jowang/reuse-serial-monitor

59c93ec

Luke Slevinsky (LukeSlev) reviewed Aug 15, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) added 3 commits August 15, 2019 11:45

Move Yes and No to DialogReponses section

2cb1b26

Merge dev into users/t-jowang/reuse-serial-monitor to fix merge confl…

8b67d7b

…icts

Use new constants

e886912

Luke Slevinsky (LukeSlev) approved these changes Aug 15, 2019

View reviewed changes

Jonathan Wang (jonathanwangg) merged commit 041c985 into dev Aug 15, 2019

Jonathan Wang (jonathanwangg) deleted the users/t-jowang/reuse-serial-monitor branch August 23, 2019 01:15

		@@ -362,7 +368,7 @@ export async function activate(context: vscode.ExtensionContext) {
		childProcess.stderr.on("data", data => {

Conversation

Jonathan Wang (jonathanwangg) commented Aug 6, 2019 • edited by vandyliu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Type of change

Limitations:

Testing:

Checklist:

Uh oh!

FMounz commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luke Slevinsky (LukeSlev) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FMounz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luke Slevinsky (LukeSlev) left a comment

Choose a reason for hiding this comment

Uh oh!

Jonathan Wang (jonathanwangg) commented Aug 6, 2019 •

edited by vandyliu

Loading

FMounz commented Aug 10, 2019 •

edited

Loading