Led while pressing Serial.read() not at maximum light

Hi guys, i've a problem with a little but curious task.
I'm trying to turn on a led (on pin12) using Serial.read() it turn on while it receiving the char choosed but it's light it's not at maximum, here is the code:

    if (incomingByte == 'H')
      digitalWrite(ledPin, HIGH);
    else 
      digitalWrite(ledPin, LOW);

    if (incomingByte == 'L') {
      digitalWrite(ledPin, LOW);
    }

maybe because there is not enough time to allow it to turn at full lights and stay a little bit on?
cause i made some test with BlinkWithoutDelay but as i don't found the right way it remains or always ON or OFF, is this the right direction? if so, this other code:

unsigned long currentMillis = millis();

    incomingByte = Serial.read();           // read the oldest byte in the serial buffer
  if ( incomingByte == 'H' ) {

    if((currentMillis - previousMillis) >= interval) {
      previousMillis = currentMillis;
    
      digitalWrite(ledPin, HIGH);
    }  else
      digitalWrite(ledPin, LOW);
  }

Please post ALL your code if you want to know what you are doing wrong.

i’ve notice that if i send via Serial Monitor the char H one time, it turn on the led and stay on instead if i send the char many times (ex: HHHHHHHHHH) it blink but turn off instantly but not at full light as i would (and as it must be normally i think)

TX (it’s a simple controller that send the char H via Serial.print)

#include <PS3BT.h>
#include <usbhub.h>

#ifdef dobogusinclude
#include <spi4teensy3.h>
#include <SPI.h>
#endif

USB Usb;

BTD Btd(&Usb); // You have to create the Bluetooth Dongle instance like so
PS3BT PS3(&Btd); // This will just create the instance

const uint8_t LED_tx = 2;

void setup() {
  pinMode(LED_tx, OUTPUT);

  Serial.begin(57600);
#if !defined(__MIPSEL__)
  while (!Serial); // Wait for serial port to connect - used on Leonardo, Teensy and other boards with built-in USB CDC serial connection
#endif
  if (Usb.Init() == -1) {
    Serial.print(F("\r\nOSC did not start"));
    while (1); //halt
  }
  Serial.print(F("\r\nPS3 Bluetooth Library Started"));

}
void loop() {
  Usb.Task();


  if (PS3.PS3Connected || PS3.PS3NavigationConnected) {

    if (PS3.getButtonClick(PS)) {
      Serial.print(F("\r\nPS"));
      PS3.disconnect();
    }
    else {
      if (PS3.getButtonClick(TRIANGLE))
        Serial.print("\r\nH");
      if (PS3.getButtonPress(CIRCLE))
        Serial.print(F("\r\nH"));
        
      if (PS3.PS3Connected)
        digitalWrite(LED_tx, PS3.getButtonPress(CROSS));
      else
        digitalWrite(LED_tx, LOW);
        
      if (PS3.getButtonClick(SQUARE))
        Serial.print(F("\r\ndigitalWrite(ledPin, HIGH);"));

    }

  }
}

RX

unsigned long previousMillis = 0;        // will store last time LED was updated

// constants won't change :
const long interval = 1000;           // interval at which to blink (milliseconds)


const int ledPin = 12;                      // the pin that the LED is attached to
int incomingByte;                           // a variable to read incoming serial data into

void setup() {
  Serial.begin(57600);                      // initialize serial communication
  pinMode(ledPin, OUTPUT);                  // initialize the LED pin as an output
}

void loop() {
  if (Serial.available() > 0) {             // see if there's incoming serial data:

unsigned long currentMillis = millis();

    incomingByte = Serial.read();           // read the oldest byte in the serial buffer
  if ( incomingByte == 'H' ) {

    if((currentMillis - previousMillis) >= interval) {
      previousMillis = currentMillis;
   
      digitalWrite(ledPin, HIGH);
    }  else
      digitalWrite(ledPin, LOW);
  }

  }
}

It looks not as bright because once the LED is turned on you almost immediately turn it off. This means it is flashing very fast and so never looks like it it full brightness.

Your problems are the incorrect use of the else clause, which will turn off the light before the time interval has expired.

but it should’t interpret it in that way, that when it receiving the char “H” it turn on the led, and after it check if the interval has expired it turn it off, otherwise it turn it on, not?

anyway i’ve also tried to put the Else in the IF IncomingByte, but it still wrong somewhere

unsigned long previousMillis = 0;        // will store last time LED was updated

// constants won't change :
const long interval = 500;           // interval at which to blink (milliseconds)


const int ledPin = 12;                      // the pin that the LED is attached to
int incomingByte;                           // a variable to read incoming serial data into

void setup() {
  Serial.begin(57600);                      // initialize serial communication
  pinMode(ledPin, OUTPUT);                  // initialize the LED pin as an output
}

