pw_ring_buffer: Varint encode preamble byte
Preamble bytes act as field keys to permit ring buffers to be
proto-compliant after a dering operation. Allow passing in field keys
larger than a single byte and varint-encode them before writing.
Change-Id: I4def047c035fcaa94312e2c156f32694aa6bf9c8
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/35340
Reviewed-by: Ewout van Bekkum <ewout@google.com>
Commit-Queue: Prashanth Swaminathan <prashanthsw@google.com>
diff --git a/pw_log_multisink/log_queue.cc b/pw_log_multisink/log_queue.cc
index 6a528ea..8fbc42c 100644
--- a/pw_log_multisink/log_queue.cc
+++ b/pw_log_multisink/log_queue.cc
@@ -23,9 +23,9 @@
namespace {
using pw::protobuf::WireType;
-constexpr std::byte kLogKey = static_cast<std::byte>(pw::protobuf::MakeKey(
+constexpr uint32_t kLogKey = pw::protobuf::MakeKey(
static_cast<uint32_t>(pw::log::LogEntries::Fields::ENTRIES),
- WireType::kDelimited));
+ WireType::kDelimited);
} // namespace
@@ -66,7 +66,7 @@
status = PW_STATUS_INTERNAL;
} else {
// Try to push back the encoded log entry.
- status = ring_buffer_.TryPushBack(log_entry, std::byte(kLogKey));
+ status = ring_buffer_.TryPushBack(log_entry, kLogKey);
}
if (!status.ok()) {
diff --git a/pw_ring_buffer/prefixed_entry_ring_buffer.cc b/pw_ring_buffer/prefixed_entry_ring_buffer.cc
index 8758240..1f49678 100644
--- a/pw_ring_buffer/prefixed_entry_ring_buffer.cc
+++ b/pw_ring_buffer/prefixed_entry_ring_buffer.cc
@@ -75,7 +75,7 @@
Status PrefixedEntryRingBufferMulti::InternalPushBack(
std::span<const byte> data,
- byte user_preamble_data,
+ uint32_t user_preamble_data,
bool drop_elements_if_needed) {
if (buffer_ == nullptr) {
return Status::FailedPrecondition();
@@ -85,10 +85,17 @@
}
// Prepare the preamble, and ensure we can fit the preamble and entry.
- byte varint_buf[kMaxEntryPreambleBytes];
- size_t varint_bytes = varint::Encode<size_t>(data.size_bytes(), varint_buf);
+ byte preamble_buf[varint::kMaxVarint32SizeBytes];
+ byte length_buf[varint::kMaxVarint32SizeBytes];
+
+ size_t user_preamble_bytes = 0;
+ if (user_preamble_) {
+ user_preamble_bytes =
+ varint::Encode<uint32_t>(user_preamble_data, preamble_buf);
+ }
+ size_t length_bytes = varint::Encode<uint32_t>(data.size_bytes(), length_buf);
size_t total_write_bytes =
- (user_preamble_ ? 1 : 0) + varint_bytes + data.size_bytes();
+ user_preamble_bytes + length_bytes + data.size_bytes();
if (buffer_bytes_ < total_write_bytes) {
return Status::OutOfRange();
}
@@ -105,10 +112,12 @@
}
// Write the new entry into the ring buffer.
+ // TODO(pwbug/337): This could be more efficient, the preamble and length
+ // data could be written in a single raw write call.
if (user_preamble_) {
- RawWrite(std::span(&user_preamble_data, sizeof(user_preamble_data)));
+ RawWrite(std::span(preamble_buf, user_preamble_bytes));
}
- RawWrite(std::span(varint_buf, varint_bytes));
+ RawWrite(std::span(length_buf, length_bytes));
RawWrite(data);
// Update all readers of the new count.
@@ -315,17 +324,27 @@
PrefixedEntryRingBufferMulti::FrontEntryInfo(Reader& reader) {
// Entry headers consists of: (optional prefix byte, varint size, data...)
- // Read the entry header; extract the varint and it's size in bytes.
- byte varint_buf[kMaxEntryPreambleBytes];
+ // If a preamble exists, extract the varint and it's bytes in bytes.
+ size_t user_preamble_bytes = 0;
+ byte varint_buf[varint::kMaxVarint32SizeBytes];
+ if (user_preamble_) {
+ RawRead(varint_buf, reader.read_idx, varint::kMaxVarint32SizeBytes);
+ uint64_t user_preamble_data;
+ user_preamble_bytes = varint::Decode(varint_buf, &user_preamble_data);
+ PW_DASSERT(user_preamble_bytes != 0u);
+ }
+
+ // Read the entry header; extract the varint and it's bytes in bytes.
RawRead(varint_buf,
- IncrementIndex(reader.read_idx, user_preamble_ ? 1 : 0),
- kMaxEntryPreambleBytes);
- uint64_t entry_size;
- size_t varint_size = varint::Decode(varint_buf, &entry_size);
+ IncrementIndex(reader.read_idx, user_preamble_bytes),
+ varint::kMaxVarint32SizeBytes);
+ uint64_t entry_bytes;
+ size_t length_bytes = varint::Decode(varint_buf, &entry_bytes);
+ PW_DASSERT(length_bytes != 0u);
EntryInfo info = {};
- info.preamble_bytes = (user_preamble_ ? 1 : 0) + varint_size;
- info.data_bytes = entry_size;
+ info.preamble_bytes = user_preamble_bytes + length_bytes;
+ info.data_bytes = entry_bytes;
return info;
}
diff --git a/pw_ring_buffer/prefixed_entry_ring_buffer_test.cc b/pw_ring_buffer/prefixed_entry_ring_buffer_test.cc
index 9b3f15f..1275801 100644
--- a/pw_ring_buffer/prefixed_entry_ring_buffer_test.cc
+++ b/pw_ring_buffer/prefixed_entry_ring_buffer_test.cc
@@ -114,8 +114,13 @@
ASSERT_EQ(ring.FrontEntryDataSizeBytes(), 0u);
ASSERT_EQ(ring.FrontEntryTotalSizeBytes(), 0u);
- ASSERT_EQ(ring.PushBack(std::span(single_entry_data, data_size), byte(i)),
- OkStatus());
+ // Limit the value of the preamble to a single byte, to ensure that we
+ // retain a static `single_entry_buffer_size` during the test. Single
+ // bytes are varint-encoded to the same value.
+ uint32_t preamble_byte = i % 128;
+ ASSERT_EQ(
+ ring.PushBack(std::span(single_entry_data, data_size), preamble_byte),
+ OkStatus());
ASSERT_EQ(ring.FrontEntryDataSizeBytes(), data_size);
ASSERT_EQ(ring.FrontEntryTotalSizeBytes(), single_entry_total_size);
@@ -136,7 +141,7 @@
ASSERT_EQ(ring.PopFront(), OkStatus());
if (user_data) {
- expect_buffer[0] = byte(i);
+ expect_buffer[0] = byte(preamble_byte);
}
// ASSERT_THAT(std::span(expect_buffer),
@@ -242,8 +247,13 @@
ASSERT_EQ(ring.FrontEntryDataSizeBytes(), 0u);
ASSERT_EQ(ring.FrontEntryTotalSizeBytes(), 0u);
- ASSERT_EQ(ring.PushBack(std::span(single_entry_data, data_size), byte(i)),
- OkStatus());
+ // Limit the value of the preamble to a single byte, to ensure that we
+ // retain a static `single_entry_buffer_size` during the test. Single
+ // bytes are varint-encoded to the same value.
+ uint32_t preamble_byte = i % 128;
+ ASSERT_EQ(
+ ring.PushBack(std::span(single_entry_data, data_size), preamble_byte),
+ OkStatus());
ASSERT_EQ(ring.FrontEntryDataSizeBytes(), data_size);
ASSERT_EQ(ring.FrontEntryTotalSizeBytes(), single_entry_total_size);
@@ -262,7 +272,7 @@
ASSERT_EQ(ring.PopFront(), OkStatus());
if (user_data) {
- expect_buffer[0] = byte(i);
+ expect_buffer[0] = byte(preamble_byte);
}
ASSERT_EQ(
diff --git a/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h b/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
index ee6b393..c9d8e66 100644
--- a/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
+++ b/pw_ring_buffer/public/pw_ring_buffer/prefixed_entry_ring_buffer.h
@@ -169,7 +169,7 @@
//
// Preamble argument is a caller-provided value prepended to the front of the
// entry. It is only used if user_preamble was set at class construction
- // time.
+ // time. It is varint-encoded before insertion into the buffer.
//
// Return values:
// OK - Data successfully written to the ring buffer.
@@ -177,15 +177,22 @@
// FAILED_PRECONDITION - Buffer not initialized.
// OUT_OF_RANGE - Size of data is greater than buffer size.
Status PushBack(std::span<const std::byte> data,
- std::byte user_preamble_data = std::byte(0)) {
+ uint32_t user_preamble_data = 0) {
return InternalPushBack(data, user_preamble_data, true);
}
+ // [Deprecated] An implementation of PushBack that accepts a single-byte as
+ // preamble data. Clients should migrate to passing uint32_t preamble data.
+ Status PushBack(std::span<const std::byte> data,
+ std::byte user_preamble_data) {
+ return PushBack(data, static_cast<uint32_t>(user_preamble_data));
+ }
+
// Write a chunk of data to the ring buffer if there is space available.
//
// Preamble argument is a caller-provided value prepended to the front of the
// entry. It is only used if user_preamble was set at class construction
- // time.
+ // time. It is varint-encoded before insertion into the buffer.
//
// Return values:
// OK - Data successfully written to the ring buffer.
@@ -195,10 +202,17 @@
// RESOURCE_EXHAUSTED - The ring buffer doesn't have space for the data
// without popping off existing elements.
Status TryPushBack(std::span<const std::byte> data,
- std::byte user_preamble_data = std::byte(0)) {
+ uint32_t user_preamble_data = 0) {
return InternalPushBack(data, user_preamble_data, false);
}
+ // [Deprecated] An implementation of TryPushBack that accepts a single-byte as
+ // preamble data. Clients should migrate to passing uint32_t preamble data.
+ Status TryPushBack(std::span<const std::byte> data,
+ std::byte user_preamble_data) {
+ return TryPushBack(data, static_cast<uint32_t>(user_preamble_data));
+ }
+
// Get the size in bytes of all the current entries in the ring buffer,
// including preamble and data chunk.
size_t TotalUsedBytes() { return buffer_bytes_ - RawAvailableBytes(); }
@@ -266,7 +280,7 @@
// Push back implementation, which optionally discards front elements to fit
// the incoming element.
Status InternalPushBack(std::span<const std::byte> data,
- std::byte user_preamble_data,
+ uint32_t user_preamble_data,
bool pop_front_if_needed);
// Internal function to pop all of the slowest readers. This function may pop
@@ -311,10 +325,6 @@
// List of attached readers.
IntrusiveList<Reader> readers_;
- // Worst case size for the variable-sized preable that is prepended to
- // each entry.
- static constexpr size_t kMaxEntryPreambleBytes = sizeof(size_t) + 1;
-
// Maximum bufer size allowed. Restricted to this to allow index aliasing to
// not overflow.
static constexpr size_t kMaxBufferBytes =
