From 277552cef09bc54197ec0493fd26d4b4f1cd56aa Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 9 Apr 2026 00:13:23 -1000 Subject: [PATCH] Address review: reserve_byte, varint(0), field_ifdef, getattr guard --- esphome/components/api/api_pb2.cpp | 3 ++- esphome/components/api/proto.h | 6 ++++++ script/api_protobuf/api_protobuf.py | 30 +++++++++++++++++++++++------ 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/esphome/components/api/api_pb2.cpp b/esphome/components/api/api_pb2.cpp index 038c1c7800..c2d513f0d3 100644 --- a/esphome/components/api/api_pb2.cpp +++ b/esphome/components/api/api_pb2.cpp @@ -2333,7 +2333,8 @@ uint8_t *BluetoothLERawAdvertisementsResponse::encode(ProtoWriteBuffer &buffer P for (uint16_t i = 0; i < this->advertisements_len; i++) { auto &sub_msg = this->advertisements[i]; ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 10); - uint8_t *len_pos = pos++; + uint8_t *len_pos = pos; + ProtoEncode::reserve_byte(pos PROTO_ENCODE_DEBUG_ARG); ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 8); ProtoEncode::encode_varint_raw_64(pos PROTO_ENCODE_DEBUG_ARG, sub_msg.address); ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 16); diff --git a/esphome/components/api/proto.h b/esphome/components/api/proto.h index e0a4e03189..8cac7fff3b 100644 --- a/esphome/components/api/proto.h +++ b/esphome/components/api/proto.h @@ -352,6 +352,12 @@ class ProtoEncode { PROTO_ENCODE_CHECK_BOUNDS(pos, 1); *pos++ = b; } + /// Reserve one byte for later backpatch (e.g., sub-message length). + /// Advances pos past the reserved byte without writing a value. + static inline void ESPHOME_ALWAYS_INLINE reserve_byte(uint8_t *__restrict__ &pos PROTO_ENCODE_DEBUG_PARAM) { + PROTO_ENCODE_CHECK_BOUNDS(pos, 1); + pos++; + } /// Write raw bytes to the buffer (no tag, no length prefix). static inline void ESPHOME_ALWAYS_INLINE encode_raw(uint8_t *__restrict__ &pos PROTO_ENCODE_DEBUG_PARAM, const void *data, size_t len) { diff --git a/script/api_protobuf/api_protobuf.py b/script/api_protobuf/api_protobuf.py index b28a5a0c3d..6dc691249b 100755 --- a/script/api_protobuf/api_protobuf.py +++ b/script/api_protobuf/api_protobuf.py @@ -446,7 +446,7 @@ class TypeInfo(ABC): def _varint_max_size(bits: int) -> int: """Return the maximum varint encoding size for a value with the given number of bits.""" - return (bits + 6) // 7 # ceil(bits / 7) + return (max(bits, 1) + 6) // 7 # ceil(bits / 7), min 1 byte for varint(0) TYPE_INFO: dict[int, TypeInfo] = {} @@ -1615,7 +1615,8 @@ def _generate_inline_encode_block( lines = [] lines.append(f"auto &sub_msg = {element};") lines.append(f"ProtoEncode::write_raw_byte(pos, {tag});") - lines.append("uint8_t *len_pos = pos++;") + lines.append("uint8_t *len_pos = pos;") + lines.append("ProtoEncode::reserve_byte(pos);") # Generate inline field encoding for each sub-message field for field in sub_desc.field: @@ -1625,7 +1626,13 @@ def _generate_inline_encode_block( encode_line = ti.encode_content # Replace this-> with sub_msg reference for the sub-message fields encode_line = encode_line.replace("this->", "sub_msg.") - lines.extend(encode_line.split("\n")) + # Honor per-field ifdef guards + field_ifdef = ( + get_field_opt(field, pb.field_ifdef, None) + if field.options.HasExtension(pb.field_ifdef) + else None + ) + lines.extend(wrap_with_ifdef(encode_line, field_ifdef)) lines.append("*len_pos = static_cast(pos - len_pos - 1);") return "\n".join(lines) @@ -1659,7 +1666,13 @@ def _generate_inline_size_block( size_line = ti.get_size_calculation(f"sub_msg.{ti.field_name}", force) # Replace hardcoded this-> references (e.g., FixedArrayBytesType uses this->field_len) size_line = size_line.replace("this->", "sub_msg.") - lines.extend(size_line.split("\n")) + # Honor per-field ifdef guards + field_ifdef = ( + get_field_opt(field, pb.field_ifdef, None) + if field.options.HasExtension(pb.field_ifdef) + else None + ) + lines.extend(wrap_with_ifdef(size_line, field_ifdef)) return "\n".join(lines) @@ -2686,16 +2699,21 @@ def build_message_type( # Check if this message uses inline_encode — if so, skip generating standalone # encode/calculate_size methods since the encoding is inlined into the parent. + inline_opt = getattr(pb, "inline_encode", None) is_inline_only = ( message_id is None # Not a service message (no id) - and get_opt(desc, getattr(pb, "inline_encode", None), False) + and inline_opt is not None + and get_opt(desc, inline_opt, False) ) # Only generate encode method if this message needs encoding and has fields if needs_encode and encode and not is_inline_only: # Add PROTO_ENCODE_DEBUG_ARG after pos in all proto_* calls encode_debug = [ - line.replace("(pos,", "(pos PROTO_ENCODE_DEBUG_ARG,") for line in encode + line.replace("(pos,", "(pos PROTO_ENCODE_DEBUG_ARG,").replace( + "(pos)", "(pos PROTO_ENCODE_DEBUG_ARG)" + ) + for line in encode ] o = f"uint8_t *{desc.name}::encode(ProtoWriteBuffer &buffer PROTO_ENCODE_DEBUG_PARAM) const {{\n" o += " uint8_t *__restrict__ pos = buffer.get_pos();\n"