Won me over to the dark side #2

Go To Last Post
108 posts / 0 new

Pages

Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hey,

Some *Cough* (explicitive deleted) *Cough*... Person... locked my thread, so this is a continuation.

I still have C questions, though less ambitious as before. Here's my question, which I typed up last night:

===============================================

I've decided to forgo the complex tasks and get started with the basics instead. I like to get stuck into somthing - trying the hardest thing and learn on the way, but that doesn't seem to be working.

My 16x2 LCD (standard hitatchi based) doesn't seem to like the LCD routines i'm using (downloaded off, wait for it... AVRfreaks academy section), but I wont ask - I know that there are a million threads here on how to get your LCD working. Better not frustrate the masses any more. By the way, EW, ask me to read that thread again. I'll make an exception and see how long your temper's fuse is :D .

I got a LED to flash - with my own code, rather than the blinky.c. There's no joy in copying code that you know works, as debugging is half the fun when it's a non-commercial project and there's no $$$ on the line. I wrote a simple 4x4 keypad scanning routine in BASCOM, just to make sure I got the theory right. No sense in wasting time trying to debug an application that couldn't work from the start. Here's the BASIC routine I wrote below:

$sim                                                        ' Use fast simulation, comment out for programming
$eepleave                                                   ' Don't alter any existing EEPROM file
$regfile = "8535def.dat"                                    ' Use the AT90S8535 microcontroller
$crystal = 8000000                                          ' 8MHz Crystal - 8 Million Cycles/Second

Declare Function Scankpd() As Integer

Dim Scanned As Integer
Dim Scanned2 As Integer
Dim Keypd As Integer

Do
   Scanned = Scankpd()                                      ' Scan the keypad
   Waitms 30                                                ' Debounce
   Scanned2 = Scankpd()                                     ' Scan the keypad a second time
   If Scanned = Scanned2 Then Print Scanned                 ' If results are the same, print the result to the serial port
Loop



Function Scankpd() As Integer
   Ddrb = &B00001111
   Portb = &B11110000

   If Pinb.1 = 1 Then Scankpd = 4                           ' \
   If Pinb.2 = 1 Then Scankpd = 8                           ' | Columns, add 4 per column to get pressed key
   If Pinb.3 = 1 Then Scankpd = 12                          ' /

   Ddrb = &B11110000
   Portb = &B00001111

   If Pinb.4 = 1 Then Scankpd = Scankpd + 1                 ' \
   If Pinb.5 = 1 Then Scankpd = Scankpd + 2                 ' | Should actually be logic low = pressed, but
   If Pinb.6 = 1 Then Scankpd = Scankpd + 3                 ' | the BASCOM simulator cannot simulate pullups
   If Pinb.7 = 1 Then Scankpd = Scankpd + 4                 ' /
End Function

This works like a charm - to make it work in the "real" world (go the matrix!). Change all the "= 1"'s to "= 0"'s (active low due to pullups).

I wrote a seperate - but for the most part identicle - program in C, which is below. I won't include the headder file, but this just has macros and defines the macros and the KBDport, etc. as PORTB/DDRB/PINB.

/*
	===================================
  	 Keypad (4x4 Matrix) Scan Routines

	      By Dean Camera, Dec 2004

        Put ROWS on Portx.0-3, COLS on
                   Portx.4-7
	===================================
    
*/

#include 

int GetKPD(void) { // Perform debounce on KPDScan sub
	int PrevValue = 0; // Declare integer variable
	
	PrevValue = KPDScan();
	_delay_loop_2(20000); // Wait for 40000 cycles
	if (PrevValue == KPDScan()) {return PrevValue;} else {return 0;} // Check again, if same then return result
}

int KPDScan(void) { // Scan 4x4 matrix - 0 for no key, 1 for (0,1), 5 for (1,1), etc.
	int KPDResult = 0; // Declare integer variable with start value of 0

	KBDddr = 0xFF; // Set upper PORT Bits as inputs - Binary 00001111
	bit_clear(KBDddr,BIT(0)|BIT(1)|BIT(2)|BIT(3));
	KBDport = 0xFF; // Enable Pullups, outputs low 
	bit_clear(KBDport,BIT(4)|BIT(5)|BIT(6)|BIT(7));

	if (bit_get(KBDpin,BIT(1))==0) {KPDResult = 4;}
	if (bit_get(KBDpin,BIT(2))==0) {KPDResult = 8;}
	if (bit_get(KBDpin,BIT(3))==0) {KPDResult = 12;}
		
	KBDddr = 0xFF; // Set lower PORT Bits as inputs - Binary 11110000
	bit_clear(KBDddr,BIT(4)|BIT(5)|BIT(6)|BIT(7));
	KBDport = 0xFF; // Enable pullups, outputs low
	bit_clear(KBDport,BIT(0)|BIT(1)|BIT(2)|BIT(3));

	if (bit_get(KBDpin,BIT(4))==0) {KPDResult++;}
	if (bit_get(KBDpin,BIT(5))==0) {KPDResult+= 2;}
	if (bit_get(KBDpin,BIT(6))==0) {KPDResult+= 3;}
	if (bit_get(KBDpin,BIT(7))==0) {KPDResult+= 4;}		
	
	return KPDResult; // Give the result back to the calling function
}

Don't shout!! I'm heavily using macros to start off, as i'm not dead confident with the bit masking, yet. I'll get onto that later and fix up all the code. I originally used just a = 0x0F and = 0xF0 for the PORT/DDR - but switched to macros after it failed to work.

To ensure the hardware's correct, I'm using the same hardware as my BASCOM-AVR PLD2. This has an LCD on PORTD, 4x4 Keypad on PORTB, LEDs on PORTC and a single LED on PORTA.0. I tested my BASCOM PLD2 firmware first to ensure there were no hardware issues (that I know of).

I downloaded the C program - I'm using a ABCmini board rather than a JTAG/etc. board, so AVR Studio won't download it. Instead, I compile the HEX with WinAvr, and then download with BASCOM's inbuilt programmer, which I know works and really is the best one out there. Why can't they make it into a seperate EXE??!!

Anywho, I then ran the code. It is supposed to flash a LED on PORTC (via main.c), with the flashed counting the key number pressed - with a delay between each key scan. After all this, it dosn't work! I get an oddball number of flashes (that don't correspond) on some keys, and no flashes on others! Before anyone asks, i've spend many hours trying to solve the problem by myself. No point bothering the experts when I could solve it myself, but this one has me totally stumped. It's so simple, yet so difficult.

By the way, I'm offline when i'm writing this (will post when online) so forgive me if this ignores previous questions. I'll add my answers (if any) as a seperate post or as an edit. I just had some time to kill.

C'mon Smokey Joe! This should be right up your alley!

===============================================

Responses to previous questions:

1) I'll TRY to link files instead from now on.
2) NO MORE OFF TOPIC JUNK, please
3) I deliberatly used const char arrays, as the data isn't supposed to be altered, just read.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your use of macros, which we can't actually see since you haven't posted the header file, makes it well nigh impossible to know why your code doesn't work.
I'm sure you heard this before, but you really do need to get to grips with the standard C method for testing/setting bits. It's not difficult, for example:

if (bit_get(KBDpin,BIT(3))==0) 

/*would be,*/

if((KBDpin & 0x08)  == 0)

/*or, if you can't get your head around hex, and the fact that bit 3 is equivalent to 0x08*/

if((KBDpin & (1<<3)) == 0)

In real C you wouldn't use the == 0 form, but you'll learn that later.

Oh, and by the way, please remember that I have RBS (redundant bracket syndrome).

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ok, the headder file is thus:

#include 
#include 

// Function Declares
int KPDScan(void); // Declare Function
int GetKPD(void); // Declare Function

// Macros
#define bit_get(p,m) ((p) & (m)) 
#define bit_set(p,m) ((p) |= (m)) 
#define bit_clear(p,m) ((p) &= ~(m)) 
#define bit_flip(p,m) ((p) ^= (m)) 
#define bit_write(c,p,m) (c ? bit_set(p,m) : bit_clear(p,m)) 
#define BIT(x) (0x01 << (x)) 
#define LONGBIT(x) ((unsigned long)0x00000001 << (x))
#define get_bits(num, from, to) (((num) >> (from)) & ((0x1 << ((to) - (from) + 1)) - 1))   

