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

fix: implement Send and Sync for smart devices #53

Merged
merged 2 commits into from
May 4, 2024

Conversation

Tropix126
Copy link
Member

Describe the changes this PR makes. Why should it be merged?

Smart devices are currently not Send or Sync, which makes them not shareable between async tasks. This is because we store a raw device handle V5_DeviceT for convenience when interacting with the SDK. Sending raw pointers across threads isn't inherently unsound, and the lack of Send + Sync on raw pointers exists more as a lint.

Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@@ -19,6 +19,11 @@ pub struct DistanceSensor {
device: V5_DeviceT,
}

// SAFETY: Required because we store a raw pointer to the device handle to avoid it getting from the
// SDK each device function. Simply sharing a raw pointer across threads is not inherent unsafe.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that these commends should say "inherently unsafe" instead of "inherent unsafe".

Copy link
Member

@Gavin-Niederman Gavin-Niederman left a comment

Choose a reason for hiding this comment

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

LGTM

@Tropix126 Tropix126 merged commit 01d098e into main May 4, 2024
8 checks passed
@Gavin-Niederman Gavin-Niederman deleted the fix/unsafe-send-sync-impl branch May 4, 2024 22:38
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.

None yet

2 participants