void loop() {
  if (Serial.available() > 0) {             // see if there's incoming serial data:

unsigned long currentMillis = millis();

    incomingByte = Serial.read();           // read the oldest byte in the serial buffer
  if ( incomingByte == 'H' ) {

      digitalWrite(ledPin, HIGH);


    if((currentMillis - previousMillis) <= interval) {
    previousMillis = currentMillis;
   
        digitalWrite(ledPin, LOW);
    }  
    
  }

  }
}

but it should’t interpret it in that way,

But it is, it interpenetrates it like you have written it. You have written it incorrectly.

In that last code the section:-

if((currentMillis - previousMillis) <= interval) {
    previousMillis = currentMillis;
   
        digitalWrite(ledPin, LOW);
    }

To get to that statement you have to have received something and set the currentMillis variable, then if that something was a ‘H’ the turned the LED and immediately tested if the currentMillis minus the previousMillis is less than 500mS. As the first time previousMillis was a zero then the if statement is satisfied and the LED is turned off IMMEDIATELY after turning it on.
If you wait longer than half a second between sending successive ‘H’ then the LED will never be turned off.

Normally this is written as
((currentMillis - previousMillis) >= interval)
so that when the interval is passed, the next interval gets set up with
previousMillis = currentMillis + interval;
and your action occurs.

    if((currentMillis - previousMillis) >= interval) {
    previousMillis = currentMillis;
   
        digitalWrite(ledPin, LOW);
    }

Was wrong comparison!


But not the only error.

I will give you a chance to figure it out.

the closest result that i archived was to set it in this way:

  if ( incomingByte == 'H' ) {

      digitalWrite(ledPin, HIGH);


    if((currentMillis - previousMillis) >= interval) {
    previousMillis = currentMillis + interval;
   
        digitalWrite(ledPin, LOW);
    }  
    
  }

And the interval as 5ms, but it instead to stay fixed with light it continue simply to blink very quickly, and sometimes happen that stay on even when i release the H button

You are going about things backwards. Your code must:-

  1. check if the LED is on and if the time it has been on has expired - if it has then turn the LED off.
  2. check if new data has arrived in the serial port - if it has read it AND if this is a 'H' then turn the LED on and set the previousMillis value to the current millis by
previousMillis = millis();

