avr-gcc keypad interfacing code problem

Question

I have this function to read key press from a 4x3 key-board:

uint8_t GetKeyPressed()
{
  uint8_t r,c;

  KEYPAD_PORT|= 0X0F;

  for(c=0;c<3;c++)
  {
     KEYPAD_DDR&=~(0X7F);

     KEYPAD_DDR|=(0X40>>c);
     for(r=0;r<4;r++)
     {
        if(!(KEYPAD_PIN & (0X08>>r)))
        {
           return (r*3+c);
        }
     }
  }

return 0XFF;//Indicate No key pressed
}

Some macro, I missed:

    #define KEYPAD A  
    #define KEYPAD_PORT PORT(KEYPAD)
    #define KEYPAD_DDR   DDR(KEYPAD)
    #define KEYPAD_PIN   PIN(KEYPAD)

But I don't understand this code, pretty well, because of those bit shifting operations.

Can anyone help me with this code?

Compiler : avr-gcc

Micro-controller : ATmega328

Answer 1

In an abstract manner the code does this:

for each pin PA6 to PA4 (column)
    set pin as output, driving '0'
    for each pin PA3 to PA0 (row)
        if pin reads '0' then
            return key code calculated as row*3+column
return 0xFF as key code, meaning "no key"

• KEYPAD_PORT|= 0X0F; should preset the output register of the port with '0's for PA6 to PA4. The other pins (bit 7 and bits 3 to 0) are not relevant. But here is an error because of the operator |=: If any of the bits 7 to 4 is already 1, it will keep this value. The correct statement is KEYPAD_PORT &= 0x8F;.

• The KEYPAD_DDR register selects the direction of the pins of your keypad port. Each bit corresponds to a pin. Setting a bit to 1 make the pin an output, 0 an input.

• The KEYPAD_PIN register is used to read the pins of your keypad port.

Now to the shifting operations:

KEYPAD_DDR|=(0X40>>c);: The hex value 0x40 is shifted to the right by the value of c. This results in values of 0x40 (0b01000000), 0x20 (0b00100000), and 0x10 (0b00010000). This value is then ORed to KEYPAD_DDR which was ANDed before with the complement of 0x7F = 0x80 (0b10000000). The results are 0xC0 (0b11000000), 0xA0 (0b10100000), and 0x90 (0b10010000), resp.

!(KEYPAD_PIN & (0X08>>r)): The hex value 0x08 is shifted to the right by the value of r. This results in values of 0x08 (0b00001000), 0x04 (0b00000100), 0x02 (0b00000010), and 0x01 (0b00000001). The value read from KEYPAD_PIN is ANDed with this value, giving zero if the "masked" pin is '0' and non-zero otherwise. By the unary operator ! a zero is converted to true and a non-zero to false. So the statement of the if will be executed if the masked pin is '0'.

Note: I like a lowercase 'X'/'B' better than the uppercase for hex and binary constants. But this is a bit of personal taste.

Only you can tell about the pin PA7. That's why I ignored it.

Answer 2

Here is your code with the comments that it should have had all along added in, plus tables showing exactly what the shift operations are doing (the latter shouldn't be necessary for any experienced C programmer):

#define KEYPAD A  
#define KEYPAD_PORT PORT(KEYPAD)
#define KEYPAD_DDR   DDR(KEYPAD)
#define KEYPAD_PIN   PIN(KEYPAD)

uint8_t GetKeyPressed()
{
  uint8_t r, c;

  /* Preset the data for the row inputs to 1.
   */
  KEYPAD_PORT |= 0X0F;

  /* Scan through the three columns - 0, 1, 2
   */
  for (c=0; c<3; c++) {
    /* Enable the output for the column we want to drive, but also be sure
     * not to modify the existing DDR value for bit 7. This sets DDR to
     * c == 0: ?1000000
     * c == 1: ?0100000
     * c == 2: ?0010000
     */
    KEYPAD_DDR &= ~(0X7F);
    KEYPAD_DDR |= (0X40>>c);
    /* Scan through the four rows - 0, 1, 2, 3
     */
    for (r=0; r<4; r++) {
      /* If the corresponding input pin is low, return the key location,
       * a number in the range 0-11. The mask (0x08 >> r) becomes
       * r == 0: 00001000
       * r == 1: 00000100
       * r == 2: 00000010
       * r == 3: 00000001
       */
      if (!(KEYPAD_PIN & (0X08>>r))) {
        return (r*3+c);
      }
    }
  }

  /* No keys pressed, return special code.
   */
  return 0XFF;
}

Note that there's a subtle bug. The intent is to drive the column lines low one at a time, but we don't actually know what the port data is for those lines. The line

  KEYPAD_PORT |= 0X0F;

Only sets bits 0-3, but doesn't affect bits 4-7. This is a useless operation, since we never enable those pins as outputs anyway. This line needs to be replaced by

  KEYPAD_PORT &= 0X8F;

in order to force bits 4-6 low so that we can use the data to drive the columns one at a time. This leaves bit 7 alone, since we don't want to affect it, and we don't care about the data for bits 0-3.