From baaeb038de50603a98a780db9043607a7c5975e1 Mon Sep 17 00:00:00 2001 From: Ben Deane Date: Fri, 28 Aug 2026 16:31:38 -0600 Subject: [PATCH 1/2] :bug: Fix write mask for whole-register write Problem: - When some bits in a register are not allocated into fields, writing a value to that register (`"reg"_r = value`) may incur a read-modify-write, because the mask used is the fields mask, despite the intention to write to the whole register. Solution: - When writing to a path, use the mask for the path rather than the mask aggregated from its known children. --- include/groov/write.hpp | 6 ++--- test/test.cpp | 4 ++-- test/write.cpp | 50 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 5 deletions(-) diff --git a/include/groov/write.hpp b/include/groov/write.hpp index 45cc893..25ad5db 100644 --- a/include/groov/write.hpp +++ b/include/groov/write.hpp @@ -118,9 +118,9 @@ auto write(Spec const &s) -> async::sender auto { all_fields_per_reg_t, written_fields_per_reg_t>; - using field_masks_t = boost::mp11::mp_transform; + using field_masks_t = + boost::mp11::mp_transform; detail::check_read_only(); detail::check_rmw(value).value_or(0); }); - groov::sync_write(grp0("reg0"_r = 0x76)); + groov::sync_write(grp0("reg0"_r = 0x10325476u)); CHECK(data0 == 0xa5a5'a5a5u); CHECK(my_bus::num_reads == 0); CHECK(my_bus::num_writes == 0); CHECK(write_call_count == 1); CHECK(write_addr == &data0); - CHECK(write_value == 0xbabefa76u); + CHECK(write_value == 0x10325476u); auto v = groov::test::get_value("reg0"_f); REQUIRE(v); CHECK(*v == 0xbabefaceu); diff --git a/test/write.cpp b/test/write.cpp index 7f0858b..efce816 100644 --- a/test/write.cpp +++ b/test/write.cpp @@ -15,6 +15,7 @@ #include #include +#include namespace { struct bus { @@ -549,3 +550,52 @@ TEST_CASE("write a field in a register containing WO fields which won't be " CHECK(groov::write(grp_be("reg4.field0"_r = 1)) | async::sync_wait()); CHECK(data3 == 42); } + +namespace { +struct rmw_check_bus { + static inline std::uint32_t expected_mask{}; + + template + static auto write(auto addr, auto value) -> async::sender auto { + CHECK((Mask | IdMask) == expected_mask); + return async::just_result_of([=] { + auto prev = *addr & ~(Mask | IdMask); + *addr = prev | value | IdValue; + }); + } + + template + static auto read(auto addr) -> async::sender auto { + return async::just_result_of([=] { return *addr; }); + } +}; + +std::uint32_t rmw_check_data{}; +using rmw_check_R = groov::reg<"r", std::uint32_t, &rmw_check_data, + groov::w::replace, F0, F1, F2>; + +using rmw_check_G = groov::group<"group", rmw_check_bus, rmw_check_R>; +constexpr auto rmw_check_grp = rmw_check_G{}; +} // namespace + +TEST_CASE("writing a register that is not covered by fields does not RMW", + "[write]") { + using namespace groov::literals; + rmw_check_bus::expected_mask = 0xffff'ffffu; + rmw_check_data = 0; + CHECK(groov::write(rmw_check_grp("r"_r = 0xa5a5u)) | async::sync_wait()); + CHECK(rmw_check_data == 0xa5a5u); +} + +TEST_CASE("writing register fields in a register that is not covered by fields " + "incurs RMW", + "[write]") { + using namespace groov::literals; + rmw_check_bus::expected_mask = 0xffu; + rmw_check_data = 0; + CHECK( + groov::write(rmw_check_grp("r.field0"_f = 0b1u, "r.field1"_f = 0b1010u, + "r.field2"_f = 0b110u)) | + async::sync_wait()); + CHECK(rmw_check_data == 0b110'1010'1u); +} From ac610879a662798cec6028893ec137461eac5259 Mon Sep 17 00:00:00 2001 From: Ben Deane Date: Mon, 31 Aug 2026 13:41:44 -0600 Subject: [PATCH 2/2] :white_check_mark: Add test for overlapping field writes Problem: - There is no test exercising overlapping field writes. Solution: - Add a test. - This is a niche situation, but when fields overlap and are written, the ordering of the arguments determines the actual written value. Example: ```cpp // given fields: using F0 = field<"f0", std::uint8_t, 0, 0>; using F1 = field<"f1", std::uint8_t, 1, 0>; // contains (overlaps) F0 // when we say write(grp("r.f0"_f = 1, "r.f1"_f = 0)); // the resulting value is 0b00 // i.e. as-if 1 is written to F0, then 0 is written to F1 // F1 "overwrites" F0 // when we say write(grp("r.f1"_f = 0, "r.f0"_f = 1)); // the resulting value is 0b01 // i.e. as-if 0 is written to F1, then 1 is written to F0 // F0 "overwrites" F1 ``` Note: - Fields which overlap in a parent-child relationship already provoke a compile-time error. i.e. the following is ill-formed: ```cpp write(grp("r"_r = 0, "r.f0" = 1)); ``` --- test/write.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/write.cpp b/test/write.cpp index efce816..2f82bab 100644 --- a/test/write.cpp +++ b/test/write.cpp @@ -599,3 +599,25 @@ TEST_CASE("writing register fields in a register that is not covered by fields " async::sync_wait()); CHECK(rmw_check_data == 0b110'1010'1u); } + +namespace { +using F_overlap = groov::field<"f_overlap", std::uint8_t, 3, 0>; + +std::uint32_t overlap_data{}; +using R_overlap = groov::reg<"r", std::uint32_t, &overlap_data, + groov::w::replace, F0, F_overlap>; + +using G_overlap = groov::group<"group", bus, R_overlap>; +constexpr auto overlap_grp = G_overlap{}; +} // namespace + +TEST_CASE("overlapping writes: last one takes priority", "[write]") { + using namespace groov::literals; + overlap_data = 0xffff'ffffu; + CHECK(sync_write(overlap_grp("r.f_overlap"_r = 0, "r.field0"_f = 1))); + CHECK(overlap_data == 0xffff'fff1u); + + overlap_data = 0xffff'ffffu; + CHECK(sync_write(overlap_grp("r.field0"_f = 1, "r.f_overlap"_r = 0))); + CHECK(overlap_data == 0xffff'fff0u); +}