Door-opener program

Question

I've been working on a little project on an Arduino and I need some help with making the code more efficient / cleaner. I really don't like the three for loops and feel like I could condense them somehow.

How it works:

1. An HC-05 Bluetooth module is connected via a serial connection to an Arduino Micro.
2. The Arduino filters the serial data and extracts the MAC addresses that it finds.
3. The discovered MAC addresses are compared against a white-list.
4. Once a white-listed device comes in range of the Bluetooth module AND passes a light barrier, the door opens.
5. The door can only be re-opened if either a NEW device comes in range, or if the already entered device has left the Bluetooth range for at least 10 seconds and re-enters.

#include <SoftwareSerial.h>
#include "Timer.h"

SoftwareSerial mySerial(10, 11); // RX, TX

Timer t;

// Pinbelegung
int keyPin = 13;
int detectionPin = 8;
int powerPin = 9;
int doorPin = 4;

// Wert von detectionPin Eingang
int detectionPinRead;

// Serielle Daten
String serialData;

// Rausgefilterte MAC Addresse
String mac;

// Zugangsarrays
const char* whitelist[2] = {"+INQ:BCF5:AC:74DDB7", "+INQ:1068:3F:E1081F"};
int time[2] = {0, 0};
boolean entered[2] = {false, false};

// Zeit seit dem Start
unsigned long currentTime;

// Zugangsbedingung
boolean detected;
boolean retrigger;

void setup() {
  Serial.begin(38400);   
  while (!Serial) { ; }

  mySerial.begin(38400);

  pinMode(keyPin, OUTPUT); 
  pinMode(detectionPin, INPUT); 
  pinMode(powerPin, OUTPUT); 
  pinMode(doorPin, OUTPUT);   

  // Versetze Bluetoothmodul in AT Modus 
  digitalWrite(keyPin, HIGH);
  delay(500);
  digitalWrite(powerPin, HIGH);
  delay(500);
  mySerial.println("AT+INIT");
  delay(500);
  mySerial.println("AT+INQM=1,0,48");
  delay(500);
  mySerial.println("AT+INQ");

  // Ein Scan dauert ca. 62 Sekunden. Starte alle 63 Sekunden einen neuen Scan
  t.every(63000, startInq);
}

void loop() {  
  t.update();
  currentTime = millis() / 1000;

  //Wenn Serielle Daten verfügbar sind, lese so lange bis end of line
  while (mySerial.available() > 0) {        
    char received = mySerial.read();
    serialData += received;       

    if (received == '\n') { 
      mac = getToken(serialData, ',', 0);

      // Debug
       Serial.println(mac);
       Serial.println(currentTime); 
       Serial.println(time[0]);  
       Serial.println(time[1]); 

      // Lese den Eingang aus. Wenn HIGH und retrigger ist verhindert, setze Zugangsbedingung
      detectionPinRead = digitalRead(detectionPin);

      if (detectionPinRead == HIGH && !retrigger) {
        for (int i = 0; i < 2; i++) {
          if (mac == whitelist[i]) {
              detected = true;
              retrigger = true;    
              break;
          } 
        }        
      }

      // Überprüfe ob MAC seit mindestens 10 Sekunden nicht mehr in Rechweite ist
      for (int i = 0; i < 2; i++) {
        if(mac == whitelist[i] && currentTime - time[i] >= 10) {            
          entered[i] = false; 
          retrigger = false;
          break;
        } 
      }       

      // Öffne Tür sobald Zugansbedingungen erfüllt sind
      for (int i = 0; i < 2; i++) {
        if (mac == whitelist[i]) {
          time[i] = currentTime;
          if (!entered[i] && detected) {                      
            entered[i] = true;
            openDoor();
            break;
          } 
        }      
      }      
      serialData = "\0";
    } 
  }
}

String getToken(String data, char separator, int index) {
  int found = 0;
  int strIndex[] = {0, -1};
  int maxIndex = data.length() - 1;

  for(int i = 0; i <= maxIndex && found <= index; i++) {
    if(data.charAt(i) == separator || i == maxIndex) {
      found++;
      strIndex[0] = strIndex[1] + 1;
      strIndex[1] = (i == maxIndex) ? i + 1 : i;
    }
  }
  return found > index ? data.substring(strIndex[0], strIndex[1]) : "";
}

void openDoor() {  
  digitalWrite(doorPin, HIGH);
  delay(500);
  digitalWrite(doorPin, LOW);
  detected = false;  
}

