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: update monitor settings only if it's changed #1703

Merged
merged 1 commit into from
Nov 29, 2022
Merged

fix: update monitor settings only if it's changed #1703

merged 1 commit into from
Nov 29, 2022

Conversation

kittaakos
Copy link
Contributor

Depends on #1662


Motivation

Do not send monitor settings changes when there are no settings updates.

Change description

I wanted to keep the changeset as small as possible. From now on, setSettings promise resolves with undefined if there are no effective monitor settings updates. In such a case, the IDE2 monitor service does not send a MonitorRequest to the CLI.

Other information

Closes #375

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 Nov 22, 2022
@palazzol
Copy link
Contributor

@kittaakos: I tried this commit in my branch, and it seems to work great. The only remaining issue in the IDE with #375 is that, when you change the baud rate from the serial monitor pulldown, there are still 6 CONFIGURE commands sent out instead of only the one for the baud rate. I believe the issue is here: https://github.com/arduino/arduino-ide/blob/theia-1.31.1/arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx#L237 Instead of only sending the baud rate, it looks like this code grabs all the serial settings, updates the baud rate, and then sends them all out. I tested the serial-plotter as well, and it acts the same way when you change the baud rate.

@kittaakos kittaakos force-pushed the #375 branch 2 times, most recently from 0a67591 to 2d5df27 Compare November 28, 2022 14:27
@kittaakos
Copy link
Contributor Author

Thank you for the handy input, @palazzol 💪 Could you please check the tester build from my updated PR?

I completely rewrote my proposed changes: this time, IDE2 calculates a diff of monitor settings and sends only a diff of configuration commands.

Thank you!


When starting the monitor, IDE2 sends the configure commands:

2022-11-28T14:28:30.203Z daemon INFO INFO[0016] Starting monitor process                      monitor=serial-monitor

2022-11-28T14:28:30.207Z daemon INFO INFO[0016] Monitor process started successfully!         monitor=serial-monitor
INFO[0016] sending command                               command="HELLO 1 \"arduino-cli 0.29.0\"" monitor=serial-monitor

2022-11-28T14:28:30.211Z daemon INFO INFO[0016] received message                              error=false event_type=hello message=OK monitor=serial-monitor
INFO[0016] sending command                               command=DESCRIBE monitor=serial-monitor

2022-11-28T14:28:30.212Z daemon INFO INFO[0016] received message                              error=false event_type=describe message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE dtr on" monitor=serial-monitor
INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE parity none" monitor=serial-monitor
INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE rts on" monitor=serial-monitor
INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE stop_bits 1" monitor=serial-monitor

2022-11-28T14:28:30.213Z daemon INFO INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE baudrate 9600" monitor=serial-monitor
INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="CONFIGURE bits 8" monitor=serial-monitor
INFO[0016] received message                              error=false event_type=configure message=OK monitor=serial-monitor
INFO[0016] sending command                               command="OPEN 127.0.0.1:49640 /dev/cu.usbmodem14301" monitor=serial-monitor

2022-11-28T14:28:30.216Z daemon INFO INFO[0016] received message                              error=false event_type=open message=OK monitor=serial-monitor

2022-11-28T14:28:30.216Z daemon INFO INFO[0016] Port /dev/cu.usbmodem14301 successfully opened 

2022-11-28T14:28:30.219Z monitor-service INFO Using port configuration for serial:/dev/cu.usbmodem14301: {"settingsList":[{"settingId":"dtr","value":"on"},{"settingId":"parity","value":"none"},{"settingId":"rts","value":"on"},{"settingId":"stop_bits","value":"1"},{"settingId":"baudrate","value":"9600"},{"settingId":"bits","value":"8"}]}
2022-11-28T14:28:30.219Z monitor-service INFO started monitor to /dev/cu.usbmodem14301 using serial

When changing the baud rate, only the baud rate is configured:

2022-11-28T14:29:14.679Z monitor-service INFO Updated the port configuration for serial:/dev/cu.usbmodem14301: {"settingsList":[{"settingId":"dtr","value":"on"},{"settingId":"parity","value":"none"},{"settingId":"rts","value":"on"},{"settingId":"stop_bits","value":"1"},{"settingId":"baudrate","value":"4800"},{"settingId":"bits","value":"8"}]}
2022-11-28T14:29:14.679Z monitor-service INFO Sending monitor request with new port configuration: {"settingsList":[{"settingId":"baudrate","value":"4800"}]}
2022-11-28T14:29:14.680Z daemon INFO INFO[0060] sending command                               command="CONFIGURE baudrate 4800" monitor=serial-monitor

2022-11-28T14:29:14.681Z daemon INFO INFO[0060] received message                              error=false event_type=configure message=OK monitor=serial-monitor

When changing the line ending, it's a NOOP:

2022-11-28T14:30:23.139Z monitor-service INFO No port configuration changes have been detected. No need to send configure commands to the running monitor serial:/dev/cu.usbmodem14301.

@palazzol
Copy link
Contributor

I tried the tester build and it seems to work very well - thanks for doing this! I do find it odd that when changing one parameter, the information about which parameter has changed gets "lost" somehow and needs to be regenerated by diffing at another level. But, I don't really understand to full communications path - and these changes do seem to fix the issue.

@kittaakos
Copy link
Contributor Author

I tried the tester build and it seems to work very well

Great!

thanks for doing this!

You provided excellent support and helped me understand the problem. I appreciate your help.

I do find it odd that when changing one parameter, the information about which parameter has changed gets "lost" somehow and needs to be regenerated by diffing at another level.

I agree. IDE2 deserves better logic. I am unfamiliar with the monitor code, but I wanted to chime in after your #1677 (comment). I am happy we could find a point where we can work around the bug without eventually changing the monitor behavior.

@kittaakos kittaakos marked this pull request as ready for review November 29, 2022 08:09
Base automatically changed from theia-1.31.1 to main November 29, 2022 08:39
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.

LGTM code-wise

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.

I verified this fixes one component of #375

changing the selection in the line ending menu triggers the immediate sending of additional spurious data in addition to the regular periodic data reported above.

I notice the same happens when you toggle the "Toggle Autoscroll" or "Toggle Timestamp" icons.


The other component of #375, where spurious data is sent periodically even without a settings change has been fixed upstream via bugst/go-serial#147 + arduino/serial-monitor#31 and will reach Arduino IDE 2.x users without any need for action in this repository once the next release of serial-monitor is made.

Thanks Akos and @palazzol!

@kittaakos kittaakos merged commit d6a4b0f into main Nov 29, 2022
@kittaakos kittaakos deleted the #375 branch November 29, 2022 13:07
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
Development

Successfully merging this pull request may close these issues.

Serial Monitor sends spurious data
4 participants