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);
    }
}
  • "Hope to run on Arduino" — so it doesn't actually run? In another version of this question on Code Review (now deleted), a concern was raised over the correctness of if(third = 0). – 200_success Sep 28 '15 at 23:57
  • Does this code actually work with whatever device you want to talk to? This is not the way to perform serial IO. – user5402 Sep 29 '15 at 0:20
  • @user5402 The write function is ok, and it works. However, this is my attempt at serial communication. Can you please tell me what things I am doing wrong so I can perform a more correct serial IO. – Jonathan Smit Sep 29 '15 at 1:47
  • Does your readRx function work? – user5402 Sep 29 '15 at 1:50
  • Not exactly the way I want it to. Some of the bits are lost. I think it's because of the interval time of the data. I'm guessing it would actually pick up the signal correctly if the clock was right. However, I am not doing synchronous communication. Just asynchronous. THerefore, I am sending a clock signal. – Jonathan Smit Sep 29 '15 at 1:58
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/

  • But the thing is, I don't want to do synchronous communication. I actually want to do asynchronous and then implement different ways of synchronizing the clock such as NRZ. – Jonathan Sep 29 '15 at 2:25
  • Also, i was told that if i know the interval of data, i can implement that and not worry about the clock signal. How should I approach that? What exactly does the interval of data* mean? – Jonathan Sep 29 '15 at 2:26
  • You still want hardware to do this for you. "knowing the interval of data" just means that you have set a specific baud rate - i.e. so many bits per second. The USART can handle asynchronous comms, and it can monitor the receive pin and respond to changes a lot faster than you can in code. – ErikR Sep 29 '15 at 2:34
  • OO! i see what you are saying. ok ok. And so, should I just restart the code I'm trying to make? I've tried to create a buadrate function before, but I had very bad luck with it. One big problem was, when i tested how many times my receiver transmitter a bit in 1 second, it was 1000. So it wouldn't really be any use if I set the baud rate to be 1200 or something. Cause if the maximum it sent (since the pin was on high for the whole second) was 1000 transmissions, that's max hardware capability. right? – Jonathan Sep 29 '15 at 2:41
  • What you do is start learning how to program the USART of the AVR. Using the USART you can reliably handle 1Mbit per second. The article I mention in the answer is a good start. Start with getting standard RS-232 working. Get two Arduinos talking to each other over RS-232. The USART has a lot of registers you can configure - it takes a while to fully understand what can be done with it. And an upvote would be nice if you've found any of what I've said to be helpful. – ErikR Sep 29 '15 at 2:46
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).

  • Thank you very much for that explanation. I will do what you suggested as well. – Jonathan Sep 29 '15 at 5:46
  • Hey, the actual software serial library, frmo the installation files looks much different from the snippets you posted. Can you please show me where you are pulling your code from? Thank you so much! – Jonathan Sep 30 '15 at 4:13
  • 1
    I got that from IDE 1.0.6. It looks like they changed it a bit in 1.6.5, however conceptually it is similar. The changes look like efficiency tweaks to me. Fundamentally it is the same. They are reading/writing bits with a carefully calculated delay between each one. – Nick Gammon Sep 30 '15 at 4:39

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.