-
-
Notifications
You must be signed in to change notification settings - Fork 491
fix: update monitor settings only if it's changed #1703
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
Conversation
870bfd0 to
de3794a
Compare
@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.
0a67591 to
2d5df27
Compare
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月28日T14:28:30.203Z daemon INFO INFO[0016] Starting monitor process monitor=serial-monitor
2022年11月28日T14: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月28日T14: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月28日T14: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月28日T14: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月28日T14:28:30.216Z daemon INFO INFO[0016] received message error=false event_type=open message=OK monitor=serial-monitor
2022年11月28日T14:28:30.216Z daemon INFO INFO[0016] Port /dev/cu.usbmodem14301 successfully opened
2022年11月28日T14: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月28日T14: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月28日T14: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月28日T14:29:14.679Z monitor-service INFO Sending monitor request with new port configuration: {"settingsList":[{"settingId":"baudrate","value":"4800"}]}
2022年11月28日T14:29:14.680Z daemon INFO INFO[0060] sending command command="CONFIGURE baudrate 4800" monitor=serial-monitor
2022年11月28日T14: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月28日T14: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.
2d5df27 to
3403e6f
Compare
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.
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.
Closes #375 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
3403e6f to
59e82f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM code-wise
There was a problem hiding this 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!
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,
setSettingspromise resolves withundefinedif there are no effective monitor settings updates. In such a case, the IDE2 monitor service does not send aMonitorRequestto the CLI.Other information
Closes #375
Reviewer checklist