Proper ordering of suffix frame layer.

arobenko · arobenko · commit 2e5b8c809d65 · 2025-12-26T15:29:23.000+10:00
diff --git a/app/commsdsl2comms/test/test55/test55Test.th b/app/commsdsl2comms/test/test55/test55Test.th
@@ -17,15 +17,6 @@ public:
     void test1();
     void test2();
 
-    struct Handler
-    {
-        template <typename TMsg>
-        void handle(TMsg& msg)
-        {
-            static_cast<void>(msg);
-        }
-    };
-
     using WrapInterface = test55::wrap::Message<>;
     using WrapFrame = test55::wrap::frame::WrapFrame<WrapInterface>;
 
diff --git a/app/commsdsl2comms/test/test56/Schema.xml b/app/commsdsl2comms/test/test56/Schema.xml
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<schema name="test56" endian="big">
+    <description>
+        Testing sync suffix layer together with checksum
+    </description>
+    <fields>
+        <enum name="MsgId" type="uint8" semanticType="messageId" >
+            <validValue name="M1" val="0" />
+            <validValue name="M2" val="1" />
+        </enum>
+    </fields>
+
+    <message name="Msg1" id="MsgId.M1">
+        <data name="F1" />
+    </message>
+
+    <message name="Msg2" id="MsgId.M2">
+        <string name="F1" />
+    </message>
+
+    <frame name="Frame">
+        <sync name="Prefix">
+            <int name="PrefixField" type="uint16" defaultValue="0x3d3d" />
+        </sync>
+        <id name="Id" field="MsgId" />
+        <payload name="Data" />
+        <sync name="Suffix" from="Data" seekField="true">
+            <int name="SuffixField" type="uint16" defaultValue="0x4040" />
+        </sync>
+        <checksum name="Checksum" from="Data" alg="sum">
+            <int name="ChecksumField" type="uint8" />
+        </checksum>
+    </frame>
+</schema>
diff --git a/app/commsdsl2comms/test/test56/test56Test.th b/app/commsdsl2comms/test/test56/test56Test.th
@@ -0,0 +1,69 @@
+#include "cxxtest/TestSuite.h"
+
+#include "test56/Message.h"
+#include "test56/frame/Frame.h"
+
+#include "comms/process.h"
+
+#include <cstdint>
+#include <type_traits>
+
+class TestSuite : public CxxTest::TestSuite
+{
+public:
+    void test1();
+
+    // struct Handler
+    // {
+    //     template <typename TMsg>
+    //     void handle(TMsg& msg)
+    //     {
+    //         static_cast<void>(msg);
+    //     }
+    // };
+
+    using Interface = test56::Message<
+        comms::option::app::IdInfoInterface,
+        comms::option::app::LengthInfoInterface,
+        comms::option::app::ReadIterator<const std::uint8_t*>,
+        comms::option::app::WriteIterator<std::uint8_t*>,
+        comms::option::app::NameInterface
+    >;
+    using Frame = test56::frame::Frame<Interface>;
+
+    TEST56_ALIASES_FOR_ALL_MESSAGES_DEFAULT_OPTIONS(,,Interface);
+};
+
+void TestSuite::test1()
+{
+    const std::uint8_t Buf[] = {
+        0x3d, 0x3d, // Prefix
+        static_cast<std::uint8_t>(test56::MsgId_M2),
+        'h', 'e', 'l', 'l', 'o',
+        0x40, 0x40, // Suffix
+        0x94 // Checksum
+    };
+    const std::size_t BufSize = std::extent<decltype(Buf)>::value;
+
+    Frame frame;
+    Frame::MsgPtr msgPtr;
+
+    auto iter = comms::readIteratorFor<Interface>(Buf);
+    auto es = frame.read(msgPtr, iter, BufSize);
+    TS_ASSERT_EQUALS(es, comms::ErrorStatus::Success);
+    TS_ASSERT(msgPtr);
+    TS_ASSERT_EQUALS(msgPtr->getId(), test56::MsgId_M2);
+
+    auto* msg2 = static_cast<const Msg2*>(msgPtr.get());
+    TS_ASSERT_EQUALS(msg2->field_f1().value(), "hello");
+
+    std::vector<std::uint8_t> outputBuf;
+    outputBuf.resize(frame.length(*msgPtr));
+
+    auto writeIter = comms::writeIteratorFor<Interface>(outputBuf.data());
+    es = frame.write(*msgPtr, writeIter, outputBuf.size());
+    TS_ASSERT_EQUALS(es, comms::ErrorStatus::Success);
+    TS_ASSERT_EQUALS(outputBuf.size(), BufSize);
+    TS_ASSERT(std::equal(outputBuf.begin(), outputBuf.end(), Buf));
+}
+
diff --git a/lib/include/commsdsl/gen/GenLayer.h b/lib/include/commsdsl/gen/GenLayer.h
@@ -74,6 +74,8 @@ class COMMSDSL_API GenLayer : public GenElem
     virtual bool genWriteImpl() const;
     virtual bool genForceCommsOrderImpl(GenLayersAccessList& layers, bool& success) const;
 
