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

Make monitor only send key on press, not on release #661

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

LelouBil
Copy link
Contributor

@LelouBil LelouBil commented Aug 2, 2024

Fixes #660

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for your contribution! Do you mind rebasing/fixing the rustfmt error and sharing your testing results?

This might be an improvement but, as you mentioned, there are differences between os/terminals, so I would like to test it properly.

@LelouBil LelouBil force-pushed the main branch 2 times, most recently from 49fc1e0 to ccde9d4 Compare August 7, 2024 11:37
@LelouBil
Copy link
Contributor Author

LelouBil commented Aug 7, 2024

Test results, works for :

  • IntelliJ/RustRover terminal
  • PuTTY Serial
  • Old Windows terminal (conhost.exe)
  • New Windows terminal

Works is : key press sends character, release does not. Key repetition works if held for a bit

I think it's fine, I can't test it on Linux now, but since it works with PuTTY I assume it will work with anything. PuTTY is the most basic serial client you can find.

I might also try tracking the issue from some kind of upstream dependency I think, that added difficulty debugging this : the fact that on the new Windows terminal and rustrover terminal key press and release events were sent only on key press.

But for the espflash issue, I think we're good

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR!

@SergioGasquez SergioGasquez added this pull request to the merge queue Aug 12, 2024
Merged via the queue into esp-rs:main with commit 36bea52 Aug 12, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Serial Monitor : Data is sent both for key press and release events
2 participants