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

Don't update the Monitor with unrelated information #1677

Closed
wants to merge 1 commit into from

Conversation

palazzol
Copy link
Contributor

Motivation

I am working on this issue, which has multiple fixes in various components:
#375

Change description

Right now, The IDE sends redundant CONFIGURE commands via the serial monitor for many state changes in the GUI, making the above issue worse. You can see this by the many CONFIGURE messages in the console. This fix removes the updates for all the events which do not affect the serial communication state. (For example, dark mode, line endings, etc.)

Other information

Ideally, this fix would also filter the notifications to only reset the baudRate when the baudRate pulldown is selected. Right now, it appears that every commState parameter is updated in a sequence, even when only one is changed. (I expect to see one CONFIGURE statement in the console.) I couldn't figure out how to do that - maybe there is a better fix which would include this.
Specifically this function:
https://github.com/arduino/arduino-ide/blob/main/arduino-ide-extension/src/browser/serial/monitor/monitor-widget.tsx#L240
calls changeSettings() which even has comments indicating it should only change one thing:
https://github.com/arduino/arduino-ide/blob/main/arduino-ide-extension/src/node/monitor-service.ts#L485
And yet all of them seem to change?

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)

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2022

CLA assistant check
All committers have signed the CLA.

@per1234 per1234 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 16, 2022
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.

Thanks for your work to fix #375 @palazzol!

I am able to reproduce the spurious data that is sent when irrelevant settings are changed in the version of the IDE from the main branch, and see that it does not occur when using the build from this PR.

Unfortunately I did discover one small regression:

🐛 Line Ending menu is no longer synced between Serial Monitor and Serial Plotter

To reproduce

  1. Select any board and port in Arduino IDE.
  2. Open the "Serial Monitor" view (Tools > Serial Monitor) if it is not already open.
  3. Select Tools > Serial Plotter from the Arduino IDE menus.
  4. Change the selection in the Line Ending menu in the Serial Plotter window.
  5. Check the setting in the Line Ending menu in the Serial Monitor view.
    🐛 The setting was not changed in the Serial Monitor view's menu to match the setting in the Serial Plotter window's menu.
  6. Change the selection in the Line Ending menu in the Serial Monitor view.
  7. Check the setting in the Line Ending menu in the Serial Plotter window.
    🐛 The setting was not changed in the Serial Plotter window's menu to match the setting in the Serial Monitor view's menu.

image

Expected behavior

Line Ending setting is kept synced between Serial Monitor and Serial Plotter in order to make it easier for the user to send correctly formatted data to the board from either interface.

Arduino IDE version

9efd384 (tester build for 794ad37).

Operating system

Windows

Operating system version

10

Additional context

There is a bug where the two menus are not synced when Serial Monitor or Serial Plotter are opened: #1688

That bug is not a regression resulting from the changes proposed in this PR, so that behavior can be disregarded by the reviewers.


I believe this is the system used to communicate settings changes between Arduino IDE and Serial Plotter:

https://github.com/arduino/arduino-serial-plotter-webapp#websocket-communication-protocol

@palazzol
Copy link
Contributor Author

Sadly, even if this doesn't cause #1688 this fix does disrupt the mechanism by which a live Serial Monitor tells a live Serial Plotter that the Line Endings setting has changed. And now I see that the Serial Plotter also sends extra redundant info to the serial port. I am willing to dig in and sort out more about how this communication really works. @per1234 If there is a TypeScript developer who is familiar with this part of the code and would be willing to help me out with this, please have them contact me. Extra notifications are mostly "harmless" as long as they don't result in sending more updates to the serial port. Maybe they should be filtered in a different spot.

@kittaakos
Copy link
Contributor

Can somebody try my proposed changes from #1703, please?

If you are wondering how to test a build from a PR, please see the Beta Testing Guide. Thanks for the help!

@kittaakos
Copy link
Contributor

I am going to close this PR as a duplicate of #1703.

Thank you for your contribution and for initiating the change. Excellent work! If you disagree with the PR close, ping me here.

@kittaakos kittaakos closed this Nov 29, 2022
@kittaakos kittaakos added the conclusion: duplicate Has already been submitted label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: duplicate Has already been submitted 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.

4 participants