Serial drops when ISR is attached

Projects Programming
1

Hello all,

I am developing on an ATMega 2560 (Arduino Mega) and have an issue. I have an ISR attached to Timer3 that runs every 500us to read/write to the digital I/O to emulate a parallel port host. This part works well and I'm able to send/receive data on the parallel port. Total execution time of the ISR is around 200us.

However, in the main loop of the program, I'm reading/writing to the serial port (outside the ISR, invoked in the main loop), and I share data with the ISR via a ring buffer.

My problem is when I enable the ISR, the main loop effectively stops receiving data from the USB serial line, or only does it intermittently. If I disable the ISR, it works flawlessly. Keep in mind that transmitting data after reading the ring buffer works flawlessly, and many times if I manipulate the parallel port bits to invoke code that transmits a message over USB serial, it temporarily (for a few seconds) gets the USB RX working back on the board side.

Does anyone have any input? This seems to have me stumped. I thought the ISR might be starving the main loop, but transmitting data works every time, and its handled in the main loop.

Thanks!

2

Compy:
Does anyone have any input?

Not without seeing your code.

And please use the code button </> so your code looks like this and is easy to copy to a text editor

...R

3

The sketch is rather large, so I've attempted to distill it into the meaningful bits.

#include <QueueArray.h>
#include <TimerThree.h>
#include "p2k.h"

void setup()
{
  // Initialize interrupts at 500us intervals

  Timer3.initialize(500);
  
  Serial.begin(115200);

  pinMode(13, OUTPUT);

  DDRA = B11111111;
  DDRC = B11111111;

  boot_time = millis();

  // We have not processed our HELLO packet yet, so disable all high power drives
  hello_processed = false;
}

// ISR
volatile unsigned long _isr_blink_ctr = 0;
volatile unsigned int _isr_led_state = LOW;
void ISR_rt_tasks(void) {
  unsigned long isr_begin = micros();
  // Increment interrupt counter here
  // Check to see if its time to tick the solenoids, switches or lamps.
    
  switches_handler(); // Defined this huge function down at the bottom of the post

  isr_time = micros() - isr_begin;
}



// Main loop
void loop()
{
  
  // c will hold our incoming command object
  struct P2KCommand c;
  

  // If we have data on the serial line, read it
  if (Serial.available() > 0)
  {
    
    // Only parse the command after we've received a full P2KCommand object
    if (StreamSend::receiveObject(Serial, &c, sizeof(struct P2KCommand)) == GOOD_PACKET)
    {
      if (_led_state == LOW) _led_state = HIGH;
      else _led_state = LOW;
      digitalWrite(13, _led_state);

      if (c.opcode == CMD_HELLO)
      {
        // Once we receive a HELLO command from the host, boot the driver board
        // and enable the high power relay.

        hello_processed = true;

        
        
        init_driverboard();
        
        schedule_driver((byte)DRIVER_SOLENOID, (byte)HEALTH_LED, 0xFF00FF00, (byte)0);
        last_heartbeat = millis();
        schedule_driver((byte)DRIVER_LAMP, (byte)LAMP_START_BUTTON, 0xFFFF0000, (byte)0);
        enable_lamp((byte)LAMP_COINDOOR, 1);
        Timer3.attachInterrupt(ISR_rt_tasks);

        outbound_command.int1 = 0;
        outbound_command.int2 = 0;
        outbound_command.opcode = CMD_HELLO;
        outbound_command.int3 = 0;
        outbound_command.int4 = 0;
        send_msg(outbound_command);
      }
      else if (c.opcode == CMD_HEARTBEAT)
      {
        _start_btn_state = !_start_btn_state;
        // Update the heartbeat time so that our emulated software watchdog stays alert
        last_heartbeat = millis();
        //schedule_driver((byte)DRIVER_LAMP, (byte)LAMP_START_BUTTON, 0xFF00FF00, (byte)0);

        outbound_command.int1 = 0;
        outbound_command.int2 = 0;
        outbound_command.opcode = CMD_HEARTBEAT;
        outbound_command.int3 = 0;
        outbound_command.int4 = isr_time;
        send_msg(outbound_command);
        
        
        
        
        
      }
    }
  }

    // Check if we have messages in the Queue
  
  _evtq_process_ctr = (_evtq_process_ctr + 1) % 100;
  
  // Only do this once every few thousand cycles otherwise our interrupt handling code might get crazy.
  // This snags the junk from the ring buffer written by the ISR and shoots it out over the serial line
  if (_evtq_process_ctr == 0) {
    
    if (!eventq.isEmpty()) {
      P2KCommand c = eventq.dequeue();
      c.int4 = isr_time;
      if (millis() - boot_time >= BURST_IGNORE_TIME)
        send_msg(c);
    }
  }

  // If we have not processed a HELLO from the host, don't do any other driver board stuff.
  if (hello_processed == false) return;
  
  // Has the watchdog timed out?

  
  if (millis() - last_heartbeat >= WATCHDOG_TIMEOUT)
  {
    // Blank out everything.
    p->send_data(REGISTER_SOLENOID_D, 0x0);
    p->send_data(REGISTER_SOLENOID_A, 0x0);
    p->send_data(REGISTER_SOLENOID_B, 0x0);
    p->send_data(REGISTER_SOLENOID_C, 0x0);
    p->send_data(REGISTER_SOLENOID_FLIPPER, 0x0);
    p->send_data(REGISTER_SOLENOID_LOGIC, 0x0);
    
    
    hello_processed = false;
    Timer3.detachInterrupt();
    digitalWrite(13, LOW);
  }  
}



