Buttons don't work

Go To Last Post
12 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello,

I've connected 4 buttons to my avr mega 16 and they should be active low.
The code I use is:

        for(;;)	// Infinite loop
	{			
		
		AvrXDelay(&MyTimer3, 5);
		
		if( bit_is_clear( PORTC , PINC0 ) == 1 )	// check if low
		{
			AvrXDelay(&MyTimer3, 100);    // Dake care of disco stuff
			while( bit_is_clear( PORTC , PINC0 ) == 1 ) // Wait until released
			{
				AvrXDelay(&MyTimer3, 1);
			}

			AvrXSendMessage(&button,&right);
			AvrXWaitMessageAck(&right);
                 }
	
                 if( bit_is_clear( PORTC , PINC1 ) == 1 )	// check if low
		 {
			AvrXDelay(&MyTimer3, 100);    // Dake care of disco stuff
			while( bit_is_clear( PORTC , PINC1 ) == 1 ) // Wait until released
			{
				AvrXDelay(&MyTimer3, 1);
			}

			AvrXSendMessage(&button,&right);
			AvrXWaitMessageAck(&right);
                 }

                 if( bit_is_clear( PORTC , PINC2 ) == 1 )	// check if low
		{
			AvrXDelay(&MyTimer3, 100);    // Dake care of disco stuff
			while( bit_is_clear( PORTC , PINC2 ) == 1 ) // Wait until released
			{
				AvrXDelay(&MyTimer3, 1);
			}

			AvrXSendMessage(&button,&right);
			AvrXWaitMessageAck(&right);
                 }

                 if( bit_is_clear( PORTC , PINC3 ) == 1 )	// check if low
		{
			AvrXDelay(&MyTimer3, 100);    // Dake care of disco stuff
			while( bit_is_clear( PORTC , PINC3 ) == 1 ) // Wait until released
			{
				AvrXDelay(&MyTimer3, 1);
			}

			AvrXSendMessage(&button,&right);
			AvrXWaitMessageAck(&right);
                 }
         }

Now I've defined portc all inputs. And I've tested all buttons so that they work as they should but... How come only pinc0 works and no other pins? I've even tried this on porta and portd and it makes no difference.

Thank you very much.

/Gert

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

this appears to be incorrect

if( bit_is_clear( PORTC , PINC0 ) == 1 )   // check if low

you should be referencing PINC not PORTC

if( bit_is_clear( PINC , PINC0 ) == 1 )   // check if low

and again here

while( bit_is_clear( PORTC , PINC0 ) == 1 ) // Wait until released 

should be

while( bit_is_clear( PINC , PINC0 ) == 1 ) // Wait until released 

the way you've written it here, none of the routines will work correctly. they will all enter into the 'if' statement (assuming that the PORTC bit was last written to 0), and get stuck forever in the while loop, unless some ISR writes to the PORT register to change teh state of teh pull-up.

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

Hello again,

Yeah, your right! I forgot to change that back.
It does look like that already and it only works on the pinc0

/Gert

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

do you have the internal pull-ups enabled, or some external pull-up on the line, to pull the state high when the switch is open?

try writng 0xFF to PORTC at the top of this block, to see if it fixes the problem.

Also note that in all four cases you appear to take the exact same action. You do not have any changes to your messages, so all four buttons will generate the exact same result.

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

Well, I have external pull-ups and I've used the same set before so there shouldn't be no problem. And I've done some measurements on them to test them out as well. They do work as they should.

>>try writng 0xFF to PORTC at the top of thsi block, to see if it fixes teh problem.

Do you meen I should do a
outp(0xff,PORTC); .....?
Why do this?

/Gert

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

To enable the internal pull-ups. But if you do indeed have external pull-ups it wont make any difference.

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

ok! Thanks anyway.

If I'd put the
outo(0xff,PORTC);
in the code to enable the internal pull-ups, where in the code should I put it?

/Mr G

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

PORTC = 0xFF is generally regarded as a better than outp(0xff,PORTC);

Also, does bit_is_clear expect a bitmask or a shift value? I'm guessing a shift value, so what is PINC0 PINC1 PINC2 etc defined as? 0, 1, 2, 3, 4, .... or 0x01, 0x02, 0x04, 0x08, 0x10

try changing
if( bit_is_clear( PORTC , PINC0 ) == 1 ) to if ( ~PINC & (1<<0))
if( bit_is_clear( PORTC , PINC1 ) == 1 ) to if ( ~PINC & (1<<1))
if( bit_is_clear( PORTC , PINC2 ) == 1 ) to if ( ~PINC & (1<<2))

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

Does the bit_is_clear function return 0 or 1 or does it return zero or non-zero?

Instead of:

if ( bit_is_clear( PORTC, PINC0 ) == 1 )

I'd use

if ( bit_is_clear( PORTC, PINC0 ) != 0 )

Don

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

Hello again,

NogginGTS wrote:

Also, does bit_is_clear expect a bitmask or a shift value? I'm guessing a shift value, so what is PINC0 PINC1 PINC2 etc defined as? 0, 1, 2, 3, 4, .... or 0x01, 0x02, 0x04, 0x08, 0x10

Well, I guess you're right, it's a shift value. All is defined inputs.

I tried the changes you suggested...

NogginGTS wrote:

try changing
if( bit_is_clear( PORTC , PINC0 ) == 1 ) to if ( ~PINC & (1<<0))
if( bit_is_clear( PORTC , PINC1 ) == 1 ) to if ( ~PINC & (1<<1))
if( bit_is_clear( PORTC , PINC2 ) == 1 ) to if ( ~PINC & (1<<2))

...and it works like a charm. Thank you very much.

Thanx everybody for your time and quick responds.

Peace
-Gert

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

One more change that I'd suggest, between the two lines

AvrXDelay(&MyTimer3, 100);    // Dake care of disco stuff
while( bit_is_clear( PORTC , PINC0 ) == 1 ) // Wait until released 

you may need a 20-30ms delay, unless AvrXDelay is intended for debounce. Otherwise you could end up with a button being hit multiple times.

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

Ok thanx!

This is how it looks now. I changed the
~PINC & (1<<0)
to
~PINC & 0x01
Something I had a discussion aboud with a friend. It not only works the same way but is more clear.

                if( ~PINC & 0x01 )	// Right button pressed?
		{
			AvrXDelay(&ButtonTimer, 20);	// Don't care for disco
			while( ~PINC & 0x01 )		// Wait until button released
			{
				AvrXDelay(&ButtonTimer, 20);
			}

			AvrXSendMessage(&button,&right);	// Send message
			AvrXWaitMessageAck(&right);		// Wait ack!
		}