// Aliases
#define KBDport PORTB
#define KBDpin PINB
#define KBDddr DDRB

As I said, I only used the macros once conventional methods failed me. What would you use instead of the ==0 operator? I assume you'd just ~ the statement, so it would be inverted logic - C executes the FOR code if the statement is true. Am I right?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Writing Mr. Brown's example as

if (!(KBDpin & 0x08))

is succinct and popular. Just use whatever form most clearly conveys what you are doing (unless you are writing for an obfuscated C contest). Sometimes comparing against zero might be clearer, for example, when reading a serial data line.

- John

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Interesting, but it isn't helping me get my keypad routine to work. Any ideas?

I assume that the NOT is faster/smaller than a zero compare, since you don't need to load another register with 0, compare and then branch in assembly.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well I assume that a lot of compilers would generate the same code anyway.
By the way your comments don't match the code. You comment low bits, then manipulate high bits, and vice-versa.
Are you 100% sure that you're managing to program the flash OK?
Write it without the macros, fix the comments (and add some more, aim for a comment for every line when you're starting out) and I'll look at it for you, but I'm not wading through all those macros.
Alternatively, you can wait until the American contingent wake up, and they'll give you the same answers.

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Interesting, but it isn't helping me get my keypad routine to work. Any ideas?

No, but I do have a suggestion.

You might try to comment your code in plain English and write your comments so that someone who knows your general problem could understand what you are trying to do without looking at the code. That will force you to think things through and often helps you recognize conceptual errors you may blame on the compiler and its code, "errors" they are making that are just the two doing exactly what you have asked them to.

I hope that helps.

-John

- John

Last Edited: Fri. Dec 10, 2004 - 03:57 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I see some potential issues with the code you provided:

First of all, in BASCOM, you set DDRB to a binary value of &B00001111 to set up pins 0 through 3 as inputs and pins 4 through 7 as outputs.

This makes me realize: BASCOM and gcc must use different bit numbering schemes. In BASCOM, you must go from least significant bit on the left, to the most sifnificant bit on the right. (ie bit numbering goes 0 1 2 3 4 5 6 7 ...)

Well, in the C language, the least significant figure is always on the right. That means, bit numbering goes like this: ... 7 6 5 4 3 2 1 0.

If you want to configure port B pins 0 to 3 as inputs and pins 4 to 7 as outputs, you'd write:

// Set up data direction on port B.  Pins 0..3 are inputs, pins 4..7 are outputs.
DDRB = 0b11110000;
//or, in hexadecimal:
DDRB = 0xF0;
//or, explicitly:
DDRB = ((1<<PB7) | (1<<PB6) | (1<<PB5) | (1<<PB4));

// Set up internal pull-up resistors on port B pins 0..3, and output 0 on pins 4..7.
PORTB = 0x0F;

Then, to inspect the status of bits 0..3, you'd do something like:

<...>
if(!(PINB & (1<<PB1)))
   KPDResult = 1;
if(!(PINB & (1<<PB2)))
   KPDResult = 2;
if(!(PINB & (1<<PB3)))
   KPDResult = 3;
<...>
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

lfmorrison wrote:

Then, to inspect the status of bits 0..3, you'd do something like:

<...>
if(!(PINB & (1<<PB1)))
   KPDResult = 1;
if(!(PINB & (1<<PB2)))
   KPDResult = 2;
if(!(PINB & (1<<PB3)))
   KPDResult = 3;
<...>

You're one off on the bit values. To match your description it should be:

<...>
if(!(PINB & (1<<PB0)))
   KPDResult = 1;
if(!(PINB & (1<<PB1)))
   KPDResult = 2;
if(!(PINB & (1<<PB2)))
   KPDResult = 3;
<...>

Which, with those macros that he uses (which BTW are fine to use):

<...>
if(!bit_get(PINB, PB0))
   KPDResult = 1;
if(!bit_get(PINB, PB1))
   KPDResult = 2;
if(!bit_get(PINB, PB2))
   KPDResult = 3;
<...>
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmm, seems like everytime I ask a question, I partially solve it before I get an answer (due to the nessesity of getting off the internet for that nuicence called sleep).

I worked out that the MSB/LSB etc. is reversed in C (as opposed to BASCOM), but this shouldn't make much difference. This would only alter the orientation of the keys. The returned results do not correspond in any way, and are in no particular order. Furthermore, I get 20 flashes (key 20!!) when no key is pressed, which should be impossible.

I've ditched the macros alltogether and started to use either NOT Hex compare, or NOT bit shifting. It DOES seem a little easier then messing around with the macros, but it makes no difference.

Ok, ok, I'll put in better comments. I'm used to making them so that I could read them. I know it isnt good practice, but I don't usually intend for other people to read my code.

BTW, why was my old thread locked??

- Dean

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

PINx values are prefetched before the current instruction executes (read the datasheets). If you try to see the influence of a PORTx changing instruction you have to wait at least one cycle to read the affected PINy. Probably BASCOM is not generating a very tied code but it's almost sure that avr-gcc does it. In your code this translates to insert "something" between these lines

bit_clear(KBDport,BIT(4)|BIT(5)|BIT(6)|BIT(7));
if (bit_get(KBDpin,BIT(1))==0) {KPDResult = 4;}
...
bit_clear(KBDport,BIT(0)|BIT(1)|BIT(2)|BIT(3));
if (bit_get(KBDpin,BIT(4))==0) {KPDResult++;}

Something like this:

bit_clear(KBDport,BIT(4)|BIT(5)|BIT(6)|BIT(7));
__asm__ __volatile__("nop");
if (bit_get(KBDpin,BIT(1))==0) {KPDResult = 4;}
...
bit_clear(KBDport,BIT(0)|BIT(1)|BIT(2)|BIT(3));
__asm__ __volatile__("nop");
if (bit_get(KBDpin,BIT(4))==0) {KPDResult++;}

Regards,

Carlos.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Ok, ok, I'll put in better comments. I'm used to making them so that I could read them. I know it isnt good practice, but I don't usually intend for other people to read my code.

You appear to be under the impression I have suggested writing comments for other people to read. Actually, I think you should be quite selfish and write comments entirely for your own gain. Writing plain-English comments that convey what you are trying to do, and why, helps you think through the concepts and strategies behind your code and helps catch such errors. Later, it also help you understand your code as you maintain and revise it, since code by itself usually only conveys how you did something.

That such sort of commenting often helps others understand your code is in your best interest as well. They will ask fewer questions about it and you can get more work done! That it may also happen to benefit them, need not concern you. :wink:

- John

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser: I would suggest that you learn C the easy way...i.e. write C programs to run on your PC. Then....revert back to avrgcc when you have more confidence.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
BTW, why was my old thread locked??

It was locked to prevent me from making any more offensive off-subject posts.

--Steve

Life is sweet in Sugar Land.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jfiresto wrote:
abcminiuser wrote:
Ok, ok, I'll put in better comments. I'm used to making them so that I could read them. I know it isnt good practice, but I don't usually intend for other people to read my code.

You appear to be under the impression I have suggested writing comments for other people to read. Actually, I think you should be quite selfish and write comments entirely for your own gain. Writing plain-English comments that convey what you are trying to do, and why, helps you think through the concepts and strategies behind your code and helps catch such errors. Later, it also help you understand your code as you maintain and revise it, since code by itself usually only conveys how you did something.

That such sort of commenting often helps others understand your code is in your best interest as well. They will ask fewer questions about it and you can get more work done! That it may also happen to benefit them, need not concern you. :wink:


What makes you think that it was your idea for abcminiuser to add more comments? I think you'll find that I made that suggestion first! (at least within the confines of this thread(insert noise for clashing egos here(triple nested brackets, this is cool))) .

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Ok, ok, I'll put in better comments. I'm used to making them so that I could read them.

When you are writing a program, it's pretty clear that you understand what you are doing and that it's really quite obvious. Six months or a year later you may look at the same code and swear that it wasn't you that wrote it. Learning to write good comments comes from experience and one of the best ways is to revisit your old programs. Can you follow the comments? Do they seem as clear today as they did back then? Do they still represent what the code is doing after you have had to make some changes? I hate to admit the number of times, early in my career, that I would be working on an old program and spot what I thought were "obvious" errors, "fix" them and then discover later, "oh, that's why I did it that way". In a commercial environment, good comments can save a lot of money. Even for your own projects, they can save a lot of grief.

Dave

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Amen Dave!

I've referenced Code Complete in the past. In chapter 19 of this book, Steve McConnell devotes an entire chapter to Self-Documenting Code which mostly discusses effective commenting and commenting techniques.

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Don -
I had never heard of Code Complete until I read your post. However, after reading your comments and checking the reviews on Amazon.com, I ordered the book on the spot. Many thanks for the tip - I look forward to reading it.

Dave

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dean,

How about something like this:

// 4x4 keypad matrix on portb
// rows are on 0-3, cols on 4-7
// pin 0 = top row,  pin 3 = bottom row
// pin 4 = left col, pin 7 = right col
// note: that more than one key can be pressed!
// note: zero bits in result = rows or cols that were pressed
// result = 0xFF = no keys pressed
//          0xEE = left column,  top row
//          0x7E = right column, top row
//          0xE7 = left column,  bottom row
//          0x77 = right column, bottom row
//          0x9B = middle two keys on the second row

int KPDScan(void)
{
   int KPDResult;               // Declare integer variable

   // read rows
   DDRB  = 0x0F;                // cols=output, rows=input
   PORTB = 0x0F;                // drive cols low and enable input pullups
   __asm__ __volatile__("nop"); // wait for it...
   __asm__ __volatile__("nop"); // wait for it... (once is probably enough)
   KPDResult = (PINB & 0x0F);   // read pins, mask off high bits and store low bits
                                // here note bit-wise AND with mask (0x0F) and
                                // plane old assignment so we dont need to assign
                                // an initial value to the result

   // read cols
   DDRB  = 0xF0;                // rows=output, cols=input
   PORTB = 0xF0;                // drive rows low and enable input pullups
   __asm__ __volatile__("nop"); // wait for it...
   __asm__ __volatile__("nop"); // wait for it... (once is probably enough)
   KPDResult |= (PINB & 0xF0);  // read pins, mask off low bits and store high bits
                                // here note bit-wise AND again but with high nibble
                                // mask (0xF0) this time and bit-wise OR with existing
                                // result to add the column result

   return KPDResult;            // Give the result back to the calling function 

   // you could invert the logic back to ones for pressed cols and rows
   // then you would get something like
   // result = 0x00 = no keys pressed
   //          0x11 = left column,  top row
   //          0x81 = right column, top row
   //          0x18 = left column,  bottom row
   //          0x88 = right column, bottom row
   //          0x62 = middle two keys on the second row
   //return !KPDResult;           // invert the bits then Give the result back to caller
}

--Steve

Life is sweet in Sugar Land.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hey,

Thanks for the code. Rather than copy somone elses, i'd rather get my own to work. Any old shmuck can copy code, only an arteest can write it!

Yes, I HAVE had thoses commenting problems. I "optimised" my code once - and then spent an hour tracking down a bug that was the result of me "fixing" up some dodgy code, only to forget how it worked!!

Anywho, i'm in the library and have in my hot little hand a book on writing C (for computers). I WONT start on computers yet however (although I shall forgo my Visual basic in favour of C) as I want to get my micro programming off the ground. Stary tuned for less dumb questions.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

John_A_Brown wrote:

What makes you think that it was your idea for abcminiuser to add more comments? I think you'll find that I made that suggestion first! (at least within the confines of this thread(insert noise for clashing egos here(triple nested brackets, this is cool))) .

I don't - you were definitely first!

If you recheck what I wrote, I think you will find I didn't ask for more comments: I first suggested writing plain-English comments as a way to debug code and then I responded to abcminiusers's remark about writing better ones.

If you look at the sum of our posts, I think you'll find we are working as a team arguing for more and better comments. :)

- John

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

John_A_Brown wrote:
(at least within the confines of this thread(insert noise for clashing egos here(triple nested brackets, this is cool))) .

Those are paranthesis :)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

