Attaching interrupt kills main loop

So I'm working on a device that takes in mouse and keyboard commands. With this I'm using the ps2.h, and PS2Mouse.h libraries. From the ps2.h library I erased all of the mouse information since the other gives me the functionality I want. The keyboard library also allows me to test for up and down keystrokes.

The problem is with the interrupt for the keyboard. If it is setup, then the main loop will not run at all, even if the actual function for the interrupt isn't running either. While interrupt is for the keyboard, the mouse should be polled in the main loop. So basically I am only getting the keyboard, and not the mouse. Any ideas?

Here is the code. the 'hmmms' are debug prints to see if the loop and function are running at the right times.

Thanks for helping!

#include <PS2Mouse.h>
#include <ps2.h>


#define MOUSE_DATA 12
#define MOUSE_CLOCK 13

PS2Mouse mouse(MOUSE_CLOCK, MOUSE_DATA, STREAM);

int ledPin = 9;    // LED connected to digital pin 9
int ledPin2 = 10;
int ledPin3 = 3;
int ledPin4 = 11;
int ltrigger = 5;
PS2 kbd(2, 7);

void setup()  { 
    Serial.begin(9600);
  
  analogWrite(ledPin, 80);
  analogWrite(ledPin2, 80);
  analogWrite(ledPin3, 80);
  analogWrite(ledPin4, 80);
  mouse.initialize();
   
  char ack;
  kbd.write(0xff);  // send reset code
  ack = kbd.read();  // byte, kbd does self test
  ack = kbd.read();  // another ack when self test is done
  
  attachInterrupt(0, kbInterrupt, LOW);
} 

void loop()  { 

  Serial.println("hmmmmm");

  int data[2];
  mouse.report(data);

  if (data[0] == 9)
  {
    analogWrite(ltrigger, 60);
  }
  else
  { 
    analogWrite(ltrigger, 250);
  }

  if (data[1] > 0 )
  {
    analogWrite(ledPin4, 160);

  }
  else if (data[1] < 0 )
  {
    analogWrite(ledPin4, 0);

  }  
  else
  {
    analogWrite(ledPin4, 80);
  }


  if (data[2] > 0 )
  {
    analogWrite(ledPin3, 160);

  }
  else if (data[2] < 0 )
  {
    analogWrite(ledPin3, 0);

  }  
  else
  {
    analogWrite(ledPin3, 80);
  }

  Serial.print(data[0]); // Status Byte
  Serial.print(":");
  Serial.print(data[1]); // X Movement Data
  Serial.print(",");
  Serial.print(data[2]); // Y Movement Data
  Serial.println();
  interrupts();
}

void kbInterrupt() {

  unsigned char code;
  /* read a keycode */
  code = kbd.read();
  /* send the data back up */

  delay(10);  /* twiddle */

  int coded = code;

  if (coded == 29){
    Serial.println("W-d");
    analogWrite(ledPin2, 160);
  }

  if (coded == 240){
    unsigned char code1;
    code1 = kbd.read();

    int coded1 = code1;
    if (coded1 == 29){
      analogWrite(ledPin2, 90);
      Serial.println("W-u");
    }
  }
  else{
    Serial.println("hmmm");
  }
}
  attachInterrupt(0, kbInterrupt, LOW);

You almost certainly don't want a LOW interrupt because that fires continuously. I would suggest FALLING.

  delay(10);  /* twiddle */

The delay() function does not work properly in an interrupt service routine.

Thanks a lot for the reply. Meant to get back to this last night, but the internet went down for some reason.

Nick, I tried both LOW and FALLING, but I get the same results both ways, and the delay was an accident (Good catch! heh heh, but no change)

It is looking like even though I did put it inside of an interrupt that detects a keypress, the 'kbd.read();' function still takes over the program because it is waiting for something. It does get the right keys when pressed rather responsively, but it gets stuck regardless. I would use a different keyboard library that had an 'available' function, but the only one I found does not detect up and down strokes, only the key that was pressed. any ideas?

