Blink not working

Question

I have written this code to write and receive signals over i2c between two Uno's. I have a master and a slave code, this is working fine. Now I want the led to blink, for this I added the blink without delay example. Unfortunately I can't get it working at all...

// Wire Slave Receiver
#include <Wire.h> //include Library
// define Led Pins
#define LED1 3 //Pin 3
#define LED2 4 //Pin 4
#define LED3 5 //Pin 5
int ledState = LOW; // ledState used to set the LED
unsigned long previousMillis = 0; // will store last time LED was updated
const long interval = 500; // interval at which to blink (milliseconds)
void setup()
{
 Wire.begin(4); // join i2c bus with address #4
 Wire.onReceive(receiveEvent); // register event
 Serial.begin(9600); // start serial for output
 //set Pin modes
 pinMode(LED1, OUTPUT);
 pinMode(LED2, OUTPUT);
 pinMode(LED3, OUTPUT);
 //intern pulldown
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
}
void loop()
{
 unsigned long currentMillis = millis();
 delay(100);
}
// function that executes whenever data is received from master
// this function is registered as an event, see setup()
void receiveEvent(int howMany)
{
 while (1 < Wire.available()) // loop through all but the last
 {
 char c = Wire.read(); // receive byte as a character
 Serial.print(c, BIN); // print the character as Binary
 }
 int x = Wire.read(); // receive byte as an integer
 Serial.println(x, BIN); // print the integer as Binary
 if (x == 1) { //turn LED1 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 Serial.println("LED1 on");
 }
 if (x == 2) { //blink LED1
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED1, ledState);
 }
 }
 if (x == 3) { //turn LED2 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, HIGH);
 delay(100);
 }
 if (x == 4) { //blink LED2
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED2, ledState);
 }
 }
 if (x == 5) { //turn LED3 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, HIGH);
 }
 if (x == 6) { //blink LED3
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED3, ledState);
 }
 }
 if (x == 7) { //turn LED1 nad 2 on
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 digitalWrite(LED2, HIGH);
 }
 if (x == 8) { //Blink LED1 and 2
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 }
 if (x == 9) { //turn LED2 and 3 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED2, HIGH);
 digitalWrite(LED3, HIGH);
 }
} 

---

Thanks for your response. I tried to put all the of statements into the loop, but it's still not working. :P I have no idea how to put it into the loop so it works.

// Wire Slave Receiver
#include <Wire.h> //include Library
// define Led Pins
#define LED1 3 //Pin 3
#define LED2 4 //Pin 4
#define LED3 5 //Pin 5
int ledState = LOW; // ledState used to set the LED
unsigned long previousMillis = 0; // will store last time LED was updated
const long interval = 500; // interval at which to blink (milliseconds)
void setup()
{
 Wire.begin(4); // join i2c bus with address #4
 Wire.onReceive(receiveEvent); // register event
 Serial.begin(9600); // start serial for output
 //set Pin modes
 pinMode(LED1, OUTPUT);
 pinMode(LED2, OUTPUT);
 pinMode(LED3, OUTPUT);
 //intern pulldown
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
}
void loop()
{
 if (x == 1) { //turn LED1 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 Serial.println("LED1 on");
 }
 if (x == 2) { //blink LED1
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED1, ledState);
 }
 }
 if (x == 3) { //turn LED2 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, HIGH);
 delay(100);
 }
 if (x == 4) { //blink LED2
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED2, ledState);
 }
 }
 if (x == 5) { //turn LED3 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, HIGH);
 }
 if (x == 6) { //blink LED3
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED3, ledState);
 }
 }
 if (x == 7) { //turn LED1 nad 2 on
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 digitalWrite(LED2, HIGH);
 }
 if (x == 8) { //Blink LED1 and 2
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 }
 if (x == 9) { //turn LED2 and 3 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED2, HIGH);
 digitalWrite(LED3, HIGH);
 }
 unsigned long currentMillis = millis();
 delay(100);
}
// function that executes whenever data is received from master
// this function is registered as an event, see setup()
void receiveEvent(int howMany)
{
 while (1 < Wire.available()) // loop through all but the last
 {
 char c = Wire.read(); // receive byte as a character
 Serial.print(c, BIN); // print the character as Binary
 }
 int x = Wire.read(); // receive byte as an integer
 Serial.println(x, BIN); // print the integer as Binary
}

Answer 1