+    bool genAdjustSuffixLayersOrder(GenLayersAccessList& layers, bool& success) const;
+
 private:
     std::unique_ptr<GenLayerImpl> m_impl;
 };
diff --git a/lib/src/gen/GenChecksumLayer.cpp b/lib/src/gen/GenChecksumLayer.cpp
@@ -81,6 +81,7 @@ bool GenChecksumLayer::genForceCommsOrderImpl(GenLayersAccessList& layers, bool&
         return false;
     }
 
+    genGenerator().genLogger().genDebug("Layer \"" + genParseObj().parseName() + "\" is a suffix expected to be relocated");
     auto& fromStr = obj.parseFromLayer();
     if (fromStr.empty()) {
         [[maybe_unused]] static constexpr bool Should_not_happen = false;
@@ -105,19 +106,17 @@ bool GenChecksumLayer::genForceCommsOrderImpl(GenLayersAccessList& layers, bool&
         return false;
     }
 
-    auto iterTmp = iter;
-    std::advance(iterTmp, 1U);
-    if (iterTmp == fromIter) {
-        // Already in place
-        success = true;
-        return false;
+    auto dist = std::distance(iter, fromIter);
+    if (dist < 0) {
+        // Not relocated yet
+        genGenerator().genLogger().genDebug("Relocating \"" + genParseObj().parseName() + "\" to precede \"" + (*fromIter)->genParseObj().parseName() + "\"");
+        auto* thisPtr = *iter;
+        layers.erase(iter);
+        layers.insert(fromIter, thisPtr);
     }
 
-    auto thisPtr = std::move(*iter);
-    layers.erase(iter);
-    layers.insert(fromIter, std::move(thisPtr));
-    success = true;
-    return true;
+    // Already in place
+    return genAdjustSuffixLayersOrder(layers, success);
 }
 
 GenChecksumLayer::ParseChecksumLayer GenChecksumLayer::genChecksumDslObj() const
diff --git a/lib/src/gen/GenLayer.cpp b/lib/src/gen/GenLayer.cpp
@@ -23,8 +23,9 @@
 #include "commsdsl/gen/strings.h"
 #include "commsdsl/gen/util.h"
 
-#include <cassert>
 #include <algorithm>
+#include <cassert>
+#include <iterator>
 
 namespace commsdsl
 {
@@ -342,6 +343,79 @@ bool GenLayer::genForceCommsOrderImpl(GenLayersAccessList& layers, bool& success
     return false;
 }
 
+bool GenLayer::genAdjustSuffixLayersOrder(GenLayersAccessList& layers, bool& success) const
+{
+    auto* frame = genParentFrame();
+    assert(frame != nullptr);
+    auto& origLayers = frame->genLayers();
+    auto thisOrigIter =
+        std::find_if(
+            origLayers.begin(), origLayers.end(),
+            [this](auto& lPtr)
+            {
+                return lPtr.get() == this;
+            });
+
+    assert(thisOrigIter != origLayers.end());
+
+    auto origPayloadIter =
+        std::find_if(
+            origLayers.begin(), origLayers.end(),
+            [](const auto& lPtr)
+            {
+                return lPtr->genParseObj().parseKind() == commsdsl::parse::ParseLayer::ParseKind::Payload;
+            });
+
+    auto thisIter =
+        std::find_if(
+            layers.begin(), layers.end(),
+            [this](const auto* l)
+            {
+                return l == this;
+            });
+    assert(thisIter != layers.end());
+
+    auto payloadIter =
+        std::find_if(
+            layers.begin(), layers.end(),
+            [](const auto* l)
+            {
+                return l->genParseObj().parseKind() == commsdsl::parse::ParseLayer::ParseKind::Payload;
+            });
+
+    for (auto iter = origPayloadIter + 1U; iter != thisOrigIter; ++iter) {
+        auto* otherLayer = iter->get();
+        auto otherIter =
+            std::find_if(
+                layers.begin(), layers.end(),
+                [otherLayer](const auto* l)
+                {
+                    return l == otherLayer;
+                });
+        assert(otherIter != layers.end());
+
+        auto payloadDist = std::distance(payloadIter, otherIter);
+        if (0 < payloadDist) {
+            // The other layer is not relocated yet
+            continue;
+        }
+
+        auto dist = std::distance(thisIter, otherIter);
+        if (dist < 0) {
+            // The wrapping order is incorrect
+            auto* thisPtr = *thisIter;
+            layers.erase(thisIter);
+            layers.insert(otherIter, thisPtr);
+            success = true;
+            return true;
+        }
+    }
+
+    // All suffix layers are in correct order
+    success = true;
+    return false;
+}
+
 } // namespace gen
 
 } // namespace commsdsl
