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

Add a signal for when the CDC control state changes #1926

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

m5p3nc3r
Copy link
Contributor

This allows an async wait for changes in dts for example.

@@ -30,6 +32,9 @@ const REQ_SET_LINE_CODING: u8 = 0x20;
const REQ_GET_LINE_CODING: u8 = 0x21;
const REQ_SET_CONTROL_LINE_STATE: u8 = 0x22;

/// Signal will be send whenever the CDC control state changes
pub static CDC_CONTROL_CHANGED: Signal<CriticalSectionRawMutex, ()> = Signal::new();
Copy link
Member

@Dirbaio Dirbaio Sep 18, 2023

Choose a reason for hiding this comment

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

this should not be global, because you can create multiple instances of the CDC ACM class. It should be a field in State.

Also, ideally it should be a WakerRegistration instead of a Signal, because it's lighter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, didn't realise it could be instantiated multiple times. Will have a go at refactoring.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 18, 2023

Directly exposing a Signal allows the user to do other stuff with it, like signal it. We want the user to only be able to wait.

The public API should be methods on CdcAcmClass like wait_for_control_lines_change (or concrete ones like wait_for_dtr_high if you prefer).

@m5p3nc3r
Copy link
Contributor Author

I have updated this to use a WakerRegistration now. Ready for review again.

Sorry, but my git-fu is horribly lacking and my attempt to squash the commits failed. I assume you can do that on merge?

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

lgtm, thank you! :)

@Dirbaio Dirbaio added this pull request to the merge queue Oct 3, 2023
Merged via the queue into embassy-rs:main with commit ad52437 Oct 3, 2023
@m5p3nc3r m5p3nc3r deleted the async_dtr branch October 3, 2023 06:55
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