fix: add tests for FlashAligner and fix it (#88)

richardapeters · web-flow · commit 2133475931bb · 2025-06-13T09:05:05.000+02:00
* fix: add tests for FlashAligner and fix it

* Configure tests

* Only build postmaster.programmer.test when the tests are enabled
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -6,6 +6,7 @@ set(CMAKE_CXX_STANDARD 20)
 set(CMAKE_CXX_STANDARD_REQUIRED true)
 
 option(CMAKE_COMPILE_WARNING_AS_ERROR "Enable warnings-as-error" On)
+option(HALST_BUILD_TESTS "Enable building the tests" Off)
 
 include(CheckIPOSupported)
 check_ipo_supported(RESULT supported OUTPUT error)
@@ -17,9 +18,18 @@ add_definitions(-DMEMP_NUM_UDP_PCB=10)
 
 set(HALST_INCLUDE_DEFAULT_LINKER_SCRIPTS Off CACHE INTERNAL "")
 
+if (POSTMASTER_BUILD_TESTS)
+    include(CTest)
+endif()
+
 add_subdirectory(amp-embedded-infra-lib)
 add_subdirectory(amp-hal-st)
 add_subdirectory(amp-preview)
+
+if (POSTMASTER_BUILD_TESTS)
+    emil_enable_testing()
+endif()
+
 add_subdirectory(postmaster)
 
 emil_folderize_all_targets()
diff --git a/CMakePresets.json b/CMakePresets.json
@@ -14,6 +14,7 @@
       "generator": "Ninja Multi-Config",
       "cacheVariables": {
         "CMAKE_CONFIGURATION_TYPES": "Debug;RelWithDebInfo",
+        "POSTMASTER_BUILD_TESTS": true,
         "EMIL_BUILD_TESTS": true
       }
     },
diff --git a/amp-embedded-infra-lib b/amp-embedded-infra-lib
@@ -1 +1 @@
-Subproject commit 43db523f76363e31125c2a2d63d4b10cc4786cdd
+Subproject commit 72a8a2c67803912c9d9232b57b0d6c4998b1b0de
diff --git a/postmaster/instantiations/EchoServer.hpp b/postmaster/instantiations/EchoServer.hpp
@@ -18,11 +18,11 @@ namespace main_
             infra::ProxyCreator<hal::SerialCommunication, void(const UartConfig& config)> serial;
             services::MethodSerializerFactory::ForServices<>::AndProxies<> serializerFactory;
             hal::BufferedSerialCommunicationOnUnbuffered::WithStorage<256> bufferedSerial{ *serial };
-            main_::EchoOnSesame<256> echoUart{ bufferedSerial, serializerFactory };
+            main_::EchoOnSesame::WithMessageSize<256> echoUart{ bufferedSerial, serializerFactory };
             services::EchoOnConnection echoConnection{ serializerFactory };
 
-            services::ServiceForwarderAll forwardLeft{ echoUart, echoConnection };
-            services::ServiceForwarderAll forwardRight{ echoConnection, echoUart };
+            services::ServiceForwarderAll forwardLeft{ echoUart.echo, echoConnection };
+            services::ServiceForwarderAll forwardRight{ echoConnection, echoUart.echo };
 
             operator services::ConnectionObserver&();
             operator const services::ConnectionObserver&() const;
diff --git a/postmaster/instantiations/TargetUartEcho.hpp b/postmaster/instantiations/TargetUartEcho.hpp
@@ -42,12 +42,12 @@ namespace main_
             infra::ProxyCreator<hal::SerialCommunication, void(const UartConfig& config)> serial;
             services::MethodSerializerFactory::ForServices<>::AndProxies<> serializerFactory;
             hal::BufferedSerialCommunicationOnUnbuffered::WithStorage<256> bufferedSerial{ *serial };
-            main_::EchoOnSesame<256> echoUart{ bufferedSerial, serializerFactory };
+            main_::EchoOnSesame::WithMessageSize<256> echoUart{ bufferedSerial, serializerFactory };
             services::EchoOnConnection echoConnection{ serializerFactory };
             WebSocketServerConnectionObserver webSocket{ *this };
 