void startInq() {
  mySerial.println("AT+INQ");
}

Answer 1

There are definitely some things you can do to make this code more clear.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. For global variables such as currentTime, consider wrapping them in objects to make it easy to differentiate between read access and an update.

Consolidate the logic

Extracting the logic from your text description, we can express it in psuedocode:

for each detected mac:
    if (isWhitelisted(mac) && sensor && isEligible(mac)) 
        openDoor()

All that remains is keeping those variables updated and correct. The sensor variable is simply your detectionPinRead == HIGH statement, so that one is very simple. Checking for whitelisting is what you're already doing inside each loop, so that's very simple, too. The only thing left is to determine whether the given mac device is eligible to open the door. A device is eligible if it's new or if it has been out of range for at least 10 seconds. In later sections, I'll show one way to do this.

Use objects

Your devices in the whitelist have both data and logic associated with each. This strongly suggests an object. I'd propose something like this:

class AuthorizedBT 
{
public:
    AuthorizedBT(const char *mac) : MAC(mac), lasttime(0), outside(true) {}
    bool matches(const String& otherMAC) const { return MAC == otherMAC; }
    bool isEligible(int currTime) const { 
        return outside || currTime >= (lasttime + 10); }
    void markSeen(int currTime) { lasttime = currTime; }
    void markInside() { outside = false; }
private:
    const char* MAC;
    int lasttime;
    bool outside;
};

Your whitelist can now be constructed of objects:

AuthorizedBT whitelist[2] = {"one", "two"};

Separate I/O from logic

A simplified version of your loop can be rewritten to separate I/O from logic:

void loop() {
    // update time, fetch mac from serial port
    update(currentTime, mac, HIGH == digitalRead(detectionPin));
}

Now the active portion of your code which does serial I/O, updates time and reads the port is cleanly separated from the logic portion of your code which is now placed into a routine called update:

bool update(int currentTime, const char *mac, bool sensor)
{
  bool result = false;
  for (int i=0; i < 2; ++i) {
    if (whitelist[i].matches(mac)) {
      if (sensor && whitelist[i].isEligible(currentTime)) {
        openDoor();
        whitelist[i].markInside();
        result = true;
      }
      whitelist[i].markSeen(currentTime);
    }
  }
  return result;
}

This now eliminates the global variables retrigger and detected. Even better, you could remove the call to openDoor from within this routine and use the return value to determine when to open the door. That results in an even cleaner separation. Also, if you can assure that currentTime will always be at least 10, you can eliminate the outside data member and all references to it.

Test your logic

With that clean separation of I/O from logic, it becomes possible to test the logic separately from the actual machine. This is highly beneficial especially when working on a prototype.

int main()
{
  assert(false == update(1, "foo", true));   // not whitelisted; no action  
  assert(false == update(20, "one", false)); // didn't trigger sensor; no action
  assert(true == update(21, "one", true));   // "one" is now inside
  assert(false == update(22, "one", true));  // "one" is already inside; no action
  assert(false == update(31, "one", true));  // "one" is already inside; no action
  assert(true == update(41, "one", true));   // "one" was out of range for 10 sec; open door
  assert(true == update(42, "two", true));   // "two" is now inside
  assert(false == update(43, "one", true));   // "one" is already inside; no action
  assert(false == update(43, "two", true));   // "two" is already inside; no action
}

Be careful with integers

The code currently has a global variable currentTime which is declared as unsigned long, but then the value is stored within the time array, each member of which is an int. Even if your compiler treats long and int as having the same size, it surely will not treat signed and unsigned versions identically.

Be careful with variable names

The time array could easily cause a conflict with the built-in time function within the <ctime> header. Best would be to use a variable name which is not already a commonly used variable or function name.

Use bool rather than boolean

The standard type is bool rather than boolean. Unless there's something very wrong with your C++ compiler, you should use bool.

Careful with strings

The code uses a String type but does not show the implementation. If it's a std::string type or other object, be careful that you don't inadvertently create temporary copies. You can avoid it by careful selection of parameter declarations. For example, instead of

String getToken(String data, char separator, int index)

you might consider this form:

String getToken(const String &data, char separator, int index)

Eliminate "magic numbers"

There are a few numbers in the code, such as 2 and 10 that have a specific meaning in their particular context. By using named constants such as NUMBER_OF_WHITELISTED_OBJECTS or MIN_OUT_OF_RANGE_SECONDS, the program becomes easier to read and maintain. For cases in which the constant only has sense with respect to a particular object, consider making that constant part of the object.