4
\$\begingroup\$

I have a message coming in and I need to match it against the expected messages. The program will eventually do something as a result of receiving those messages. I am not very experienced at programming, but surely there should be a better way to declare all those messages it can be in like a separate entity and then be able to use it within this HexSearch.cpp file?

I tried to search how to do that but I couldn't find the right words to ask about this using a search engine. There is many more messages than those show here which still need to be declared but this is just a sample, which I don't like to look at already.

#include "HexSearch.h"

void searchFunction(int num, char msg[]) {

    static const char readReq[] = { 0x92 };                            
    static const char readResp[] = { 0x00, 0x02, 0x12, 0x34, 0xA1 };

    static const char writeReq[] = { 0x0A, 0xE0 };                     
    static const char writeResp[] = { 0x00, 0x02, 0x11, 0x01, 0x98 };

    static const char resetReq[] = { 0x00, 0xFF };                       
    static const char resetResp[] = { 0x00, 0x21, 0x23, 0x0E, 0xAE, 0x11, 0x3A };

    static const char verReq[] = {0x00, 0xA2};
    static const char verResp[] = {0x00, 0x03, 0x82, 0xAA, 0x07, 0x88, 0xA9};

    static const char typeReq[] = {0x00, 0x67};
    static const char typeResp[] = {0x00, 0x03, 0x00, 0x00, 0xC4, 0x77};

    static const char askReq[] = {0x00, 0x55};
    static const char askResp[] = {0x00, 0x01, 0xFE, 0xFF};

    if (num == 4) {
        replyMsg(msg, 2, 3,  readReq, readResp, sizeof(readResp) / sizeof(readResp[0]));
    }
    else if (num == 5) {
        replyMsg(msg, 2, 4, writeReq, writeResp, sizeof(writeResp) / sizeof(writeResp[0]));
        replyMsg(msg, 2, 4, resetReq, resetResp, sizeof(resetResp) / sizeof(resetResp[0]));
        replyMsg(msg, 2, 4, verReq, verResp, sizeof(verResp) / sizeof(verResp[0]));
        replyMsg(msg, 2, 4, typeReq, typeResp, sizeof(typeResp) / sizeof(typeResp[0]));
        replyMsg(msg, 2, 4, askReq, askResp, sizeof(askResp) / sizeof(askResp[0]));
    }
}

void replyMsg(char msg[], int startArr, int endArr, const char* receiv, const char* resps, int respL) {
    if (std::equal(msg + startArr, msg + endArr, receiv)) {
        for (int x = 0; x < respL; x++) {
            serialPC.putc(resps[x]);
        }
    }
}

The code works. I am interested in improving it only. num is the total number of bytes of a message. E.g. readReq has one byte of data, but has also got 2 start bytes and 1 end byte so a total of 4. readResp array has the 2 start bytes, 2 data bytes, and one end byte and so it has a total size of 5 bytes. The 2nd byte is the one which specifies the length of a message. msg[] is the message coming in from a serial connection essentially.

As an example, if msg[] = { 0x00, 0x01, 0x92, 0x56 } then num = 4 and replyMsg will compare the 3rd byte to see that it matches readReq and so output readResp...

\$\endgroup\$
  • \$\begingroup\$ Does your code work? It doesn't seem very clear in your post. Not working code is not ready to be reviewed :) \$\endgroup\$ – IEatBagels Feb 15 at 15:26
  • 1
    \$\begingroup\$ For a meaningful advice we need to know quite a few details of the protocol. Where num is coming from? could byte 2 be used to tell one-byte requests from 2-byte requests? etc. \$\endgroup\$ – vnp Feb 15 at 17:01
  • \$\begingroup\$ @vnp That's correct. \$\endgroup\$ – Tryb Ghost Feb 18 at 8:18
  • 1
    \$\begingroup\$ @vnp is there anything else I can add/edit to take the question off hold status? \$\endgroup\$ – Tryb Ghost Feb 18 at 11:22
  • \$\begingroup\$ @Snowhawk Hi, is there anything more I can do to take the question off its hold status? \$\endgroup\$ – Tryb Ghost Feb 18 at 11:23
