Arduino Forum upgrade scheduled for Monday, October 20th, 11am-4pm (CEST). Sorry for the inconvenience!
Pages: [1]   Go Down
Author Topic: Bug in Keypad library  (Read 19973 times)
0 Members and 1 Guest are viewing this topic.
Global Moderator
Melbourne, Australia
Offline Offline
Brattain Member
*****
Karma: 535
Posts: 19768
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

In the Keypad library, which is distributed with the IDE, there is a bug in initializePins which currently looks like this:

Code:
void Keypad::initializePins(){
  for (byte r=0; r<size.rows; r++){
    for (byte c=0; c<size.columns; c++){
      pinMode(columnPins[c],OUTPUT);
      digitalWrite(columnPins[c],HIGH);
    }
    //configure row pin modes and states
    pinMode(rowPins[r],INPUT);
    digitalWrite(rowPins[r],HIGH);
  }
}

Notice that if you have a 4x4 keypad the inner loop which does this:

Code:
pinMode(columnPins[c],OUTPUT);
digitalWrite(columnPins[c],HIGH);

... is done 16 times, which is unnecessary. This should be rewritten as:

Code:
void Keypad::initializePins(){
  for (byte r=0; r<size.rows; r++){
    //configure row pin modes and states
    pinMode(rowPins[r],INPUT);
    digitalWrite(rowPins[r],HIGH);
  }
  for (byte c=0; c<size.columns; c++){
    //configure column pin modes and states
    pinMode(columnPins[c],OUTPUT);
    digitalWrite(columnPins[c],HIGH);
  } 
}

Then when I was investigating another problem I noticed this in Keypad.h:

Code:
typedef struct {
    byte rows : 4;
    byte columns : 4;
} KeypadSize;

When I saw that code I felt a great disturbance in the Force.  Hmmm ... I haven't seen code like that for ...

The declaration "rows : 4" means "allocate 4 bits for rows (ditto for columns).

Now on the face of it, this might seem like a clever way to save one byte (since rows/columns now take one byte rather than two).

However compiling a test program (below) shows that this actually uses an extra 68 bytes rather than saving one!

Using the original library, the sketch below takes 1826 bytes. Then by making a copy of the library and removing the ": 4" from the declarations, the sketch size drops to 1758 bytes. That is 68 bytes less.

So if the library was amended to remove the two lots of ": 4" from Keypad.h, everyone's sketches would be smaller, not larger.

Code:
#include <Keypad.h>

  const byte ROWS = 4; //four rows
  const byte COLS = 3; //three columns
  char keys[ROWS][COLS] = {
    {'1', '2', '3'},
    {'4', '5', '6'},
    {'7', '8', '9'},
    {'A', 'B', 'C'},
  };
 
  byte rowPins[ROWS] = {5, 10, 9, 7}; //connect to the row pinouts of the keypad
  byte colPins[COLS] = {6, 4, 8}; //connect to the column pinouts of the keypad
 
  // Create the Keypad
Keypad kpd = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup ()
{
 byte key =  kpd.getKey();
}

void loop () {}

I am guessing the reason for this is that, whilst the structure might be slightly smaller, the code generated to extract out the 4-bit fields is somewhat larger.
Logged

http://gammon.com.au/electronics
Please post technical questions on the forum - not by personal message. Thanks!

Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Hi Nick,

I have encountered errors while programming which is the programming line below highlighted in red, the keypad defining kpd, the program cannot read it as a type. I am using a Waspmote board instead of arduino. I placed the keypad.cpp and keypad.h file in the library and followed your instructions to rewrite the keypad library, the program manages to read the 'Keypad' defining the kpd as the colour of the 'keypad' changes. However, the error still persists. The error states : "Keypad does not name a type in function void loop()".

The following is my code:

const byte ROWS = 4; //four rows
const byte COLS = 3; //three columns
char keys[ROWS][COLS] = {
  {'1','2','3'},
  {'4','5','6'},
  {'7','8','9'},
  {'#','0','*'}
};
byte rowPins[ROWS] = {5, 4, 3, 2}; //connect to the row pinouts of the keypad
byte colPins[COLS] = {8, 7, 6}; //connect to the column pinouts of the keypad

Keypad kpd = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){
  USB.begin();
}
 
void loop(){
  byte key = kpd.getKey();
 
  if (key != NO_KEY){
    USB.println(key);
  }
}

Please advice
Thanks in advance.
Logged

Global Moderator
Melbourne, Australia
Offline Offline
Brattain Member
*****
Karma: 535
Posts: 19768
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Did you do this?

Code:
#include <Keypad.h>
Logged

http://gammon.com.au/electronics
Please post technical questions on the forum - not by personal message. Thanks!

Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Hi,

It is only when i exclude Keypad.h, i will have the error stated above.

If i include Keypad.h, i will have more complex errors as in the picture in the link: http://i1131.photobucket.com/albums/m542/couchpotato8/error.png

Regards.
Logged

Global Moderator
Melbourne, Australia
Offline Offline
Brattain Member
*****
Karma: 535
Posts: 19768
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

You don't need to post screenshots of error messages you know. You can just  copy and paste them which is a lot more readable.

Is that the whole sketch? There is no USB defined anywhere. If I change USB to Serial, this compiles for me without errors:

Code:
#include <Keypad.h>

const byte ROWS = 4; //four rows
const byte COLS = 3; //three columns
char keys[ROWS][COLS] = {
  {'1','2','3'},
  {'4','5','6'},
  {'7','8','9'},
  {'#','0','*'}
};
byte rowPins[ROWS] = {5, 4, 3, 2}; //connect to the row pinouts of the keypad
byte colPins[COLS] = {8, 7, 6}; //connect to the column pinouts of the keypad

Keypad kpd = Keypad( makeKeymap(keys), rowPins, colPins, ROWS, COLS );

void setup(){

}
 
void loop(){
  byte key = kpd.getKey();
 
  if (key != NO_KEY){
    Serial.println(key);
  }
}
Logged

http://gammon.com.au/electronics
Please post technical questions on the forum - not by personal message. Thanks!

Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I used the code you posted and i am still getting the same results, is it because i am using the waspmote IDE instead of Arduino or do i need to add anything else into my library?

Regrads.
Logged

Global Moderator
Melbourne, Australia
Offline Offline
Brattain Member
*****
Karma: 535
Posts: 19768
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

I am not familiar with the waspmote IDE, so I can't say what else you have to do. My test was under the Arduino IDE.
Logged

http://gammon.com.au/electronics
Please post technical questions on the forum - not by personal message. Thanks!

Pages: [1]   Go Up
Arduino Forum upgrade scheduled for Monday, October 20th, 11am-4pm (CEST). Sorry for the inconvenience!
Jump to: