Go Down

Topic: Writing ISR to read 4-bit output from 74c922 keypad encoder ic (Read 2242 times) previous topic - next topic

raschemmel

Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

raschemmel

Rob,
I think I can say conclusively that the ISR never ran by vertue of the fact that the serial print statement
in the ISR was never output. As far as the wrong pin , the attachinterupt statement has a consta pin2ISR
declared for pin 2 which is where the wire was. As far as the code being terrible, that's probably true
since this is my first attempt at an ISR or any interupt code whatsoever, having only been doing this
since Oct 30, 2013, when I bought my first Arduino. I have no idea what you mean , any more than
someone who has only been in electronics would know their soldering is terrible. I can't see what you
see when you look at my code. All I know is that while I got the interface working, I never got an
ISR working. I know it's not rocket science but unless I can query the CPU registers I don't know how
I can determine why it didn't execute, unless you can tell me.
Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

Graynomad

#17
Dec 09, 2013, 05:56 am Last Edit: Dec 09, 2013, 05:58 am by Graynomad Reason: 1
The ISR looks good to me (at least down to newKey = true;), there must be something we're not seeing and someone will come along and say "What's with the comma after X?" Did you check Ledpin to see if it changes state?

The main body of the code...when you see a 100 if-this-do-that-else-if-something-do-something-else... it's a pretty good sign that there's a better way.

Also the fact that you have almost identical code in the conditions, for example

Code: [Select]
if (total==13 &&  setflag==false)  // 0
            {
              Serial.print("total= ");
              Serial.print(total);
              total=0;
              setflag=true;
              digitalWrite(Ledpin,LOW);
              delay(300);
              Serial.print(" ");
              Serial.println("setflag SET! (key=0) !");
             
            }


Is repeated many times with the only real difference being the value that total is set to. And the "&&  setflag==false" is repeated for every test, put all the tests inside a single block that tests for that. (Mind you setflag does nothing anyway as it's set to false just before the tests)

I haven't analysed the code much but you have 9 if blocks based on the value in total, I would say you could use either a lookup table or a case statement for example

Code: [Select]
byte  chars [] = "0123456789*#";

void loop () {

  total =  digitalRead(OutD) << 3;
  total !=  digitalRead(OutC) << 2;
  total !=  digitalRead(OutB) << 1;
  total !=  digitalRead(OutA);

  Serial.print("total= ");
  Serial.print(total);

  digitalWrite(Ledpin,LOW);
  delay(300);

  switch(total) {
  case 13:
    total=0;
    break;

  case 0:
  case 1:
  case 2:
    total++;
    break;

  case 8:
  case 9:
  case 10:
    total--;
    break;

  case 12:
    total = 10;
    break;

  case 14:
    total = 11;
    break;
  }

  Serial.print(" setflag SET! (key=");
  Serial.print(chars[total]);
  Serial.println(")!");
}



This has not been compiled, probably has a few typos, and almost certainly doesn't do exactly what you have in mind; but you can see the difference, it's immediately clear what values do what.

______
Rob
Rob Gray aka the GRAYnomad www.robgray.com

raschemmel

Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

lar3ry


Do you still have any punch cards ?

No, but I still have a few old circuit boards from machines we scrapped.
There are 10 kinds of people in the world,
those who understand binary, and those who don't.

lar3ry

I downloaded your latest code, keypad_4x3_Program_Working_CORRECTLY_ISR_and_setflag_test_5.ino, and I see you have the attachInterrupt() commented out. I am goig to assume you commented it out because it didn't work, but looking at your code, I see the following lines:

Code: [Select]

int DATA_AV=12;
const int pin2ISR = 2;
attachInterrupt(pin2ISR,readEncoder , RISING); // assuming you uncommented it to test


If you have Data Available wired to pin 12, it's never going to give you an interrupt on pin 2.

Of course, I may be misunderstanding the attachInterrupt statement.

You might as well get rid of the Serial. print statements in the ISR, as they won't work. The LED on pin 6 will indicate going through the interrupt, but then if you're testing the ISR, you should comment out the line that sets it HIGH near the top of loop(). In fact you should probably toggle it on/off each time through the interrupt, and comment out all the other lines that set it LOW, or you won't even see it if it does go on. Things happen mighty quick in Arduino county.

Graynomad mentioned the best way to optimize your endless if statements. He suggested using the low order 4 bits of PortD, and using

Code: [Select]
total = PIND & 0xF;

That's probably not the best port to use, since Tx and Rx are on bits 0 and 1, but basically, any 4 consecutive bits in one port are desirable for this purpose.

Suppose you used PortD, bits 3, 4, 5, and 6 (which are Arduino pins 3, 4, 5, and 6). You could then read them with the single line:

Code: [Select]
total = (PIND & 0x78) >> 3 ;

Read this as "get bits 3, 4, 5, and 6, and shift that value 3 bits to the right", which will give you the value of the pressed key. If the value is wrong, you can change the wires between the keypad and the 74c922. It'll save you a whack of parsing.
There are 10 kinds of people in the world,
those who understand binary, and those who don't.

Graynomad

I couldn't resist, that entire switch block can be replaced with an array and a single line of code

Code: [Select]

// array of values to set "total" to, adjust as appropriate
// -1 means not used, could be any value
byte totVals [] = {
// 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14
  1, 2, 3, -1, -1, -1, -1, -1, 7, 8, 9, -1, 10, 0, 11};
....
// now instead of the long switch all we need is
total = totVals[total];



Whenever you have a relationship like this that's not linear or easy/possible to calculate a small lookup table is a simple and fast way to go.

______
Rob



Rob Gray aka the GRAYnomad www.robgray.com

raschemmel

I have to study this. I've never done anything with arrays so it may take me a little time to understand it.
Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

UKHeliBob


Rob,
I think I can say conclusively that the ISR never ran by vertue of the fact that the serial print statement
in the ISR was never output.
I don't think that you can say that at all because Serial.print does not work inside an ISR.
Please do not send me PMs asking for help.  Post in the forum then everyone will benefit from seeing the questions and answers.

Graynomad

In essence we use total to find it's own new value by indexing into an array.

So let's say total is 8, we get the value at index 8 in the array, it's the same as having

total = totVals[8];

the value in the array is 7 so we have assigned the value 7 to total when it was 8.

With a method like this (sometimes called "table driven") you can often change the program's logic by just editing the array, no code changes required.

______
Rob
Rob Gray aka the GRAYnomad www.robgray.com

raschemmel

Perfect.
That sounds like a much better way to do it !
Thanks !
Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

raschemmel

Rob,
So if I understand this correctly,

int myVals[12];
                             // Desired Key values
                         // 0   1   2   3   4   5   6   7   8   9   10   11   12   13   14
                              // index values from encoder
byte totVals [] = {1,   2,   3,   -1,   4,   5,   6,   -1,   7,   8,   9,   -1,   10,   0,   11};
                               '3' and '7' don't exist in the code because they correspond to keys that would have
                                been in the 4th column in a 16-key matrix like the chip was intended for.
                                 '4' , '5', and '6', return correct values so those should have the same value in their place holders
                                 I don't understand about the -1 for those because the following statement stills needs to
                                  still to retrieve a those values.

Until the ISR is working I will still need to use 'DATA_AV' (pin 12) as an input to my UNO pin 12 as a trigger

I understand the four bits have to be converted to a 4-bit BCD-encoded value that can be done as I did
or by bit shifting left as in your example. Once that value is obtained, the correct value can be obtained
by using the received value to index the lookup table or by the 'CASE' method you mentioned (below).
so,  I'm not sure yet which approach is better, the above with 'total = totVals[total];' | total is obtained from
the following:



byte  chars [] = "0123456789*#";
void loop ()

{
                     if (digitalRead(DATA_AV)==HIGH)

  total =  digitalRead(OutD) << 3;
  total !=  digitalRead(OutC) << 2;
  total !=  digitalRead(OutB) << 1;
  total !=  digitalRead(OutA);

 
  digitalWrite(Ledpin,LOW);
  delay(300);

  switch(total)
{
  case 13:
    total=0;
    break;

  case 0:
  case 1:
  case 2:
    total++;
    break;

  case 8:
  case 9:
  case 10:
    total--;
    break;      

  case 12:
    total = 10;
    break;

  case 14:
    total = 11;
    break;
  }
Serial.print("total= ");
  Serial.print(total);

  Serial.print(" setflag SET! (key=");
  Serial.print(chars[total]);
  Serial.println(")!");
}



....

Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

Graynomad

#27
Dec 10, 2013, 01:42 am Last Edit: Dec 10, 2013, 01:56 am by Graynomad Reason: 1
Quote
I don't understand about the -1 for those

You insert numbers as appropriate, I may have mis-interpreted which numbers were used.

The -1 can be anything because in theory you will never retrieve values at those locations, in the real world having a non-valid number there allows you to test for a bad key press or otherwise bad value returned.

Quote
the correct value can be obtained by using the received value to index the lookup table or by the 'CASE' method you mentioned

Correct.

Quote
I'm not sure yet which approach is better

One line of code verses 30, seems like a no-brainer to me although I admit that with lookup tables it's not as immediately obvious what key does what so the switch version might make for clearer code for a beginning programmer.

That said the table version is still very simple and probably a better learning experience.

What is

int myVals[12];

for?

______
Rob
Rob Gray aka the GRAYnomad www.robgray.com

raschemmel

Quote
The -1 can be anything because in theory you will never retrieve values at those locations, in the real world having a non-valid number there allows you to test for a bad key press or otherwise bad value returned.


My question is how do you get a '4' from a key-4 press if the lookup table doesn't have anything there ?
If you index a lookup table using the returned value '4' and have -1 in the lookup table at that index,
how does it use that value '4' if it is the index and not in the table ? Are you going to say "if array contents
= -1 then total =total; " ?
I'm sorry, I just not getting that.
Either way, the lookup table is useless until you convert the 4 bits to an  integer equal to the BCD value.
I don't want to sacrifice 4 pins to use a PIND statement and I don't know if I can declare the 4 pins  not
used for the keypad for other purposes, like digital temp inputs or whatever.
The bit shifting looks much more efficient that my method (probably anything would be) .



Quote
What is

int myVals[12];
for? 


I got that from the website on arrays, here

http://arduino.cc/en/Reference/Array
Quote
Creating (Declaring) an Array

All of the methods below are valid ways to create (declare) an array.

  int myInts[6];
  int myPins[] = {2, 4, 8, 3, 6};



Why did you use:
Code: [Select]
byte totVals [] = 
?


Arduino UNOs, Pro-Minis, ATMega328, ATtiny85, LCDs, MCP4162, keypads,
DS18B20s,74c922,nRF24L01, RS232, SD card, RC fixed wing, quadcopter

lar3ry


Either way, the lookup table is useless until you convert the 4 bits to an  integer equal to the BCD value.
I don't want to sacrifice 4 pins to use a PIND statement


You aren't sacrificing anything. You need 4 pins to read the output of your chip. Reading them using input from PinD is just another way of getting the data from the chip.

Quote

and I don't know if I can declare the 4 pins  not used for the keypad for other purposes, like digital temp inputs or whatever.


Of course you can! You read in PinD, which gets the (digital) state of all pins in PortD. ANDing them with a value just zeros out all the bits IN YOUR VARIABLE. The operation does not affect the pins in that port at all. The read is totally passive, in that it does not alter any bits in the port. The & (AND) only affects the variable with the PinD contents in it.

Quote

The bit shifting looks much more efficient that my method (probably anything would be) .


There is only a small difference in execution time between assigning bits 0-3 to the 1, 2, 4, and 8 value inputs, and shifting bits or ecen shifting and adding/ORing. In any case, scanning through the bits one at a time, using switch/case or if statements should be actively avoided .

There are 10 kinds of people in the world,
those who understand binary, and those who don't.

Go Up