Add Safe Abstractions for `FuriStreamBuffer` #177

cptpiepmatz · 2024-10-19T15:32:25Z

Hey 👋🏻,
In this PR, I added some safe abstractions for the FuriStreamBuffer.

While working on it, I came across this note in the stream buffer implementation:

 * ***NOTE***: Stream buffer implementation assumes there is only one task or
 * interrupt that will write to the buffer (the writer), and only one task or
 * interrupt that will read from the buffer (the reader).

So, I figured, why not enforce this constraint in the type system? The result is a split into a sender and receiver, similar to the mpsc channel in the standard library. They implement Send, so they can be moved between threads, but not Clone or Sync, making sure we only have one of each in a pair. Unfortunately, I had to use Arc to give ownership to both sides, which means this requires alloc. Other than that, I’m happy with how the API turned out.

I also added an example where data is sent between different threads.

JarvisCraft

The API looks great. Here is the first iteration of nitpicking, but generally the PR looks super accurate

crates/flipperzero/src/furi/stream_buffer.rs

JarvisCraft · 2024-10-19T15:55:54Z

crates/flipperzero/src/furi/mod.rs

+#[cfg(feature = "alloc")]
+pub mod stream_buffer;


Not sure about the following point, but I would prefer a separate stream-buffer feature which depends on alloc

It feels really bloated to a feature just because of the Arc I used. I could replace it with some custom counting logic using an AtomicUsize and sys::malloc to get a pointer but I'm not sure if that is better.

crates/flipperzero/src/furi/stream_buffer.rs

JarvisCraft · 2024-10-19T16:01:28Z

crates/flipperzero/src/furi/stream_buffer.rs

+    }
+}
+
+unsafe impl Send for Receiver {}


JarvisCraft · 2024-10-19T16:06:21Z

It will take me some time to think on the Arc-related part, but it looks good for the start

dcoles

Hi @cptpiepmatz!

Thanks for the PR! I really like the Sender / Receiver concept and how extensively you've documented everything 💖.

I think it would be useful to expose the low-level StreamBuffer API for no-alloc cases, but that's something we can certainly expose after merging this PR. Vast majority of comments are just little things for consistency with the other parts of the flipperzero-rs API.

crates/flipperzero/src/furi/stream_buffer.rs

dcoles · 2024-10-20T04:45:41Z

crates/flipperzero/src/furi/stream_buffer.rs

