5

I have a small piece of code here that I hope to run on Arduino. Basically what this code is doing is, it takes a nibble from the user, appends a startbit to it, and transmits the code through a transmitter module. The receiver end receives the bits and checks if startbit is being received. If startbit is not being received, then it prints out whatever was transmitted. If startbit is received, then it doesn't print the startbit, just the nibble all in one line.

My question is: is it convention/clever to use the bool array or should I not do that? Second, is there a way to make my code more efficient? Where can I clean it up? Also, is my code legible or very convoluted?

#include <Arduino.h>
#include <WirelessComm.h>

int tx_pin;
int rx_pin;
WirelessComm::WirelessComm(int rxPin, int txPin)
{
    pinMode(rxPin, INPUT);
    tx_pin = txPin;
    rx_pin = rxPin; 
}

int countDigit(long n)
{
    int count = 0;
    while (n > 0)
    {
        n /= 10;
        count++;
    }
    return count;
}

void getDigit(bool *data, long num)
{
    int cd = countDigit(num);
    long shift = 1;
    for (int i = 0; i < cd; i++)
    {
        data[i] = (num / shift) % 10;
        shift *= 10;
    }
}

int WirelessComm::writeTx(long nibble)
{   
    int i;
    bool data[11]; // warning: I didn't check for buffer overrun!
    int32_t startBit = 110101L; // larger than 32767, so cannot be in int
    if(nibble != 0)
    {
        int sbLen = countDigit(startBit);
        for (int i = 0; i < (int)(sizeof(data) / sizeof(data[0])); i++) 
            data[i] = 0;
        getDigit(data, startBit);
        getDigit(data + sbLen, nibble);
    }
    else
        return -1;

    for(i = 0; i < 10; i++)
    {
        if(data[i] == 1)
        {
            digitalWrite(tx_pin, HIGH);
        }
        else
        {
            digitalWrite(tx_pin, LOW);
        }

    }
    return 1;
}

int WirelessComm::readRx(long nibble)
{
    bool first = 0;
    bool second = 2; //initially set to some random value
    bool third = 2;
    bool fourth = 2;
    bool fifth = 2;
    bool sixth = 2;
    bool counter = 0;
    bool val = 0;
    while(true)
    {
        first = digitalRead(rx_pin);
        if(first == 1);
        {
            second = digitalRead(rx_pin);
            if(second == 1)
            {
                third = digitalRead(rx_pin);
                if(third == 0)
                {
                    fourth = digitalRead(rx_pin);
                    if(fourth == 1)
                    {
                        fifth = digitalRead(rx_pin);
                        if(fifth == 0)
                        {
                            sixth = digitalRead(rx_pin);
                            if(sixth == 1)
                            {
                                while(counter < 4)
                                {
                                    val = digitalRead(rx_pin);
                                    Serial.print(val);
                                    counter++;
                                }
                                Serial.println();

                            }
                            else
                                Serial.println(first);
                                first = digitalRead(rx_pin);  
                        }
                        else
                            Serial.println(first);
                            first = digitalRead(rx_pin);  
                    }
                    else
                        Serial.println(first);
                        first = digitalRead(rx_pin);  
                }
                else
                    Serial.println(first);
                    first = digitalRead(rx_pin);  
            }
            else
                Serial.println(first);
                first = digitalRead(rx_pin);  
        }
        else
            Serial.println(first);
            first = digitalRead(rx_pin);
    }
}
Jonathan
  • 264
  • 3
  • 15

2 Answers2

5

What you are doing is called "bit-banging" (wikipedia link) but you have no guarantees about the timing of your bits.

In your writeTx routine you are changing tx_pin at some rate which depends on:

  1. the clock speed of your MCU
  2. the code that the C compiler generates

Moreover, the timing will be all screwed up if you happen to get an interrupt while sending the bits.

The same applies to your read routine.

The Arduino (really the Atmel MCU) has special hardware to perform both synchronous and asynchronous serial IO. The set up is rather complex because the hardware is very versatile and capable of a lot of different functions. In particular, it can deliver the bits at a specific baud rate for you so your code doesn't have to do it. It can also read in a serial stream and set a flag or invoke an interrupt when a complete word has been received.

This article can give you an idea of what the USART in an Atmel processor is capable of:

http://maxembedded.com/2013/09/the-usart-of-the-avr/

ErikR
  • 166
  • 1
3

So to clarify, SoftwareSerial uses bit-bang? How are you able to tell?

By reading the code. For example, this is how it reads 8 bits. It does a delay (which you don't) for the exact amount of time, and then reads the pin, and "ors" it into the variable which is the assembled byte.

// Read each of the 8 bits
for (uint8_t i=0x1; i; i <<= 1)
{
  tunedDelay(_rx_delay_intrabit);
  DebugPulse(_DEBUG_PIN2, 1);
  uint8_t noti = ~i;
  if (rx_pin_read())
    d |= i;
  else // else clause added to ensure function timing is ~balanced
    d &= noti;
}

And to write the 8 bits:

// Write each of the 8 bits
for (byte mask = 0x01; mask; mask <<= 1)
{
  if (b & mask) // choose bit
    tx_pin_write(HIGH); // send 1
  else
    tx_pin_write(LOW); // send 0

  tunedDelay(_tx_delay);
}

Your rather elaborate nested if just seems complete overkill, when you see how SoftwareSerial does it in half a dozen lines.


Another typo:

if(first == 1);

You don't want that semicolon.


Second, is there a way to make my code more efficient?

Use loops, like SoftwareSerial does.

Also, is my code legible or very convoluted?

Very convoluted.

Where can I clean it up?

I suggest you browse through the SoftwareSerial library for ideas.

Meanwhile, HardwareSerial is available, so you don't need to write any of this. For that matter, SoftwareSerial is available too. If you are just doing it to learn, well and good.


You really should read about loops. Whenever I have to do something 8 times I have a loop of 8 (or even for "x" times, a loop of "x"). Unrolling into 8 different variables is just confusing and prone to errors. As you had, before you fixed it, doing if(fourth = 1) instead of if(fourth == 1).

Nick Gammon
  • 38,901
  • 13
  • 69
  • 125