Porblem with 16bit uint comparison in 8bit AVR

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

Hello guys, I have been programming an ATMEGA16 using Atmel Studio 7 in assembly so far and I am now moving in C. The project I am on is not that hard but I fell into a minor problem. I have a simple function that takes a 16bit uint and only contains a switch. I noticed that it only works for the cases that involve the 8 LSB of the 16bit uint while every other case just goes to default. I suppose this has to do with the fact that ATMEGA16 is an 8bit controller and the switch involves a comparison which only occurs for the 8 LSB. Of course the simple solution would be converting my 16bit number into 2 8bit numbers and having 2 switches instead. So my questions are:

1) Atmel Studio really does not support comparisons with 16bit integers or it can be done with maybe some type casting?(I did try some type castings but I seemed to get the same result every time)

2) If it is not supported is there a smarter way than converting the number into 2 8bit numbers?

3) Is there an option that makes it show warnings for that? (which in my opinion should show up in the first place)

 

I am also attaching my code. Thanks for your time! smiley

 

uint8_t keypad_to_ascii_single_switch(uint16_t keyspressed)
{
	
	switch(keyspressed)
	{
		case 0x0001:
		return 'E';
		case 0x0002:
		return '0';
		case 0x0004:
		return 'F';
		case 0x0008:
		return 'D';
		case 0x0010:
		return '7';
		case 0x0020:
		return '8';
		case 0x0040:
		return '9';
		case 0x0080:
		return 'C';
		case 0x0100:
		return '4';
		case 0x0200:
		return '5';
		case 0x0400:
		return '6';
		case 0x0800:
		return 'B';
		case 0x1000:
		return '1';
		case 0x2000:
		return '2';
		case 0x4000:
		return '3';
		case 0x8000:
		return 'A';
		default:
		return 0;
	}

}

 

This topic has a solution.
Last Edited: Sat. Dec 30, 2017 - 09:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

mvaki wrote:
it only works for the cases that involve the 8 LSB of the 16bit uint while every other case just goes to default. I suppose this has to do with the fact that ATMEGA16 is an 8bit controller

There is absolutely no reason why a switch on a uint16_t should not work correctly - the 'C' compiler handles getting that right.

 

You need to post a minimum but complete program which illustrates the problem.

 

At a guess, I would say that the most likely problem is with the value you are passing in to the function - so how have you verified that it is correct?

 

The ATMega16 has on-chip debug - so the most obvious way would be to simply step into the function and see what's happening!

You could also do that in the simulator.

 

1) Atmel Studio really does not support comparisons with 16bit integers or it can be done with maybe some type casting?(I did try some type castings but I seemed to get the same result every time)

Of course Studio can handle this

 

 

