4
\$\begingroup\$

I made a 26-bit binary Wiegand calculator for an ESP32 following this format: 26-Bit Wiegand Format

This is "Arduino C" :

unsigned int * getWiegand(unsigned int dec) {
  unsigned int* wiegandNum = new unsigned int[26];

  Serial.println();
  // transform dec number into binary number and store it in binaryNum[]
  int n = 1;
  for(int i = 23; i>=0; i--) {
    int k = dec >> i;
    if (k & 1)
      wiegandNum[n++] = 1;
    else 
      wiegandNum[n++] = 0;
  }
  // check for parity of the first 12 bits
  bool even = true;
  for(int i=1;i<13;i++) {
    if(wiegandNum[i] == 1) {
      even = !even;
    }
  }
  // add 0 or 1 as first bit (leading parity bit - even) based on the number of ones in the first 12 bits
  if(even) {
    wiegandNum[0] = 0;
  } else {
    wiegandNum[0] = 1;
  }

  // check for parity of the last 12 bits
  bool odd = false;
  for(int i=13; i<25;i++) {
    if(wiegandNum[i] == 1) {
      odd = !odd;
    }
  }
  // add 0 or 1 as last bit (trailing parity bit - odd) based on the number of ones in the last 12 bits
  if(odd) {
    wiegandNum[25] = 0;
  } else {
    wiegandNum[25] = 1;
  }

  return wiegandNum;
}

void setup() {
  Serial.begin(115200);

  unsigned int* wiegandArray = getWiegand(12679548);
  for(int i=0;i<26;i++) {
    Serial.print(wiegandArray[i]);
  }
}

12679548 will return 01100000101111001011111000 which, based on this online tool is correct.

Execution time for this function is 255 microseconds. I don't know if the time is good or not, but I believe that the code can be improved.

What do you think ?

References:

\$\endgroup\$
  • 1
    \$\begingroup\$ The structure or class Serial is used within the code but is not defined within the question. This code can't be reviewed because it is incomplete. We need to see the declaration and instantiation of the object Serial. \$\endgroup\$ – pacmaninbw Dec 9 '19 at 13:11
  • 1
    \$\begingroup\$ @pacmaninbw Serial.println() is just like cout<<endl;. It's from the ESP32 Core libraries. I have never defined it inside a program. arduino.cc/reference/en/language/functions/communication/serial/… \$\endgroup\$ – GeorgicaFaraFrica Dec 9 '19 at 13:59
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Simon Forsberg Dec 9 '19 at 15:19
6
\$\begingroup\$

Here are a number of ideas that may help you improve your program.

Separate calculation from I/O

The Serial.println(); call from within getWeigand really shouldn't be there. It's better if the function does just one thing, and that is to calculate and return the number.

Don't leak memory

The code calls new but there isn't any matching delete which means that the program leaks memory. It's better to make sure all memory is freed, but better still, see the next suggestion.

Avoid using new and delete

Modern C++ doesn't use new and delete as much as it used to. In this case, much better than passing around a raw array pointer and hoping the other end knows what the size is, is to use std::array instead. That is an improvement over a plain array because it works with all of the standard container algorithms and has its own .size() operator. See I.13 for more details on why passing around a single array pointer is not a good idea.

Prefer single bit shifting to multi-bit shifting

Many processors have hardware optimizations for multi-bit shifts, but not all do. That means on some machines, multi-bit shifts take longer. For that reason, and because I think it's more readable, I'd suggest changing the first for loop to this:

for (int i = 24; i > 0; --i) {
    wiegandNum[i] = dec & 1;
    dec >>= 1;
}

Simplify your code

Instead of doing things like this:

if(even) {
    wiegandNum[0] = 0;
} else {
    wiegandNum[0] = 1;
}

just use the value directly:

wiegandNum[0] = !even;

Use whitespace for readability

Lines like this one:

