chuds: check body with a concept

cev-lib/chuds/catalog.hpp

@@ -20,7 +20,7 @@ struct BodyState {
     static constexpr std::uint8_t kBlinkPhase = 1 << 5;
 
     std::uint8_t bits{};
-    static constexpr bool chuds_wire_body = true;
+    static constexpr bool is_chuds_wire_body = true;
 };
 static_assert(sizeof(BodyState) == 1);
 
@@ -36,7 +36,7 @@ struct Telemetry {
     std::uint16_t rpm_left{};   // wheel edges in the last window
     std::uint16_t rpm_right{};  // wheel edges in the last window
     std::uint16_t steering{};   // raw adc, 0-4095
-    static constexpr bool chuds_wire_body = true;
+    static constexpr bool is_chuds_wire_body = true;
 };
 static_assert(sizeof(Telemetry) == 6);
 
@@ -52,7 +52,7 @@ struct Telemetry {
     std::uint16_t rpm_left{};   // wheel edges in the last window
     std::uint16_t rpm_right{};  // wheel edges in the last window
     std::uint16_t brake{};      // raw adc, 0-4095
-    static constexpr bool chuds_wire_body = true;
+    static constexpr bool is_chuds_wire_body = true;
 };
 static_assert(sizeof(Telemetry) == 6);
 
@@ -69,7 +69,7 @@ struct Telemetry {
     float current{};  // bus amps, averaged over the report period
     float power{};    // watts, averaged over the report period
     float energy{};   // joules since boot
-    static constexpr bool chuds_wire_body = true;
+    static constexpr bool is_chuds_wire_body = true;
 };
 static_assert(sizeof(Telemetry) == 16);
 

cev-lib/chuds/docs/serialization.md

@@ -6,32 +6,32 @@ wire format. That is fast and simple, but it means not every struct is safe to s
 
 ## Requirements
 
-`make_message<T>` / `body_as<T>` accept a body only if all of these hold:
+`make_message<T>` / `body_as<T>` accept a body only if all of these hold. The `WireBody` concept
+in `message.hpp` checks them.
 
-1. Opt-in marker: the struct declares `static constexpr bool chuds_wire_body = true;`.
+1. Opt-in marker: the struct declares `static constexpr bool is_chuds_wire_body = true;`.
 2. Trivially copyable and standard-layout.
 3. `sizeof(T) <= 62` (fits the CAN-FD payload after the 2-byte header).
+4. No padding and no pointer members. Padding bytes are indeterminate and would leak onto the bus.
+   The check builds `T{}` at compile time, so the body must have a constexpr default constructor.
+   A plain struct of numbers always has one. A user-written constructor that is not `constexpr`
+   does not count.
 
 Plus two invariants every body must uphold, each backed by an assert:
 
-- No padding. Each body carries `static_assert(sizeof(T) == <sum of field bytes>)`. If the
-  compiler inserts padding, `sizeof` exceeds the total and it fails to compile. Padding bytes are
-  indeterminate and would leak onto the bus.
+- Fixed wire size. Each body carries `static_assert(sizeof(T) == <sum of field bytes>)`, so a
+  change to a body's size fails to compile until the expected size is edited too.
 - Little-endian host with IEEE 754 floats, asserted once in `message.hpp`
   (`std::endian::native == std::endian::little`, `std::numeric_limits<float>::is_iec559`).
 
 ## Why the marker
 
-The compiler cannot check "safe to serialize" generically. There is no standard trait for "no
-padding" or "no pointer members" without C++26 static reflection, which the firmware toolchain
-(arm-none-eabi-gcc) does not have. `std::has_unique_object_representations` is close but rejects
-floats (the joulemeter sends four), so it is unusable here.
+The compiler can check padding and pointers (requirement 4), but not "meant to go on the wire". A
+stray `int` or an unrelated plain struct passes every other check.
 
-So a body declares itself safe to send by opting in. Anything not marked, such as a
-`std::string_view`, a struct with a pointer member, or a stray trivially-copyable type, is
-rejected at compile time.
-The author owns the parts the compiler cannot see: no pointers, no padding (the `sizeof` assert
-proves this), fixed-width fields.
+So a body declares itself safe to send by opting in. Anything not marked, such as a stray
+trivially-copyable type, is rejected at compile time.
+The author owns the parts the compiler cannot see: fixed-width fields and no `bool`.
 
 ## What is not checked
 
@@ -51,14 +51,8 @@ themselves.
 ## Adding a body
 
 1. Use only fixed-width integers (`std::uint16_t`, ...), IEEE floats, or fixed-underlying-type
-   enums. No pointers, no `std::string`/`std::string_view`/`std::vector`/`std::span`, and no
-   `bool` (see above).
+   enums. No pointers, no `std::string`/`std::string_view`/`std::vector`/`std::span`, no
+   `bool` (see above), and no bit-fields (their bit layout is up to the compiler).
 2. Order fields so there is no padding, then `static_assert(sizeof(T) == N)`.