Here is the ps2.cpp with the read function in it if you can help.

/*
 * ps2.cpp - an interface library for ps2 devices.  Common devices are
 * mice, keyboard, barcode scanners etc.  See the examples for mouse and
 * keyboard interfacing.
 * limitations:
 *      we do not handle parity errors.
 *      The timing constants are hard coded from the spec. Data rate is
 *         not impressive.
 *      probably lots of room for optimization.
 *
 * PS2Mouse class by Jonathan Laloz - http://thepotterproject.com
 * Released into the public domain
 *
 */

#include "ps2.h"

/*
 * the clock and data pins can be wired directly to the clk and data pins
 * of the PS2 connector.  No external parts are needed.
 */
PS2::PS2(int clk, int data)
{
	_ps2clk = clk;
	_ps2data = data;
	gohi(_ps2clk);
	gohi(_ps2data);
}

/*
 * according to some code I saw, these functions will
 * correctly set the clock and data pins for
 * various conditions.  It's done this way so you don't need
 * pullup resistors.
 */
void
PS2::gohi(int pin)
{
	pinMode(pin, INPUT);
	digitalWrite(pin, HIGH);
}

void
PS2::golo(int pin)
{
	pinMode(pin, OUTPUT);
	digitalWrite(pin, LOW);
}

/* write a byte to the PS2 device */
void
PS2::write(unsigned char data)
{
	unsigned char i;
	unsigned char parity = 1;
	
	gohi(_ps2data);
	gohi(_ps2clk);
	delayMicroseconds(300);
	golo(_ps2clk);
	delayMicroseconds(300);
	golo(_ps2data);
	delayMicroseconds(10);
	gohi(_ps2clk);	// start bit
	/* wait for device to take control of clock */
	while (digitalRead(_ps2clk) == HIGH)
		;	// this loop intentionally left blank
	// clear to send data
	for (i=0; i < 8; i++)
	{
		if (data & 0x01)
		{
			gohi(_ps2data);
		} else {
			golo(_ps2data);
		}
		// wait for clock
		while (digitalRead(_ps2clk) == LOW)
			;
		while (digitalRead(_ps2clk) == HIGH)
			;
		parity = parity ^ (data & 0x01);
		data = data >> 1;
	}
	// parity bit
	if (parity)
	{
		gohi(_ps2data);
	} else {
		golo(_ps2data);
	}
	// clock cycle - like ack.
	while (digitalRead(_ps2clk) == LOW)
		;
	while (digitalRead(_ps2clk) == HIGH)
		;
	// stop bit
	gohi(_ps2data);
	delayMicroseconds(50);
	while (digitalRead(_ps2clk) == HIGH)
		;
	// mode switch
	while ((digitalRead(_ps2clk) == LOW) || (digitalRead(_ps2data) == LOW))
		;
	// hold up incoming data
	golo(_ps2clk);
}


/*
 * read a byte of data from the ps2 device.  Ignores parity.
 */
unsigned char
PS2::read(void)
{
	unsigned char data = 0x00;
	unsigned char i;
	unsigned char bit = 0x01;

	// start clock
	gohi(_ps2clk);
	gohi(_ps2data);
	delayMicroseconds(50);
	while (digitalRead(_ps2clk) == HIGH)
		;
	delayMicroseconds(1);	// not sure why.
	while (digitalRead(_ps2clk) == LOW)
		;	// eat start bit
	for (i=0; i < 8; i++)
	{
		while (digitalRead(_ps2clk) == HIGH)
			;
		if (digitalRead(_ps2data) == HIGH)
		{
			data = data | bit;
		}
		while (digitalRead(_ps2clk) == LOW)
			;
		bit = bit << 1;
	}
	// eat parity bit, ignore it.
	while (digitalRead(_ps2clk) == HIGH)
		;
	while (digitalRead(_ps2clk) == LOW)
		;
	// eat stop bit
	while (digitalRead(_ps2clk) == HIGH)
		;
	while (digitalRead(_ps2clk) == LOW)
		;
	golo(_ps2clk);	// hold incoming data

	return data;
}

