Skip to content

Add ArcBBQueue framed constructors with explicit LenHeader#128

Open
x46085 wants to merge 2 commits intojamesmunns:mainfrom
elodin-sys:framed-header-generic
Open

Add ArcBBQueue framed constructors with explicit LenHeader#128
x46085 wants to merge 2 commits intojamesmunns:mainfrom
elodin-sys:framed-header-generic

Conversation

@x46085
Copy link
Copy Markdown

@x46085 x46085 commented Apr 24, 2026

The existing framed_producer() and framed_consumer() methods on ArcBBQueue return handles hardcoded to u16 frame headers, which limits individual frames to 64 KiB. All the underlying types (FramedProducer, FramedConsumer, FramedGrantW, FramedGrantR) are already generic over the LenHeader trait, but the ArcBBQueue convenience constructors don't expose that generic. This adds framed_producer_with_header() and framed_consumer_with_header() companion methods that let the caller select the header type, such as usize for workloads that need frames larger than 64 KiB. The existing default-u16 methods are untouched.

In support of this upgrade; would resolve need for our fork in production:
elodin-sys/elodin#610

This restores caller control over frame header width so downstream users can opt into usize-framed queues for payloads larger than 64 KiB.

Made-with: Cursor
@x46085 x46085 marked this pull request as ready for review April 24, 2026 05:00
@jamesmunns
Copy link
Copy Markdown
Owner

I'm open to this, one potential concern is that you could take producer/consumer halves with different header lengths, which probably wouldn't cause any soundness issues, but might cause problems.

I'd have to check if non-Arc BBQueue has the same issues, but it might be worth including in the doc comments for these methods. Happy to merge and push a release otherwise.

Also very open to smarter ways to handle this in general, though I think probably the Right way to handle this is to have separate StreamBBQueue and FramedBBQueue outer types, the latter which includes the header kind as a type parameter, but that's a more invasive change.

@titaniumtraveler
Copy link
Copy Markdown

titaniumtraveler commented Apr 24, 2026

The existing framed_producer() and framed_consumer() methods on ArcBBQueue return handles hardcoded to u16 frame headers, which limits individual frames to 64 KiB. All the underlying types (FramedProducer, FramedConsumer, FramedGrantW, FramedGrantR) are already generic over the LenHeader trait, but the ArcBBQueue convenience constructors don't expose that generic. This adds framed_producer_with_header() and framed_consumer_with_header() companion methods that let the caller select the header type, such as usize for workloads that need frames larger than 64 KiB. The existing default-u16 methods are untouched.

Doesn't ArcBBQueue implement BbqHandle, which does have the the methods with an H: LenHeader parameter?

/// Create a [`FramedProducer`] from our `Target`'s `BBQueue`.
///
/// Must be equivalent to `self.bbq_ref().framed_producer()`.
fn framed_producer<H: LenHeader>(&self) -> FramedProducer<Self, H> {
FramedProducer {
bbq: self.bbq_ref(),
pd: PhantomData,
}
}
/// Create a [`FramedConsumer`] from our `Target`'s `BBQueue`.
///
/// Must be equivalent to `self.bbq_ref().framed_consumer()`.
fn framed_consumer<H: LenHeader>(&self) -> FramedConsumer<Self, H> {
FramedConsumer {
bbq: self.bbq_ref(),
pd: PhantomData,
}
}

Though it might well be that it currently needs to be called explicitly because of how these two APIs overlap.
(i.e. use something like BbqHandle::framed_producer<u16>(&arc_bbq))

@x46085
Copy link
Copy Markdown
Author

x46085 commented Apr 24, 2026

Appreciate the quick turnaround and the thoughtful feedback. Just pushed doc comments on both new methods calling out the producer/consumer header-match requirement so future users don't foot-gun themselves. Keeping the change additive and narrowly scoped — happy to use the StreamBBQueue/FramedBBQueue split someday, happy to help test whenever you need.

@x46085
Copy link
Copy Markdown
Author

x46085 commented May 2, 2026

bump! ready to make any other requested changes!

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.

3 participants