I tend to find that a simple switch "lookup table" is more clearly written like this:

	switch(keyspressed)
	{
		case 0x0001: 	return 'E';
		case 0x0002: 	return '0';
		case 0x0004:	return 'F';
		case 0x0008:	return 'D';
		case 0x0010:	return '7';
		case 0x0020:	return '8';
		case 0x0040:	return '9';

etc, ...

 

EDIT

 

typo

Last Edited: Sat. Dec 30, 2017 - 12:10 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Show the invocation of the function. There may be some promotion/truncation happening there.

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

Yep - that's the kind of thing I was thinking of with, "the most likely problem is with the value you are passing in to the function" ...

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

Thanks for your instant reply awneil. I am having the 16bit number converted into 2 8bit numbers and displayed on LEDs to verify it is indeed a 16bit number. But you are right since adding the Number=0x1000; line or the keyspressed=0x8000; works as expected. However the displayed values at the LEDs in PORTA and PORTB are correct when using the number returned from scan_keypad_rising_edge() which is strange. I did use Studio's debug and the switch function was entered with Number=0x1000 and it selected the default case instead of the case 0x1000 as you can see in the pictures attached. So the program works fine if it is given a value but does not work as expected if the value is returned from the function scan_keypad_rising_edge() even though the value returned is a valid 16bit uint. Any ideas?

 

 

 

 

This is my complete code

 

#define F_CPU 8

#include <avr/io.h>
#include <util/delay.h>

uint16_t *PreviousNumber;
uint8_t scan_row(uint8_t rownumber);
uint16_t scan_keypad();
uint16_t scan_keypad_rising_edge();
uint8_t keypad_to_ascii_single_switch(uint16_t keyspressed);


int main(void)
{
    /* Replace with your application code */
    
    uint16_t Number=0;
    while (1) 
    {
    
    Number=scan_keypad_rising_edge();
    //Number=0x1000;
    if(Number)
    {
        DDRA=0xff;
        DDRB=0xff;
        DDRD=0xff;
        uint8_t NumberH,NumberL;
        NumberL=Number&0x00ff;
        NumberH=(Number>>8)&0x00ff;
        PORTA = NumberL;
        PORTB = NumberH;
        //uint16_t debug=0;
        
        PORTD = keypad_to_ascii_single_switch(Number);
    }
    
    _delay_ms(10);
    }
}

uint8_t keypad_to_ascii_single_switch(uint16_t keyspressed)
{
    //keyspressed=0x8000;
    
    switch(keyspressed)
    {
        case 0x0001:	return 'E';
        case 0x0002:	return '0';
        case 0x0004:	return 'F';
        case 0x0008:	return 'D';
        case 0x0010:	return '7';
        case 0x0020:	return '8';
        case 0x0040:	return '9';
        case 0x0080:	return 'C';
        case 0x0100:	return '4';
        case 0x0200:	return '5';
        case 0x0400:	return '6';
        case 0x0800:	return 'B';
        case 0x1000:	return '1';
        case 0x2000:	return '2';
        case 0x4000:	return '3';
        case 0x8000:	return 'A';
        default:	return 0;
    }

}

uint8_t scan_row(uint8_t rowNumber)
{
	DDRC=0b11110000;
	uint8_t row=0b00010000;
	for(uint8_t i=1;i<rowNumber;i++)
	{
		row=(row<<1);
	}
	PORTC=row;
	asm volatile ("nop");
	asm volatile ("nop");
	uint8_t readNumber=PINC&0b00001111;
	//PORTC = readNumber; debug
	return readNumber;
}

uint16_t scan_keypad()
{
	uint16_t ReadNumber=0;
	uint8_t rowNumber;
	
	for(rowNumber=1;rowNumber<=4;rowNumber++)
	{
		ReadNumber= (ReadNumber<<4);
		ReadNumber|=scan_row(rowNumber);//+= not correct. 
	}
	return ReadNumber;
}

uint16_t scan_keypad_rising_edge()
{
	
	uint16_t ReadNumber1,ReadNumber2,Previous;
	ReadNumber1=scan_keypad();
	_delay_ms(15);
	ReadNumber2=scan_keypad();
	ReadNumber2&=ReadNumber1;
	Previous= ~(*PreviousNumber);
	*PreviousNumber=ReadNumber2;
	ReadNumber2&=Previous;
	return ReadNumber2;
}

 

 

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

It might not be the problem, but where does "uint16_t *PreviousNumber" point to?

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

tedy wrote:

It might not be the problem, but where does "uint16_t *PreviousNumber" point to?

 

The idea is to store the PreviousNumber that was read from the keypad, complement it and then with a logical AND with the currently pressed buttons you should only have the ones pressed now. It is used only in the scan_keypad_rising_edge() function. It seems to be working fine (if I keep a button pressed and then press a new one only the value of the new one is returned)

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But that hasn't answered what it points to. You create a POINTER TO uint16_t but I don't see where you ever assign an address of a unit16_t to that pointer ? But later you write through the pointer with *PreviouNumber=xxx

 

Why is PreviousNumber even a pointer  in the first place? Why not a simple uint16_t?

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

clawson wrote:

But that hasn't answered what it points to. You create a POINTER TO uint16_t but I don't see where you ever assign an address of a unit16_t to that pointer ? But later you write through the pointer with *PreviouNumber=xxx

 

Why is PreviousNumber even a pointer  in the first place? Why not a simple uint16_t?

 

I was transferring my program from assembly to C and a pointer was used there but you are right it is not needed. Also making it a simple uint16_t fixes the problem laugh Thank you!!!!! And now I must ask... I thought that when you create a pointer to a uint16_t it is automatically assigned an address which is obviously wrong. How do you assign an address to the pointer?

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

mvaki wrote:
I thought that when you create a pointer to a uint16_t it is automatically assigned an address

So what made you think that?

 

which is obviously wrong.

Indeed!

 

How do you assign an address to the pointer?

Go on - what does your 'C' textbook say?

 

How about:

 

uint16_t   a_uint_16;

uint16_t * pointer_to_uint_16;

pointer_to_uint_16; = &a_uint_16;

Here are some 'C' learning & reference materials - including a free online textbook: http://blog.antronics.co.uk/2011...

 

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

One of the classic errors in C. Perhaps THE classic error is creating pointers that then do not have a valid target when you write through them. Creating pointers does NOT create assigned storage. You either need to malloc() or you need to use &some_object to assign to the pointer to give it the storage it can then read/write.

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

It sounds like the OP is confusing the storage for the pointer itself - which, of course, has to be created when the pointer is created - with the storage for whatever the pointer points to.

 

As you say, that's probably quite a common confusion/misunderstanding.

 

EDIT

 

See also: http://www.avrfreaks.net/comment...

Last Edited: Thu. Jan 11, 2018 - 10:18 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

mvaki wrote:
I thought that when you create a pointer to a uint16_t it is automatically assigned an address

So what made you think that?

 

which is obviously wrong.

Indeed!

 

How do you assign an address to the pointer?

Go on - what does your 'C' textbook say?

 

How about:

 

uint16_t   a_uint_16;

uint16_t * pointer_to_uint_16;

pointer_to_uint_16; = &a_uint_16;

Here are some 'C' learning & reference materials - including a free online textbook: http://blog.antronics.co.uk/2011...

 

 

I tried that before but I tried it right under the declaration (which is outside of functions) and I got compile errors. I added the address assignment inside main and now it works! Thank you smiley

 

awneil wrote:

It sounds like the OP is confusing the storage for the pointer itself - which, of course, has to be created when the pointer is created - with the storage for whatever the pointer points to.

 

As you say, that's probably quite a common confusion/misunderstanding.

 

 

 

To be honest it has been more than a year since I last used pointers so I needed some refreshing. The fact that the function that had to do with the pointer returned a result that seemed correct completely mislead me. That and the fact that at the debugging the contents of the location pointed by the pointer were correctly updated.

 

clawson wrote:

One of the classic errors in C. Perhaps THE classic error is creating pointers that then do not have a valid target when you write through them. Creating pointers does NOT create assigned storage. You either need to malloc() or you need to use &some_object to assign to the pointer to give it the storage it can then read/write.

 

Yes I get it now. Thanks for finding that mistake! I would have never found it  cheeky

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

mvaki wrote:
I tried it right under the declaration (which is outside of functions) and I got compile errors

Of course - because 'C' does not allow executable code outside of functions.

 

But you could have initialised it in the definition:

uint16_t   a_uint_16;

uint16_t * pointer_to_uint_16 = & a_uint_16;  // Definition with initialisation

 

To be honest it has been more than a year since I last used pointers

The above is not specific to pointers.

 

Definitely time for a re-read of that textbook...

 

wink

 

The fact that the function that had to do with the pointer returned a result that seemed correct completely mislead me.

Just because something appears to "work" doesn't necessarily mean it's correct!

 

"Beware the 'Proven Product' Syndrome"

 

eg, see: http://www.avrfreaks.net/comment...

 

 

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

Credit where credit is due. It was 'tedy' who actually spotted / fixed this. I simply pointed out you hadn't answered the question he asked.

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

awneil wrote:

 

 

Definitely time for a re-read of that textbook...

 

wink

 

 

 

That is 100% correct cheeky

 

clawson wrote:

Credit where credit is due. It was 'tedy' who actually spotted / fixed this. I simply pointed out you hadn't answered the question he asked.

 

Yes you are right!

 

 

 

tedy wrote:

It might not be the problem, but where does "uint16_t *PreviousNumber" point to?

 

Thank you tedy! You fixed my problem!laugh