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

Implement Send + Sync for LedCanvas #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StefanBossbaly
Copy link

Checklist

Thanks for contributing! Please ensure

  • All tests are passing
  • Tests or examples are added, if necessary
  • Add a CHANGELOG.md entryin the Unreleased section under the appropriate heading

Pull Request Description

Since led_matrix_swap_on_vsync() will block until the next vsync, implementing the Send + Sync traits for LedCanvas will allow the canvas to be sent between a render thread and a draw thread. Since the underlying canvas is created with CreateFrameCanvas(), which allocates the canvas memory on the heap, it is safe to share between threads.

@TDHolmes
Copy link

Could you add a safety comment explaining why this is acceptable? I agree with your description in the PR, just would be good to have in the code as well.

@TDHolmes
Copy link

@StefanBossbaly , any update here?

Since led_matrix_swap_on_vsync() will block until the next vsync,
implementing the Send + Sync traits for LedCanvas will allow the canvas
to be sent between a render thread and a draw thread. Since the
underlying canvas is created with CreateFrameCanvas(), which allocates
the canvas memory on the heap, it is safe to share between threads.
@StefanBossbaly StefanBossbaly force-pushed the feature-send-sync-canvas branch from 970c338 to f8e156a Compare December 22, 2024 00:28
@StefanBossbaly
Copy link
Author

@StefanBossbaly , any update here?

@TDHolmes sorry forgot about this. Added rustdoc in code for an explaination as to why this can be safely marked Send and Sync.

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