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

Pluggable monitor #982

Merged
merged 74 commits into from
Jun 7, 2022
Merged

Pluggable monitor #982

merged 74 commits into from
Jun 7, 2022

Conversation

AlbyIanna
Copy link
Contributor

@AlbyIanna AlbyIanna commented May 9, 2022

Motivation

The main reason for this PR is to use the latest "pluggable monitor" gRPC APIs #769. But in order to use the new APIs, we needed to refactor all the Serial Monitor architecture. All this work allowed us to solve a number of issues that weren't impossible or too expensive to solve before the change, and we also took advantage of this effort to implement a more solid architecture.

This PR solves:

Change description

The architecture has been redesigned like this:
image

Call Stack / Event Flow when opening Serial monitor or Plotter

Numbers correspond to those in the diagram above.

  1. A monitor is opened (monitor-widget or plotter-frontend-contribution) from the IDE;
  2. monitor-widget or plotter-frontend-contribution invokes .startMonitor() method on monitor-manager-proxy-client-impl from .onBeforeAttach() or .startPlotter(), respectively;
  3. monitor-manager-proxy-client-impl.startMonitor() invokes .startMonitor() method on monitor-manager-proxy-impl;
  4. monitor-manager-proxy-impl.startMonitor() invokes .startMonitor() method on monitor-manager;
  5. monitor-manager creates a monitor-service from monitor-service-factory if not existing, injecting monitor-settings-provider-impl and web-socket-provider-impl;
  6. monitor-manager invokes .start() method on created/existing monitor-service;
    • this creates a duplex stream (gRPC) to communicate with our CLI;
    • An injected WebSocket server is instantiated here;
  7. The result from monitor-service.start() method is returned to the monitor-manager-proxy-impl when it's duplex connection is successfully connected (if no error);
  8. monitor-manager-proxy-impl invokes .connect() on the monitor-manager-proxy-client-impl if value returned from monitor-service.start() method is Status.OK or Status.ALREADY_CONNECTED;
    • this creates a WebSocket client linked to the WebSocket server created for our monitor-service;

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)

@AlbyIanna AlbyIanna requested a review from fstasi May 9, 2022 13:12
@fstasi
Copy link
Contributor

fstasi commented May 9, 2022

  • reset board does not reconnect
    • closing the monitor and re-opening fixes the problem
  • missing disconnected status in the serial monitor panel
  • serial plotter connect to the right port only when the serial monitor is open
    • this is probably due to the fact that the monitor updates the status of the UI with the correct port
  • upload is broken

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels May 10, 2022
@ubidefeo
Copy link

ubidefeo commented Jun 7, 2022

@PaulStoffregen

builds are at the bottom of this page, let me know if you can't see them
https://github.com/arduino/arduino-ide/actions/runs/2447002574

Akos Kitta and others added 3 commits June 7, 2022 10:56
Signed-off-by: Akos Kitta <[email protected]>
…into pluggable-monitor

# Conflicts:
#	arduino-ide-extension/package.json
#	arduino-ide-extension/src/browser/arduino-frontend-contribution.tsx
#	arduino-ide-extension/src/browser/dialogs/firmware-uploader/firmware-uploader-dialog.tsx
#	arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx
#	arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
#	arduino-ide-extension/src/node/arduino-ide-backend-module.ts
@PaulStoffregen
Copy link

@ubidefeo - I'm running it now on my Linux desktop machine. Seems to be working pretty well. I'm happy to see it's handling Teensy 4.0 maximum speed printing pretty well, with 2 processes each using about 70% of a CPU core, and 4 others adding up to about 55%.

image

@PaulStoffregen
Copy link

Overall speed of more than half a million lines/sec is also quite good.

@PaulStoffregen
Copy link

Running a Teensy 4.0 overclocked to 912 MHz, and the pluggable monitor is managing 785000 lines/sec. Very respectable speed.

I have some rough edges to fix in my package's uploading utility. Looks like the IDE has some minor serial monitor bugs, probably unrelated to pluggable monitor.

…monitor

# Conflicts:
#	arduino-ide-extension/package.json
#	arduino-ide-extension/src/browser/arduino-ide-frontend-module.ts
#	arduino-ide-extension/src/browser/dialogs/firmware-uploader/firmware-uploader-dialog.tsx
#	arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx
#	arduino-ide-extension/src/browser/serial/monitor/serial-monitor-send-input.tsx
#	arduino-ide-extension/src/browser/theia/core/shell-layout-restorer.ts
#	arduino-ide-extension/src/node/arduino-ide-backend-module.ts
@ubidefeo
Copy link

ubidefeo commented Jun 7, 2022

Thank you, @PaulStoffregen
We really appreciate your feedback and we know some things need adjusting but feel like this is a great milestone for CLI and IDE 2.0 :)

@AlbyIanna AlbyIanna merged commit df8658e into main Jun 7, 2022
@AlbyIanna AlbyIanna deleted the pluggable-monitor branch June 7, 2022 13:51
@per1234 per1234 linked an issue Jun 8, 2022 that may be closed by this pull request
@per1234 per1234 linked an issue Jun 8, 2022 that may be closed by this pull request
@per1234 per1234 linked an issue Jun 8, 2022 that may be closed by this pull request
@per1234 per1234 linked an issue Jun 8, 2022 that may be closed by this pull request
@per1234 per1234 linked an issue Jun 8, 2022 that may be closed by this pull request
@per1234 per1234 linked an issue Jun 10, 2022 that may be closed by this pull request
@per1234 per1234 mentioned this pull request Jun 10, 2022
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: enhancement Proposed improvement
Projects
None yet
9 participants