HardwareSerial::write() and other HardwareSerial functions locking up..

[curtis63 5]

There have been a few situations where I've had HardwareSerial::write() lock up on me and end up in an infinite while loop.

 

Could some changes be made to any and all possibly infinite while(); loops?

 

I think it would just take some sort of counter that indicates how many iterations through the while(); loop have happened.  Then if some upper limit is reached, exit with a fail.

 

Let's make it bullet proof as possible.

 

 

[spirilis 1,265]

That can happen if the TX queue is full. It continues until a byte entry frees (via UART TX IRQ firing and getting serviced) thus allowing write to proceed. Under what circumstance are you experiencing this? If you are using write() inside an Interrupt callback, do not do this, it's an unsafe operation that can lock up your firmware.

[curtis63 5]

So, I've been experimenting with sleep, sleepSeconds, suspend() and wakeup.

 

These are AWESOME by the way...

 

Here's what I've found.

 

suspend() puts the controller into LPM4 which makes it ONLY come back to life after a hardware interrupt is properly handled.

 

sleep and sleepSeconds() put the controller into LPM3 which allows it to wake up when the timeout specified in the sleep() or sleepSeconds() calls expires.  You can ALSO wake it up from LPM3 with a hardware interrupt and call wakeup() inside the hardware interrupt handler.

 

So, when we are in LPM3 or LPM4 modes, we are not supposed to use delay() or Serial.print() commands.  It has been said on the forums that you need to be sure to call Serial.flush() after a Serial.print() if you plan on going into LPM3 or LPM4 modes.  

 

My problem is that even when I do call Serial.flush(), I sometimes have my controller locking up inside the HardwareSerial::write() function:

 

   

 size_t HardwareSerial::write(uint8_t c)

    {

     unsigned int i = (_tx_buffer->head + 1) % SERIAL_BUFFER_SIZE;

     

     // If the output buffer is full, there's nothing for it other than to

     // wait for the interrupt handler to empty it a bit

     // ???: return 0 here instead?

     unsigned int giveup = 0;

     

     while (i == _tx_buffer->tail);

    

     _tx_buffer->buffer[_tx_buffer->head] = c;

     _tx_buffer->head = i;

    

    #if defined(__MSP430_HAS_USCI_A0__) || defined(__MSP430_HAS_USCI_A1__) || defined(__MSP430_HAS_EUSCI_A0__) || defined(__MSP430_HAS_EUSCI_A1__)

     *(&(UCAxIE) + uartOffset) |= UCTXIE;

    #else

     *(&(UC0IE) + uartOffset) |= UCA0TXIE;

    #endif 

    

     return 1;

    }

 

The while loop above becomes an infinite loop.  So, is there any way we can perhaps fix this loop so that we don't have it be infinite.  I think that it should just timeout after 2-3 seconds or give up after a certain number of iterations and return 0...  Then, we aren't locked up.  I'd rather have some text undelivered than have my program lock up...

 

I've noticed that the lock up occurs when I have a call to Serial.println() immediately after my call to wakeup(), or if I call Serial.println() immediately after a call to sleepSeconds().  

 

So, any advice on how to avoid locking up when using sleep, sleepSeconds, wakeup, and Serial.println near each other would be greatly appreciated.

[spirilis 1,265]

 

So, I've been experimenting with sleep, sleepSeconds, suspend() and wakeup.

 

These are AWESOME by the way...

 

Here's what I've found.

 

suspend() puts the controller into LPM4 which makes it ONLY come back to life after a hardware interrupt is properly handled.

 

sleep and sleepSeconds() put the controller into LPM3 which allows it to wake up when the timeout specified in the sleep() or sleepSeconds() calls expires.  You can ALSO wake it up from LPM3 with a hardware interrupt and call wakeup() inside the hardware interrupt handler.

 

So, when we are in LPM3 or LPM4 modes, we are not supposed to use delay() or Serial.print() commands.  It has been said on the forums that you need to be sure to call Serial.flush() after a Serial.print() if you plan on going into LPM3 or LPM4 modes.  

 