4
\$\begingroup\$

Unfortunately your code is incomplete and can't be run in this state. Still I want to recommend some improvements to this snippets.

My first impression from this code is that it looks like plain C, not C++ (besides the use of std::equal).

To pass your Message and compare it you are using plain char arrays. With this approach you have to also pass the size. You shouldn't feel the need for this in C++ any more. Check out containers such as std::array (fixed size) std::vector, std::string or std::string_view.

There's a bug which can make your program truncate the compared values depending on the platform:

static const char readReq[] = { 0x92 };

Consider that 0x92 = 146(int).

Not all platforms have char == unsigned char. If you are not lucky it can mean char == signed char. See https://stackoverflow.com/questions/2054939/is-char-signed-or-unsigned-by-default

I found this by accident when I switched your plain C Array:

static const char readReq[] = { 0x92 }; 

To C++ std::vector:

const std::vector<char> readRequest = { 0x92 };

This doesn't event compile. The compiler complains that 0x92 truncates. So I changed it to:

const std::vector<unsigned char> readRequest = { 0x92 };

Then I thought probably it is event better to really declare your values as an array of constants. So I changed to std::array like this:

constexpr std::array<unsigned char, 1> readRequest = { 0x92 };
constexpr std::array<unsigned char, 5> readResponse = { 0x00, 0x02, 0x12, 0x34, 0xA1 

Now it is similar to the old macro defines in C but better because it follows the rules of the language.:

#define readRequest 0x92 

With std::array I refactored your code like this:

#include <algorithm>
#include <array>
#include <iostream>
#include <vector>

template<typename MessageBegin, typename MessageEnd, typename ReceiveType, 
    typename ResponseType>
void replyMessage(
    const MessageBegin& messageBegin,
    const MessageEnd& messageEnd,
    const ReceiveType& receive,
    const ResponseType& response)
{
    if (std::equal(messageBegin, messageEnd, receive.begin())) {
        for (const auto& sign : response) {
            std::cout << sign;
            //serialPC.putc(sign);
        }
    }
}

void searchFunction(int number, const std::vector<unsigned char>& message)
{
    constexpr std::array<unsigned char, 1> readRequest = { 0x92 };
    constexpr std::array<unsigned char, 5> readResponse = { 0x00, 0x02, 0x12, 0x34, 0xA1 };

    constexpr std::array<unsigned char, 2> writeRequest = { 0x0A, 0xE0 };
    constexpr std::array<unsigned char, 5> writeResponse = { 0x00, 0x02, 0x11, 0x01, 0x98 };

    constexpr std::array<unsigned char, 2> resetRequest = { 0x00, 0xFF };
    constexpr std::array<unsigned char, 7> resetResponse = { 0x00, 0x21, 0x23, 0x0E, 0xAE, 0x11, 0x3A };

    constexpr std::array<unsigned char, 2> verReqeust = { 0x00, 0xA2 };
    constexpr std::array<unsigned char, 7> verResponse = { 0x00, 0x03, 0x82, 0xAA, 0x07, 0x88, 0xA9 };

    constexpr std::array<unsigned char, 2> typeRequest = { 0x00, 0x67 };
    constexpr std::array<unsigned char, 6> typeResponse = { 0x00, 0x03, 0x00, 0x00, 0xC4, 0x77 };

    constexpr std::array<unsigned char, 2> askRequest = { 0x00, 0x55 };
    constexpr std::array<unsigned char, 4> askResponse = { 0x00, 0x01, 0xFE, 0xFF };

    if (number == 4) {
        replyMessage(message.begin() + 2, message.begin() + 3, readRequest, readResponse);
    }
    else if (number == 5) {
        replyMessage(message.begin() + 2, message.begin() + 4, writeRequest, writeResponse);
        replyMessage(message.begin() + 2, message.begin() + 4, resetRequest, resetResponse);
        replyMessage(message.begin() + 2, message.begin() + 4, verReqeust, verResponse);
        replyMessage(message.begin() + 2, message.begin() + 4, typeRequest, typeResponse);
        replyMessage(message.begin() + 2, message.begin() + 4, askRequest, askResponse);
    }
}

In this code, other stuff was improved:

  • Cryptic names like msg, resps, respl or replyMsg were renamed. Sure, you can find out what they mean, but it is more stressful to read the code and get the meaning. Make a experiment. Forget the code for 6 months and come back to it. How long does it take you to get its meaning?

  • Functions can now accept Containers, which know their size. Also, we can simply iterate over them.

Compare this:

void replyMsg(char msg[], int startArr, int endArr, const char* receiv, const char* resps, int respL) {
    if (std::equal(msg + startArr, msg + endArr, receiv)) {
        for (int x = 0; x < respL; x++) {
            serialPC.putc(resps[x]);
        }
    }
}

with this:

template<typename MessageBegin, typename MessageEnd, typename ReceiveType, 
    typename ResponseType>
void replyMessage(
    const MessageBegin& messageBegin,
    const MessageEnd& messageEnd,
    const ReceiveType& receive,
    const ResponseType& response)
{
    if (std::equal(messageBegin, messageEnd, receive.begin())) {
        for (const auto& sign : response) {
            std::cout << sign;
            //serialPC.putc(sign);  // btw what is this???
        }
    }
}

Besides the template, which we had to use because of different array sizes, which is easier to understand?

\$\endgroup\$
3
\$\begingroup\$

Use std::span (there are free backports if needed), and see most of the boilerplate evaporate:

void replyMsg(std::span<char> msg, std::span<const char> receiv, std::span<const char> resps) {
    if (msg.size() == 3 + receiv.size() && msg.subspan(2, receiv.size()) == receiv)
        for (auto x : resps)
            serialPC.putc(x);
}

void searchFunction(std::span<char> msg) {

    static const char readReq[] = { 0x92 };
    static const char readResp[] = { 0x00, 0x02, 0x12, 0x34, 0xA1 };

    static const char writeReq[] = { 0x0A, 0xE0 };
    static const char writeResp[] = { 0x00, 0x02, 0x11, 0x01, 0x98 };

    static const char resetReq[] = { 0x00, 0xFF };
    static const char resetResp[] = { 0x00, 0x21, 0x23, 0x0E, 0xAE, 0x11, 0x3A };

    static const char verReq[] = {0x00, 0xA2};
    static const char verResp[] = {0x00, 0x03, 0x82, 0xAA, 0x07, 0x88, 0xA9};

    static const char typeReq[] = {0x00, 0x67};
    static const char typeResp[] = {0x00, 0x03, 0x00, 0x00, 0xC4, 0x77};

    static const char askReq[] = {0x00, 0x55};
    static const char askResp[] = {0x00, 0x01, 0xFE, 0xFF};

    replyMsg(msg, readReq, readResp);
    replyMsg(msg, writeReq, writeResp);
    replyMsg(msg, resetReq, resetResp);
    replyMsg(msg, verReq, verResp);
    replyMsg(msg, typeReq, typeResp);
    replyMsg(msg, askReq, askResp);
}

A miniscule amount of templating (alternatively use a function-template for bytes), and it's easily condensed even more:

template <char... x>
static constexpr char bytes[] = { x...};

void searchFunction(std::span<char> msg) {
    /* read  */ replyMsg(msg, bytes<0x92 /* */>, bytes<0x00, 0x02, 0x12, 0x34, 0xA1>);
    /* write */ replyMsg(msg, bytes<0x0A, 0xE0>, bytes<0x00, 0x02, 0x11, 0x01, 0x98>);
    /* reset */ replyMsg(msg, bytes<0x00, 0xFF>, bytes<0x00, 0x21, 0x23, 0x0E, 0xAE, 0x11, 0x3A>);
    /* ver   */ replyMsg(msg, bytes<0x00, 0xA2>, bytes<0x00, 0x03, 0x82, 0xAA, 0x07, 0x88, 0xA9>);
    /* type  */ replyMsg(msg, bytes<0x00, 0x67>, bytes<0x00, 0x03, 0x00, 0x00, 0xC4, 0x77>);
    /* ask   */ replyMsg(msg, bytes<0x00, 0x55>, bytes<0x00, 0x01, 0xFE, 0xFF>);
}

Far less code, and it's far simpler too.

\$\endgroup\$
  • \$\begingroup\$ Theres the gsl header as a support for the c++ core guidelines which contains std::span. Otherwise it will be in C++ 20. \$\endgroup\$ – Sandro4912 May 28 at 11:50
1
\$\begingroup\$

Here are some ideas to help you improve your code.

Use const where practical

The passed msg parameter is not and probably shouldn't be modified by the code, and so it should be declared as const.

Use objects

It seems that fundamentally, what's required here is a list of rules and a way to match them. I'd be inclined to keep things neat and use objects. I'd suggest wrapping things up into a collection of Rule objects.

Prefer constexpr where practical

Especially with embedded systems, constexpr can really allow much more compact code when it's used. This is also often useful for desktop applications, but more typically it's for speed rather than space. All of the fixed data structures could be constexpr.

Consider changing the interface

Instead of the old C-style length and pointer, it is often useful to declare and use a class for this. For example, one might have a Message class that would return a const char * that points to the data.

Think about future expansion

If you only want to be able to send back static messages in response to each requests, the data-oriented approach you have may be fine. However, it may be better to have both a response and some appropriate action instead.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Putting it all together

Using all of these suggestions, here's one way to do it:

#include <algorithm>
#include <iostream>
#include <iomanip>
#include <string_view>

struct Message : public std::string_view {
    // I'm assuming that only valid Messages are 
    // created.  A static_assert could be used here.
    const char *dataptr() const { return begin() + 2; }
};

void doProcess(std::string_view sv, std::string_view name) {
    std::cout << "Processing " << name << " Request: ";
    for (std::size_t i{0}; i < sv.size(); ++i) {
        //serialPC.putc(response[i]);
        std::cout << std::setfill('0') << std::setw(2) << std::hex << (static_cast<unsigned>(sv[i]) & 0xff) << ' ';
    }
    std::cout << '\n';
}

void processRead(std::string_view sv) {
    doProcess(sv, "Read");
}

void processWrite(std::string_view sv) {
    doProcess(sv, "Write");
}

void processReset(std::string_view sv) {
    doProcess(sv, "Reset");
}

void processVersion(std::string_view sv) {
    doProcess(sv, "Version");
}

void processType(std::string_view sv) {
    doProcess(sv, "Type");
}

void processAsk(std::string_view sv) {
    doProcess(sv, "Ask");
}

void searchFunction(const Message &msg) {
    using namespace std::literals;
    static constexpr struct Rule {
        std::string_view match;
        std::string_view response;
        void (*action)(std::string_view);
        bool isMatch(const Message &msg) const {
            return msg.size() == (match.size() + 3) && std::equal(std::begin(match), std::end(match), msg.dataptr());
        }
    } rules[]{
        { "\x92"sv, "\x00\x02\x12\x34\xA1"sv, processRead },
        { "\x0A\xE0"sv, "\x00\x02\x11\x01\x98"sv, processWrite },
        { "\x00\xFF"sv, "\x00\x21\x23\x0E\xAE\x11\x3A"sv, processReset },
        { "\x00\xA2"sv, "\x00\x03\x82\xAA\x07\x88\xA9"sv, processVersion },
        { "\x00\x67"sv, "\x00\x03\x00\x00\xC4\x77"sv, processType },
        { "\x00\x55"sv, "\x00\x01\xFE\xFF"sv, processAsk },
    };
    for (const auto &rule : rules) {
        if (rule.isMatch(msg)) {
            rule.action(rule.response);
            return;
        }
    }
}

int main() {
    using namespace std::literals;
    Message msg{"\x12\x34\x00\x55\x88"sv};
    searchFunction(msg);
}

This assumes a C++17 compiler to be able to use std::string_view objects extensively.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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