void switches_handler()
{
  byte direct_switch_banks[] = {
    REGISTER_SWITCH_FLIPPER, REGISTER_SWITCH_MISC, REGISTER_SWITCH_COIN, REGISTER_SWITCH_ZERO,
    REGISTER_SWITCH_DIP, REGISTER_LAMP_A_DIAGNOSTIC, REGISTER_LAMP_B_DIAGNOSTIC,
    REGISTER_FUSE_A_DIAGNOSTIC, REGISTER_FUST_B_DIAGNOSTIC        };

  sw_col_val = (1 << sw_col);
  p->send_data(REGISTER_SWITCH_COLUMN, sw_col_val);
  sw_data = p->receive_data(REGISTER_SWITCH_ROW);

    for (sw_row = 0; sw_row < 8; sw_row++)
    {
      sw_bit = 1 << sw_row;
      sw_sw = (sw_col << 4) + sw_row;
      
      // Is the change bit set for this and is the switch state the same as the last time, then fire a debounce event
      // and unset the change bit
      if ((sw_bit & switch_matrix_changing[sw_col]) && (sw_bit & sw_data) == (sw_bit & switch_matrix[sw_col]))
      {
        switch_matrix_changing[sw_col] -= sw_bit;
        broadcast_event(SWITCH_MATRIX, sw_sw, (sw_bit & sw_data) != 0, 1);
      }
    
      
      // The state has changed
      // If the change bit is set for this, and the state has changed, unset the change bit
      // If the change bit is set for this, and the state has NOT changed, fire a debounce event
      // If the change bit is not set for this, set the change bit
      if ((sw_bit & sw_data) != (sw_bit & switch_matrix[sw_col]))
      {
        
        
        // Is the change bit set? The state has also changed
        if ((sw_bit & switch_matrix_changing[sw_col]))
        {
          // Unset the change bit
          switch_matrix_changing[sw_col] -= sw_bit;
        }
        else
        {
          // Set the change bit
          switch_matrix_changing[sw_col] += sw_bit;
        }

        if ((sw_bit & sw_data) == 0) switch_matrix[sw_col] -= sw_bit;
        else switch_matrix[sw_col] += sw_bit;
        broadcast_event(SWITCH_MATRIX, sw_sw, (sw_bit & sw_data) != 0, 0);
      }
    }

  // Direct switches -- Only check every few columns
  if (sw_col % 2 == 0)
  {
    sw_bank = direct_switch_banks[sw_bank_cnt];
    sw_data = p->receive_data(sw_bank);
    // Check if any switches have changed since last time
      for (sw_switches = 0; sw_switches < 8; sw_switches++)
      {
        sw_bit = 1 << sw_switches;
        
        sw_sw = (sw_bank << 4) + sw_switches;
        
        // Is the change bit set for this and is the switch state the same as the last time, then fire a debounce event
        // and unset the change bit
        if ((sw_bit & direct_switches_changing[sw_bank]) && (sw_bit & sw_data) == (sw_bit & direct_switches[sw_bank]))
        {
          direct_switches_changing[sw_bank] -= sw_bit;
          broadcast_event(SWITCH_DIRECT, sw_sw, (sw_bit & sw_data) != 0, 1);
        }

        if ((sw_bit & sw_data) != (sw_bit & direct_switches[sw_bank]))
        {          
          
          // Is the change bit set? The state has also changed
          if ((sw_bit & direct_switches_changing[sw_bank]))
          {
            // Unset the change bit
            direct_switches_changing[sw_bank] -= sw_bit;
          }
          else
          {
            // Set the change bit
            direct_switches_changing[sw_bank] += sw_bit;
          }
          
          if ((sw_bit & sw_data) == 0) direct_switches[sw_bank] -= sw_bit;
          else direct_switches[sw_bank] += sw_bit;

          broadcast_event(SWITCH_DIRECT, sw_sw, (sw_bit & sw_data) != 0, 0);
        }
      }
    sw_bank_cnt = ++sw_bank_cnt % 2;
  }
  sw_col = ++sw_col % 8;
}
4