diff --git a/lib/src/gen/GenSyncLayer.cpp b/lib/src/gen/GenSyncLayer.cpp
@@ -180,19 +180,17 @@ bool GenSyncLayer::genForceCommsOrderImpl(GenLayersAccessList& layers, bool& suc
         return false;
     }
 
-    auto iterTmp = iter;
-    std::advance(iterTmp, 1U);
-    if (iterTmp == nextIter) {
-        // Already in place
-        success = true;
-        return false;
+    auto dist = std::distance(iter, nextIter);
+    if (dist < 0) {
+        // Not relocated yet
+        genGenerator().genLogger().genDebug("Relocating \"" + genParseObj().parseName() + "\" to precede \"" + (*nextIter)->genParseObj().parseName() + "\"");
+        auto* thisPtr = *iter;
+        layers.erase(iter);
+        layers.insert(nextIter, thisPtr);
     }
 
-    auto thisPtr = std::move(*iter);
-    layers.erase(iter);
-    layers.insert(nextIter, std::move(thisPtr));
-    success = true;
-    return true;
+    // Already in place
+    return genAdjustSuffixLayersOrder(layers, success);
 }
 
 GenSyncLayer::ParseSyncLayer GenSyncLayer::genSyncLayerDslObj() const
diff --git a/lib/src/parse/ParseChecksumLayerImpl.cpp b/lib/src/parse/ParseChecksumLayerImpl.cpp
@@ -87,6 +87,11 @@ bool ParseChecksumLayerImpl::parseVerifyImpl(const ParseLayerImpl::ParseLayersLi
                 "Layer \"" << parseFrom() << "\" must appear before the \"" << parseName() << "\".";
             return false;
         }
