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

Feature gate local echo. #9

Merged
merged 4 commits into from
Mar 16, 2023
Merged

Feature gate local echo. #9

merged 4 commits into from
Mar 16, 2023

Conversation

badrbouslikhin
Copy link
Contributor

@badrbouslikhin badrbouslikhin commented Jan 8, 2023

Hello!

I'd like to propose adding a feature gate that disables local echo.

To implement this, I've added a new no-echo feature flag to the Cargo.toml file and wrapped the relevant code in an #[cfg(not(feature = "no-echo"))] block. I've also added code to write the whole command after a carriage-return is a sent. This is wrapped in an #[cfg(feature = "no-echo")].

I believe this feature will be useful for users who want to disable local echo in certain cases (when using menu with RTT transport for ex.). Please let me know if you have any questions or concerns.

Thank you!

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you!
Could you flip the logic to rather add an echo feature enabled by default?
I think that would fit better with features being additive in Rust.

@badrbouslikhin
Copy link
Contributor Author

It took me a long time to come back to this. It's done!

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Great, thanks!
Looks good to me.
Could you add an entry to the changelog section in the README?

@badrbouslikhin
Copy link
Contributor Author

Done!
I've also added a brief doc comment.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@eldruin eldruin merged commit 5c0c0fd into rust-embedded-community:master Mar 16, 2023
@thejpster thejpster mentioned this pull request Nov 15, 2023
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.

2 participants