diff --git a/cev-lib/chuds/catalog.hpp b/cev-lib/chuds/catalog.hpp index e255650..3340a1b 100644 --- a/cev-lib/chuds/catalog.hpp +++ b/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); diff --git a/cev-lib/chuds/docs/serialization.md b/cev-lib/chuds/docs/serialization.md index 0847c52..dc6634b 100644 --- a/cev-lib/chuds/docs/serialization.md +++ b/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` / `body_as` accept a body only if all of these hold: +`make_message` / `body_as` 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) == )`. 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) == )`, 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::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()` that walks the members and checks layout directly. -Until then the marker is the bridge. diff --git a/cev-lib/chuds/message.hpp b/cev-lib/chuds/message.hpp index 1675948..3ba6630 100644 --- a/cev-lib/chuds/message.hpp +++ b/cev-lib/chuds/message.hpp @@ -65,18 +65,54 @@ struct Message { static_assert(std::endian::native == std::endian::little); static_assert(std::numeric_limits::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 +consteval bool has_no_padding_or_pointers() { + for (const auto byte : std::bit_cast>(T{})) { + static_cast(byte); + } + return true; +} + +template +concept HasWireBodyMarker = requires { requires T::is_chuds_wire_body; }; + +// satisfied when has_no_padding_or_pointers() is a constant expression +template +concept NoPaddingOrPointers = + requires { typename std::bool_constant()>; }; + +} // 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 +concept WireBody = + detail::HasWireBodyMarker && std::is_trivially_copyable_v && + std::is_standard_layout_v && sizeof(T) <= kMaxBody && detail::NoPaddingOrPointers; + +namespace detail { + +// one message per WireBody rule, then WireBody itself so a rule missing here still fails template 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, + "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, "body must be trivially copyable"); static_assert(std::is_standard_layout_v, "body must be standard-layout"); static_assert(sizeof(T) <= kMaxBody, "body is larger than the CAN-FD payload"); + static_assert(NoPaddingOrPointers, + "body must have no padding or pointers, and a constexpr default constructor"); + static_assert(WireBody); } +} // 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 @@ -84,7 +120,7 @@ template [[nodiscard]] constexpr std::optional make_message(MsgClass cls, std::uint8_t sub, MsgType type, const T& value) { // ensure the struct can be sent over wire - check_wire_body(); + detail::check_wire_body(); if (!is_valid(cls) || !is_valid(type)) { return std::nullopt; @@ -104,7 +140,7 @@ template template [[nodiscard]] constexpr std::optional body_as(const Message& m) { // ensure the struct can be sent over wire - check_wire_body(); + detail::check_wire_body(); if (m.body_len != sizeof(T)) { return std::nullopt; diff --git a/cev-lib/chuds/tests/CMakeLists.txt b/cev-lib/chuds/tests/CMakeLists.txt index d629b7f..62fff18 100644 --- a/cev-lib/chuds/tests/CMakeLists.txt +++ b/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") diff --git a/cev-lib/chuds/tests/compile_fail.cpp b/cev-lib/chuds/tests/compile_fail.cpp new file mode 100644 index 0000000..a817516 --- /dev/null +++ b/cev-lib/chuds/tests/compile_fail.cpp @@ -0,0 +1,35 @@ +#include + +#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( + chuds::make_message(chuds::MsgClass::Telemetry, 0, chuds::MsgType::Update, SendBody{})); + static_cast(chuds::body_as(chuds::Message{})); +} +} // namespace diff --git a/cev-lib/chuds/tests/test_message.cpp b/cev-lib/chuds/tests/test_message.cpp index 69d7c88..5e21943 100644 --- a/cev-lib/chuds/tests/test_message.cpp +++ b/cev-lib/chuds/tests/test_message.cpp @@ -26,9 +26,101 @@ static_assert(!CanReadStop); 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 values{}; + std::uint16_t d{}; + static constexpr bool is_chuds_wire_body = true; +}; + +static_assert(WireBody); +static_assert(WireBody); +static_assert(WireBody); +static_assert(WireBody); + +// 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 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 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); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); +static_assert(!WireBody); } // namespace // encode and decode run at compile time (constexpr)