0

I'am new to Arduino and c++

i get his code from the INTERNET

void setup() {
    Serial.begin(57600);  
    pinMode(13, OUTPUT);
}
int f = 0, pos;
void loop() {
    boolean has_request = false;
    String in = "";
    if (Serial.available()) {
        in = "";
        while (true) {  // should add time out here
            while (Serial.available() == false) {}
            in += (char)(Serial.read());
            if (in.endsWith("\r\n\r\n")) {
                has_request = true;  break;
            } 
        }
    }
    if (has_request) {
        int i1 = in.indexOf("GET /blink?f="), i2;
        if (i1 != -1) {
            i2 = in.indexOf(" ", i1+13);
            f = in.substring(i1+13, i2).toInt();
        }
        Serial.println("HTTP/1.1 200 OK\nContent-Type: text/html\nConnection: close");
        String sr = "<!DOCTYPE HTML>\n";
        sr += "<html>\n";
        sr += "LED frequency: ";
        sr += f;
        sr += "Hz.</html>";
        Serial.print("Content-Length: ");
        Serial.print(sr.length());
        Serial.print("\r\n\r\n");
        Serial.print(sr);
        has_request = false;
    }
    if (f>0) {
        static unsigned long t = millis();
        if (millis() > t + 1000/f) {
            digitalWrite(13, 1-digitalRead(13));
            t = millis();
        }
    }
}

see this link if you cant see the code formated here in the site

this code is used for the Arduino uno wifi shield it is working perfectly except it is too complicated for me while loop inside a while loop

and i don't want to do frequency i only need to send turn on or off without any complexity

anyone could help me ?

  • The test if (millis() > t + 1000/f) will not work properly close to the millis() rollover. It should rewritten as if (millis() - t > 1000/f). See this answer to How can I handle the millis() rollover? for an extended discussion. – Edgar Bonet Jul 8 '15 at 8:01
0

I would recommend starting out with some easy stuff you understand or making code yourself... Starting out with code you don't understand will backfire you.

I'll explain the code for you anyway.

This part will "set everything up". The serial connection and the direction (in/output) of the pin(s).

void setup() {
    Serial.begin(57600);  
    pinMode(13, OUTPUT);
}

Somehow did the person who made this think it was logical to put this here instead of on the very top of code. There are 2 integer variables declared here of which one is initialized with 0. (f).

int f = 0, pos;

This part of the code (untill the very last bracket) will be ran continiously, in a "loop". Many, if not all, embedded systems will run "forever".

void loop() {

This is basically a "flag", it indicates if something has, or has not happened. At this point it's only initialized.

    boolean has_request = false;

Also we're keeping a buffer (text string) of the characters we receive.

    String in = "";

Check if there are characters in our serial buffer.

    if (Serial.available()) {

If so, we empty our own buffer.

        in = "";

And keep doing this code.

        while (true) {  // should add time out here

We wait until the next character is ready.

            while (Serial.available() == false) {}

And add it to our buffer.

            in += (char)(Serial.read());

But when we see our buffer ends with \r\n\r\n (carriage return, line break x2).

            if (in.endsWith("\r\n\r\n")) {

We're going to set te flag, indicating that we have received a message. And we stop the: "Keep doing this code".

                has_request = true;  break;
            } 
        }//(this is actually the parenthesis of the while(true) which we break out.

    }

So we had this message, right? Let's do the following:

    if (has_request) {

Parse the variable in the GET request (f=??) to an int.

        int i1 = in.indexOf("GET /blink?f="), i2;
        if (i1 != -1) {
            i2 = in.indexOf(" ", i1+13);
            f = in.substring(i1+13, i2).toInt();
        }

Send a 200OK (with body) back to the thing that sent the http request (actually pretty neat to do so.)

        Serial.println("HTTP/1.1 200 OK\nContent-Type: text/html\nConnection: close");
        String sr = "<!DOCTYPE HTML>\n";
        sr += "<html>\n";
        sr += "LED frequency: ";
        sr += f;
        sr += "Hz.</html>";
        Serial.print("Content-Length: ");
        Serial.print(sr.length());
        Serial.print("\r\n\r\n");
        Serial.print(sr);
        has_request = false;
    }

We also keep doing this blinking. (Fun fact: While receiving/sending stuff, your blinking will stop/desync. This is because the receive/send code is blocking the blinking code. Making the blinking hardware and/or receive/send interrupt based will improve the systems reaction time and efficiency.)

    if (f>0) {
        static unsigned long t = millis();
        if (millis() > t + 1000/f) {
            digitalWrite(13, 1-digitalRead(13));
            t = millis();
        }
    }
}