+
+        auto payloadIdx = parseFindLayerIndex(layers, ParseKind::Payload);
+        if (!parseVerifySuffixLayersOrder(layers, payloadIdx, thisIdx, fromIdx)) {
+            return false;
+        }
     }
 
     if (!parseUntil().empty()) {
diff --git a/lib/src/parse/ParseLayerImpl.cpp b/lib/src/parse/ParseLayerImpl.cpp
@@ -31,6 +31,7 @@
 #include <cassert>
 #include <iterator>
 #include <limits>
+#include <map>
 #include <set>
 
 namespace commsdsl
@@ -39,6 +40,23 @@ namespace commsdsl
 namespace parse
 {
 
+namespace
+{
+
+const std::string& parseRetrieveSyncFrom(const ParseLayerImpl& layer)
+{
+    assert(layer.parseKind() == ParseLayerImpl::ParseKind::Sync);
+    return static_cast<const ParseSyncLayerImpl&>(layer).parseFrom();
+}
+
+const std::string& parseRetrieveChecksumFrom(const ParseLayerImpl& layer)
+{
+    assert(layer.parseKind() == ParseLayerImpl::ParseKind::Checksum);
+    return static_cast<const ParseChecksumLayerImpl&>(layer).parseFrom();
+}
+
+} // namespace
+
 ParseLayerImpl::ParsePtr ParseLayerImpl::parseCreate(
     const std::string& kind,
     ::xmlNodePtr node,
@@ -247,6 +265,45 @@ bool ParseLayerImpl::parseVerifyBeforePayload(const ParseLayerImpl::ParseLayersL
     return true;
 }
 
+bool ParseLayerImpl::parseVerifySuffixLayersOrder(const ParseLayersList& layers, std::size_t payloadIdx, std::size_t layerIdx, std::size_t fromIdx)
+{
+    for (auto midIdx = payloadIdx + 1U; midIdx < layerIdx; ++midIdx) {
+        auto& midLayerPtr = layers[midIdx];
+        assert(midLayerPtr);
+
+        using FromRetrieveFunc = const std::string& (*)(const ParseLayerImpl& layer);
+        static const std::map<ParseKind, FromRetrieveFunc> Map = {
+            {ParseKind::Sync, &parseRetrieveSyncFrom},
+            {ParseKind::Checksum, &parseRetrieveChecksumFrom},
+        };
+
+        auto iter = Map.find(midLayerPtr->parseKind());
+        if (iter == Map.end()) {
+            continue;
+        }
+
+        auto func = iter->second;
+        auto& midFrom = func(*midLayerPtr);
+
+        if (midFrom.empty()) {
+            continue;
+        }
+
+        auto midFromIdx = parseFindLayerIndex(layers, midFrom);
+        if (midFromIdx < fromIdx) {
+            auto layerIter = Map.find(layers[layerIdx]->parseKind());
+            assert(layerIter != Map.end());
+            auto layerFunc = layerIter->second;
+
+            parseLogError() << ParseXmlWrap::parseLogPrefix(parseGetNode()) <<
+                "Layer \"" << layerFunc(*layers[layerIdx]) << "\" specified by the \"" << common::parseFromStr() << "\" follows \"" << midFrom << "\" specified by the preceding layer.";
+            return false;
+        }
+    }
+
+    return true;
+}
+
 std::size_t ParseLayerImpl::parseFindThisLayerIndex(const ParseLayerImpl::ParseLayersList& layers) const
 {
     auto iter =
diff --git a/lib/src/parse/ParseLayerImpl.h b/lib/src/parse/ParseLayerImpl.h
@@ -146,6 +146,7 @@ class ParseLayerImpl : public ParseObject
     void parseReportUnexpectedPropertyValue(const std::string& propName, const std::string& propValue);
     bool parseVerifySingleLayer(const ParseLayersList& layers, const std::string& kindStr);
     bool parseVerifyBeforePayload(const ParseLayersList& layers);
+    bool parseVerifySuffixLayersOrder(const ParseLayersList& layers, std::size_t payloadIdx, std::size_t layerIdx, std::size_t fromIdx);
     std::size_t parseFindThisLayerIndex(const ParseLayersList& layers) const;
     std::size_t parseFindLayerIndex(const ParseLayersList& layers, ParseKind lKind);
     std::size_t parseFindLayerIndex(const ParseLayersList& layers, const std::string& name);
diff --git a/lib/src/parse/ParseSyncLayerImpl.cpp b/lib/src/parse/ParseSyncLayerImpl.cpp
diff --git a/lib/test/frame/Schema36.xml b/lib/test/frame/Schema36.xml
diff --git a/lib/test/frame/Schema37.xml b/lib/test/frame/Schema37.xml
diff --git a/lib/test/frame/frameTest.th b/lib/test/frame/frameTest.th

Original file line number	Diff line number	Diff line change
`@@ -87,6 +87,11 @@ bool ParseChecksumLayerImpl::parseVerifyImpl(const ParseLayerImpl::ParseLayersLi`
`87`	`87`	`"Layer \"" << parseFrom() << "\" must appear before the \"" << parseName() << "\".";`
`88`	`88`	`return false;`
`89`	`89`	`}`
	`90`	`+`
	`91`	`+ auto payloadIdx = parseFindLayerIndex(layers, ParseKind::Payload);`
	`92`	`+ if (!parseVerifySuffixLayersOrder(layers, payloadIdx, thisIdx, fromIdx)) {`
	`93`	`+ return false;`
	`94`	`+ }`
`90`	`95`	`}`
`91`	`96`
`92`	`97`	`if (!parseUntil().empty()) {`