+    ///
+    /// In an interrupt routine, this method behaves like [`send`](Self::send).
+    pub fn send_with_timeout(&self, data: &[u8], timeout: core::time::Duration) -> usize {
+        let timeout = sys::furi::duration_to_ticks(timeout);


For the furi APIs we generally use furi::Duration rather than core::time::Duration to provide tick-level control over the timeout**.

(** Though we should really implement From/TryFrom<core::time::Duration> for furi::Duration)

crates/flipperzero/src/furi/stream_buffer.rs

cptpiepmatz · 2024-10-20T12:11:45Z

I made the StreamBuffer public with all of its methods, the send and receive methods are unsafe as they need to uphold the guarantee that only writer and one reader can exists at a time.

The Sender and Receiver is moved in the private stream module. I re-exported the entire module but that allowed to just mark it as #[cfg(feature = "alloc")]. For that purpose I also removed the stream-buffer feature again. I like this setup now, you can use the StreamBuffer without alloc but if you don't want to manage send and receive manually, you can use Sender and Receiver by enabling the alloc feature.

Since the StreamBuffer itself is now public, I added some methods on the Sender and Receiver to access the StreamBuffer and removed all the redirect methods. I also heavily updated the documentation to incorporate the new API.

dcoles · 2024-10-21T00:11:39Z

Hi @cptpiepmatz. This looks great! I'd be happy to merge this once @JarvisCraft has taken a final look.

cptpiepmatz · 2024-10-21T08:19:56Z

@dcoles could you please approve the workflow again? I added a SetTriggerLevelError to make clippy happy

cptpiepmatz · 2024-11-05T09:04:45Z

@JarvisCraft is this ready to merge?

JarvisCraft · 2024-11-05T14:53:39Z

@JarvisCraft is this ready to merge?

Hi there, apologies for the long reply. I will take a look at the PR this week.

JarvisCraft

Apologies for the delay on code-review.

The PR looks amazing and the detailed docs are just the chef's kiss.

Thanks a lot for your contribution <3

JarvisCraft · 2024-11-14T20:37:31Z

crates/flipperzero/src/furi/stream_buffer.rs

+    #[cfg_attr(feature = "alloc", doc = "[`into_stream`](Self::into_stream)")]
+    /// which is available using the `alloc` feature.
+    pub fn new(size: NonZeroUsize, trigger_level: usize) -> Self {
+        let size: usize = size.into();


Non-blocker nit (ptobably, for the future changes): given that type annotation is already used, from seems more appropriate:

Suggested change

let size: usize = size.into();

let size = usize::from(size);

JarvisCraft · 2024-11-14T20:40:00Z

crates/flipperzero/src/furi/stream_buffer.rs

+    use core::{cell::Cell, marker::PhantomData};
+
+    /// A zero-sized type used to mark types as `!Sync`.
+    type PhantomUnsync = PhantomData<Cell<()>>;


For future work on our side: this should probably be moved to some internal utils module

JarvisCraft · 2024-11-14T20:56:52Z

Squashed the commits so that fix-named intermediate comments don't end up in main

cptpiepmatz · 2024-11-14T23:14:27Z

Sweet!

cptpiepmatz added 6 commits October 19, 2024 12:08

initial implementation of safe abstractions for stream buffers

586b24d

started implementing an example for the stream buffer

a5e0323

fixed use of incorrect function

9b50cab

updated some docs

ec67af1

finished example

628ac5a

fmt

3a78fbb

JarvisCraft requested changes Oct 19, 2024

View reviewed changes

handled some nitpicks

54c1cae

cptpiepmatz requested a review from JarvisCraft October 19, 2024 19:26

dcoles requested changes Oct 20, 2024

View reviewed changes

dcoles reviewed Oct 20, 2024

View reviewed changes

crates/flipperzero/src/furi/stream_buffer.rs Outdated Show resolved Hide resolved

cptpiepmatz added 6 commits October 20, 2024 12:13

exposed the StreamBuffer type and updated stream pair

666c572

typo

8ca373f

updated example to use updated stream buffer api

bc16adc

added into_stream_buffer methods

7c761d5

call it Interrupt Routines everywhere

3333284

updated docs

0a8e60b

cptpiepmatz requested a review from dcoles October 20, 2024 12:11

added a brief description of the module

cb95d86

dcoles approved these changes Oct 21, 2024

View reviewed changes

added error type to make clippy happy

491f549

JarvisCraft mentioned this pull request Nov 12, 2024

Create 0.13.0 release #188

Merged

Merge branch 'main' into safe-stream-buffer

7f02bbd

JarvisCraft approved these changes Nov 14, 2024

View reviewed changes

JarvisCraft added the enhancement New feature or request label Nov 14, 2024

JarvisCraft added this to the v0.13.0 milestone Nov 14, 2024

JarvisCraft self-assigned this Nov 14, 2024

fix: update to latest Status API

8ed6d51

JarvisCraft merged commit b676f70 into flipperzero-rs:main Nov 14, 2024
9 checks passed

cptpiepmatz deleted the safe-stream-buffer branch November 14, 2024 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Safe Abstractions for `FuriStreamBuffer` #177

Add Safe Abstractions for `FuriStreamBuffer` #177

cptpiepmatz commented Oct 19, 2024

JarvisCraft left a comment

JarvisCraft Oct 19, 2024

cptpiepmatz Oct 19, 2024

JarvisCraft Oct 19, 2024

JarvisCraft commented Oct 19, 2024

dcoles left a comment

dcoles Oct 20, 2024

cptpiepmatz commented Oct 20, 2024 •

edited

Loading

dcoles commented Oct 21, 2024

cptpiepmatz commented Oct 21, 2024

cptpiepmatz commented Nov 5, 2024

JarvisCraft commented Nov 5, 2024

JarvisCraft left a comment

JarvisCraft Nov 14, 2024

JarvisCraft Nov 14, 2024

JarvisCraft commented Nov 14, 2024

cptpiepmatz commented Nov 14, 2024

Add Safe Abstractions for FuriStreamBuffer #177

Add Safe Abstractions for FuriStreamBuffer #177

Conversation

cptpiepmatz commented Oct 19, 2024

JarvisCraft left a comment

Choose a reason for hiding this comment

JarvisCraft Oct 19, 2024

Choose a reason for hiding this comment

cptpiepmatz Oct 19, 2024

Choose a reason for hiding this comment

JarvisCraft Oct 19, 2024

Choose a reason for hiding this comment

JarvisCraft commented Oct 19, 2024

dcoles left a comment

Choose a reason for hiding this comment

dcoles Oct 20, 2024

Choose a reason for hiding this comment

cptpiepmatz commented Oct 20, 2024 • edited Loading

dcoles commented Oct 21, 2024

cptpiepmatz commented Oct 21, 2024

cptpiepmatz commented Nov 5, 2024

JarvisCraft commented Nov 5, 2024

JarvisCraft left a comment

Choose a reason for hiding this comment

JarvisCraft Nov 14, 2024

Choose a reason for hiding this comment

JarvisCraft Nov 14, 2024

Choose a reason for hiding this comment

JarvisCraft commented Nov 14, 2024

cptpiepmatz commented Nov 14, 2024

Add Safe Abstractions for `FuriStreamBuffer` #177

Add Safe Abstractions for `FuriStreamBuffer` #177

cptpiepmatz commented Oct 20, 2024 •

edited

Loading