But how will we make this better? Or atleast easier to read? By using functions, you can split up the code, and giving the functions a logic name, it all becomes quite obvious. The whole thing is doing 3 jobs: Blinking a led on a given frequency. Receiving requests/commands through serial (handling serial). And handling these requests afterwards.

void BlinkLed(int f);
void HandleRequest();
void HandleSerial();

int f = 0, pos;//Pos doesn't have to be a global variable...

void setup() {
    Serial.begin(57600);  
    pinMode(13, OUTPUT);
}

void loop() {
    boolean has_request = false;//Reset the flag every loop.
    String in = "";//Clear the string every new loop.
    if (Serial.available()) {
        HandleSerial();
    }
    if (has_request) {
        HandleRequest();
    }
    BlinkLed(f);
}

void BlinkLed(int f){
if (f>0) {
        static unsigned long t = millis();
        if (millis() > t + 1000/f) {
            digitalWrite(13, 1-digitalRead(13));
            t = millis();
        }
    }
}

void HandleSerial(){
        in = "";
        while (true) {  // should add time out here
            while (Serial.available() == false) {}
            in += (char)(Serial.read());
            if (in.endsWith("\r\n\r\n")) {
                has_request = true;  break;
            } 
        }
}

void HandleRequest(void){

        int i1 = in.indexOf("GET /blink?f="), i2;
        if (i1 != -1) {
            i2 = in.indexOf(" ", i1+13);
            f = in.substring(i1+13, i2).toInt();
        }
        Serial.println("HTTP/1.1 200 OK\nContent-Type: text/html\nConnection: close");
        String sr = "<!DOCTYPE HTML>\n";
        sr += "<html>\n";
        sr += "LED frequency: ";
        sr += f;
        sr += "Hz.</html>";
        Serial.print("Content-Length: ");
        Serial.print(sr.length());
        Serial.print("\r\n\r\n");
        Serial.print(sr);
        has_request = false;
}

V2 (more likely to compile, but kinda changes the structure):

void BlinkLed(int f);
void HandleRequest();
boolean HandleSerial();

int f = 0;//Frequency of the led (blinking)

void setup() {
    Serial.begin(57600);  
    pinMode(13, OUTPUT);
}

void loop() {
    String in = "";//Clear the string every new loop.
    if (Serial.available()) {
        if(HandleSerial()){
            HandleRequest();
        }
    }
    BlinkLed(f);
}

void BlinkLed(int f){
if (f>0) {
        static unsigned long t = millis();
        if (millis() > t + 1000/f) {
            digitalWrite(13, 1-digitalRead(13));
            t = millis();
        }
    }
}

boolean HandleSerial(){
        in = "";
        while (true) {  // should add time out here
            while (Serial.available() == false) {}
            in += (char)(Serial.read());
            if (in.endsWith("\r\n\r\n")) {
                return true;
            } 
        }
        return false;
}

void HandleRequest(void){
    int i1 = in.indexOf("GET /blink?f="), i2;
    if (i1 != -1) {
        i2 = in.indexOf(" ", i1+13);
        f = in.substring(i1+13, i2).toInt();
    }
    Serial.println("HTTP/1.1 200 OK\nContent-Type: text/html\nConnection: close");
    String sr = "<!DOCTYPE HTML>\n";
    sr += "<html>\n";
    sr += "LED frequency: ";
    sr += f;
    sr += "Hz.</html>";
    Serial.print("Content-Length: ");
    Serial.print(sr.length());
    Serial.print("\r\n\r\n");
    Serial.print(sr);
}