NogginGTS wrote:

Those are paranthesis :)

parenthesis :twisted:

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Thanks for the code. Rather than copy somone elses, i'd rather get my own to work. Any old shmuck can copy code, only an arteest can write it!

You are welcome. But I did not mean for you to actually just copy it, but rather to use it to gain a little understanding of the ANDs, ORs and bit manipulation. Just wanted to show you a different way to get the job done.

--Steve

Life is sweet in Sugar Land.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The plural is actually parenthesEs, so you're all wrong. In Brtish english we refer to parentheses as brackets, although I would agree that in C one should use the correct terms.

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmm, kudos for the code, I think I may have found the solution:

Your code uses identicle DDR and PORT declarations (DDRB = 0x0F, PORTB = 0x0F), but my code uses inverted declarations (DDRB = 0x0F, PORTB = 0xF0). I though my way was correct (1's in the DDR for outputs, then 0's in the port to make them Low). What's wrong with my logic?

[OT Rant]
I can't test the code yet, as my secondhand Compaq Armada E500 laptop has developed the same major structual fault that I thought I fixed a month ago. Pity, as it seems i'll have to purchase another laptop for my next birthday. THIS time i'm avoiding Compaq's like the plague, and switching to a decent brand, like IBM.
[/Rant]

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
THIS time i'm avoiding Compaq's like the plague, and switching to a decent brand, like IBM.

Dean, my sentiments exactly!

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
THIS time i'm avoiding Compaq's like the plague, and switching to a decent brand, like IBM.

You might want to pick another brand - IBM is selling it's PC business :D

http://www.cnn.com/2004/BUSINESS...

Dave

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Your code uses identicle DDR and PORT declarations (DDRB = 0x0F, PORTB = 0x0F), but my code uses inverted declarations (DDRB = 0x0F, PORTB = 0xF0). I though my way was correct (1's in the DDR for outputs, then 0's in the port to make them Low). What's wrong with my logic?

You were right the first time: DDRB takes 1's for outputs and 0's for inputs.

PORTB on inputs takes 1's to turn inputs' pull-ups on. On outputs, it takes 0 to output a logic LOW (0 V, can sink no more than 200 mA) and 1 to output a logic HIGH (VCC; on an 8535, can source no more than 20 mA)

The problem is likely in the insufficient delay between adjusting DDR and PORT settings, and the subsequent polling of the PIN register -- you really have to insert some sort of delay there.

To do it elegantly:
Perhaps you could set up the registers for the first stage polling (rows) on system start-up (before you ever read the first key) then start the keypad scanning function directly by reading the PIN register. Store the PIN register in a temporary variable, then set up the DDR and PORT registers for reading the columns.

Next, perform the logic on evaluating the values read in the temporary variable from the row scanning. Inserting this logic here will give the PIN register time to settle on new levels.

Next, read the PIN register from the second-stage polling (columns) and evaluate the apporpriate logic.

Finally, pre-set the DDR and PORT registers for first-stage polling (rows) one last time at the end of the keypad scanning routine, so that they'll be ready for the next time you poll the keypad.

void SetupChip(void)
{
<...>
/* Set up Port B for our initial scan of the keypad.  We'll start by scanning the rows,
   so set pins 0, 1, 2, 3 as inputs (binary 0) and pins 4, 5, 6, 7 as outputs.  We will 
   turn on pull-up resistors on the input pins (binary 1) and set logic LOW output 
   in the output pins (binary 0). */
   DDRB = 0xF0;
   PORTB = 0x0F;
<...>
}

