first, the Read-Modify-Write issue (from http://www.voti.nl/DB038/DB038-1-1.pdf, p51):
PICs use a funny IO architecture where a reading of a pin always returns the current external level of the pin, which can be different from the last value written to a pin, even when the pin is set as output, for two reasons:
• PIC instructions execute in a 2-stage pipeline, and for IO pins the read part of the next instruction takes place before the writing of the previous instruction.
• The load on the pin can be too high for it to reach its 'desired' level. This can easily happen (for a short time) when the load is capacitive, but it can also happen with a static load that is well within the normal operating specifications.
All PIC instructions that modify some bits in a byte are read-modify-write instructions, so when for instance two consecutive BCF (bit clear) instructions are executed on the same IO port the second instruction can ruin the effect of the first because of the first of the two reasons. Actually a BCF instruction (and a lot of other instructions, like INCF) on a port can ruin any previous setting of that port because of the second reason! It should be noted that the first reason occurs far more often, and can be avoided by placing a NOP or another instruction between any two read-modify-write instructions on the same IO port. But to really avoid all problems it is advised to allocate a separate register, do manipulations on that register, and copy it to the port register after each change. This technique is called a “using a shadow register”.
=====================================================
Next: RC delay. When you change an I/O pin connected to a non-trivial amount of wire always wait a reasonable amount of time before expecting the result. In the absense of another source of 'reasonable': find out what seems to work, then double the time.
=====================================================
Last: debouncing. AFAIK 50 ms is a reasonable upper limit for the bounce time of a switch. When your sample interval is longer than this time (you use 100 ms), you have no bounce problem. The proof is left as an excercise to the reader :)
The switch statement looks overly complicated, and at least to me wasn't immediately obvious what it was doing. I would go with a more straightforward implementation.
Like Abdullah said, delays after you set TRISD
and PORTD
registers may help, as well as debouncing the input.
Finally, I would get a o-scope and/or multimeter to probe on these pins to make sure everything is connected properly, toggling as expected, and make sure the keypad behaves as expected.
EDIT:
I noticed that there are internal Schmitt Triggers on PORTD
inputs that are enabled by default. This will inject some latency, though I'm not sure how much, as the datasheet doesn't say.
EDIT2 :
Added hard delays before sampling and capacitive charge removal
int getKey(void)
{
// Like Abdullah said, these can be bytes
// Also, you can save image space by not initializing these
unsigned char toRet;
unsigned char firstHalf, secHalf;
// Clear lines (attempt to get rid of capacitive charge)
TRISD = 0xff; // Hop off
PORTD = 0x00; // Prepare to drive low
TRISD = 0x00; // Drive all lines
__delay_us(10000);// Wait arbitrary about of time
TRISD = 0xff; // Stop driving while we change values
// Drive first half
PORTD = 0xf0;
TRISD = 0x0f; // Hex makes things easier to read
__delay_us(10000);
firstHalf = (PORTD & 0x0f) >> 0; // Mask only the stuff we care about
TRISD = 0xff;
// Clear lines
PORTD = 0x00;
TRISD = 0x00;
__delay_us(10000);
TRISD = 0xff;
// Drive second half
PORTD = 0x0f;
TRISD = 0xf0;
__delay_us(10000);
secHalf = (PORTD & 0xf0) >> 4; // Mask only the stuff we care about
// and shift the nibble we care about down
TRISD = 0xff;
// Simple selection logic
// Keep in mind that if multiple keys are pressed, this function will return "null"
switch (firstHalf) {
case 0x1:
switch (secHalf) {
case 0x1: toRet = 7; break; //1
case 0x2: toRet = 8; break; //2
case 0x4: toRet = 9; break; //3
default: toRet = 255; break; //null
}
break;
case 0x2:
switch (secHalf) {
case 0x1: toRet = 4; break; //4
case 0x2: toRet = 5; break; //5
case 0x4: toRet = 6; break; //6
default: toRet = 255; break; //null
}
break;
case 0x4:
switch (secHalf) {
case 0x1: toRet = 1; break; //7
case 0x2: toRet = 2; break; //8
case 0x4: toRet = 3; break; //9
default: toRet = 255; break; //null
}
break;
case 0x8:
switch (secHalf) {
case 0x1: toRet = 10; break; //*
case 0x2: toRet = 0; break; //9
case 0x4: toRet = 11; break; //#
default: toRet = 255; break; //null
}
break;
default: toRet = 255; break; //null
}
return toRet;
}
Best Answer
Assuming `row is an input:
You were missing the
begin
for the architecture body. You also mispelled column, while spelling it correctly one place.Your modified code analyzes, elaborates and the added testbench simulates:
Note that not having an else for the if statement based on a particular value of
row
causes latches preserving the value ofsevenseg
for other values ofrow
.This isn't particularly robust, `row = "0111" is a combinatoric evaluation that can cause glitches. You could consider using the result of the equality comparison as an enable to something with a clock.
You could also get rid of the latches by using an else for the if statement assigning
sevenseg
to some value.