My problem is that even when I do call Serial.flush(), I sometimes have my controller locking up inside the HardwareSerial::write() function:

 

   

 size_t HardwareSerial::write(uint8_t c)
    {
     unsigned int i = (_tx_buffer->head + 1) % SERIAL_BUFFER_SIZE;
     
     // If the output buffer is full, there's nothing for it other than to
     // wait for the interrupt handler to empty it a bit
     // ???: return 0 here instead?
     unsigned int giveup = 0;
     
     while (i == _tx_buffer->tail);
    
     _tx_buffer->buffer[_tx_buffer->head] = c;
     _tx_buffer->head = i;
    
    #if defined(__MSP430_HAS_USCI_A0__) || defined(__MSP430_HAS_USCI_A1__) || defined(__MSP430_HAS_EUSCI_A0__) || defined(__MSP430_HAS_EUSCI_A1__)
     *(&(UCAxIE) + uartOffset) |= UCTXIE;
    #else
     *(&(UC0IE) + uartOffset) |= UCA0TXIE;
    #endif 
    
     return 1;
    }

 

The while loop above becomes an infinite loop.  So, is there any way we can perhaps fix this loop so that we don't have it be infinite.  I think that it should just timeout after 2-3 seconds or give up after a certain number of iterations and return 0...  Then, we aren't locked up.  I'd rather have some text undelivered than have my program lock up...

 

I've noticed that the lock up occurs when I have a call to Serial.println() immediately after my call to wakeup(), or if I call Serial.println() immediately after a call to sleepSeconds().  

 

So, any advice on how to avoid locking up when using sleep, sleepSeconds, wakeup, and Serial.println near each other would be greatly appreciated.

 

 

"I've noticed that the lock up occurs when I have a call to Serial.println() immediately after my call to wakeup()"

 

Serial commands should not be run inside ISRs!  ISRs are the only place where you run wakeup().

 

A sleepSeconds() command (run from main execution context, i.e. not inside an ISR) shouldn't cause trouble with a Serial.println running afterward.

 

Can you post some example sketches where you've run into problems?

 

While I do generally agree one should not lock up with an infinite while loop, the implementation is quite space-sensitive (as the G2553 is pretty constrained on memory) so the additional code space to implement a loop there will probably not be welcome.  Maybe not that big a deal to add, but still, a counter will probably be more complex to implement than you might imagine while ensuring valid use-cases don't drop bytes on pending output at low baud rates.

[curtis63 5]

You were exactly right.  I'm fine with Serial.print in the loop, but NOT in the Button Press Handler (handlePress()) method.

 

I will now keep all Serial.print statements out of the interrupt handlers..

 

Here's my code.  I have commented out the offending line of code, so the code as it stands works just fine as fast as you want to click the button, however, unclick the commented out line in the handlePress() method, and it will lock up:

#include <wire.h>

int count=0;

void setup() {
 Serial.begin(9600);
 while(Serial.available());
 Serial.flush();
 Wire.begin();

 pinMode(P1_3, INPUT_PULLUP);
 attachInterrupt(P1_3, handlePush, FALLING);
}

void loop()
{
count++;
Serial.print("I'm still alive...  ");
Serial.println(count);
sleepSeconds(2);
}

void handlePush()
{
wakeup();
// Serial.println("Button Pressed !!!");
}

[roadrunner84 466]

As @@spirilis mentioned, you should never run scheduling dependent code inside an ISR. handlePush() is an ISR, println() is a scheduling dependent call, so no go there. Instead, wake from sleep and do the printing in your loop().

#include <wire.h>

int count=0;
bool button_pressed = false;

void setup() {
  Serial.begin(9600);
  while(Serial.available());
  Serial.flush();
  Wire.begin();

  pinMode(P1_3, INPUT_PULLUP);
  attachInterrupt(P1_3, handlePush, FALLING);
}

void loop()
{
  count++;
  Serial.print("I'm still alive...  ");
  Serial.println(count);
  sleepSeconds(2);
  if (button_pressed)
  {
    button_pressed = false;
    Serial.println("Button Pressed !!!");
  }
}

void handlePush()
{
  button_pressed = true;
  wakeup();
}