for(int i=1;i<13;i++) {

are much easier to read with a little more whitespace:

for (int i = 1; i < 13; i++) {

Create a test routine

With code like this, it's usually good to do extensive testing to make sure it does what you intend. I am not using an Arduino, but created a version that runs on Linux (or really any computer that supports C++):

#include <string>
#include <iostream>
#include <iomanip>
std::string weiString(unsigned int dec)
{
    std::string str{};
    auto answer = getWiegand(dec);
    for (const auto digit: answer) {
        str.push_back(digit ? '1' : '0');
    }
    return str;
}

int main()
{
    struct {
        unsigned n;
        std::string expected;
        bool operator()() const {
            std::string calculated = weiString(n);
            bool ok{calculated == expected};
            std::cout << std::hex << n 
                << std::dec 
                << '\t' << expected 
                << '\t' << calculated 
                << '\t' << (ok ? "OK" : "incorrect!") 
                << '\n';
            return ok;
        }
    } tests[]{
        {12679548,  "01100000101111001011111000" },
        {0xffff,    "00000000011111111111111111" },
        {0x1ffff,   "10000000111111111111111111" },
        {0xffffff,  "01111111111111111111111111" },
        {0xdecade,  "01101111011001010110111101" },
        {0xfffffff, "01111111111111111111111111" },  // ignore high bits
    };

    for (const auto& test: tests) {
        test();
    }
}

Results

Here's the alternate version that uses these suggestions. It also uses the exclusive-or operator ^ to simplify parity calculations:

#include <array>

std::array<unsigned, 26> getWiegand(unsigned int dec)
{
    std::array<unsigned, 26> wiegandNum;
    for (int i = 24; i > 0; --i) {
        wiegandNum[i] = dec & 1;
        dec >>= 1;
    }
    // check for parity of the first 12 bits
    bool even = false;
    for (int i = 1; i < 13; i++) {
        even ^= wiegandNum[i];
    }
    wiegandNum[0] = even;

    // check for parity of the last 12 bits
    bool odd = true;
    for (int i = 13; i < 25; i++) {
        odd ^= wiegandNum[i];
    }
    wiegandNum[25] = odd;

    return wiegandNum;
}

Even shorter is to process one of the parity bits over the entire array as it's generated and then recalculate just the other one and adjust. It might be less obvious why this works, but it does. See if you can see why.

std::array<unsigned, 26> getWiegand(unsigned int dec)
{
    bool odd = true;
    std::array<unsigned, 26> wiegandNum;
    for (int i = 24; i > 0; --i) {
        wiegandNum[i] = dec & 1;
        odd ^= wiegandNum[i];
        dec >>= 1;
    }
    // check for parity of the first 12 bits
    bool even = false;
    for (int i = 1; i < 13; i++) {
        even ^= wiegandNum[i];
    }
    wiegandNum[0] = even;
    wiegandNum[25] = odd ^ even;
    return wiegandNum;
}
\$\endgroup\$
  • \$\begingroup\$ Wow. What a great answer. Great tricks with wiegandNum[0] = even; and the XOR operator. Can you please explain to me single bit shifting ? The for changed from this: for(int i = 23; i>=0; i--) to this for (int i = 24; i > 0; --i). They don't look alike, yet they function exactly the same. What is this magic ? \$\endgroup\$ – GeorgicaFaraFrica Dec 9 '19 at 15:20
  • 1
    \$\begingroup\$ No magic at all. :) Your original code started from the most significant bit while this version starts from the least. The advantage is that we can shift the whole number (dec >>= 1;) to the right by one bit each time and just test the low bit with dec & 1. It's a pretty common trick. \$\endgroup\$ – Edward Dec 9 '19 at 15:23
  • \$\begingroup\$ I will need to do some reading on that. Some drawings will most certainly help :)). \$\endgroup\$ – GeorgicaFaraFrica Dec 9 '19 at 15:24
  • 1
    \$\begingroup\$ Just for fun, I added an even shorter version to my answer at the end, but you'll probably want to think about it for a while to understand why it works. \$\endgroup\$ – Edward Dec 9 '19 at 15:30
4
\$\begingroup\$

Be careful when allocating memory with new[] and passing to the caller:

unsigned int * getWiegand(unsigned int dec)
{
  unsigned int* wiegandNum = new unsigned int[26];
  // ...
  return wiegandNum;
}

void setup()
{
  unsigned int* wiegandArray = getWiegand(12679548);
  // BUG: never deleted!
}

This is a memory leak. Prefer to use a smart pointer or container instead.

BTW, is it really necessary to use 26 unsigned int values each storing one bit, or could/should you use a more compact representation?

\$\endgroup\$
  • \$\begingroup\$ I have no idea what's good or not. That program is all my knowledge about C++. I don't know anything about smart pointers or containers. Storing one bit in one unsigned int is the only representation I could think off. I need access to every bit for an if(wiegandArray[0] == 1) {// do something} else if(wiegandArray[0] == 0) {// do something else}. \$\endgroup\$ – GeorgicaFaraFrica Dec 9 '19 at 14:11
  • \$\begingroup\$ I also need the container of the binary number to be re-usable. The program will get an unspecified number of binary numbers that will have to be placed into an array to be manipulated later. Something like this: unsigned int* wiegandArray; and then this: wiegandArray = getWiegand(12679548); gets called each time a new number is being served. I couldn't put all the code of the project here. \$\endgroup\$ – GeorgicaFaraFrica Dec 9 '19 at 14:22

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.