-            services::ServiceForwarderAll forwardLeft{ echoUart, echoConnection };
-            services::ServiceForwarderAll forwardRight{ echoConnection, echoUart };
+            services::ServiceForwarderAll forwardLeft{ echoUart.echo, echoConnection };
+            services::ServiceForwarderAll forwardRight{ echoConnection, echoUart.echo };
 
             operator services::ConnectionObserver&();
             operator const services::ConnectionObserver&() const;
diff --git a/postmaster/programmer/CMakeLists.txt b/postmaster/programmer/CMakeLists.txt
@@ -33,3 +33,5 @@ target_link_libraries(postmaster.programmer PUBLIC
     services.st_util
     protobuf.echo
 )
+
+add_subdirectory(test)
diff --git a/postmaster/programmer/FlashAligner.cpp b/postmaster/programmer/FlashAligner.cpp
@@ -9,8 +9,14 @@ namespace services
 
     void FlashAligner::Flush(const infra::Function<void()>& onDone)
     {
-        alignedBuffer.resize(alignedBuffer.max_size());
-        FlashDelegate::WriteBuffer(infra::MakeRange(alignedBuffer), address, onDone);
+        if (alignedBuffer.empty())
+            onDone();
+        else
+        {
+            auto size = alignedBuffer.size();
+            alignedBuffer.resize(alignedBuffer.max_size());
+            FlashDelegate::WriteBuffer(infra::MakeRange(alignedBuffer), address - size, onDone);
+        }
     }
 
     void FlashAligner::WriteBuffer(infra::ConstByteRange buffer, uint32_t address, infra::Function<void()> onDone)
@@ -20,6 +26,7 @@ namespace services
             auto range = infra::Head(buffer, alignedBuffer.max_size() - alignedBuffer.size());
             alignedBuffer.insert(alignedBuffer.end(), range.begin(), range.end());
             buffer.pop_front(range.size());
+            address += range.size();
         }
 
         if (alignedBuffer.full())
@@ -28,21 +35,27 @@ namespace services
             this->address = address;
             this->onDone = onDone;
 
-            FlashDelegate::WriteBuffer(infra::MakeRange(alignedBuffer), address, [this]()
+            FlashDelegate::WriteBuffer(infra::MakeRange(alignedBuffer), address - alignedBuffer.size(), [this]()
                 {
                     alignedBuffer.clear();
                     auto onDoneCopy = this->onDone.Clone();
                     this->onDone = nullptr;
-                    WriteBuffer(this->buffer, this->address + alignedBuffer.max_size(), onDoneCopy);
+                    WriteBuffer(this->buffer, this->address, onDoneCopy);
                 });
         }
         else
         {
-            auto range = infra::Head(buffer, buffer.size() - buffer.size() % alignedBuffer.max_size());
-            alignedBuffer.insert(alignedBuffer.end(), range.end(), buffer.end());
+            auto sendNowSize = buffer.size() - buffer.size() % alignedBuffer.max_size();
+            auto sendNow = infra::Head(buffer, sendNowSize);
+            auto sendLater = infra::DiscardHead(buffer, sendNowSize);
+            alignedBuffer.insert(alignedBuffer.end(), sendLater.begin(), sendLater.end());
+
+            this->address = address + buffer.size();
 
-            FlashDelegate::WriteBuffer(range, address, onDone);
-            this->address = address + range.size();
+            if (sendNow.empty())
+                onDone();
+            else
+                FlashDelegate::WriteBuffer(sendNow, address, onDone);
         }
     }
 }