Hi,
You should probably not call kbd.read inside an interrupt handler. It can take hundreds of microseconds to return. I'm not sure, but delayMicroseconds() might also have problems inside of an ISR.

In the read routine, code of the form:

		while (digitalRead(_ps2clk) == HIGH)
			;

can get "stuck" if the transition is missed. This normally doesn't happen, but I don't know what effect the ISR has on it.
Doing serial operations inside an ISR is also discouraged.

Another thing to look at is if the mouse and keyboard routines are conflicting with each other. For example, if the mouse routine has the same timing constraints as the keyboard, then if mouse calls delay and then gets an interrupt, they are going to trip each other up. In fact, I'd bet that's what is happening. The call to mouse.data() is the one that's hanging up, because it gets derailed by the interrupt handler, and misses some of the incoming bits. The PS2 routines are not very robust, in that they don't really detect that sort of problem.

If I were you, I'd shorten the ISR so that all it does is set a flag. Then in the main loop, check the flag value and if it is set, do the keyboard stuff (resetting the flag when done). Always keep interrupt handlers as short as possible. That way the mouse and keyboard routines are never running at the same time, and there are no timing issues. An ISR with only a couple of instructions in it would probably not throw off the timing enough to matter, if it was not called frequently.

Thanks Cklick, I tried your way, but now the mouse only reports when a keystroke occurs. Starting to think its in the library.

here is the updated code.

#include <PS2Mouse.h>
#include <ps2.h>


#define MOUSE_DATA 12
#define MOUSE_CLOCK 13

PS2Mouse mouse(MOUSE_CLOCK, MOUSE_DATA, STREAM);

int ledPin = 9;    
int ledPin2 = 10;
int ledPin3 = 3;
int ledPin4 = 11;
int ltrigger = 5;
volatile boolean kbe = false;
PS2 kbd(2, 7);

void setup()  { 
  Serial.begin(9600);

  analogWrite(ledPin, 80);
  analogWrite(ledPin2, 80);
  analogWrite(ledPin3, 80);
  analogWrite(ledPin4, 80);
  mouse.initialize();

  char ack;
  kbd.write(0xff);  // send reset code
  ack = kbd.read();  // byte, kbd does self test
  ack = kbd.read();  // another ack when self test is done

  attachInterrupt(0, kbInterrupt, FALLING);
} 

void loop()  { 

  int data[2];
  mouse.report(data);

  if (data[0] == 9)
  {
    analogWrite(ltrigger, 60);
  }
  else
  { 
    analogWrite(ltrigger, 250);
  }

  if (data[1] > 0 )
  {
    analogWrite(ledPin4, 160);

  }
  else if (data[1] < 0 )
  {
    analogWrite(ledPin4, 0);

  }  
  else
  {
    analogWrite(ledPin4, 80);
  }


  if (data[2] > 0 )
  {
    analogWrite(ledPin3, 160);

  }
  else if (data[2] < 0 )
  {
    analogWrite(ledPin3, 0);

  }  
  else
  {
    analogWrite(ledPin3, 80);
  }

  Serial.print(data[0]); // Status Byte
  Serial.print(":");
  Serial.print(data[1]); // X Movement Data
  Serial.print(",");
  Serial.print(data[2]); // Y Movement Data
  Serial.println();
  interrupts();

  noInterrupts();
  if(kbe == true){

    unsigned char code;
    code = kbd.read();
    int coded = code;

    if (coded == 29){
      Serial.println("W-d");
      analogWrite(ledPin2, 160);
    }

    if (coded == 240){
      unsigned char code1;
      code1 = kbd.read();
      int coded1 = code1;

      if (coded1 == 29){
        analogWrite(ledPin2, 90);
        Serial.println("W-u");
      }
    }
    else{
      Serial.println(coded, DEC);
      Serial.println("BLAMMO!");
    }
    kbe = false;
  }
  interrupts();
}