///ScanKeypad
///Precondition: The DDRB and PORTB registers must have already been set up to be
///in a state to poll the rows of the keypad.
///Returns an integer corresponding to the key that is currently pressed.  (0 = no key,
///or key # from 1 to 16)
///This function doesn't play well when more than one key is pressed at a time.
int ScanKeypad(void)
{
   unsigned char TempResult;
   int KeyPressed = 0;
   // Since the DDRB and PORTB registers are already set-up, we can read the PINB
   //register immediately.
   TempResult = PINB;

   // Set-up DDRB and PORTB for use in the column scanning.  Then, we'll evaluate the
   // result of the row scanning in the interim, so that the PINB register has time to
   // obtain a valid value.  Pins 0, 1, 2, 3 are set up as outputs with output level 
   // LOW.
   // Pins 4, 5, 6, 7 are set up as inputs wil pull-up resistors.
   DDRB = 0x0F;
   PORTB = 0xF0;

   // Now to evaluate the logic of the row scan.
   // The row scan will be used to determine an offset for the key press.
   // If the no buttons were pressed, or if a button was pressed in the first row 
   // (pin 0)  then the offset is 0.  (buttons 1,2,3,4, or NONE are possible.)

// This code is commented out because this option actually doesn't *do* 
// anything.  Left in the code to illustrate the logic.
// if(!(TempResult & (1<<PB0)))
//      KeyPressed = 0;
   // If the button was pressed on the second row (pin 1) then buttons 5, 6, 7, or 8 are
   // possible.
   if(!(TempResult & (1<<PB1)))
      KeyPressed = 4;
   // Ditto for rows 3 and 4 (pins 2 and 3) (buttons 9,10,11,12  or  13,14,15,16)
   if(!(TempResult & (1<<PB2)))
      KeyPressed = 8;
   if(!(TempResult & (1<<PB3)))
      KeyPressed = 12;

   // By this time, the PIN register has settled down on the new set of values for the
   // polling of the columns.  Evaluate its logic directly.
   // If col 1 was pressed, then the first button on the selected row was pushed.
   if(!(PINB & (1<<PB4)))
      KeyPressed += 1;
   // If col 2 was pressed, then the second button on the selected column was pushed.
   else if(!(PINB & (1<<PB5)))
      KeyPressed += 2;
   // Similar for columns 3 and 4.
   else if(!(PINB & (1<<PB6)))
      KeyPressed += 3;
   else if(!(PINB & (1<<PB7)))
      KeyPressed += 4;
   // If none of the above statements evaluates to true, then it must be that none of 
   // the keys were pressed.  In this case, KeyPressed = 0, so we know that 
   // there is no key currently pressed.

   // Finally, set up the DDR and PORT registers so they'll be ready for use the next
   // time we scan the keypad.  Pins 0,1,2,3 are inputs with pull-up turned on.  \
   // Pins 4,5,6,7 are outputs outputting a logic LOW.
   DDRB = 0xF0;
   PORTB = 0x0F;

   // And, we're done!
   return KeyPressed;
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

lfmorrison wrote:
abcminiuser wrote:
Your code uses identicle DDR and PORT declarations (DDRB = 0x0F, PORTB = 0x0F), but my code uses inverted declarations (DDRB = 0x0F, PORTB = 0xF0). I though my way was correct (1's in the DDR for outputs, then 0's in the port to make them Low). What's wrong with my logic?

You were right the first time: DDRB takes 1's for outputs and 0's for inputs.
[/code]

This is correct. Years of PIC coding got me here. In the PIC 0=output and 1=input in the direction register. It sort of makes an intuitive sense. In the AVR it's the opposite: 1=output and 0=input when used in the direction register. So my little code example should read:

// 4x4 keypad matrix on portb 
// rows are on 0-3, cols on 4-7 
// pin 0 = top row,  pin 3 = bottom row 
// pin 4 = left col, pin 7 = right col 
// note: that more than one key can be pressed! 
// note: zero bits in result = rows or cols that were pressed 
// result = 0xFF = no keys pressed 
//          0xEE = left column,  top row 
//          0x7E = right column, top row 
//          0xE7 = left column,  bottom row 
//          0x77 = right column, bottom row 
//          0x9B = middle two keys on the second row 

int KPDScan(void) 
{ 
   int KPDResult;               // Declare integer variable 

   // read rows 
   DDRB  = 0xF0;                // cols=output, rows=input 
   PORTB = 0x0F;                // drive cols low and enable input pullups 
   __asm__ __volatile__("nop"); // wait for it... 
   __asm__ __volatile__("nop"); // wait for it... (once is probably enough) 
   KPDResult = (PINB & 0x0F);   // read pins, mask off high bits and store low bits 
                                // here note bit-wise AND with mask (0x0F) and 
                                // plane old assignment so we dont need to assign 
                                // an initial value to the result 

   // read cols 
   DDRB  = 0x0F;                // rows=output, cols=input 
   PORTB = 0xF0;                // drive rows low and enable input pullups 
   __asm__ __volatile__("nop"); // wait for it... 
   __asm__ __volatile__("nop"); // wait for it... (once is probably enough) 
   KPDResult |= (PINB & 0xF0);  // read pins, mask off low bits and store high bits 
                                // here note bit-wise AND again but with high nibble 
                                // mask (0xF0) this time and bit-wise OR with existing 
                                // result to add the column result 

   return KPDResult;            // Give the result back to the calling function 

   // you could invert the logic back to ones for pressed cols and rows 
   // then you would get something like 
   // result = 0x00 = no keys pressed 
   //          0x11 = left column,  top row 
   //          0x81 = right column, top row 
   //          0x18 = left column,  bottom row 
   //          0x88 = right column, bottom row 
   //          0x62 = middle two keys on the second row 
   //return !KPDResult;           // invert the bits then Give the result back to caller 

It is also true that your problem is most likely just the delay between setting up the outputs/inputs and reading the results. Aside from any setup time needed by the chip itself, you may have some capacitive quality to the circuit that you have attached to the PORTB lines that needs time to settle. lfmorrison's code does a bit of work inbetween setting up the pins and reading the results and should allow ample time for settling.

--Steve

Life is sweet in Sugar Land.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hey,

[On Topic]
So, for once I was correct!? Hmmm, I actually made that mistakle originally (my first code had 1's and inputs, which I think makes more sense but there you go). My new code is almost identical to the code just posted by sbates - but I use a asm("NOP") after each port/ddr register change for a single cycle wait.

I don't want multi-key functionality, as the device I am making only requires one key at a time, plus it's easier to program. Much better to say "if (KeyPressed==1) {}" to see if the first key is pressed, than having to write a bit mask to see the same thing when multiple keys are pressed.
[/On Topic]

[Rant]
I'm only 15, so my budget for a modest second-hand computer is only about $900 or so after I save up (I paid $700 for my sec-hand Compaq Armada e500) and so it dosn't matter wherether IBM still wants to sell them brand new. Both my parents use IBM ThinkPads (from work) and although they are not the most attractive things in the world, they have excellent build quality and are reasonably fast. Compaq (now HP) make brilliant servers, etc. and so I reasoned (incorrectly) at the time of purchanse that this should translate into great quality laptops. I was wrong.

The IBM laptops use a VERY strong and smooth actioned metal hinge. This connected up to a metal frame in the chassis (thus anchoring it to the main body) and to a metal bracket running inside the plasic lid. This results in a very strong system for openign and closing the lid. Nothing short of a hammer could break the hinges.

Compaq's however use a similar system, except there's no metal bracket in the lid. The hinges are strong, but eventually they become stiff, requiring more force to actuate them. When you put preassure on the top of the lid (to close the laptop), this force is placed upon the hinges, and the place where the hinge is anchored to the lid. If there was a metal bracket to share the force, this wouldn't matter, but the engineers at Compaq decided to save the $0.30 and just sandwich the hinge between the two plastic parts of the lid. One side is thick, tho other thin. When you close the lid, eventually all the force cracks the thick side of the lid (so that the hinge is only anchored bu the thin front plastic) until eventually both break off and the whole thing tries to bend in two.

To repair the thing, you have to dissasemble the entire laptop without moving the screen, until you can seperate the damaged pieces, clean them and reattach with fibreglass or glue, or purchase a new plastic lid assembly for AU$80, which will only last about 9 months before the problem reoccurs. The hinge/lid assembly has broken AGAIN for me, even after cleaning and arolditing (super-super glue) the pieces back together. That means that the quick fix lasts a month or so.

It gotten to the point where i'd rather just rip the thing apart and sell the pieces off on ebay until I have enough funds (along with the money I have saved up) to purchase a decent brand, like an old IBM. I don;t care if the new system isn't any better (spec wise) than my old one, the Compaq's given MUCH more trouble than it's worth.
[/Rant]

Sorry for the long rant, but this has really irritated me and I had to complain to someone!

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

[Off Topic]

Ok, that's enough ranting for now. Suffice to say that i'm very unhappy with Compaq.

I've just (with the help of my dad and some super-super-super-super, etc. superglue) fixed the laptop, but havn't had a chance to try out the code. For now i'm stuck writing in C in notepad untill I jump back onto my laptop tomorrow morning.
[/Off Topic]

Ok, back on topic now. I've written the following C code for a 4x4 keypad scanning routine. Don't shout, it's almost identical to the one that sbates contributed, but I put it into (theoretically) working C and added a very crude debouncing routine. I decided in the end that I like his way better (with scan codes returned that allows multiple keys to be pressed at the same time) than my way, although I needed a way of turnign the scan codes into numercal variables. My keypad is as follows:

   1  2  3  Start
   4  5  6  Stop
   7  8  9  Enter
   .   0  <  >

But the scan routine:

/*
	      Keypad Scanning Routine
	         (Allows multiple keys)
	
	Original: sbates (AVRFreaks.net)
	Modified: Dean Camera

	4x4 Keypad - Matrix Configuration,
	         Colums are on pins 0-3,
	           Rows are on pins 4-7

	     === Key Values (in Hex) ===
	           0x11 0x21 0x41 0x81
	           0x12 0x22 0x42 0x82
	           0x14 0x24 0x44 0x84
	           0x18 0x28 0x48 0x88
	     ======================

	NB: For simultaneous keys, add key
	values together. The debounce routine
	is designed to be called in a loop (as
	opposed to an interrupt, as it uses a
	delay routine that does not allow other
	operations to be processed until finished.
*/

#include "KBDScan.h"

int KBDScanDebounce (void)
{
	int PrevKPD; // Declare integer variable for first keypad scan result
	
	PrevKPD = KBDScan(); // Scan the Keypad, save result in variable
	for (int i=1,i<101,i++) { _delay_loop_2(10000); } // Wait 100,000 cycles (equivalent to .1 sec on a 8mhz clock)
	
	If ( PrevKPD == KPDScan() ) { return PrevKPD; } // If results identical, return the pressed key(s)
}

int KBDScan (void)
{
	int KBDResult; // Declare integer variable for pin status
	
	KBDDDR = 0x0F; // Set colums as outputs, rows as inputs (binary 00001111)
	KBDPort = 0xF0; // Set outputs low, and enable pullups on the inputs (binary 11110000)
	asm("nop"); // Assembler command to wait a cycle, ensures port registers are ready
	
	KBDResult = (KBDPin & 0xF0); // Uses a bit mask (binary 11110000) to get the status of the
				// rows (bits 4-7 on the PIN register), stores in variable
	
	KBDDDR = 0xF0; // Set colums as inputs, rows as outputs (binary 11110000)
	KBDPort = 0x0F; // Set outputs low, and enable pullups on the inputs (binary 00001111)
	asm("nop"); // Assembler command to wait a cycle, ensures port registers are ready
	
	KBDResult |= (KBDPin & 0x0F); // Uses a bit mask (binary 00001111) to get the status of the
				// columns (bits 0-3 on the PIN register), stores in variable
				// without changing the lower bits set by the first assignment
	
	return !KBDResult; // Invert the bits and return the result - inverting makes for a better system,
			// as hex "0x00" for no key presses is better than hex "0xFF", etc.
}

only returns scan codes. This will make life harder when I try to a) see if the key that is pressed is one of the numericals, and b) manipulate the key's number (if the pressed key is numerical). I've partially solved this, using the routine:

int KeyIndex2PLDIndex (PressedKey as integer) // Converts the returned key index from keypad scanning
{				// sub into a new index that is easier to deal with in programming
	/*
	New button indexes (4x4 Keypad)
	==========================
		1    2    3    11
		4    5    6    12
		7    8    9    13
		10  0    14 15
		  No Key: 16
	==========================
	*/

	int NewKeyIndex = 16;

	// Numerics
	if (PressedKey == 0x28) NewKeyIndex = 0;
	if (PressedKey == 0x11) NewKeyIndex = 1;
	if (PressedKey == 0x21) NewKeyIndex = 2;
	if (PressedKey == 0x41) NewKeyIndex = 3;
	if (PressedKey == 0x12) NewKeyIndex = 4;
	if (PressedKey == 0x22) NewKeyIndex = 5;
	if (PressedKey == 0x42) NewKeyIndex = 6;
	if (PressedKey == 0x14) NewKeyIndex = 7;
	if (PressedKey == 0x24) NewKeyIndex = 8;
	if (PressedKey == 0x44) NewKeyIndex = 9;

	// Function Buttons
	if (PressedKey == 0x18) NewKeyIndex = 10;
	if (PressedKey == 0x81) NewKeyIndex = 11;
	if (PressedKey == 0x82) NewKeyIndex = 12;
	if (PressedKey == 0x84) NewKeyIndex = 13;
	if (PressedKey == 0x48) NewKeyIndex = 14;
	if (PressedKey == 0x88) NewKeyIndex = 15;

	return NewKeyIndex;
}

To modify the returned index into straight numbers (integers). Crude, I know, but can anyone suggest a better method? Also, i've learnt how to link my code (rather than include the .c files), but it seems I have to include the function's .h header in both the main C file (so it recognises the function names) and the actual functions .c code (so it knows all the #define commands, etc.). Is this normal, and will it resultin larger code?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

May I ask a question, Dean?
How does returning the sum of two scan codes added together help with anything?
If you add scancode 0x11 and 0x42, for example, you get 0x53.
But, if you add scancode 0x12 and 0x41, you also get 0x53.

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I just assumed - like I said, built from someone elses' code. This isn;t the case, as for some reason, the "middle two buttons in the second row" is supposed to give 0x62 as a result. How's this work?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hmm, apparently, you just "add" the binary together, without inverting bits:

0x22 and 0x42 <- Middle two buttons of second row

0x22 = 00100010
0x42 = 01000010

"Added" = 01100010 = 0x62

But your two examples give the same binary. sbates, comment! At least your being constructive, John, rather than flaming me.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've no desire to flame you.
The sbates thing works fine if the multiple keys pressed are in the same row or the same column, if they're not, then there will be ambiguity.
One thing to think about: However clever your software is, you will not be able to deal with more than two simultaneous keypresses. If three keys which form 3 corners of a rectangle are pressed at the same time, the 4th corner will appear to be pressed also. The way round this is to use diodes, but then you can't do the "rows as outputs, columns as inputs/rows as inputs, columns as outputs" trick.
Having said that, I've written a few keypad scanning routines in the past, and I've never bothered using that trick. It may save a few bytes of code.
I've always done it the long-winded way, ie:

1) Make all the rows inputs, with internal pullups.
2) Make ONE of the columns an output and drive it low.
3) Wait for a couple of NOPs
4) Read the row inputs, check if any ar low, you can exit the loop if they are and you don't care about multiple keys.