Compy:
The sketch is rather large, so I've attempted to distill it into the meaningful bits.

No, upload it as an attachment. How can you know what is meaningful, when it doesn't work?

5

Is there any particular reason that the chip should spend nearly half the time executing one interrupt? That is generally considered to be bad design, and is guaranteed to cause problems with other interrupts.

7

Attached is the full sketch! I appreciate all of the input.

Delta_G:
Your switches_handler function seems like an awful lot to do inside an ISR. What is p that is sending and receiving data? It's not just Serial that won't work in an ISR. Anything interrupt driven will fail. What about broadcast_event? What does that do?

The application here is a pinball machine. broadcast_event checks to see if there are any switch event listeners when a switch event is read from the parallel port. This basically ensures that in the event you press a flipper button the coil will immediately fire rather than waiting for a roundtrip message from the USB host which actually houses the game rules and other logic.

p is the simple parallel port emulator that I wrote that sends data using a subset of the digital pins on the arduino. The board that the arduino connects to expects to be controlled with a typical parallel port. The arduino replaces the upstream portion and handles the real time control of the driver logic for the machine such as strobing the lamp and switch matrices.

jremington:
Is there any particular reason that the chip should spend nearly half the time executing one interrupt? That is generally considered to be bad design, and is guaranteed to cause problems with other interrupts.

I get that, and I am working on crunching things down so that the switch and lamp matrix code only does one single strobe rather than the entire matrix (8 columns, 8 rows. 8 strobes total).

9

Delta_G:
If you're reading switches on a pinball game then there is not really any need for interrupts. Just keep the loop nice and tight and it should be able to poll all those switches in a matter of a few tens of microseconds.

So a bit of history. I did have everything working nicely just by running in the main loop and using micros() to only poll everything every ms or so. The issue actually came when there would be a burst of switch data for the serial port and it would try to cram everything down the line. I suppose I could roll back the code and try sending only one message in the queue per loop iteration.

Hmmm! That could work! Thanks for the heads up :slight_smile:

10

ITS ALIVE!

So yeah, just buffering up my serial messages and processing one send per loop actually fixed the issue. Now lots of switch events don't cause missed timing intervals and flicker in the general illumination bulbs!

Thank you all for your help!

Here it is in action: