Checksumlayer does not report invalid message id error

[TimonTerBraak]

I have a bit of a weid and complex protocol, where I attempt to be robust for the following pattern:
XX XX ID DATA CHECKSUM XX XX

Since the message length is variable due to a lot of optional / conditional fields, the decoding attempts to reason back from the checksum towards the front of the message. At least that is my interpretation. If there are garbage bytes around the message, the decoder is unable to parse the data properly. I use a deque for the incoming datastream, and I have played around with error status and discarding bytes, but it does not seem the right approach.

While debugging I discovered the following when garbage bytes are found in front of the message. The message id layer reports (correctly) an invalid message id. However, the checksum layer only checks for the NotEnoughData and ProtocolError states. See the screenshot below.

In the end a 'ProtocolError' is reported instead, which is also 'sort of fine', but a ProtocolError can be anything. Ideally, I would like to pop bytes from the front of the stream while there is enough data and an invalid message id is found. This would allow me to properly align the datastream with the protocol decoding. Does that make sense?

[arobenko]

I guess you're trying to use comms::processAllWithDispatch() function to process the input. Indeed the code consumes all the bytes to the point where the iterator has progressed to.

auto es = processSingleWithDispatch(iter, len - consumed, std::forward<TFrame>(frame), msg, handler);
consumed += static_cast<decltype(consumed)>(std::distance(begIter, iter));

I've modified the code on develop branch to shave only one byte in case there is "protocol error" reported and try again:

auto es = processSingleWithDispatch(iter, len - consumed, std::forward<TFrame>(frame), msg, handler);
if (es == comms::ErrorStatus::ProtocolError) {
    ++consumed;
    continue;
}

consumed += static_cast<decltype(consumed)>(std::distance(begIter, iter));

Please let me know if it works as expected for you. If it does, the update will find its way to the next release some time in January 2026 alongside the <sync> suffix support.

Alternatively (if you don't want to use non-release versions) you can write the processing loop yourself similar to one in tutorial 7 which simply ignores the first byte in the case of protocol error and tries again.

In case the es value in the processSingleWithDispatch() invocation is something else than comms::ErrorStatus::ProtocolError you don't have much of a choice but to write the handling loop yourself and ignore first byte on that other error status as well.

It's OK to re-write some portions of the comms library that don't work as expected, adapting them to your own needs. The library was explicitly designed as "lego blocks" where one block can be replaced and/or adapted to the actual protocol needs. It is very difficult to generalize and provide a solution that "fits them all".

Hope it helps

[TimonTerBraak]

My protocol is quite variable in the message length. So, the decoder cannot really predict where the end of the message is supposed to be. After the checksum, the messages are terminated with a CR + LF. So, having suffix support probably helps, at least partially. I current search for that pattern before the payload is handed to Comms. Of course, I would like to get rid of this custom tinkering.

I currently use the following prototype to make it work. I know it is not pretty...

 comms::ErrorStatus decode(std::deque<uint8_t>& data)
    {
        Frame frame;        
        typename Frame::MsgPtr msgp;
        comms::ErrorStatus es = comms::ErrorStatus::NotEnoughData;

        do
        {
            std::vector<uint8_t> pattern = {0x0D, 0x0A};  // CR+LF
            auto it = std::search(data.begin(), data.end(), pattern.begin(), pattern.end());

            if (it != data.end())
            {
                auto readIter = data.begin();
                std::size_t avail = std::distance(readIter, it) + pattern.size();

                auto es = frame.read(msgp, readIter, avail);

                if (es == comms::ErrorStatus::Success)
                {
                    assert(msgp); // Message object must be allocated
                    comms::dispatchMsg<Messages>(*msgp, m_handler);

                    data.erase(data.begin(), readIter);
                    break;
                }
                else //if (es != comms::ErrorStatus::InvalidMsgId)
                {
                    data.pop_front();
                }
#if 0                
                else // if (es != comms::ErrorStatus::ProtocolError)
                {
                    // Erase up to after CR+LF to skip the garbage/corrupted "message"
                    data.erase(data.begin(), it + pattern.size());
                }
#endif
            }
            else if (data.size() > 22) // Max frame size
            {
                // Ensure that the buffer does not grow indefinitely when no CR + LF is found
                data.pop_front();
            }
            else
            {
                // No complete frame found, wait for more data
                break;
            }
        } while (!data.empty());

        return es;
    }

[arobenko]

In this particular use case (assuming presence of the fields like <string> or <data> in the message payload unbound by any means) I don't think pre-scanning of the input data to find the termination sequence can be avoided. Even if usage of the <sync> after the <payload> is supported, the library code will have to do such a pre-scanning anyways. So no real performance benefit will be achieved, maybe it will make the client side code a bit simpler.

I would implement it a bit differently though (using recent update to develop branch):

auto it = std::search(data.begin(), data.end(), pattern.begin(), pattern.end());
auto avail = std::distance(data.begin(), it);
if (PacketLimit < msg_len) {
    // Too long
    auto erase_len = avail - PacketLimit;
    data.erase(data.begin(), data.begin() + erase_len);
    avail -= erase_len
}

if (it != data.end()) {
  consumed = comms::processAllWithDispatch(data.begin(), avail, frame, m_handler); // consumes and dispatches only one message
  data.erase(data.begin(), data.begin() + consumed + pattern.size());
}

Looks a bit simpler and more manageable.