6
\$\begingroup\$

I want a good design to implement a master-slave protocol. The protocol is proprietary and similar to cctalk. We have:

Master: HEADER [ DATA ] CHECKSUM
Slave: HEADER [ DATA ] CHECKSUM

My choice was to have concrete base classes for request and for response:

class PacketReq {
public:
    typedef std::vector<uint8_t>       container;
    typedef container::const_iterator  const_iterator;

    PacketCommand command() const
    {
        return static_cast< PacketCommand >( mBuffer[ 2 ] );
    };

    const container& data() const
    {
        return mBuffer;
    }

    const_iterator begin() const
    {
        return mBuffer.begin();
    }

    const_iterator end() const
    {
        return mBuffer.end();
    }

protected:
    container mBuffer;
};

class PacketOut1 : public PacketReq {
public:
    PacketOut1()
    {
        mBuffer.reserve( PacketHeaderHelper::HEADER_SIZE );
        mBuffer.push_back( /* something. */ );
        mBuffer.push_back( /* something. */ );
        mBuffer.push_back( /* something. */ );
        mBuffer.push_back( /* something. */ );
    }
};

The response can be a good response or a NACK (an error). My Implentation is:

class PacketRes {
public:
    PacketRes(const std::vector<uint8_t>& buffer) 
    {
        if (!checkChecksum( buffer ))
        {
            throw Error("invalid checksum");
        }

        PacketHeaderHelper header( buffer );
        mCommand = header.command();

        if (header.len() > PacketHeaderHelper::HEADER_SIZE)
        {
            const size_t bodySize = header.len() - PacketHeaderHelper::HEADER_SIZE;

            mData.reserve( bodySize );

            mData.insert(
                mData.end(),
                buffer.begin() + PacketHeaderHelper::HEADER_SIZE - 1,
                buffer.end() - 1
                );
        }        
    }

    virtual ~PacketReq() {}

    PacketCommand command() const
    {
        return mCommand;
    }    

    const std::vector<uint8_t>& body() const
    {
        return mData;
    }

private:
    bool checkChecksum(const std::vector<uint8_t>& buffer) const
    {
        /* calc checksum. */
    }

private:
    std::vector<uint8_t> mData;
    PacketCommand mCommand;
};

class PacketInAck : public PacketReq {
public:
    PacketInAck(const std::vector<uint8_t>& buffer) : PacketReq( buffer )
    {
    }
};

So if I want make a request the sequence is:

lock_guard 
send( const PacketOut& p ) 
std::unique_ptr< PacketRes > read();

But I don't like this design, but it works.

Is there some other way to implement this?

\$\endgroup\$

2 Answers 2

4
+50
\$\begingroup\$

Preliminaries

I would decouple the data transmission + message encoding/decoding from the data connection problem. The linked Uncle Bob article also shows how to distinguish connection management differences for master and slave. You probably want this to work with any reasonable networking implementation. Since you already use boost::noncopyable, I take that as a hint that you are willing to use Boost in general.

My recommendation would be for you to study, and then modify, the simple chat_message/chat_client/chat_server example from the Boost.Asio documentation. That library -or a modified version of it- is likely to become the official networking component in a future version of the Standard Library. So it's a good investment to learn about it.

Message Interface

Compared to the simple chat_message example, your question is a little more involved. I assume that the header information is used to identify which type of message is going to follow. I would recommend to factor all the common message stuff HEADER|BODY|CHECKSUM into an abstract base class IMessage

class IMessage
{
pubic:
    virtual ~IMessage() {} // virtual destructor

    // virtual functions that can be overriden for concrete message types        

    static std::string header(std::string const& input)
    {
            return input.substr(0, HeaderLength);
    }

    static std::string body(std::string const& input)
    {
            return input.substr(HeaderLength, HeaderLength + BodyLength);
    }

    enum { 
        HeaderLength = /* bla */,
        BodyLength = /* bla */,
        ChecksumLength = /* bla */
    };
};

Message Implementations

You could then define a bunch of different concrete messages that inherit the general message interface IMessage, and add some stuff of their own:

class Request: public IMessage 
{
    Request(std::string const& body) { /* decode the buffer */ }

    static std::unique_ptr<IMessage> create(std::string body const&) 
    { 
        return std::make_unique<Request>(body); 
    }

    // Request specific overrides of virtual functions
    // Request unique functions
};

class Acknowledge: public IMessage
{
    Acknowledge(std::string const& body) { /* decode the buffer */ }

    static std::unique_ptr<IMessage> create(std::string const& body) 
    { 
         return std::make_unique<Acknowledge>(body); 
    }

    // Acknowledge specific overrides of virtual functions
    // Acknowledge unique functions
};

You might find it puzzling that I have written both a constructor and a create() function. That is explained in the next section. But for now, note that the declared return type is std::unqiue_ptr<IMessage> but the function body return a unique pointer to the concrete message types. This legal C++ feature is known as covariant return types, and makes it possible to have polymorphic object construction.

Note that std::make_unique is not yet officially availabe but you can get a working version that has been accepted for the upcoming C++14 Standard.

Reading messages

If you look at the Boost.Asio example, the chat_client and chat_server read from a character buffer that is being filled from a socket. So you need to decode the header in order to distinguish which message has to be created.

