Skip to content
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

fix: propagate monitor errors to the frontend #1965

Merged
merged 1 commit into from
Apr 13, 2023
Merged

fix: propagate monitor errors to the frontend #1965

merged 1 commit into from
Apr 13, 2023

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Mar 17, 2023

Motivation

This PR enables clear communication of the problem when starting a monitor process fails.

Shows monitor connection status in the <input>

  • not-connected (board or port is missing),
  • connecting (when trying to connect to the monitor port),
  • connected ✅, and
  • error (the connection has failed for whatever reason, and the user should select another board or close + reopen the widget after fixing any port issues outside of IDE2)
1508.mp4

Change description

  • Propagate error from backend to frontend via the monitor ws.

  • Open the ws connection before creating the monitor duplex. Otherwise, the status update or error cannot reach the frontend.

  • Fixed monitor widget DI.

  • Fixed monitor service lifecycle inside the widget. onBeforeAttach happened multiple times on IDE2 start if the widget was previously opened.

  • Fixed connect when creating the ws connection to the monitor. Previously, the promise might have been resolved before the ws OPEN event.

  • Moved monitor state to common.

  • Monitor client proxy waits for reconciled boards after the app startup, then starts the monitor.

  • Replaced the connected: boolean with a fine-grained connectionStatus and kept the code backward compatible with the plotter app.

  • Add translation for baud rate.

  • Smoother monitor widget re-render on monitor reset. (On monitor reset, the monitor widget is forcefully re-rendered causing the <input> and <select>s to disappear for a short time #1985)

1985.mp4
1984.mp4
1974.mp4
682.mp4
  • Aligned HC warning colors with the error colors:

    2.0.4:
    Screen Shot 2023-03-30 at 18 55 54

    PR:
    Screen Shot 2023-03-30 at 18 56 29

    Screen Shot 2023-03-30 at 18 56 45

Other information

Closes #1508
Closes #1985
Closes #1984
Closes #1974
Ref #682

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project topic: serial monitor Related to the Serial Monitor labels Mar 17, 2023
@kittaakos kittaakos self-assigned this Mar 17, 2023
@kittaakos kittaakos force-pushed the #1508 branch 6 times, most recently from 305b1b8 to eb2fc72 Compare March 17, 2023 17:03
@kittaakos kittaakos marked this pull request as ready for review March 20, 2023 08:49
@kittaakos
Copy link
Contributor Author

I have verified the followings:

  • macOS
    • upload to the same board while two connected monitors are running (with Uno and 33 BLE),
    • start monitor when another process is using the port, such as Arduino IDE 1.x.
  • Windows
    • upload to the same board while two connected monitors are running (with Uno and Micro),
    • start monitor when another process is using the port, such as Arduino IDE 1.x.

I could not upload to 33 BLE on Windows. I restarted my notebook and ran the CMD.exe as an Administrator, but it did not help:

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe version
arduino-cli.exe  Version: 0.31.0 Commit: 940c9457 Date: 2023-02-22T16:15:54Z

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe core list
ID                Installed Latest Name
arduino:avr       1.8.6     1.8.6  Arduino AVR Boards
arduino:mbed_nano 4.0.2     4.0.2  Arduino Mbed OS Nano Boards


c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe board list
Port Protocol Type              Board Name          FQBN                        Core
COM3 serial   Serial Port (USB) Arduino Nano 33 BLE arduino:mbed_nano:nano33ble arduino:mbed_nano
COM4 serial   Serial Port (USB) Unknown


c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe compile -b arduino:mbed_nano:nano33ble "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508"
Sketch uses 84680 bytes (8%) of program storage space. Maximum is 983040 bytes.
Global variables use 44560 bytes (16%) of dynamic memory, leaving 217584 bytes for local variables. Maximum is 262144 bytes.

Used platform     Version Path
arduino:mbed_nano 4.0.2   C:\Users\kittaakos\AppData\Local\Arduino15\packages\arduino\hardware\mbed_nano\4.0.2

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe upload -b arduino:mbed_nano:nano33ble -p COM3 "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508"
TOUCH: error during reset: opening port at 1200bps: Serial port busy
No device found on COM3
Failed uploading: uploading error: exit status 1

c:\Users\kittaakos\Desktop\cli 0.31.0>arduino-cli.exe upload -b arduino:mbed_nano:nano33ble -p COM3 "..\\..\\Desktop\\sketchbooks\\test_sketchbook__2\\monitor__1_1508" -v
Performing 1200-bps touch reset on serial port COM3
TOUCH: error during reset: opening port at 1200bps: Serial port busy
Waiting for upload port...
No upload port found, using COM3 as fallback
"C:\Users\kittaakos\AppData\Local\Arduino15\packages\arduino\tools\bossac\1.9.1-arduino2/bossac.exe" -d --port=COM3 -U -i -e -w "C:\Users\kittaakos\AppData\Local\Temp\arduino\sketches\9374601C8719AF4D46679576E3DD0DC6/monitor__1_1508.ino.bin" -R
No device found on COM3
Failed uploading: uploading error: exit status 1

c:\Users\kittaakos\Desktop\cli 0.31.0>

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I perform the steps described in #1508 I get an incorrect message in the input field of the Serial Monitor view:

image

The problem that caused the monitor initialization to fail is that I had the port open in another application. I already have the correct board and port selected, so this "Select a board and a port to connect automatically." advice is wrong.

@kittaakos
Copy link
Contributor Author

The problem that caused the monitor initialization to fail is that I had the port open in another application. I already have the correct board and port selected, so this "Select a board and a port to connect automatically." advice is wrong.

@91volt, please help with how this should work. Thank you!

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I perform the steps described in #1508, there is a delay of ~15 seconds before the "Port monitor error: command 'open' failed: Serial port busy." notification appears.

The error appears in the logs immediately upon opening the Serial Monitor view:

2023-03-24T07:58:20.085Z monitor-service ERROR Error: 2 UNKNOWN: Port monitor error: command 'open' failed: Serial port busy
    at Object.callErrorFromStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\call.js:31:19)
    at Object.onReceiveStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client.js:413:49)
    at Object.onReceiveStatus (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client-interceptors.js:328:181)
    at E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\call-stream.js:187:78
    at processTicksAndRejections (node:internal/process/task_queues:78:11)