still somethiing wrong, it turn on but at 40% of light ;(

Did you mean in this way?

  if (Serial.available() > 0) {             // see if there's incoming serial data:

unsigned long currentMillis = millis();

    incomingByte = Serial.read();           // read the oldest byte in the serial buffer

  if ( ledPin == HIGH && (currentMillis - previousMillis) >= interval);
  else digitalWrite(ledPin, LOW);

  if ( incomingByte == 'H' ) {

      digitalWrite(ledPin, HIGH);

      previousMillis = currentMillis;
   
  }

  }

Did you mean in this way?

No, read reply #9 again.

OK this is not going to happen. This is what I was trying to get you to do:-

unsigned long previousMillis = 0;        // will store last time LED was updated

// constants won't change :
const long interval = 500;           // interval at which to blink (milliseconds)

const int ledPin = 12;                      // the pin that the LED is attached to
int incomingByte;                           // a variable to read incoming serial data into
boolean LEDisOn = false;

void setup() {
  Serial.begin(57600);                      // initialize serial communication
  pinMode(ledPin, OUTPUT);                  // initialize the LED pin as an output
}

void loop() {
// stage 1 is the LED on if it is and it has been on long enough turn it off
      if((millis() - previousMillis) > interval && LEDisOn) {
        LEDisOn = false;  
        digitalWrite(ledPin, LOW);
    } 
// stage 2 look at the serial input
  if (Serial.available() > 0) {             // see if there's incoming serial data:
    incomingByte = Serial.read();           // read the oldest byte in the serial buffer
     if ( incomingByte == 'H' ) {
         digitalWrite(ledPin, HIGH);
         previousMillis = millis();
         LEDisOn = true;
      }
  } // end of if Serial.available
}  // end of loop function

This will keep the LED on for half a second when you get a 'H'. Change the "interval" to change the on time.

Yesss, that worked like a charm! yeah, now i've understand what did you mean :slight_smile:
Thank you guys, especially to you Mike!!!

hey guys, i'm still here, i'm just trying to add a feature to this this sketch but this other feature interferes with the one that we archieved above.

What i'm trying to do now, is to add a button hold detection by the char received in Serial.read.
for example, now
if i press one time the char H the led will turn off after 10ms
if i keep hold on H the led will stay on till i push the char H
but now i would to leave it ON if i keep it pushed for 2 sec, i've try all the afternoon but without success, this is what i tried:

// constants won't change :
const long interval = 10;					// interval at which to blink (milliseconds)
const int ledPin = 12;                      // the pin that the LED is attached to
const int intervalHOLD = 2000;

// Variables will change :
unsigned long previousMillis1, previousMillis2;			// will store last time LEDs was updated
boolean led1isOn = false, led1isOnHOLD = false, led2isOn = false;
char incomingByte = 0;						// a variable to read incoming serial data into

void setup() {
  Serial.begin(57600);                      // initialize serial communication
  pinMode(ledPin, OUTPUT);                  // initialize the LED pin as an output
}

void loop() {

	if((millis() - previousMillis1) > interval && led1isOn) {		// stage 1 is the LED on if it is and it has been on long enough turn it off
		led1isOn = false; 
		digitalWrite(ledPin, LOW);
	}

	if (Serial.available() > 0) {			// see if there's incoming serial data:
	//	delay(1);							//small delay to allow input buffer to fill
	incomingByte = Serial.read();           // read the oldest byte in the serial buffer

		if ( incomingByte == 'H' ){
			while ( incomingByte == 'H' && (millis() - previousMillis1) >= intervalHOLD ){
			Serial.println("led1isOnHOLD");
			}
			digitalWrite(ledPin, HIGH);
			previousMillis1 = millis();
			led1isOn = true;
			led1isOnHOLD = true;
		}
  }											// end of if Serial.available
}											// end of loop function

if i keep hold on H the led will stay on till i push the char H

I am not understanding this. The H button is actually a key on the keyboard is it not? Therefore the number of times you send a character when the key is held down is determined by your settings in your PC. There is an initial delay ( settable ) before the first repeat and after that there is a delay between repeats also settable. Therefore you do not know how many characters you are going to receive when you hold this key down for 2 seconds.

but now i would to leave it ON if i keep it pushed for 2 sec,

OK so how is it going to ever turn off?

this is what i tried:

I can't see much difference between that code and the code I gave you.

This problem is much more complicated than the previous one and requires you to know the time between characters sent. You need to implement what is called a missing pulse detector. This is where you look at how long it has been since you received a character and if it exceeds the repetition rate of your keyboard then you start timing again. Only when this timer exceeds two seconds can you set a flag which you then use to inhibit the turn off portion of the code.

I would suggest this is not the path to go on if you are learning stuff as it is much too complex for you to implement with your current state of knowledge.

Grumpy_Mike:
I am not understanding this. The H button is actually a key on the keyboard is it not? Therefore the number of times you send a character when the key is held down is determined by your settings in your PC.

The char is received by pressing a buttn on a contrler on the first arduino.

Grumpy_Mike:
OK so how is it going to ever turn off?

it will going to turn off it when the controller will send again the char

I tried to take a look to what you suggest me about the missing pulse detector but it seems to interfere with the Usb.Task(); infact it don't want to connect when add that little test code, is there some other alternative i may try?

about the missing pulse detector but it seems to interfere with the Usb.Task();

There is no reason why it should.

infact it don't want to connect when add that little test code,

What code. The way this forum works is that you keep posting code when you have changed it and it is not doing what you want. You say what it does and what you want it to do.

after some test it seems that the problem start when i add the line:
duration = pulseIn(button, HIGH);
if i remove it the controller can back to work fine again, if i leave the line sometimes it can connect with the arduino but doesn’t respond.

this is the TX code:

/*
 Example sketch for the PS3 Bluetooth library - developed by Kristian Lauszus
 For more information visit my blog: http://blog.tkjelectronics.dk/ or
 send me an e-mail:  kristianl@tkjelectronics.com
 */

#include <PS3BT.h>
#include <usbhub.h>

// Satisfy the IDE, which needs to see the include statment in the ino too.
#ifdef dobogusinclude
#include <spi4teensy3.h>
#include <SPI.h>
#endif

USB Usb;
//USBHub Hub1(&Usb); // Some dongles have a hub inside

BTD Btd(&Usb); // You have to create the Bluetooth Dongle instance like so
/* You can create the instance of the class in two ways */
PS3BT PS3(&Btd); // This will just create the instance
//PS3BT PS3(&Btd, 0x00, 0x15, 0x83, 0x3D, 0x0A, 0x57); // This will also store the bluetooth address - this can be obtained from the dongle when running the sketch

bool printTemperature;
bool printAngle;
const uint8_t LED_tx = 2;
const uint8_t LED_tx2 = 12;

char dataPacket[64];

int Servo = 90; // 2 char

const int button = (PS3.getButtonClick(TRIANGLE));
unsigned long duration;

void setup() {
  pinMode(LED_tx, OUTPUT);
  pinMode(LED_tx2, OUTPUT);
  pinMode(button, INPUT);
  
  Serial.begin(57600);
#if !defined(__MIPSEL__)
  while (!Serial); // Wait for serial port to connect - used on Leonardo, Teensy and other boards with built-in USB CDC serial connection
#endif
  if (Usb.Init() == -1) {
    Serial.print(F("\r\nOSC did not start"));
    while (1); //halt
  }
  Serial.print(F("\r\nHello CAR, i'm ready"));
  Serial.print(F("\r\nPS3 Bluetooth Library Started"));

}
void loop() {
  Usb.Task();

  if(PS3.PS3Connected || PS3.PS3NavigationConnected) {
	duration = pulseIn(button, HIGH);
  }
  
	if ( duration > 0 ) {
		Serial.print(duration);
	}



  if (PS3.PS3Connected || PS3.PS3NavigationConnected) {

    if (PS3.getButtonClick(PS)) {
      Serial.print(F("\r\nPS"));
      PS3.disconnect();
    }
    else {
      if (PS3.getButtonClick(TRIANGLE))
        Serial.print("\r\nH");
      if (PS3.getButtonPress(CIRCLE))
        Serial.print(F("\r\nH"));
        
      if (PS3.PS3Connected)
        digitalWrite(LED_tx, PS3.getButtonPress(CROSS));
      else
        digitalWrite(LED_tx, LOW);
        
      if (PS3.getButtonPress(SQUARE))
        digitalWrite(LED_tx2, HIGH);					//	Serial.print(F("\r\ndigitalWrite(ledPin, HIGH);"));
      else
        digitalWrite(LED_tx2, LOW);
		
      if (PS3.getButtonClick(UP)) {
        Serial.print(F("\r\nUp"));
        if (PS3.PS3Connected) {
          PS3.setLedOff();
          PS3.setLedOn(LED4);
        }
      }
      if (PS3.getButtonClick(RIGHT)) {
        Serial.print(F("\r\nRight"));
        if (PS3.PS3Connected) {
          PS3.setLedOff();
          PS3.setLedOn(LED1);
        }
      }
      if (PS3.getButtonClick(DOWN)) {
        Serial.print(F("\r\nDown"));
        if (PS3.PS3Connected) {
          PS3.setLedOff();
          PS3.setLedOn(LED2);
        }
      }
      if (PS3.getButtonClick(LEFT)) {
        Serial.print(F("\r\nLeft"));
        if (PS3.PS3Connected) {
          PS3.setLedOff();
          PS3.setLedOn(LED3);
        }
      }

      if (PS3.getButtonPress(L1)) {
        Serial.print(F("L"));
		analogWrite(3000, 10);
      } else if (PS3.getButtonPress(R1)) {
        Serial.print(F("R"));
		analogWrite(3000, 10);
	  } else
	    analogWrite(3000, 0);
		
      if (PS3.getButtonClick(L3))
        Serial.print(F("\r\nL3"));
      if (PS3.getButtonClick(R3))
        Serial.print(F("\r\nR3"));

      if (PS3.getButtonClick(SELECT)) {
        Serial.print(F("\r\nSelect - "));
        PS3.printStatusString();
      }
      if (PS3.getButtonClick(START)) {
        Serial.print(F("\r\nStart"));
        printAngle = !printAngle;
      }
    }
#if 0 // Set this to 1 in order to see the angle of the controller
    if (printAngle) {
      Serial.print(F("\r\nPitch: "));
      Serial.print(PS3.getAngle(Pitch));
      Serial.print(F("\tRoll: "));
      Serial.print(PS3.getAngle(Roll));
    }
#endif
  }
}

As you can see the Button it’s not an hardware button but i define one button of the controller to be used as button as his dev say:
“To check if a button is pressed simply use the function getButtonPress: USB Host Shield 2.0: PS3BT Class Reference”.

But there’s something wrong with that:
const int buttonPin = (PS3.getButtonClick(TRIANGLE));
infact even with other simple code like this:
buttonState = digitalRead(button);
to read simply his value, the controller give some problem of responding till i don’t remove the part, instead if i use it just in this way:
if (PS3.getButtonPress(CIRCLE))
Serial.print((PS3.getButtonPress(CIRCLE)));
it continue to return 1 as long as i press it without problems.

after some test it seems that the problem start when i add the line:
pulseIn(button, HIGH);

This function is like the delay, it is a blocking function. While waiting for the pulse to start and for the duration of the pulse nothing else can happen. This is why it is stopping other stuff from working correctly because it is not getting polled often enough.

Do you actually need this? What sort of time accuracy do you need?

In a similar way as the blink without delay was written you can write a "measure pulse without pulseIn" sketch. It is different from other state machine function in that you check the state of the input every pole, not the time, when you see it go high you make a note of the current time with millis() and set a "timing underway" flag. Then, next time when you check it and the input is LOW and the "timing underway" flag is set you can work out how long it was by subtracting the time at the start of the pulse from the current time - that is the value returned by millis(). At this point you should also clear the "timing underway" flag.

This will only give you the time of the pulse in milliseconds, if you want better then you will have to use the microseconds timer, but that only has 4 uS resolution.