Especially since you tagged this question with design-patterns, I would recommend to write a small Factory class that keeps a registry of function pointers. At startup, you fill the factory's registry with function pointers to the static member functions create() of all the message types in your application. In this case the registry would be e.g. of type

typedef std::map<std::string, std::function<std::unique_ptr<IMessage>(std::string const&)> Registry;

What this says is that every unique header (encoded in a std::string) stores a pointer to a function taking a std::string and returning a std::unique_ptr<IMessage>. Those type of function pointers are preciesly the create() functions of your messages.

Given a buffer that has been read from the socket, and converted to a std::string, your factory would do something like:

    std::unique_ptr<IMessage> Factory::create(std::string const& input) const
    {
            auto const fun = registry_.find(IMessage::header(input));
            // here you can also do your CHECKSUM validation
            return fun? (fun)(IMessage::body(input)) : nullptr;
    }

What this does is to split the message into a header and body. These functions are static functions of the message interface, so that they can be called before we have a concrete message object. Then the header is looked up in the registry.

If it is found, we simply call the corresponding create() message that was stored, taking the body as argument. This will call the constructor (now you see why we had both a constructor and a creator, because constructors cannot be stored in a factory registry). The constructor will finally create a std::unique_ptr to the concrete message type that was encoded in the header.

If we didn't find a registered message type, we return a nullptr. The caller of the factory creator would then have to check this before being able to safely use the returned pointer.

Final thoughts

The above explained how to decode messages from a stream of tokens that you get from a socket. Writing is of course much easier because then you already know the message type. I think that the data transmission and connection management part, and the semantic part of how to respond to each message type is beyond the scope of this Q&A. The Boost.Asio example does not need too much modification to allow reading your message structures from a socket.

For the above message parsing machinery, I have implemented a heavily templated version of similar protocol called DamExchange (used for letting checkers/draughts engines over a network). You can follow the header trail on a simple use case in my BitBucket repo.

\$\endgroup\$
3
  • \$\begingroup\$ I've thought along to the solution, but I prefer my solution. I know that I've to study better so I'll try your implementation too. \$\endgroup\$ Jun 15, 2013 at 1:12
  • \$\begingroup\$ @elvis.dukaj Which parts in your answer do you prefer? \$\endgroup\$ Jun 18, 2013 at 18:24
  • \$\begingroup\$ well, not always request and response has something common, I've a low coupling and high cohesion. No need to derive request packet so I've no need to cast from request to derived. No need for a factory class, no need for a register (sometimes we need some more data to know response ) and because it's very easy to implement and to use. And also is very easy to extend \$\endgroup\$ Jun 18, 2013 at 21:58
3
\$\begingroup\$

A possible altenrative implementation can be the following:

All the packet have no inheritance, nor recieved packet nor sent packet

PacketOutA, PacketOutB, PacketOutC are all classes with a common interface, they have just two method: begin() and end() to access internal data;

The recieved class are PacketInA, PacketInB and so on.

The class Communication is used to allow me to send and recieve data to the communication port.

The ReaderAndValidator class allow me to get and validate the body of the recieved packet.

The Dispatcher class allow me to obtain a packet from a recieved one ( sorry for my english... look the example... )

The Device class instead is needed to rappresent, in a domain context, our connected device.

/** abstract the communication mechanism. */
class Communication {
public:

    Communication ();

    template < typename Iterator >
    void send( Iterator first, Iterator last ) const
    {
        // send from first to last to 
    }

    template< typename Iterator >
    std::vector< char > read( Iterator first, Iterator last ) const
    {
        // read from first to last to 
    }

private:
    // internal stuff
};

/** Validate an input packet and get the body */
class ReaderAndValidator {
public:

    ReaderAndValidator ( const Communication& comm )
        : mComm( comm )
    {
    }

    std::vector< char > read()
    {
        // read data
        return mComm.read( /* params */ );
    }

private:
    const Communication& mComm;
}

/** Given the PacketOut obtain the corrisponding PacketIn. */
template < typename PacketOut >
class Dispatcher {
    Dispatcher()
    {
        static_assert( false, "unable to instantiate Controller" );
    }
};

template <>
class Dispatcher< PacketOutA > {
public:
    Dispatcher( const Communication& comm )
        : mComm( comm )
    {
    }

    PacketInA send( const PacketOutA & packet ) const
    {
        mComm.send( std::begin( packet ), std::end( packet ) );

        ReaderAndValidator reader( mComm );
        auto body = reader.read();

        // RVO
        PacketInA result( std::begin( body ), std::end( body ) );
        return result;
    }

private:
    const Communication& mComm;
};

// same thing for Controller< PacketOutB >, Controller< PacketOutC > and so on


class Device : private boost::noncopyable {
public:

    Device ()
    {
        // init communication
    }

    void doPacketOutA( int param )
    {
        PacketOutA packet( param );
        recieve( packet );
    }

    PacketInB doPacketB()
    {
        PacketOutB packet();
        return recieve( packet );
    }

private:

    template < typename PacketOutKind >
    auto recieve( const PacketOutKind& p ) -> decltype( std::declval< Dispatcher< PacketOutKind > >().send( p ) ) )
    {
        Dispatcher< PacketOutKind > disp( mCommunication );
        return controller.send( p );
    }

private:

    Communication mCommunication;
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.