We can even make it more nicer by:

  • using hardware (PWM) to toggle/time the led.
  • rewriting the serial handling code... (https://www.arduino.cc/en/Serial/ReadStringUntil)
  • Using printf() to format the serial.prints on one line...
  • Make the receive/send code interrupt based, so that it will only take short amounts of time of your code, instead of continously waiting in a while, you can do stuff in the meanwhile ;)
  • using classes.
  • The variable pos is not used at all. – Edgar Bonet Jul 8 '15 at 8:22
  • In your “better” version, line 35: “error: ‘in’ was not declared in this scope”. Same for has_request (lines 40 and 62), in (line 47) and i2 (line 49). – Edgar Bonet Jul 8 '15 at 8:28
  • Haha, indeed, edited the code on "answer" itself. And copied from the original code. Pos indeed doesn't seem to do anything. And yeah, by putting stuff in functions you're going to need to transfer the variables from one to another, or make them global :) not sure about the i2 though, it's in: int i1 = in.indexOf("GET /blink?f="), i2;? I'll be editing the code (this time in Notepad++, don't have access to a compiler at the moment :l). – Paul Jul 8 '15 at 8:37
  • The syntax highlighting is actually very nice! And you should consider the code in my answer as an example. With the explanation on the code it'd be fairly easy to roll your own (better) version of it (that will actually compile and work as you want ;) ) I believe that in V2 it's better, but I'm not sure if it still works as intended. – Paul Jul 8 '15 at 8:45
  • Just one compilation error remaining: in is declared in loop(), and used in HandleSerial() and HandleRequest(). Simplest solution would be to make it global. – Edgar Bonet Jul 8 '15 at 8:54
3

There is only one "while loop inside a while loop" and that is hardly complex:

while (Serial.available() == false) {}

All that means is "Wait for something to become available on the serial". That is ALL it does.

There is nothing even remotely complex there. True, it may not be very well written (as is the case with lots of code on t'interwebs). It's time to start learning how the code works so you can write your own.

  • 3
    +1, that code is extremely straightforward. Written like crap, but straightforward nonetheless. – Ignacio Vazquez-Abrams Jul 7 '15 at 23:30
2

Here is my take at this. I realize that I did not really make the program simpler, but hopefully better in some respects, as it avoids blocking reads and heap memory allocation. Be warned however that I did not test it. It compiles fine though.

Preamble, hopefully simple enough that it requires no comments:

const uint8_t led_pin = 13;

void setup() {
    Serial.begin(57600);  
    pinMode(led_pin, OUTPUT);
}

Next, here is a function to read the HTTP request. A few things are worth noting in this function:

  • It is non-blocking: if there is not data in the serial buffer, it will return NULL immediately. This is to prevent the whole program from hanging if the client does not terminate its request.
  • It only saves the first 31 bytes of the request. This is because browsers tend to add lots of useless headers, which can be unfriendly to the memory of a small Arduino. And the only interesting piece of data is at the very beginning anyway.
  • The request terminating sequence (CR LF CR LF) is detected without having to store it in the buffer, just by counting how many consecutive bytes of that sequence we have already received.
  • Declaring the function as static makes it possible for the compiler to inline it, which it does as it is only called from one place. This makes for a smaller executable.
/*
 * Non-blocking read. Returns the first bytes of the request in a static
 * buffer, or NULL if we do not have a complete request yet.
 */
static char * read_request() {
    static char buffer[32];
    static uint8_t pos;
    static uint8_t crlf_bytes;  // bytes of "\r\n\r\n" received

    while (Serial.available()) {
        char c = Serial.read();

        // Store the received byte if there is enough room.
        if (pos < sizeof buffer - 1)
            buffer[pos++] = c;

        // Update the count of consecutive CRLF bytes received.
        if ((c == '\r' && crlf_bytes % 2 == 0)
         || (c == '\n' && crlf_bytes % 2 == 1))
            crlf_bytes++;
        else
            crlf_bytes = 0;

        // Return the buffer if we have a complete request.
        if (crlf_bytes == 4) {
            buffer[pos] = '\0';  // terminate the string
            pos = 0;
            crlf_bytes = 0;
            return buffer;
        }
    }
    return NULL;
}

Parsing the request is done by a different function. Here, I do not search for the "GET /blink?f=" string, because it must be at the very beginning for the request to be valid.

/*
 * Returns the requested frequency, or 0 if nothing valid is found.
 */
static int parse_request(char *request) {

    /* The "GET" should be at the very beginning. */
    if (strncmp(request, "GET /blink?f=", 13) != 0)
        return 0;

    return atoi(request + 13);
}

Now we can send the response to the client, telling it how we parsed the frequency. I avoid using String, as it involves heap allocation, and I put the bulk of the response in PROGMEM (F() macro) to save RAM. I also added a <title> to make it valid HTML.

/*
 * Tell the client how we understood the requested frequency.
 */
static void send_response(int freq) {
    char freq_str[12];
    itoa(freq, freq_str, 10);
    Serial.print(F("HTTP/1.1 200 OK\r\n"
                   "Content-Type: text/html\r\n"
                   "Content-Length: "));
    Serial.print(84 + strlen(freq_str));
    Serial.print(F("\r\nConnection: close\r\n\r\n"
                   "<!DOCTYPE html>\n"
                   "<html><head><title>Blink</title></head>\n"
                   "LED frequency: "));
    Serial.print(freq_str);
    Serial.print(F(" Hz.\n</html>\n"));
}

The next function blinks the LED in a rollover-safe fashion. Storing the current LED state in a static variable avoids a call to digitalRead().

static void blink_led(unsigned long toggle_time) {
    static uint8_t led_state;
    static unsigned long last_toggle;
    unsigned long now = millis();
    if (now - last_toggle > toggle_time) {
        led_state = !led_state;
        digitalWrite(led_pin, led_state);
        last_toggle = now;
    }
}

Putting it all together, the main loop() ends up being quite straight forward:

void loop() {
    static unsigned long period = 1000;
    char * request = read_request();
    if (request) {
        int freq = parse_request(request);
        if (freq > 0) period = 1000 / freq;
        send_response(freq);
    }
    blink_led(period);
}

PS: My version is significantly larger than the original at the source level (66 v.s. 44 source lines of code), but the compiled program is smaller in both flash and RAM usage:

text    data     bss     version
5646     168     197     original
3316      40     218     my version
  • Hmm, I agree this code is probably way faster and better optimized. Though the variable last_toggle is only known in the "blink_led" function. And that basically means that in the if-statement (in blink_led function), you're testing the toggle_time against an uninitialized variable? – Paul Jul 9 '15 at 6:42
  • No, last_toggle is a local static variable. This means that, although it is only “locally visible”, it has “global duration”: it is initialized at program startup by the C runtime (in __do_clear_bss), before main() is called, and it keeps its value between successive calls to blink_led(). Static variables are always initialized. If not explicitly, they are implicitly initialized to zero. And this is the right initialization for this variable, just like blink_led(unsigned long)::led_state, read_request()::pos and read_request()::crlf_bytes. – Edgar Bonet Jul 9 '15 at 7:13
  • Ah, yea, static in a function means something completely different as static in a variable? Thanks for the extensive argumentation! I just saw that indeed, it makes more sense to make it a static variable as global variable. As you won't need/want to access it anywhere, but you do want the "global" duration. It'll also be more efficient as just making it global? – Paul Jul 9 '15 at 8:49
  • 1
    @FuaZe: Yes, “static” has different meanings depending on whether it is used inside a function or in global scope. And no, it is not more efficient than a global variable, it's the same. You would not be able to tell the difference by disassembling the compiled program, except for the symbol table. – Edgar Bonet Jul 9 '15 at 9:44

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