5)Make the next ONE column an output and drive it low.
Repeat steps 3 and 4 as required.

Now, if you have a 2 dimensional array of keypress codes, you can assign pretty much any keycode to any key, by using the selected column as one subscript, and the row which is low as the other.

I'm not going to write the code for you, as I have code of my own to write. Unless you want to write me some code to simulaneously send two DiSEqC messages whilst receiving and decoding a third.

Four legs good, two legs bad, three legs stable.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:

return !KBDResult; // Invert the bits and return the result - inverting makes for a better system,
// as hex "0x00" for no key presses is better than hex "0xFF", etc.

only returns scan codes. This will make life harder when I try to a) see if the key that is pressed is one of the numericals, and b) manipulate the key's number (if the pressed key is numerical). I've partially solved this, using the routine:


Not quite. The ! operator does a logical negation, not bitwise. If *any* bits are set in the original KBDResult, then the return value will be zero. If *no* bits are set in the original KPDResult, then the return value is undefined, but guaranteed to be non-zero. You want to use the bitwise NOT operator, ~ (tilde).

You'd also be better off masking off the upper 8 bits of the int (ints are 16-bits in avr-gcc) because you'll always have 0xFFxx as return values for your function. You could do this in one of two simple ways: Either change the return type of the function to an 8-bit int ('unsigned char' comes to mind, or uint8_t if you #include ) or mask the bits before returning them.

// Option 1: change return type
unsigned char KBDScan(void)
{
<...>
   return ~KBDResult;
}

// Option 2: mask unused bits
int KBDScan(void)
{
<...>
   return ((~KBDResult) & 0x00FF);
}

As for making unique scan codes for multiple keys pressed at a time... That is quite a bit more complicated than it seems at first. No good algorithm comes immediately to mind. (I've tried running a couple through my mind for a couple of days now, but they all explode in complexity before I get too far)

It'd be much easier to live with the restriction of only 1 key at a time.

abcminiuser wrote:

Also, i've learnt how to link my code (rather than include the .c files), but it seems I have to include the function's .h header in both the main C file (so it recognises the function names) and the actual functions .c code (so it knows all the #define commands, etc.). Is this normal, and will it resultin larger code?

Yes, it's normal. Nothing inside a '.h' file should ever actually turn into compiled code at all. Specifically, #defines are usually just macros that the compiler uses to replace certain strings of text that it encounters in subsequent lines of code -- if you do:

#define MY_MACRO(_a, _b) ((unsigned int)_a & (1<<(unsigned int)_b))

and then later do:

result = MY_MARCO(x, y);

then the compiler will 'see':

result = <...aha!  There is a macro definition here.  I'll look it up and substitute whatever I find.  Let's see... MY_MACRO is defined to be (a & (1<<b)), so I'll just drop that in there.>
result = (x & (1<<y));

If there are some things that you want #defined inside one module, but you don't want the outside world to see them, then make two .h files for your module. One will be a public one, containing only those things that you want the outside world to be able to access. The other will be a private one, contianing only those things that you don't want the outside world to access. Include both the public and private headers in the module's .c file. Only include the public header in the mainline's .c file.

-Luke

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

[pickypicky]

Quote:
If *no* bits are set in the original KPDResult, then the return value
is undefined, but guaranteed to be non-zero.

Result in this case is defined to be 1.

[/pickypicky]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

lfmorrison wrote:
abcminiuser wrote:

return !KBDResult; // Invert the bits and return the result - inverting makes for a better system,
// as hex "0x00" for no key presses is better than hex "0xFF", etc.

only returns scan codes. This will make life harder when I try to a) see if the key that is pressed is one of the numericals, and b) manipulate the key's number (if the pressed key is numerical). I've partially solved this, using the routine:


Not quite. The ! operator does a logical negation, not bitwise. If *any* bits are set in the original KBDResult, then the return value will be zero. If *no* bits are set in the original KPDResult, then the return value is undefined, but guaranteed to be non-zero. You want to use the bitwise NOT operator, ~ (tilde).

OK, now I'm a bonehead! "!" is indeed a logical negation, "~" is the bitwise negation. Use "return ~KBDResult;" to get the bits inverted properly.

Also in avr-gcc "int" is indeed 16 bits. Either of lfmorrison's suggestions are valid:

lfmorrison wrote:

// Option 1: change return type
unsigned char KBDScan(void)
{
<...>
   return ~KBDResult;
}

// Option 2: mask unused bits
int KBDScan(void)
{
<...>
   return ((~KBDResult) & 0x00FF);
}

IMHO Option 1 is better.

abcminiuser wrote:

   4x4 Keypad - Matrix Configuration, 
            Colums are on pins 0-3, 
              Rows are on pins 4-7 

        === Key Values (in Hex) === 
              0x11 0x21 0x41 0x81 
              0x12 0x22 0x42 0x82 
              0x14 0x24 0x44 0x84 
              0x18 0x28 0x48 0x88 
        ====================== 

   NB: For simultaneous keys, add key 
   values together. The debounce routine 
   is designed to be called in a loop (as 
   opposed to an interrupt, as it uses a 
   delay routine that does not allow other 
   operations to be processed until finished. 

Don't ADD them OR them. If you ADD the codes for key 1 (0x11) and 2 (0x21) you get 0x32. That's wrong. If you OR them you get 0x31. That's the scan code you will get back: 00110001 or columns 1 and 2 and row 1. there is possible ambiguity though: if I press keys 1,2 and 4 the scan code will be (0x11 | 0x21 | 0x12) = 0x33 = 00110011 = row 1 and 2 and column 1 and 2. You get the same scan code for 1,2 and 5 or even just 1 and 5. If you really want multiple keys and you want to make sure you always get it correct, you can "scan" the rows and columns one at a time instead of all four at once. This simple method does work for many key combinations, namely any single row or single column key combo. It also works well to detect that multiple keys ARE pressed in order to rule them out. If you get a scan code that is non-zero but is not in your table, then you have multiple keys pressed and can complain or ignore the keypress as being ambiguous. I have also used this trick to enter special modes or do special tricks. For example, you could code to detect all four keys on the top row (scan code 0xF1) to cause a RESET or to send a special status message or ... you get the idea.

So, even without going to the "scan one row at a time" level, you can still make some use of multiple keys pressed and be correct about it as long as you are looking for specific single row or single column multi-key scan codes.

Also, your lookup table would be more efficient if you use a case (switch) statement something like this:

// Converts the returned key index from keypad scanning 
// sub into a new index that is easier to deal with in programming 
unsigned char KeyIndex2PLDIndex (unsigned char KeyScanCode)
{
   /* 
   New button indexes (4x4 Keypad) 
   ========================== 
      1    2    3    11 
      4    5    6    12 
      7    8    9    13 
      10  0    14 15 
        No Key: 16 
   ========================== 
   */ 

   unsigned char NewKeyIndex;

   switch (KeyScanCode)
   {
     // Numerics 
     case 0x28: NewKeyIndex = 0;  break;
     case 0x11: NewKeyIndex = 1;  break;
     case 0x21: NewKeyIndex = 2;  break;
     case 0x41: NewKeyIndex = 3;  break;
     case 0x12: NewKeyIndex = 4;  break;
     case 0x22: NewKeyIndex = 5;  break;
     case 0x42: NewKeyIndex = 6;  break;
     case 0x14: NewKeyIndex = 7;  break;
     case 0x24: NewKeyIndex = 8;  break;
     case 0x44: NewKeyIndex = 9;  break;

     // Function keys
     case 0x18: NewKeyIndex = 10;  break;  // .
     case 0x81: NewKeyIndex = 11;  break;  // start
     case 0x82: NewKeyIndex = 12;  break;  // stop
     case 0x84: NewKeyIndex = 13;  break;  // enter
     case 0x48: NewKeyIndex = 14;  break;  // <
     case 0x88: NewKeyIndex = 15;  break;  // >

     // valid key combos
     case 0x17; NewKeyIndex = 17;  break;  // 1, 2, 3
     case 0x1F; NewKeyIndex = 18;  break;  // 1, 2, 3, start
     case 0xD1; NewKeyIndex = 19;  break;  // 1, 7, .
     case 0xF1; NewKeyIndex = 20;  break;  // 1, 4, 7, .
     case 0x4F; NewKeyIndex = 21;  break;  // 7, 8, 9, enter
     case 0x46; NewKeyIndex = 22;  break;  // 8, 9
     // you get the idea...

     // any other keycode including no key
     default:   NewKeyIndex = 16;
  }

  return NewKeyIndex; 
} 

--steve

Life is sweet in Sugar Land.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Speaking of making the keypad scanning process more efficient:

If you've got plenty of free EEPROM but code space is at a premium, you could consider placing a decoding array in EEPROM, and use the scan codes as the offset into the table.

For a scan code of 8 bits, you'd need 256 bytes of EEPROM to contain the entire decoding table, and you'd have to assign each of the 256 possible scan codes with the corresponding key meaning. (Note that most scan codes in the table would either be impossible, or would correspond to invalid key combinations. These scan codes would have to each be assigned the 'key value' of INVALID_KEY.) Then, you'd have a table that looks something like this:

// Definitions for 'special' key meanings.
#define INVALID_KEY 0xFF
#define DOT 10
#define START 11
#define STOP 12
#define ENTER 13
#define OPEN_BRACKET 14
#define CLOSE_BRACKET 15
#define ONE_TWO_THREE_CBO 17
#define ONE_SEVEN_DOT_CBO 19
(etc...)

const unsigned char KeyIndex2PLDIndex[256] __attribute__ ((section (".eeprom"))) = {
   INVALID_KEY, // Offset = Scancode = 0x00
   INVALID_KEY, // Offset = Scancode = 0x01
   INVALID_KEY, // 0x02
   <...> insert 8 more copies of the same 
   1,   // 0x11 - scan code for the '1' key
   4,   // 0x12 - scan code for the '4' key 
   ONE_FOUR_CBO, // 0x13 - '1' and '4' keys at once.
   7,   // 0x14 - scan code for the '7' key 
   ONE_SEVEN_CBO, // 0x15 - '1' and '7' at once
   FOUR_SEVEN_CBO, // 0x16 - '4' and '7' at once
   ONE_FOUR_SEVEN_CBO, // 0x17 - '1' '4' and '7' all at once
   DOT   // 0x18 - scan code for the '.' key
   ONE_DOT_CBO, // 0x19 - the '1' and '.' keys at once
   <...> make sure you have a total of 256 entries in this table when you're done.
}

// To decode the key, you do:
PLDIndex = eeprom_read_byte(&KeyIndex2PLDIndex[KeyScanCode]);

With this method, you do one function call, and can decode any key in constant time. I'd be willing to bet that, in most circumstances, the program-space overhead of using the eeprom_read_byte routine will be smaller than the cost of a similar table in the form of a 'switch' statement.

-Luke (Table-driven software is *fun*)

ps. Looking at this, I wonder: Do you really need to convert the scan codes to a more 'usable' format at all? You could just use #defines to identify the scan codes, so that you could just as easily have something like this:

#define DOT_KEY 0x18

<...>
if(ScanCode == DOT_KEY){...}

You'd just have to remember that, if you want to make use of the 'numeric' keys as part of a mathematical calculation, you'd have to substitute in the actual numeric value of the keys.

How about, a small switch-based table (funny, I've always seen my if-elseif statements take up consistently less code than equivalent switch-case statements in my AVR projects) that only converts the numeric buttons down to their literal values (0..9 or 1..10, however you want to look at it) and leaves the rest of the scan codes where they are.

A slight modification of Dean's code from a few posts up would take care of this. (By taking away the 'default' value at the top of the function) You can use #defines to check for the presence of any other valid scan code you may wish to use.

At the end of the user input interpretation routine, any scan codes that are not handled by the control software could just be ignored, with need for imposing 'default' values.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ok, I know your all tired of me, so i'll get this over with.

I took some of your suggestions on board, leaving others for later when I actually have some of the code working. I re-wrote all the code again, so this time it probably will work in both theory and practice. Plus it's also neater and nicer on the stack.

I've spent over 6 hours trying to get my code to work (i've tempoarily given up on the LCD and placed a UART code on the LCD routines for the moment) - staying up until 3AM on several days. The code correctly sends "Technichem PLD2", wait a few seconds, and then shows the main menu. Hitting the buttons dosn't seems to have any effect - except for one which acts as the ">" key even though it shoudl be the "<" key. I'm certain that this is due to a loose wire somewhere, but the enter key has no effect, even when on the "Manual Control" function - the corresponding subof which i've written.

So, could somone give it a final looksee and get me on track? Any other comments that havn't been said would be nice too...

BTW, i've added a list of what EEPROM locations will hold what data. Since there's a LOT of space left over, i'll eventually add the EEPROM key conversion table, but first things first (as "fat bastard" would say).

-------------------------------------------------------------------------------
BTW, in response to lfmorrison:

I only need to convert the keys into numerics for one or two subs, so i will just convert the numerics and leave the rest as #defines. As it is, i use #defines when the keys do not need to be numerics. At the moment, the keyscan sub returns 0x00 as no keys, rather than 0xFF (I originally preffered this) but i'll put it back as it would conflict with the actual key "0".

- Dean :twisted:

Attachment(s): 

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:

switch(CurrMenuFunc) // Run the subroutine that corresponds to the selected function
{
   case 1: // Function 1 = Dispense Liquid Mode
   case 2: // Function 2 = Manual Pump Control
      ManualControl(); // Execute the Manual Control Sub
   case 3: // Function 3 = Add Liquid to Memory
   case 4: // Function 4 = Delete Liquid from Memory
      DeleteLiquid(); // Execute the Delete Liquid Sub
}


Something pretty serious jumps out at me here. In a C switch, you have to have 'break's for all of you 'case's. (Unless you actually want the same pieces of code to run in more than one case.) The code you have above will behave like this: It will check the first 'case' and determine if the value being evaluated is equal. If not, it will move on to the next one. As soon as it finds the first instance of a value that is equal, it will begin executing code. Then execution will then continue, ignoring all the rest of 'case' lines, until it either reaches the closing brace of the switch, or until it reaches a 'break'.

If function 1 were selected at the time ENTER was pushed, you would find yourself running both ManualControl() and then DeleteLiquid() even though I get the feeling you don't want to run either!

Better to code it like this:

switch(CurrMenuFunc) // Run the subroutine that corresponds to the selected function
{
   case 1: // Function 1 = Dispense Liquid Mode
      // Presumably, you'll put the desired function for 'Dispense Liquid Mode' here.
      break;
   case 2: // Function 2 = Manual Pump Control
      ManualControl(); // Execute the Manual Control Sub
      break;
   case 3: // Function 3 = Add Liquid to Memory
      break;
   case 4: // Function 4 = Delete Liquid from Memory
      DeleteLiquid(); // Execute the Delete Liquid Sub
      break;
   default: // Not necessary here, but might be useful for debugging purposes.
      WriteErrorMessage(); // Write message explaining that an impossible value
     // is selected forCurrMenuFunc.
}

- Luke

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Regarding communication with the LCD:

I can only assume that you have the bit locations set correctly for the LCD driver, according to the definitions in the lcd header file. As I recall, you had first started working with it before having recognised the difference in the bit ordering of gcc vs. Bascom. Better to define the bit locations explicitly:

#define DB7        PD5
#define DB6        PD4
#define DB5        PD3
#define DB4        PD2
#define ENABLE PD6
#define RS          PD7
etc...

send data like so:
LCDPORT |= (1<<DB7);
LCDPORT &= (~(1<<ENABLE));
etc...

Looking over the data sheet, I see that the 'enable' cycle pulse width for communicating with the LCD controller is 500 ns minimum. That corresponds to a bus clock of 2 MHz. You say you're using an 4 MHz clock on your project.

Looking at the LCD driver code, I see that you always send the enable pulse like so:

LCDPORT |= ENABLE;
LCDPORT &= ~ENABLE;

With optimized code, both statements will result in only one op-code each, so the effective duration of the pulse is only 250 ns. You'd be safer inserting a one-cycle delay between the raising of the Enable signal, and the subsequent lowering of the signal, every time you need to send the Enable pulse.

// Using modified #defines above
LCDPORT |= (1<<ENABLE);
__asm__ __volatile__("nop"); 
LCDPORT &= (~(1<<ENABLE));

I also see that you are using dead reckoning to determine how long you have to wait before sending the next nibble out to the LCD controller. It is recommended that you poll the 'busy' flag between commands/data bytes, so that you can be sure that the LCD is ready to accept new info before you send the byte. It would abb to the complexity, so if you try doing it without the check and it works, by all means do it. But it is 'safer' to put the check in.

(That would involve changing the DDR of DB7 to 'input', waiting for the line to settle, then reading the PIN value of DB7, and continually waiting for it to be low before proceeding. Then you'd change the DDR of DB7 back to 'output' and proceed as normal).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ah, the case with "breaks" is interesting, however I can't enter ANY of the functions. I'll add the nessesary code, though.

For the LCD, I have set the pins correctly/explicitly. They should all be on PORTD. I am also using a 8MHz clock, so i'll need to quadruple the delay.

Incidentally, I swapped my shiny new 16x2 Dick Smith LCD for one of my many old 16x1 displays, which use the genuine HD-whatever chip. This now sucessfully clears, but fails to show any text. In rare cases, text does appear but it is just garbage.

Ok, doctors, diagnose! Why can't I enter any functions via the keyapd?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

re: getting the keypad to work...
Have you tried going back to the basic level of detecting a key-press, and making an LED flash? First, just have a LED toggle on when ever any key is pressed, stay on after the key is released, then toggle off again after the next time a key is pressed. This can test the electrical connectivity of each of the keys on the pad.

Next, have the LED flash from 1 to 16 times, depending on which key was pressed. You had been working on a program like that before. Have you tried it with the latest incarnation of your keypad driver?

You are successfully sending data via the serial port; how about a program that only sends out messages to a computer terminal that explicitly state the detected scan code associated with each key press, to make sure they line up with your expectations?

re: 'anything else I noticed'...

abcminiuser wrote:

const char MenuFunc0[2] = " \0";
const char MenuFunc1[13] = "Dispense    \0";
const char MenuFunc2[13] = "Manual Cntrl\0";
const char MenuFunc3[13] = "Add Liquid  \0";
const char MenuFunc4[13] = "Del Liquid  \0";
const char *MenuFuncNames[5] = {MenuFunc0, MenuFunc1, MenuFunc2, MenuFunc3, MenuFunc4};


The string declarations above aren't strictly correct. The NUL character at the end of each string is not necessary -- the double-quote (") delimiter automatically implies to gcc that a NUL character should be appended to the end of the string.

Take the string literal:
"This is a string"
Even though it looks like it's 15 characters long, it actually produces a character array 16 bytes long, that looks like this:
{'T', 'h', 'i', 's', ' ', 'i', 's', ' ', 'a', ' ', 's', 't', 'r', 'i', 'n', 'g', '\0'}

Also, you should be aware that the code above will actually produce the arrays in program memory, and also place copies of it in RAM. That's no problem if your project isn't close to using up all of the available RAM on the chip. However, if you want to be paranoid about keeping the RAM wasteage to a minimum, you might consider adding the PROGMEM attribute for each constant array, and ensuring that you only ever access the contents of the array using the pgm_read_byte() and pgm_read_word() macros. (Remember to #include )

const char MenuFunc0[2] PROGMEM  = " ";
const char MenuFunc1[13] PROGMEM = "Dispense    ";
const char MenuFunc2[13] PROGMEM = "Manual Cntrl";
const char MenuFunc3[13] PROGMEM = "Add Liquid  ";
const char MenuFunc4[13] PROGMEM = "Del Liquid  ";
const char *MenuFuncNames[5] PROGMEM = {MenuFunc0, MenuFunc1, MenuFunc2, MenuFunc3, MenuFunc4};

Similarly, take every string literal you use anywhere in your code, and switch them to predefined global PROGMEM arrays.

If it is necessary to also display some dynamically-generated strings (such as integers that you convert into decimal numbers) then you have a couple of choices:

Either create two versions of the LCD string display function, one for RAM strings, and one for Flash strings. (Fastest; smallest RAM usage; largest pgmspace usage)

Otherwise, you could have a single function that takes an additional argument that specifies whether the string resides in program memory or RAM. (possibly easier to manage, equally efficient for RAM usage)

Otherwise, you could create only one function, which displays only strings in RAM, and keep a 'buffer' string in RAM that you can use for both dynamically created strings, and for copying program-space strings into. (Smallest pgmspace usage; slower because you have to strcpy_P() all of the pgmspace strings each time you want to use them.)

Instead of accessing the string using something like

void send_string(char* str)
{
   while(*str != '\0')
      send_char(*(str++));
}

to access the contents of a string (that is in RAM)
you would then access the string using

// Option 1: This function will be used in addition to the function above;
// only used for PROGMEM strings.  For dynamically-created strings
// that reside in RAM, use the function in the code box above.
void send_string_P(char* str)
{
   char temp;
   while((temp = pgm_read_byte(str++)) != '\0')
      send_char(temp);
}

// Option 2: This function will be used for strings in both RAM and Progmem
// 'str' is a pointer to any string in RAM or program memory.
// 'offset' = 0 for RAM, non-zero for program memory
void send_string(char* str, unsigned char offset)
{
   char temp;
   while((temp = ((offset == 0) ? *(str++) : pgm_read_byte(str++))) != '\0')
      send_char(temp);
}

// Option 3: Keep the send_string function from the code box above.  When calling it with
// a string that lives in program memory, copy it to a temporary buffer before using it.
char buffer[21];

strcpy_P(buffer, MenuFunc1);
send_string(buffer);

Then, to send main menu string number 3, you'd do something like:

// Option 1:
void DisplayMenu(int menu_index)
{
   // 'menu_index' contains the number of the menu item we want to display.
   send_string_P((char*)pgm_read_word(&MenuFuncNames[menu_index]));
}

// Option 2:
void DisplayMenu(int menu_index)
{
   send_string((char*)pgm_read_word(&MenuFuncNames[menu_index]), 1);
}

// Option 3:
char buffer[21];
void DisplayMenu(int menu_index)
{
   strcpy_P(buffer, (char*)pgm_read_word(&MenuFuncNames[menu_index]));
   send_string(buffer);
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

By the way: I'm not a fan of placing anything in an '.h' file that actually turns into something that exists in compiled code.

For example, take the constant strings you use for the main menu text... every time you include that header in another file, you'll end up creating a new copy of those strings, just for that file. Ditto for the 'selected function' global variable. (In fact, unless they're declared as 'static' I think you'd actually have a compilation error for duplicate declarations of a single variable.)

The duplicate copies of constant strings can be an annoying waste of space. The duplicate copies of global variables that can change over time will result in incorrect functionality, since only the copy of the variable that exists within the current module will be changed, and the rest of the copies of the variable will remain untouched.

I'd suggest declaring and initializing all global variables in an actual '.c' file. Then, declare them with an 'extern' attribute in the appropriate '.h' file. Only one copy of the variable will be created in memory. (it happens in the single '.c' file)

The 'extern' declaration in the '.h' file will force the other modules to look for the actual instance of the variable elsewhere (in another global module).

// file: main.c
#define __main_c 1
#include "main.h"
<...>
// The global variables are created here.
char MainMenuFunc1[] PROGMEM = "This is a function.";
char MainMenuFunc2[] PROGMEM = "This is another function.";
char* MainMenuLookup[] PROGMEM = {MainMenuFunc1, MainMenuFunc2};
int MainMenuSelection = 1;

// variables that will be global to this module, but invisible to the outside world
// are marked as 'static'
static int PrivateVariable = 3;
<...>
---------------------------------

// file: main.h
<...>
// Fetch references to the global variables that exist in main.c:
#ifndef __main_c   // Protection from double-declarations when this
                              // file is included within main.c itself
extern char* MainMenuLookup PROGMEM;
extern int MainMenuSelection;
#endif

// This won't work, because it is declared as a static variable in main.c:
// extern int PrivateVariable;
<...>
---------------------------------

// file: secondary.c
#include "main.h" // Pull in references to the global variables created in main.c

static int PrivateVariable = 4; // This is a different variable, distinct 
// from the variable of the same name in main.c
---------------------------------
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Can we at least agree that my new code is far better than my original?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

abcminiuser wrote:
Can we at least agree that my new code is far better than my original?

Oh! I hope you're not taking my posts as harsh or mean-spirited! I'm just contributing little ideas here and there -- you're the one doing the hard work! For your first foray into a new language, I'd say you're making excellent progress.

If you'd like me to stop making suggestions (I could either stick with answering direct questions, or leave you alone entirely if you like!) just ask.

ps. Go Canada Go! Looks like gold in Hockey!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

lfmorrison wrote:

ps. Go Canada Go! Looks like gold in Hockey!

Yeah Baby! Pretty much glued to the tube tonight :p

@dean
You're doing fine, keep it up, and you'll be teaching us a thing or two before you know it.

Writing code is like having sex.... make one little mistake, and you're supporting it for life.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
If you'd like me to stop making suggestions (I could either stick with answering direct questions, or leave you alone entirely if you like!) just ask.

No no no! You misunderstand me! I couldn't possibly progress without the help and suggestions of the kind folks here. I was just wondering whether my new code was better than my first. If it wasn't I would have wanted to know how to write another version that woudl work. I just wanted to know if I was moving in the right direction.

By the way, the eeprom_read_byte routine only accepts uint_8 (or whatever it is, a single unsigned char) which allows a value of 0-255. What if I want to red a byte from the upper-half of the eeprom, that with addresses over 255? I know the eeprom high address register (single bit) should be set, but do I have to do this manually?

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

eeprom_read_byte() takes a *pointer* to a byte. The pointer itself is 16 bits. So, you don't have to do anything special, so long as the pointer is pointing to the right place.

It would be easiest to declare the variables (as EEPROM) in C files (see avr-libc documentation for info about memory sections and variable attributes), and then use the variable names as pointers for use in eeprom_read_byte().

eg:
char Name1[20] __attribute__ ((section (".eeprom"))) = "Insert Name Here";
or,
#define EEPROM __attribute__ ((section (".eeprom")))
(...)
char Name1[20] EEPROM = "Insert Name Here";

to read the fourth letter in the string:
eeprom_read_byte(&Name[3]);

to read the entire string:
unsigned char* ptr = Name1;
unsigned char temp;
while((temp = eeprom_read_byte(ptr++)) != '\0')
{
// do what you need to with the current charcter in 'temp'
};

- Luke

Pages