void kbInterrupt() {
  kbe = true;
}

Here is the serial output, it doesn't coninue past this point unless I press a key

8:0,0
126
BLAMMO!
8:0,0
191
BLAMMO!
8:0,0
126
BLAMMO!
8:0,0

I wouldn't be turning interrupts on and off myself.

 int data[2];
  mouse.report(data);

...

  if (data[2] > 0 )

Index 2 is not valid. You have two elements, 0 and 1.

Index 2 is not valid. You have two elements, 0 and 1.

index 2 still comes up with the right numbers, but for good measure I tried a couple of things out.

First

int data[3];
  mouse.report(data);

...

  if (data[2] > 0 )

Works just as before after setting this up, as I expected.

So, just to check something

int data[2];
  mouse.report(data);

...

Serial.print(data[3]);

This did not throw an error at compile. It wouldn't show the right number, but it didn't crash. So I'm guessing this is just a difference between explicit and implicit declarations? I've left it with the int data[3]; though just to make sure. Regardless this hasn't solved the issue, and I had ditched turning the interrupts off and on. Thanks for the help.

This did not throw an error at compile.

It won't, C is happy for you to trash all the RAM if you want. But it ain't right.

If your other examples work I can only assume that mouse.report() actually loads more than two ints into "data" and you have been lucky in that there was nothing important at offset 2 and above. Can you point to the documentation for mouse.report()?


Rob

  noInterrupts();
 ...
      Serial.println("W-d");

It's not advisable to do Serial prints with interrupts off.

Well, I thought I had it going. I'm going to have to mess with the keyboard library in order to get this going though. Thanks for the help guys, at least I got the interrupt stuff down.

Can you point to the documentation for mouse.report()?

Sorry I don't remember where I got the library from. C is a funny language.

C is a funny language.

To some maybe :slight_smile:


Rob

I've left it with the int data[3]; though just to make sure

:fearful:

Leaving out-of-bound array indexes around is a recipe for disaster. Please dont't do it.

It's also a good programming practice to use named constants for array sizes.

The reason you have to press a key to make progress in the main loop is that kbd.read is a blocking call: it will wait until there is data available from the keyboard. The mouse works the same way, but there is always data available from the mouse.

In order for this to work the way you want, the keyboard read function must be modified to be non-blocking. At the point where it is waiting for data from the keyboard, it can use a loop that breaks after a certain amount of time (or number of loops). Then it should return an invalid value that the caller can detect. There are several other ways to do it, but that one requires the smallest changes.

I don't know if you have the programming experience to do it yourself. If you do get it working please post the code in the playground for others to use.

PS:
Do not turn interrupts off and on unless you have a clear case where not doing so causes a problem. Interrupts are on by default and turning them off causes problems with timing and other aspects of the arduino environment.
You also need to figure out the correct parameters to the mouse routines. Perhaps the header file has some comments that will help.

tuxduino:
Leaving out-of-bound array indexes around is a recipe for disaster.

True, It's just a bit confusing once I changed it around. 2 is the libraries default though, so I'll just stick with that. I really have no excuse for my naming conventions ha ha, I'll work on it.

ckiick:
The reason you have to press a key to make progress in the main loop is that kbd.read is a blocking call: it will wait until there is data available from the keyboard. The mouse works the same way, but there is always data available from the mouse.

In order for this to work the way you want, the keyboard read function must be modified to be non-blocking.

That is where I'm at now. Seems the way to catch it should be through an interrupt, but the library isn't set up to handle it that way. I'm trying to get some more info on the actual patterns right now, I understand what the library does, and I think the trick is to have the clock on the interrupt, and listen for the clock to fall. Next pass the first bit of data, just wait for hi clock, then low clock. Once it's low take in data1, then wait for hi clock,and low clock again. Do this 7 more times for data2-8. The last thing to do is waste time on the last two clock cycles, report, reset and go back to listening for the interrupt again. Hopefully I'll figure it out, thanks guys!