Why does my compiler forget a pointer ?

William Gaatjes · Mar 20, 2016

I have this code as seen below.
This code will hang because the pointer is forgotten it seems because of the switch case construction.
In the for loop, it will just write anywhere and will never return.

Code:

uint32_t ReadArrayAmg8831(amg8831_t *ps_ir)
{

	uint32_t 		r;
	uint32_t 		x;
	uint32_t		index;
	uint32_t		j;
	uint8_t	 		array[128]; 	
	volatile 		uint16_t		*pointer=0;

[COLOR="Red"]removed code here that reads sensor data into array.[/COLOR]

	switch(ps_ir->buffer_cntr)
	{
		case 0:
			pointer = ps_ir->traw_0;
		break;
		
		case 1:
			pointer = ps_ir->traw_1;
		break;

		case 2:
			pointer = ps_ir->traw_2;
		break;

		case 3:
			pointer = ps_ir->traw_3;
		break;
	}
	
	ps_ir->buffer_cntr++;
	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.
	
	index=0;
	for(j=0;j<64;j++)
	{
		r = array[index++];
		x = (array[index++] & 0xF);
		r = r | (x << 8);
		

		*pointer = (uint16_t)r;

		pointer++;

	}



	
	return 0;
}

But when i do it like this, disabling the switch construction and load the pointer directly, the pointer is not forgotten and the function works.

Code:

uint32_t ReadArrayAmg8831(amg8831_t *ps_ir)
{

	uint32_t 		r;
	uint32_t 		x;
	uint32_t		index;
	uint32_t		j;
	uint8_t	 		array[128]; 	
	volatile 		uint16_t		*pointer=0;

[COLOR="Red"]removed code here that reads sensor data into array.[/COLOR]

/*
	switch(ps_ir->buffer_cntr)
	{
		case 0:
			pointer = ps_ir->traw_0;
		break;
		
		case 1:
			pointer = ps_ir->traw_1;
		break;

		case 2:
			pointer = ps_ir->traw_2;
		break;

		case 3:
			pointer = ps_ir->traw_3;
		break;
	}
*/	

	ps_ir->buffer_cntr++;
	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.

	pointer = ps_ir->traw_0;
	
	index=0;
	for(j=0;j<64;j++)
	{
		r = array[index++];
		x = (array[index++] & 0xF);
		r = r | (x << 8);
		

		*pointer = (uint16_t)r;

		pointer++;

	}



	
	return 0;

The whole idea is that i have 4 raw value buffers that are constantly updated into a circular fashion :

Code:

// AMG8831 struct.
//
typedef struct s_amg8831
{																			// 4 buffers for averaging the temperature or for detection. 	
	uint16_t					traw_0[64];				// Contains raw sensor data. 
	uint16_t					traw_1[64];				// Contains raw sensor data.
	uint16_t					traw_2[64];				// Contains raw sensor data.
	uint16_t					traw_3[64];				// Contains raw sensor data.
	uint16_t					temperature[64];	// COntains temperature in celcius.
	uint8_t						buffer_cntr;			// This counter counts from 0 up to 3. Is used by read array to select the right raw value buffer.
}amg8831_t;

What am i doing wrong ? Why is the compiler forgetting the pointer in the for loop ?

William Gaatjes · Mar 20, 2016

I think i found it in the assembly listing.
In the switch case construction, R2 (the pointer, is loaded with the address stored in R0). The address in R0 is the address of the struct.
But it seems it never gets there since there being jumped to .L27 , skipping that instruction.
This is so weird.

For some reason, the compiler is not aware that the pointer is being loaded with a value in in the switch case construction and prior for the for loop, the pointer R2 is reloaded with 0.

Does anybody know how solve this ?

Code:

316              		.loc 1 161 0 is_stmt 1
 317 00a0 8032D0E5 		ldrb	r3, [r0, #640]	@ zero_extendqisi2	@ D.6312, ps_ir_32(D)->buffer_cntr
 318 00a4 030053E3 		cmp	r3, #3	@ D.6312,
 319 00a8 03F19F97 		ldrls	pc, [pc, r3, asl #2]	@ D.6312
 320 00ac 0B0000EA 		b	.L34	@
 321              	.L32:
 322 00b0 C0000000 		.word	.L28
 323 00b4 C8000000 		.word	.L29
 324 00b8 D0000000 		.word	.L30
 325 00bc D8000000 		.word	.L31
 326              	.L28:
 162:src/amg8831.c **** 	{
 163:src/amg8831.c **** 		case 0:
 164:src/amg8831.c **** 			pointer = ps_ir->traw_0;
 327              		.loc 1 164 0
 328 00c0 0020A0E1 		mov	r2, r0	@ pointer, ps_ir
 329              	.LVL14:
 165:src/amg8831.c **** 		break;
 330              		.loc 1 165 0
 331 00c4 060000EA 		b	.L27	@
 332              	.LVL15:
 333              	.L29:
 166:src/amg8831.c **** 		
 167:src/amg8831.c **** 		case 1:
 168:src/amg8831.c **** 			pointer = ps_ir->traw_1;
 334              		.loc 1 168 0
 335 00c8 802080E2 		add	r2, r0, #128	@ pointer, ps_ir,
 336              	.LVL16:
 169:src/amg8831.c **** 		break;
 337              		.loc 1 169 0
 338 00cc 040000EA 		b	.L27	@
 339              	.LVL17:
ARM GAS  C:\Users\William\AppData\Local\Temp\cchtxe01.s 			page 10


 340              	.L30:
 170:src/amg8831.c **** 
 171:src/amg8831.c **** 		case 2:
 172:src/amg8831.c **** 			pointer = ps_ir->traw_2;
 341              		.loc 1 172 0
 342 00d0 012C80E2 		add	r2, r0, #256	@ pointer, ps_ir,
 343              	.LVL18:
 173:src/amg8831.c **** 		break;
 344              		.loc 1 173 0
 345 00d4 020000EA 		b	.L27	@
 346              	.LVL19:
 347              	.L31:
 174:src/amg8831.c **** 
 175:src/amg8831.c **** 		case 3:
 176:src/amg8831.c **** 			pointer = ps_ir->traw_3;
 348              		.loc 1 176 0
 349 00d8 062D80E2 		add	r2, r0, #384	@ pointer, ps_ir,
 350              	.LVL20:
 177:src/amg8831.c **** 		break;
 351              		.loc 1 177 0
 352 00dc 000000EA 		b	.L27	@
 353              	.LVL21:
 354              	.L34:
 123:src/amg8831.c **** 	volatile 		uint16_t		*pointer=0;
 355              		.loc 1 123 0
[COLOR="Red"] 356 00e0 0020A0E3 		mov	r2, #0	@ pointer,[/COLOR]
 357              	.LVL22:
 358              	.L27:
 178:src/amg8831.c **** 	}
 179:src/amg8831.c **** 
 180:src/amg8831.c **** 	//PrintString("address %h.\r\n",(uint32_t)pointer);
 181:src/amg8831.c **** 	//PrintString("cntr %d.\r\n",(uint32_t)ps_ir->buffer_cntr);
 182:src/amg8831.c **** 	
 183:src/amg8831.c **** 	ps_ir->buffer_cntr++;
 359              		.loc 1 183 0
 360 00e4 013083E2 		add	r3, r3, #1	@ tmp247, D.6312,
 184:src/amg8831.c **** 	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.
 361              		.loc 1 184 0
 362 00e8 033003E2 		and	r3, r3, #3	@ tmp249, tmp247,
 363 00ec 8032C0E5 		strb	r3, [r0, #640]	@ tmp249, ps_ir_32(D)->buffer_cntr
 364              	.LVL23:
 365 00f0 0D30A0E1 		mov	r3, sp	@ ivtmp.26,
 115:src/amg8831.c **** uint32_t ReadArrayAmg8831(amg8831_t *ps_ir)
 366              		.loc 1 115 0
 367 00f4 80C08DE2 		add	ip, sp, #128	@ D.6382,,
 368              	.LVL24:
 369              	.L33:

William Gaatjes · Mar 20, 2016

Well, that is weird. I just removed my debug code that prints the values and it works.

I just do not get it.

Merad · Mar 20, 2016

Use a 2d array.
Why is "pointer" volatile? Volatile should be used for a variable that aliases a hardware resource that may change unexpected, or a value that may be changed in another thread (or interrupt)
You're probably overflowing a buffer somewhere in that copy loop, which is smashing the stack so the loop never completed. Changing the debug code probably changes the memory layout so that the effects are hidden.
Seriously, just use memcpy().

sao123 · Mar 20, 2016

Without knowing the structure of the amg8831_t struct, I am not sure that the 16bit pointer is the appropriate way to access the data of the struct.

Based on the documentation (pg20) for the amg8831 (assuming this is indeed the same sensor part you are using), I agree with Merad.
http://www.farnell.com/datasheets/1992433.pdf#page=20

Are you stripping out the out the ACK bits between each 8bit data segment for each pixel half? Otherwise 2 8bit data cells and the ACK between them are going to overflow a single 16bit location.

Schmide · Mar 20, 2016

Code:

	switch(ps_ir->buffer_cntr)
	{
		case 0:
			pointer = ps_ir->traw_0;
		break;
		
		case 1:
			pointer = ps_ir->traw_1;
		break;

		case 2:
			pointer = ps_ir->traw_2;
		break;

		case 3:
			pointer = ps_ir->traw_3;
		break;
	}
	
// why are you altering buffer_cntr after assigning the buffer?
// wouldn't this change the state of the controller and make your previous
// assignment inconsistent with its state?

	ps_ir->buffer_cntr++;
	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.

See inline comments.

Edit: The volatile declaration is not necessary and counter to the execution of code.

Volatile basically makes a variable cache friendly by loading from memory every time. Since this routine never calls nor interacts with other threads, it is ignored. The compiler most likely ignores this and if it didn't it would be a performance hit without any protection.

Edit2: In addition you prob want to do something like this to the buffer_ctrl. Read many write once with controllers.

Code:

	ps_ir->buffer_cntr = (ps_ir->buffer_cntr&(~3)) + ((ps_ir->buffer_cntr+1)&3);

In addition mask your initial switch.

Code:

	switch(ps_ir->buffer_cntr&3)

You never know if upper bits are used for special things. Better safe than sorry.

Schmide · Mar 21, 2016

William Gaatjes said:
I think i found it in the assembly listing.
In the switch case construction, R2 (the pointer, is loaded with the address stored in R0). The address in R0 is the address of the struct.
But it seems it never gets there since there being jumped to .L27 , skipping that instruction.
This is so weird.

For some reason, the compiler is not aware that the pointer is being loaded with a value in in the switch case construction and prior for the for loop, the pointer R2 is reloaded with 0.
Does anybody know how solve this ?

Code:

 317 00a0 8032D0E5 		ldrb	r3, [r0, #640]	@ zero_extendqisi2	@ D.6312, ps_ir_32(D)->buffer_cntr
 318 00a4 030053E3 		cmp	r3, #3	@ D.6312,
 319 00a8 03F19F97 		ldrls	pc, [pc, r3, asl #2]	@ D.6312
 320 00ac 0B0000EA 		b	.L34	@
 321              	.L32:
 322 00b0 C0000000 		.word	.L28
 323 00b4 C8000000 		.word	.L29
 324 00b8 D0000000 		.word	.L30
 325 00bc D8000000 		.word	.L31
 326              	.L28:

This is doing the correct thing. The instruction ldrls is load word if less than unsigned. Meaning it will load the address of the table following it and execute the correct case. (pc is always pc+4 in arm apparently for this exact reason) Else it jumps to the .L34 label and is set to zero.

William Gaatjes · Mar 21, 2016

To all :

The sensor is read out in 128 bytes, 64 16 bit values of TxxH and TxxL.
I combine these values into a single 16 bit value.
But since the Low byte and H byte are provided by the amg8831 in reverse order, it is not that easy to just combine them.
Hence the swapping. The value is a signed 12 bit value.

The struct you will not find in the amg8831 datasheet.
I came up with that myself. I am just fond of structs. And ARM cores are by design also found of it.

The volatile declared pointer is indeed not necessary, it was something i was trying. This code in red causes the function to hang because of doing something with the pointer :

Code:

uint32_t ReadArrayAmg8831(amg8831_t *ps_ir)
{

	uint32_t 		r;
	uint32_t 		x;
	uint8_t		index;
	uint8_t		j;
	uint8_t	 	array[128]; 	
	uint16_t		*pointer;
	
// read 128 bytes from amg8831 into array.
[COLOR="Red"]	code has been removed. [/COLOR]
	
	switch(ps_ir->buffer_cntr)
	{
		case 0:
			pointer = ps_ir->traw_0;
		break;
		
		case 1:
			pointer = ps_ir->traw_1;
		break;

		case 2:
			pointer = ps_ir->traw_2;
		break;

		case 3:
			pointer = ps_ir->traw_3;
		break;
		
		default:
		pointer = ps_ir->traw_0;
	}
		
	ps_ir->buffer_cntr++;
	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.
	
[COLOR="Red"]	// debug code that crashes code :
	PrintString("cntr : %h.\r\n",(uint32_t)ps_ir->buffer_cntr);
	PrintString("pntr : %h.\r\n",(uint32_t)pointer);
	[/COLOR]

	index=0;
	for(j=0;j<64;j++)
	{
		r = array[index++];
		x = (array[index++] & 0xF);
		r = r | (x << 8);

		*pointer = (uint16_t)r;

		pointer++;
	}
	return 0;
}

Without the debug code it works.

The struct has 16 bit values, so it must be half word aligned automatically on the stack. I still do not understand why it happens. But i had such an issue before that under some condition, the linker would not map a give variable half word or variable word to a 2 byte or 4 byte memory boundary. Eg 00h,02h,04h or 00h,04h,08h. But that is not the linker fault, but perhaps a fault in my linker script. The cpu does not like that and will cause a data exception.

I still have to look into it. I doubt the compiler does something wrong.
I am going to test it when i have time by temporary make it a global variable and see how it is alaigned in memory in the map file.

ps_ir->buffer_cntr is initialized at 0 and can never get higher than 3 because of the and after the addition.

Schmide · Mar 21, 2016

Are you trying to print to while in a ISR? Many systems use an interrupt to exec the output and you would be calling an interrupt within a interrupt.

TheRyuu · Mar 22, 2016

William Gaatjes said:

Code:

uint32_t ReadArrayAmg8831(amg8831_t *ps_ir)
{

	uint32_t 		r;
	uint32_t 		x;
	uint8_t		index;
	uint8_t		j;
	uint8_t	 	array[128]; 	
	uint16_t		*pointer;
	
// read 128 bytes from amg8831 into array.
[COLOR="Red"]	code has been removed. [/COLOR]
	
	switch(ps_ir->buffer_cntr)
	{
		case 0:
			pointer = ps_ir->traw_0;
		break;
		
		case 1:
			pointer = ps_ir->traw_1;
		break;

		case 2:
			pointer = ps_ir->traw_2;
		break;

		case 3:
			pointer = ps_ir->traw_3;
		break;
		
		default:
		pointer = ps_ir->traw_0;
	}
		
	ps_ir->buffer_cntr++;
	ps_ir->buffer_cntr &= 0x03; //Make circular buffer.
	
[COLOR="Red"]	// debug code that crashes code :
	PrintString("cntr : %h.\r\n",(uint32_t)ps_ir->buffer_cntr);
	PrintString("pntr : %h.\r\n",(uint32_t)pointer);
	[/COLOR]

	index=0;
	for(j=0;j<64;j++)
	{
		r = array[index++];
		x = (array[index++] & 0xF);
		r = r | (x << 8);

		*pointer = (uint16_t)r;

		pointer++;
	}
	return 0;
}

Is there any reason why you're not using uintptr_t for your pointer variable? Also this may be nit picking and I may be wrong since I'm unfamiliar with ARM but why declare index and j as special integer types? For counters like those provided you don't need a larger size you should just use int.

Also more nitpicking but provided their's not a universal style in your source with regards to variable declaration it's better to declare them right before their needed (within reason and logic of the code of course) instead of at the top of a function. But I understand if that's a global style choice and you're just following it.

Just some things I saw at first glance that stood out to me.

William Gaatjes · Mar 22, 2016

Schmide said:
Are you trying to print to while in a ISR? Many systems use an interrupt to exec the output and you would be calling an interrupt within a interrupt.

I do not use an interrupt for the uart tx in a classical way.
I use the dma controller from the uart to print. This way, i just have to write a tx buffer in memory and let the dma controller do the rest. When the dma controller is finished, i get one interrupt that the dma controller is finished.

William Gaatjes · Mar 22, 2016

TheRyuu said:
Is there any reason why you're not using uintptr_t for your pointer variable? Also this may be nit picking and I may be wrong since I'm unfamiliar with ARM but why declare index and j as special integer types? For counters like those provided you don't need a larger size you should just use int.

Also more nitpicking but provided their's not a universal style in your source with regards to variable declaration it's better to declare them right before their needed (within reason and logic of the code of course) instead of at the top of a function. But I understand if that's a global style choice and you're just following it.

Just some things I saw at first glance that stood out to me.

I use ansi c99 types to prevent confusion.

An int can be 8, 16 bit, 32 bit or even 64 bit depending on the cpu architecture.

When i write a function for a 32 bit mcu, and i would just use int for that function and then later on, port that same function to a 16 bit mcu, all the int are often reduced to 16 bit. This can cause issues that do not happen when using ansi c99 types. This increases portability of the code.

Variables declared in the top of a function are not global variables. Global variables are listed in the top of a c module.

And declaring the used variables in the top of a function is a clean way of programming. The compiler does not mind. When registers are available, the compiler will use those registers. And other wise, the stack is used when non are available.

uclabachelor · Mar 23, 2016

I see a couple of issues with your original code.

You have a uint16 variables and uint32 variables and are assigning pointers based on different variable types.

If your system is a 32-bit system, use strictly uint32. If it is a 16-bit system, use uint16.

I suspect the majority of your issues are due to type casting the pointers.

Merad · Mar 24, 2016

uclabachelor said:
If your system is a 32-bit system, use strictly uint32. If it is a 16-bit system, use uint16.

I think OP indicated he is working with a microcontroller. On those, many of the normal rules fly out the window.

For example, on many 8 bit systems I've worked with, everything integer related scales linearly with respect to the number of bytes in the integer. If you consistently use int32 where int16 would suffice, you are typically:

Doubling memory usage
Doubling the number of instructions needed to process each integer operation
Doubling the number of cycles needed for each operation
Doubling the number of registers needed for each operation (which in turn increases pressure on the register allocator, meaning more loads and stores as variables are moved into and out of registers, with each load/store in turn being more expensive)
Halving the number of function parameters that can be passed in registers before you have to overflow onto the stack (see above about more expensive loads and stores)

However, you're quite correct that pointer casting could be part of OP's problem.

Mark R · Mar 24, 2016

William Gaatjes said:
I
For some reason, the compiler is not aware that the pointer is being loaded with a value in in the switch case construction and prior for the for loop, the pointer R2 is reloaded with 0.
Does anybody know how solve this ?

No. The compiler is aware of the switch/case statement. The disassembly is correct. The pointer R2 is not reloaded with 0. The MOV R2, #0 is skipped if any one of the case statements is hit.

What seems to be happening is that the case statements are not getting hit, and the switch statement is falling through. I wonder if ps_ir->buffer_cntr is not properly initialized.

I'd suggest trying:

Code:

switch (ps_ir->buffer_cntr&0x03)

As this is guaranteed to hit one of the case statements.

TheRyuu · Mar 25, 2016

William Gaatjes said:
I use ansi c99 types to prevent confusion.

An int can be 8, 16 bit, 32 bit or even 64 bit depending on the cpu architecture.

When i write a function for a 32 bit mcu, and i would just use int for that function and then later on, port that same function to a 16 bit mcu, all the int are often reduced to 16 bit. This can cause issues that do not happen when using ansi c99 types. This increases portability of the code.

Ok well there are probably caveats that I'm unaware of when dealing with the stuff you're coding for (embedded/ARM/whatever it is) so that's fine I suppose. I really only have experience with C/C++ on x86.

I was only really commenting on why the counters are also using C99 types. I mean I guess it's fine if you're expecting the size of int to change that widely but shouldn't they be signed? Using a signed int for a counter (e.g. in a loop statement like you have) can allow the compiler to do a better job optimizing.

Also wouldn't uintptr_t increase portability? IIRC that's a C99 type.

William Gaatjes said:
Variables declared in the top of a function are not global variables. Global variables are listed in the top of a c module.

And declaring the used variables in the top of a function is a clean way of programming. The compiler does not mind. When registers are available, the compiler will use those registers. And other wise, the stack is used when non are available.

Yes I know they're not global, I was just commenting about the style which I just think the opposite produces cleaner code but whatever floats your boat is fine if it's consistent. Just ignore me on this one I probably shouldn't have brought it up.

Search

Why does my compiler forget a pointer ?

William Gaatjes

Lifer

William Gaatjes

Lifer

William Gaatjes

Lifer

Merad

Platinum Member

sao123

Lifer

Schmide

Diamond Member

Schmide

Diamond Member

William Gaatjes

Lifer

Schmide

Diamond Member

TheRyuu

Diamond Member

William Gaatjes

Lifer

William Gaatjes

Lifer

uclabachelor

Senior member

Merad

Platinum Member

Mark R

Diamond Member

TheRyuu

Diamond Member

TRENDING THREADS