template-ify the cbor stuff

src/audio/chunk.cpp

@@ -10,18 +10,22 @@
 namespace audio {
 
 /*
- * The maximum size that we expect a header to take up.
+ * The amount of space to allocate for the first chunk's header. After the first
+ * chunk, we have a more concrete idea of the header's size and can allocate
+ * space for future headers more compactly.
  */
-// TODO: tune this.
-static const size_t kMaxHeaderSize = 64;
+// TODO: measure how big headers tend to be to pick a better value.
+static const size_t kInitialHeaderSize = 32;
 
 auto WriteChunksToStream(MessageBufferHandle_t *stream, uint8_t *working_buffer, size_t working_buffer_length, std::function<size_t(uint8_t*,size_t)> callback, TickType_t max_wait) -> EncodeWriteResult {
+
+  size_t header_size = kInitialHeaderSize;
   while (1) {
     // First, ask the callback for some data to write.
     size_t chunk_size =
       callback(
-          working_buffer + kMaxHeaderSize,
-          working_buffer_length - kMaxHeaderSize);
+          working_buffer + header_size,
+          working_buffer_length - header_size);
 
     if (chunk_size == 0) {
       // They had nothing for us, so bail out.
@@ -31,22 +35,28 @@ auto WriteChunksToStream(MessageBufferHandle_t *stream, uint8_t *working_buffer,
     // Put together a header.
     cbor::Encoder encoder(cbor::CONTAINER_ARRAY, 3, working_buffer, working_buffer_length);
     encoder.WriteUnsigned(TYPE_CHUNK_HEADER);
-    // Note here that we need to write the offset of the chunk into the header.
-    // We could be smarter here and write the actual header size, allowing us to
-    // pack slightly more data into each message, but this is hard so I haven't
-    // done it. Please make my code better for me.
-    encoder.WriteUnsigned(kMaxHeaderSize);
+    encoder.WriteUnsigned(header_size);
     encoder.WriteUnsigned(chunk_size);
-    if (encoder.Finish().has_error()) {
+
+    size_t new_header_size = header_size;
+    cpp::result<size_t, CborError> encoder_res = encoder.Finish();
+    if (encoder_res.has_error()) {
       return CHUNK_ENCODING_ERROR;
-    };
+    } else {
+      // We can now tune the space to allocate for the header to be closer to
+      // its actual size. We pad this by 2 bytes to allow extra space for the
+      // chunk size and header size fields to each spill over into another byte
+      // each.
+      new_header_size = encoder_res.value() + 2;
+    }
 
     // Try to write to the buffer. Note the return type here will be either 0 or
-    // kMaxHeaderSize + chunk_size, as MessageBuffer doesn't allow partial
-    // writes.
+    // header_size + chunk_size, as MessageBuffer doesn't allow partial writes.
     size_t actual_write_size =
       xMessageBufferSend(
-          *stream, working_buffer, kMaxHeaderSize + chunk_size, max_wait);
+          *stream, working_buffer, header_size + chunk_size, max_wait);
+
+    header_size = new_header_size;
 
     if (actual_write_size == 0) {
       // We failed to write in time, so bail out. This is techinically data loss
@@ -84,8 +94,7 @@ auto ReadChunksFromStream(MessageBufferHandle_t *stream, uint8_t *working_buffer
       return CHUNK_STREAM_ENDED;
     }
 
-    // Work the size and position of the chunk (don't assume it's at
-    // kMaxHeaderSize offset for future-proofing).
+    // Work the size and position of the chunk.
     header_length = decoder.ParseUnsigned().value_or(0);
     chunk_length = decoder.ParseUnsigned().value_or(0);
     if (decoder.Failed()) {

src/cbor/cbor_decoder.cpp

@@ -18,68 +18,6 @@ static auto ArrayDecoder::Create(uint8_t *buffer, size_t buffer_len) -> cpp::res
   return std::move(decoder);
 }
 
-
-auto ArrayDecoder::ParseString() -> cpp::result<std::string, CborError> {
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-
-  if (!cbor_value_is_byte_string(&it_)) {
-    error_ = CborErrorIllegalType;
-    return cpp::fail(error_);
-  }
-  uint8_t *buf; size_t len; CborValue new_val;
-  error_ = cbor_value_dup_byte_string(&it_, &buf, &len, &new_val);
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  std::string ret(buf, len);
-  free(buf);
-  val_ = new_val;
-  return ret;
-}
-
-auto ArrayDecoder::ParseUnsigned() -> cpp::result<uint32_t, CborError> {
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-
-  if (!cbor_value_is_unsigned_integer(&it_)) {
-    error_ = CborErrorIllegalType;
-    return cpp::fail(error_);
-  }
-  uint64_t ret;
-  error_ = cbor_value_get_uint64(&it_, &ret);
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  error_ = cbor_value_advance(&it_);
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  return ret;
-}
-
-auto ArrayDecoder::ParseSigned() -> cpp::result<int32_t, CborError> {
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  if (!cbor_value_is_unsigned_integer(&it_)) {
-    error_ = CborErrorIllegalType;
-    return cpp::fail(error_);
-  }
-  uint64_t ret;
-  error_ = cbor_value_get_uint64(&it_, &ret);
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  error_ = cbor_value_advance(&it_);
-  if (error_ != CborNoError) {
-    return cpp::fail(error_);
-  }
-  return ret;
-}
-
 static auto MapDecoder::Create(uint8_t *buffer, size_t buffer_len) -> cpp::result<std::unique_ptr<MapDecoder>, CborError> {
   auto decoder = std::make_unique<MapDecoder>();
   cbor_parser_init(buffer, buffer_len, &decoder->parser_, &decoder->root_);
@@ -141,6 +79,7 @@ auto MapDecoder::FindSigned(const std::string &key) -> std::optional<int32_t> {
     return {};
   }
   if (cbor_value_map_find_value(&it_, key.c_str(), &val) != CborNoError) {
+    // Don't store this as an error; missing keys are fine.
     return {};
   }
   if (!cbor_value_is_integer(&val)) {

src/cbor/include/cbor_decoder.hpp

@@ -1,15 +1,61 @@
 #pragma once
 
+#include <stdint.h>
 #include <cstdint>
+
 namespace cbor {
 
+  static auto parse_stdstring(CborValue *val, std::string *out) -> CborError {
+        uint8_t *buf; size_t len;
+        CborError err = cbor_value_dup_byte_string(val, &buf, &len, NULL);
+        if (err != CborNoError) {
+          return err;
+        }
+        *out = std::move(std::string(buf, len));
+        free(buf);
+        return err
+  }
+
   class ArrayDecoder {
     public:
-      static auto Create(uint8_t *buffer, size_t buffer_len) -> cpp::result<std::unique_ptr<ArrayDecoder>, CborError>;
+      static auto Create(uint8_t *buffer, size_t buffer_len)
+        -> cpp::result<std::unique_ptr<ArrayDecoder>, CborError>;
+
+      template<typename T>
+      auto NextValue() -> cpp::result<T, CborError>;
 
-      auto ParseString() -> cpp::result<std::string, CborError>;
-      auto ParseUnsigned() -> cpp::result<uint32_t, CborError>;
-      auto ParseSigned() -> cpp::result<int32_t, CborError>;
+      template<> auto NextValue() -> cpp::result<int64_t, CborError> {
+        return NextValue(&cbor_value_is_integer, &cbor_value_get_int);
+      }
+      template<> auto NextValue() -> cpp::result<uint64_t, CborError> {
+        return NextValue(&cbor_value_is_unsigned_integer, &cbor_value_get_uint64);
+      }
+      template<> auto NextValue() -> cpp::result<std::string, CborError> {
+        return NextValue(&cbor_value_is_byte_string, &parse_stdstring);
+      }
+
+      template <typename T>
+      auto NextValue(
+          bool(*is_valid)(CborValue*),
+          CborError(*parse)(CborValue*, T*)) -> cpp::result<T, CborError> {
+        if (error_ != CborNoError) {
+          return cpp::fail(error_);
+        }
+        if (!is_valid(&it_)) {
+          error_ = CborErrorIllegalType;
+          return cpp::fail(error_);
+        }
+        T ret;
+        error_ = parse(&it_, &ret);
+        if (error_ != CborNoError) {
+          return cpp::fail(error_);
+        }
+        error_ = cbor_value_advance(&it_);
+        if (error_ != CborNoError) {
+          return cpp::fail(error_);
+        }
+        return ret;
+      }
 
       auto Failed() -> CborError { return error_; }
 
@@ -27,9 +73,41 @@ namespace cbor {
     public:
       static auto Create(uint8_t *buffer, size_t buffer_len) -> cpp::result<std::unique_ptr<MapDecoder>, CborError>;
 
-      auto FindString(const std::string &key) -> std::optional<std::string>;
-      auto FindUnsigned(const std::string &key) -> std::optional<uint32_t>;
-      auto FindSigned(const std::string &key) -> std::optional<int32_t>;
+      template<typename T>
+      auto FindValue(const std::string &key) -> std::optional<T>;
+
+      template<> auto FindValue(const std::string &key) -> std::optional<int64_t> {
+        return FindValue(key, &cbor_value_is_integer, &cbor_value_get_int);
+      }
+      template<> auto FindValue(const std::string &key) -> std::optional<uint64_t> {
+        return FindValue(key, &cbor_value_is_unsigned_integer, &cbor_value_get_uint64);
+      }
+      template<> auto FindValue(const std::string &key) -> std::optional<std::string> {
+        return FindValue(key, &cbor_value_is_byte_string, &parse_stdstring);
+      }
+
+      template <typename T>
+      auto FindValue(
+          const std::string &key,
+          bool(*is_valid)(CborValue*),
+          CborError(*parse)(CborValue*, T*)) -> std::optional<T> {
+        if (error_ != CborNoError) {
+          return {};
+        }
+        if (cbor_value_map_find_value(&it_, key.c_str(), &val) != CborNoError) {
+          return {};
+        }
+        if (!is_valid(&val)) {
+          error_ = CborErrorIllegalType;
+          return {};
+        }
+        T ret;
+        error_ = parse(&val, &ret);
+        if (error_ != CborNoError) {
+          return cpp::fail(error_);
+        }
+        return ret;
+      }
 
       auto Failed() -> CborError { return error_; }