for call at
    at ServiceClientImpl.makeBidiStreamRequest (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\client.js:398:30)
    at ServiceClientImpl.monitor (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@grpc\grpc-js\build\src\make-client.js:105:19)
    at MonitorService.createDuplex (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:157:34)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:220:37
    at async retry (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@theia\core\lib\common\promise-util.js:73:20)
    at async MonitorService.start (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-service.js:135:13)
    at async MonitorManager.startMonitor (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\arduino-ide-extension\lib\node\monitor-manager.js:89:13)
    at async JsonRpcProxyFactory.onRequest (E:\incoming\review\1965\1-c0ca295\resources\app\node_modules\@theia\core\lib\common\messaging\proxy-factory.js:132:24)

Then it retries the monitor initialization 9 more times before finally showing the notification.

@kittaakos
Copy link
Contributor Author

The error appears in the logs immediately upon opening the Serial Monitor view:

Nothing new. See here: #1966.

Then it retries the monitor initialization 9 more times before finally showing the notification.

This is precisely the same behavior as on the main with 9b49712. Nothing has changed:

const MAX_WRITE_TO_STREAM_TRIES = 10;

// if "pollingIsSuccessful" is false
// the CLI gave us an error, lets try again
// after waiting 2 seconds if we've not already
// reached MAX_WRITE_TO_STREAM_TRIES
if (pollingIsSuccessful === false) {
attemptsRemaining -= 1;
if (attemptsRemaining > 0) {
setTimeout(startPolling, 2000);
return;
} else {
resolve(false);
return;
}
}

@per1234
Copy link
Contributor

per1234 commented Mar 24, 2023

Nothing new.

I didn't intend to imply it was something new. I mentioned it because it shows that the delay does not have an external cause. Arduino IDE knows right away that the monitor initialization failed, but doesn't immediately show the notification.

This is precisely the same behavior as on the main with 9b49712. Nothing has changed:

Something has changed. Arduino IDE now shows a notification that the monitor initialization failed. There is no notification at all on main.

The problem is there is an unnecessarily long delay before the notification appears.

@kittaakos kittaakos force-pushed the #1508 branch 2 times, most recently from 12efc24 to a0a3bf4 Compare March 30, 2023 14:45
@kittaakos
Copy link
Contributor Author

I made the changes. Please review. Thanks

@kittaakos kittaakos force-pushed the #1508 branch 2 times, most recently from 8d3eb32 to 9b8e941 Compare March 31, 2023 08:45
Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me! I just left some remarks, but they're definitely not blocking.

P.S.: for this review I focused mainly on the code and only did some basic tests of the functionality; let me know if you want me to test it more deeply.

@AlbyIanna
Copy link
Contributor

I just left some remarks, but they're definitely not blocking.

Follow up on this: all comments have been addressed. Thank you, Akos!

 - Handle when the board's platform is not installed (Closes #1974)
 - UX: Smoother monitor widget reset (Closes #1985)
 - Fixed monitor <input> readOnly state (Closes #1984)
 - Set monitor widget header color (Ref #682)

Closes #1508

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos kittaakos merged commit 80d5b5a into main Apr 13, 2023
@kittaakos kittaakos deleted the #1508 branch April 13, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself topic: serial monitor Related to the Serial Monitor type: imperfection Perceived defect in any part of project
Projects
None yet
3 participants