diff --git a/postmaster/programmer/test/CMakeLists.txt b/postmaster/programmer/test/CMakeLists.txt
@@ -0,0 +1,13 @@
+add_executable(postmaster.programmer.test)
+emil_build_for(postmaster.programmer.test BOOL EMIL_BUILD_TESTS)
+emil_add_test(postmaster.programmer.test)
+
+target_link_libraries(postmaster.programmer.test PUBLIC
+    gmock_main
+    postmaster.programmer
+    hal.interfaces_test_doubles
+)
+
+target_sources(postmaster.programmer.test PRIVATE
+    TestFlashAligner.cpp
+)
diff --git a/postmaster/programmer/test/TestFlashAligner.cpp b/postmaster/programmer/test/TestFlashAligner.cpp
@@ -0,0 +1,91 @@
+#include "hal/interfaces/test_doubles/FlashMock.hpp"
+#include "infra/util/test_helper/MemoryRangeMatcher.hpp"
+#include "infra/util/test_helper/MockCallback.hpp"
+#include "postmaster/programmer/FlashAligner.hpp"
+#include "gtest/gtest.h"
+
+class FlashAlignerTest
+    : public testing::Test
+{
+public:
+    testing::StrictMock<hal::CleanFlashMock> flash;
+    services::FlashAligner::WithAlignment<4> aligner{ flash };
+
+    std::array<uint8_t, 1> size1{ 99 };
+    std::array<uint8_t, 4> size4{ 1, 2, 3, 4 };
+    std::array<uint8_t, 6> size6{ 5, 6, 7, 8, 9, 10 };
+    std::array<uint8_t, 8> size8{ 11, 12, 13, 14, 15, 16, 17, 18 };
+};
+
+TEST_F(FlashAlignerTest, write_one_block)
+{
+    EXPECT_CALL(flash, WriteBuffer(infra::MakeConstRange(size4), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size4, 0, infra::MockFunction<void()>());
+
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_small_block)
+{
+    aligner.WriteBuffer(size1, 0, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 99, 0, 0, 0 }), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_large_block)
+{
+    EXPECT_CALL(flash, WriteBuffer(infra::Head(infra::MakeConstRange(size6), 4), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size6, 0, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 9, 10, 0, 0}), 4, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_double_block)
+{
+    EXPECT_CALL(flash, WriteBuffer(infra::MakeConstRange(size8), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size8, 0, infra::MockFunction<void()>());
+
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_one_block_at_offset)
+{
+    aligner.WriteBuffer(size1, 0, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 99, 1, 2, 3 }), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size4, 1, infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_small_block_at_offset)
+{
+    aligner.WriteBuffer(size1, 0, infra::MockFunction<void()>());
+    aligner.WriteBuffer(size1, 1, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 99, 99, 0, 0 }), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_large_block_at_offset)
+{
+    aligner.WriteBuffer(size1, 0, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 99, 5, 6, 7 }), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size6, 1, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 8, 9, 10, 0 }), 4, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.Flush(infra::MockFunction<void()>());
+}
+
+TEST_F(FlashAlignerTest, write_double_block_at_offset)
+{
+    aligner.WriteBuffer(size1, 0, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 99, 11, 12, 13 }), 0, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 14, 15, 16, 17 }), 4, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.WriteBuffer(size8, 1, infra::MockFunction<void()>());
+
+    EXPECT_CALL(flash, WriteBuffer(infra::ContentsEqual(std::vector<uint8_t>{ 18, 0, 0, 0 }), 8, testing::_)).WillOnce(testing::InvokeArgument<2>());
+    aligner.Flush(infra::MockFunction<void()>());
+}

Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@`
`14`	`14`	`"generator": "Ninja Multi-Config",`
`15`	`15`	`"cacheVariables": {`
`16`	`16`	`"CMAKE_CONFIGURATION_TYPES": "Debug;RelWithDebInfo",`
	`17`	`+ "POSTMASTER_BUILD_TESTS": true,`
`17`	`18`	`"EMIL_BUILD_TESTS": true`
`18`	`19`	`}`
`19`	`20`	`},`
Original file line number	Diff line number	Diff line change
`@@ -33,3 +33,5 @@ target_link_libraries(postmaster.programmer PUBLIC`
`33`	`33`	`services.st_util`
`34`	`34`	`protobuf.echo`
`35`	`35`	`)`
	`36`	`+`
	`37`	`+add_subdirectory(test)`