You are misinterpreting how the onReceive callback works. The millis() code like in the BlinkWithoutDelay example relies on the fact, that it is run repeatedly very fast, so that effectively the time is checked regularily very fast.

The onReceive callback is only called once every time, that the Wire library received an I2C message, from an interrupt service routine. It is not called repeatedly. So the millis() code in there, will only be executed once. Since no time has passed (and millis() wouldn't increment inside an ISR either way) nothing happens there and the callback exits. After that the code is not executed further times.

You need to put the millis() code into the loop() function, so that it is executed repeatedly. In fact you can put all the if statements with x into the loop() function.

---

EDIT:

Your second version of the code should result in compilation errors (which you really should have included into your question). They will say, that x is not declared in this scope. You read the I2C data from the buffer in the onReceive callback and save it into a newly created variable x. Since x is declared inside the function, it will cease to exist right after the end of this function. The loop() function does not know about x. You need to define x globally outside of any function at the start of your sketch.

This results in this code:

// Wire Slave Receiver
#include <Wire.h> //include Library
// define Led Pins
#define LED1 3 //Pin 3
#define LED2 4 //Pin 4
#define LED3 5 //Pin 5
uint8_t x=0; // define x at global scope, so that it is available everywhere
int ledState = LOW; // ledState used to set the LED
unsigned long previousMillis = 0; // will store last time LED was updated
const long interval = 500; // interval at which to blink (milliseconds)
void setup()
{
 Wire.begin(4); // join i2c bus with address #4
 Wire.onReceive(receiveEvent); // register event
 Serial.begin(9600); // start serial for output
 //set Pin modes
 pinMode(LED1, OUTPUT);
 pinMode(LED2, OUTPUT);
 pinMode(LED3, OUTPUT);
 //intern pulldown
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
}
void loop()
{
 if (x == 1) { //turn LED1 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 Serial.println("LED1 on");
 }
 if (x == 2) { //blink LED1
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED1, ledState);
 }
 }
 if (x == 3) { //turn LED2 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, HIGH);
 delay(100);
 }
 if (x == 4) { //blink LED2
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED2, ledState);
 }
 }
 if (x == 5) { //turn LED3 on
 digitalWrite(LED2, LOW);
 digitalWrite(LED3, HIGH);
 }
 if (x == 6) { //blink LED3
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis >= interval) {
 previousMillis = currentMillis;
 if (ledState == LOW) {
 ledState = HIGH;
 } else {
 ledState = LOW;
 }
 digitalWrite(LED3, ledState);
 }
 }
 if (x == 7) { //turn LED1 nad 2 on
 digitalWrite(LED3, LOW);
 digitalWrite(LED1, HIGH);
 digitalWrite(LED2, HIGH);
 }
 if (x == 8) { //Blink LED1 and 2
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 }
 if (x == 9) { //turn LED2 and 3 on
 digitalWrite(LED1, LOW);
 digitalWrite(LED2, LOW);
 digitalWrite(LED2, HIGH);
 digitalWrite(LED3, HIGH);
 }
 unsigned long currentMillis = millis();
 delay(100);
}
// function that executes whenever data is received from master
// this function is registered as an event, see setup()
void receiveEvent(int howMany)
{
 while (1 < Wire.available()) // loop through all but the last
 {
 char c = Wire.read(); // receive byte as a character
 Serial.print(c, BIN); // print the character as Binary
 }
 x = Wire.read(); // receive byte as an integer
 Serial.println(x, BIN); // print the integer as Binary
}

Note, that I changed the type of x to uint8_t, an unsigned int of 8 bits. Restricting x to 8 bits prevents problems with writing a variable inside an ISR. The Uno has an 8-bit microcontroller, so it handles 8 bits at a time. int has 16 bits and is thus handled in two steps. An interrupt can occur at any time, so the ISR might write x in the middle of a calculation, which results in a totally garbled value of x. Handling a 1 byte/8 bits variable cannot be interrupted (also called "atomic") and prevents this issue.

• chrisl: Then add your new code to your question. Then I can help you
• Marvinm: I don't understand.
• chrisl: You wrote, that you tried to put all the if statements in the loop, but it was not working. That means, that you actually have tried it and created a new version of your code, where you've put all the if statements into the loop function. You should add this code to your question by editing it
• Marvinm: There it is. Hope that helps
• chrisl: @Marvinm I added an extra explanation.