-3. Add `static constexpr bool chuds_wire_body = true;` inside the struct. It is `static`, so it
+3. Add `static constexpr bool is_chuds_wire_body = true;` inside the struct. It is `static`, so it
    takes up no bytes.
-
-## Future
-
-Once C++26 reflection reaches the arm toolchain, the marker and the per-body `sizeof` assert can
-be replaced by a generic `is_wire_safe<T>()` that walks the members and checks layout directly.
-Until then the marker is the bridge.

cev-lib/chuds/message.hpp

@@ -65,26 +65,62 @@ struct Message {
 static_assert(std::endian::native == std::endian::little);
 static_assert(std::numeric_limits<float>::is_iec559);
 
-// requirements to bit_cast a struct body onto the wire
+namespace detail {
+
+// compiles only if T has no padding and no pointer members
+// reading a padding byte, or bit_cast of a pointer, is not allowed at compile time
+template <class T>
+consteval bool has_no_padding_or_pointers() {
+    for (const auto byte : std::bit_cast<std::array<unsigned char, sizeof(T)>>(T{})) {
+        static_cast<void>(byte);
+    }
+    return true;
+}
+
+template <class T>
+concept HasWireBodyMarker = requires { requires T::is_chuds_wire_body; };
+
+// satisfied when has_no_padding_or_pointers<T>() is a constant expression
+template <class T>
+concept NoPaddingOrPointers =
+    requires { typename std::bool_constant<has_no_padding_or_pointers<T>()>; };
+
+}  // namespace detail
+
+// a struct that make_message and body_as can bit_cast onto the wire
+// bool, bit-fields, and enums without a fixed underlying type pass these checks but are unsafe
+template <class T>
+concept WireBody =
+    detail::HasWireBodyMarker<T> && std::is_trivially_copyable_v<T> &&
+    std::is_standard_layout_v<T> && sizeof(T) <= kMaxBody && detail::NoPaddingOrPointers<T>;
+
+namespace detail {
+
+// one message per WireBody rule, then WireBody itself so a rule missing here still fails
 template <class T>
 constexpr void check_wire_body() {
-    static_assert(
-        requires { requires T::chuds_wire_body; },
-        "wire body must opt in with `static constexpr bool chuds_wire_body = true;` "
-        "after confirming it has no padding and no pointer members");
+    static_assert(HasWireBodyMarker<T>,
+                  "wire body must opt in with `static constexpr bool is_chuds_wire_body = true;` "
+                  "after checking it has no bool, bit-field, or enum without a fixed underlying "
+                  "type (chuds/docs/serialization.md)");
     static_assert(std::is_trivially_copyable_v<T>, "body must be trivially copyable");
     static_assert(std::is_standard_layout_v<T>, "body must be standard-layout");
     static_assert(sizeof(T) <= kMaxBody, "body is larger than the CAN-FD payload");
+    static_assert(NoPaddingOrPointers<T>,
+                  "body must have no padding or pointers, and a constexpr default constructor");
+    static_assert(WireBody<T>);
 }
 
+}  // namespace detail
+
 // build a message whose body is a trivially-copyable struct
 // the struct's bytes are the wire body, little-endian
 // returns nullopt if the class or type is invalid
 template <class T>
 [[nodiscard]] constexpr std::optional<Message> make_message(MsgClass cls, std::uint8_t sub,
                                                             MsgType type, const T& value) {
     // ensure the struct can be sent over wire
-    check_wire_body<T>();
+    detail::check_wire_body<T>();
 
     if (!is_valid(cls) || !is_valid(type)) {
         return std::nullopt;
@@ -104,7 +140,7 @@ template <class T>
 template <class T>
 [[nodiscard]] constexpr std::optional<T> body_as(const Message& m) {
     // ensure the struct can be sent over wire
-    check_wire_body<T>();
+    detail::check_wire_body<T>();
 
     if (m.body_len != sizeof(T)) {
         return std::nullopt;

cev-lib/chuds/tests/CMakeLists.txt

@@ -28,6 +28,24 @@ endif()
 include(${doctest_SOURCE_DIR}/scripts/cmake/doctest.cmake)
 doctest_discover_tests(chuds-tests)
 
+# a rejected wire body is a compile error, so each case builds compile_fail.cpp
+# a case passes only if the build fails with the opt-in error, not some unrelated one
+foreach(case make_message body_as)
+    string(TOUPPER "BAD_${case}" define)
+    add_library(chuds-compile-${case} OBJECT EXCLUDE_FROM_ALL compile_fail.cpp)
+    target_link_libraries(chuds-compile-${case} PRIVATE chuds)
+    target_compile_definitions(chuds-compile-${case} PRIVATE ${define})
+    # clang-tidy can't parse a build that is meant to fail
+    set_target_properties(chuds-compile-${case} PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
+    set(test "wire body compile check: ${case}")
+    add_test(NAME ${test}
+             COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target chuds-compile-${case})
+    # concurrent builds of one build tree corrupt ninja's state
+    set_tests_properties(${test} PROPERTIES
+        PASS_REGULAR_EXPRESSION "wire body must opt in"
+        RESOURCE_LOCK build-tree)
+endforeach()
+
 # the socketcan adapter is Linux-only
 # test needs a CAN interface (skips without one)
 if(CMAKE_SYSTEM_NAME STREQUAL "Linux")

cev-lib/chuds/tests/compile_fail.cpp

@@ -0,0 +1,35 @@
+#include <cstdint>
+
+#include "chuds/ids.hpp"
+#include "chuds/message.hpp"
+
+// built once per case by tests/CMakeLists.txt
+// test_message.cpp covers the WireBody rules, this covers that both entry points enforce them
+namespace {
+struct Unmarked {
+    std::uint16_t a{};
+};
+
+struct Marked {
+    std::uint16_t a{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+
+#ifdef BAD_MAKE_MESSAGE
+using SendBody = Unmarked;
+#else
+using SendBody = Marked;
+#endif
+
+#ifdef BAD_BODY_AS
+using ReceiveBody = Unmarked;
+#else
+using ReceiveBody = Marked;
+#endif
+
+[[maybe_unused]] void send_and_receive() {
+    static_cast<void>(
+        chuds::make_message(chuds::MsgClass::Telemetry, 0, chuds::MsgType::Update, SendBody{}));
+    static_cast<void>(chuds::body_as<ReceiveBody>(chuds::Message{}));
+}
+}  // namespace

cev-lib/chuds/tests/test_message.cpp

@@ -26,9 +26,101 @@ static_assert(!CanReadStop<Message>);
 
 struct WheelSpeed {
     std::uint16_t rpm;
-    static constexpr bool chuds_wire_body = true;
+    static constexpr bool is_chuds_wire_body = true;
 };
 static_assert(sizeof(WheelSpeed) == 2);
+
+enum class Mode : std::uint8_t { Off, On };
+struct Mixed {
+    std::uint32_t a{};
+    std::int16_t b{};
+    Mode mode{};
+    std::uint8_t c{};
+    float d{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct Pair {
+    std::uint16_t a{};
+    std::uint16_t b{};
+};
+struct Nested {
+    Pair pair{};
+    std::uint32_t c{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct Array {
+    std::array<std::uint16_t, 3> values{};
+    std::uint16_t d{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+
+static_assert(WireBody<WheelSpeed>);
+static_assert(WireBody<Mixed>);
+static_assert(WireBody<Nested>);
+static_assert(WireBody<Array>);
+
+// every body below except Unmarked opts in, so each fails on the rule its name gives
+struct Unmarked {
+    std::uint16_t a{};
+};
+// NOLINTNEXTLINE(cppcoreguidelines-special-member-functions)
+struct NotTriviallyCopyable {
+    std::uint16_t a{};
+    static constexpr bool is_chuds_wire_body = true;
+    NotTriviallyCopyable()                   = default;
+    // NOLINTNEXTLINE(modernize-use-equals-default)
+    NotTriviallyCopyable(const NotTriviallyCopyable& other) : a(other.a) {}
+};
+class NotStandardLayout {
+   public:
+    std::uint16_t a{};
+    static constexpr bool is_chuds_wire_body = true;
+
+   private:
+    [[maybe_unused]] std::uint16_t b{};
+};
+struct TooBig {
+    std::array<std::uint8_t, kMaxBody + 1> bytes{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct MidPadding {
+    std::uint8_t a{};
+    float b{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct TailPadding {
+    float a{};
+    std::uint8_t b{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct NestedPadding {
+    MidPadding inner{};
+    std::uint32_t c{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct ArrayOfPadding {
+    std::array<MidPadding, 2> values{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct Pointer {
+    const char* text{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+struct NestedPointer {
+    Pointer inner{};
+    static constexpr bool is_chuds_wire_body = true;
+};
+
+static_assert(!WireBody<Unmarked>);
+static_assert(!WireBody<NotTriviallyCopyable>);
+static_assert(!WireBody<NotStandardLayout>);
+static_assert(!WireBody<TooBig>);
+static_assert(!WireBody<MidPadding>);
+static_assert(!WireBody<TailPadding>);
+static_assert(!WireBody<NestedPadding>);
+static_assert(!WireBody<ArrayOfPadding>);
+static_assert(!WireBody<Pointer>);
+static_assert(!WireBody<NestedPointer>);
 }  // namespace
 
 // encode and decode run at compile time (constexpr)