nested FOR loops in C ?

ilmenator · October 31, 2006, 8:00pm

Hi all,

I am having a problem with a nested FOR loop in C which causes the PIC to reset itself. I am trying to copy the content of an SRAM card (32kB) to a bankstick. If I use only the inner loop for reading/writing 256 bytes, everything is smooth. As soon as I activate the outer loop, the app increases the [tt]bankstick_addr[/tt] counter to around 1736 -1741 (that is 0x06C8 to 0x06CD) and then crashes. The counter varies, but the crash is always there, and the core resets itself.

This is my code:

  // read whole SRAM card and copy to Bankstick
  else if (pin == 0x14){
	//  SRAM_Read_Page();
    bankstick_addr = 0x0000;
	MIOS_LCD_PrintCString("Copy Card to Bankstick  ");

	// outer loop increments the address highbyte
	for(card_h_addr=0x00; card_h_addr<0x7F; ++card_h_addr){
	  // inner loop increments the address lowbyte
	  for(card_l_addr=0x00; card_l_addr<0xFF; ++card_l_addr){
		SRAM_Read();
		error |= MIOS_BANKSTICK_Write(bankstick_addr, ram_byte);
		++bankstick_addr;

		if( error ) {
	      // here you could do some error handling
	      MIOS_LCD_CursorSet(0x00);
	      MIOS_LCD_PrintCString("Bankstick write Error    ");
	    }

		// print current address
		MIOS_LCD_CursorSet(0x19);
		MIOS_LCD_PrintCString("Address: ");
		MIOS_LCD_PrintBCD5(bankstick_addr);
		//app_flags.DISPLAY_UPDATE_REQ = 1;
	  }
	}

[tt]bankstick_addr[/tt] is a counter that determines the address in the bankstick where the byte shall be put; it’s of type unsigned int

[tt]card_l_addr[/tt] determines the low byte of the 16bit SRAM address; type unsigned char

[tt]card_h_addr[/tt] is the high byte of the 16bit SRAM address; also unsigned char

All three are global variables because they will get modified at various places. The LCD output is mainly for debugging.

Any thoughts on this?

Best regards, ilmenator

PS: Thanks stryd, you see your explanations about that assembler code actually got me here…

audiocommander · October 31, 2006, 8:49pm

Hi ilmenator,

first, it could be interesting what happens here:

SRAM_Read();
error |= MIOS_BANKSTICK_Write(bankstick_addr, ram_byte);

All three are global variables because they will get modified at various places.

have you declared these three variables as volatile? I remember I had various problems before I did so. See the Wiki->C Tipps&Tricks for info on volatile…

You mention, all three get modified at various places; can it be they get modified while this codesnippet is being processed? Where do you have that code, is it in an ISR; is there a timer that changes any of these three values?

have you taken a look in the generated .asm file in the /output directory? sometimes one can find a valueable hint there.

regards,

Michael

ps: I doubt it has something to do with your problem, but if you’d use MIOS_Bankstick_WritePage you’d save 64 function calls. TK told me once this is a lot faster than writing single bytes.

ilmenator · October 31, 2006, 10:02pm

Thanks Audiocommander,

SRAM_Read();

calls this function:

void SRAM_Read(void) __wparam
{
	// first latch
	// Write the low byte of the address of the data you want to read to PORTB 
	// (the whole port, not just a single pin, so you write 8 bits long)
	//movff SRAM_ADDR_2, _PORTB
	PORTB = card_l_addr;
	//bsf _LATA, SRAM_PIN_LATCH1_LE	; bsf SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE
	PORTAbits.RA0 = 1;
	//bcf _LATA, SRAM_PIN_LATCH1_LE	; bcf SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE
	PORTAbits.RA0 = 0;

	// second latch
	// Write the high byte of the address of the data you want to read to PORTB 
	// (the whole port, not just a single pin, so you write 8 bits long)
	//movff SRAM_ADDR_1, _PORTB
	PORTB = card_h_addr;
	//bsf _LATA, SRAM_PIN_LATCH2_LE	; bsf SRAM_LAT_LATCH2_LE, SRAM_PIN_LATCH2_LE
	PORTAbits.RA1 = 1;
	//bcf _LATA, SRAM_PIN_LATCH2_LE	; bcf SRAM_LAT_LATCH2_LE, SRAM_PIN_LATCH2_LE
	PORTAbits.RA1 = 0;

	// remaining address lines
	//movff	SRAM_ADDR, _PORTE
	// nothing to do at this moment; maybe later more memory will be addressed

	// enable port read mode: 
	// set PORTB as input to enable SRAM contents to be written to the port
	//setf	_TRISB
	TRISB = 0xFF;

	// Enable chip select (CS1) and Output Enable (OE) pins to read the data from 
	// the ram and put it on the PORTB pins. CS an OE are active LOW, so you clear 
	// them to start the read
	//bcf _LATA, SRAM_PIN_CS ; bcf SRAM_LAT_CS, SRAM_PIN_CS
	PORTAbits.RA2 = 0;
	//bcf _LATA, SRAM_PIN_OE ; bcf SRAM_LAT_OE, SRAM_PIN_OE
	PORTAbits.RA5 = 0;

	// wait while lines are low for the sram to put the data on the I/O0 - I/O7 pins 
	// (as per the SRAM datasheet)
	__asm
	nop
	__endasm;

	// read data
	// read PORTB (which now has the contents of your SRAM at the address given) 
	// into a variable
	//movff _PORTB, _MIOS_PARAMETER1
	ram_byte = PORTB;

	// Disable OE (go HIGH)
	//bsf _LATA, SRAM_PIN_OE ; bsf SRAM_LAT_OE, SRAM_PIN_OE
	PORTAbits.RA5 = 1;
	// Disable CS (go HIGH)
	//bsf _LATA, SRAM_PIN_CS ; bsf SRAM_LAT_CS, SRAM_PIN_CS
	PORTAbits.RA2 = 1;

	// disable port read mode
	//clrf	_TRISB
	TRISB = 0x00;

	//movf    _MIOS_PARAMETER1, W */

	return;
}

This contains all the original assembler source code as comments, it might be a bit confusing.

I have tried declaring the variables volatile, but to no avail. They are also declared “extern” in main.h, of course.

You mention, all three get modified at various places; can it be they get modified while this codesnippet is being processed? Where do you have that code, is it in an ISR; is there a timer that changes any of these three values?

It should have read: …will get modified at various places… Up to now, the above piece of code is the only place where they get changed. It is triggered by pressing a button - other button presses might also change the variables, but not at the same time!

There is no timer at all in my code, except the DISPLAY_Tick routine which reads the values in order to display them on the LCD.

I have not yet looked at the .asm file, but will do so.

Thanks for your hints. Could it be that I somehow overwrite parts of my code? The strange thing is that the final counter value (bankstick_addr) varies quite a lot, so I have also situations where I can only transfer about 1400 bytes and then the app resets. Then again it copies around 1700 bytes before resetting.

Thanks, ilmenator

audiocommander · November 1, 2006, 12:26pm

void SRAM_Read(void) __wparam
{
	//movff SRAM_ADDR_2, _PORTB
	PORTB = card_l_addr;

well, afaics there are no asm-fuctions active, you’re just setting some vars over and over again… although this should not be the main problem, I’d say it smells a bit cheesy ::)

I have not yet looked at the .asm file, but will do so.

That may really help!

The SDCC generated files contain a lot of comments and are useful even for ASM-idiots like me! ;D

Could it be that I somehow overwrite parts of my code? The strange thing is that the final counter value (bankstick_addr) varies quite a lot, so I have also situations where I can only transfer about 1400 bytes and then the app resets. Then again it copies around 1700 bytes before resetting.

yes, this can easily happen.

If you have an overflow somewhere, which can very easily happen, f.ex. if you’re assigning an int value to a char without proper declaration, it is quite probable, that the remaining bytes will overwrite some other data and therefore produce nearly unpredictable strange results.

And - I don’t have exact knowledge about the inner life of MIOS or the PIC - but there are Interrupt Service Routines (ISRs) active… so I could imagine, that some strange things can happen when you’re trying to run a very long nested function, esp. if you’re dealing with common MIOS vars like PORTAbits.RA0, that might (! might, I have no clue!) be in use somewhere else…

This will be more relevant the more code you have in your program.

In your case, I would first try to comment out all function calls (esp. the SRAM_Read) and see if the general structure of the loop works; maybe in a fresh project, where you can roule out the possibility of having an overflow situation somewhere else.

If I were you, I’d also try to reduce the number of function calls to a minimum amount (inside the loops); and I would add an unsigned char buffer[64], fill that buffer and write it as a page when it’s filled. You can find some examples in the function reference list. That should also speed up things!

It would be interesting if that behavior will remain if you optimize the loops…

best regards,

Michael

ilmenator · November 1, 2006, 3:50pm

Code:

void SRAM_Read(void) __wparam

{

//movff SRAM_ADDR_2, _PORTB

PORTB = card_l_addr;

well, afaics there are no asm-fuctions active, you’re just setting some vars over and over again… although this should not be the main problem, I’d say it smells a bit cheesy ::)

Ahem, I’m not sure what you are talking about? As far as I can see, although the asm code is twice in there, it is not active and should only serve as documentation. In the asm lines, the first entry was the modified asm code for use as inline asm in C, the second entry after the “;” was the original code. Is there actually any “setting of vars over and over again” that I have overlooked? I would really like to know where so I can improve my skills!

I will try your suggestions tonight and will report back.

Thanks, ilmenator

audiocommander · November 1, 2006, 4:02pm

sure, you’re setting

PORTB = card_l_addr;
	SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE
	PORTAbits.RA0 = 1;
	SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE
	PORTAbits.RA0 = 0;

	PORTB = card_h_addr;
	SRAM_LAT_LATCH2_LE, SRAM_PIN_LATCH2_LE
	PORTAbits.RA1 = 1;
	SRAM_LAT_LATCH2_LE, SRAM_PIN_LATCH2_LE
	PORTAbits.RA1 = 0;

	TRISB = 0xFF;

	SRAM_LAT_CS, SRAM_PIN_CS
	PORTAbits.RA2 = 0;
	SRAM_LAT_OE, SRAM_PIN_OE
	PORTAbits.RA5 = 0;

	ram_byte = PORTB;

	SRAM_LAT_OE, SRAM_PIN_OE
	PORTAbits.RA5 = 1;
	SRAM_LAT_CS, SRAM_PIN_CS
	PORTAbits.RA2 = 1;

	TRISB = 0x00;

	return;
}

Comments with ‘//’ are just until a new line begins. Or am I overseeing something ??? This is so obvious, I’m a bit irritated :-\ I do also wonder, why these lines don’t produce a compile warning:

    SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE
    PORTAbits.RA0 = 1;

Cheers,

Michael

ilmenator · November 1, 2006, 5:58pm

Okay, I think there is some misunderstanding about the “new line”. Actually, the // comments out the whole line following. There is a ; in the middle of these lines, which is the “comment” identifier for asm code. It should not be considered as “end of line” or “new line” by the C compiler, I think?

So:

//bsf _LATA, SRAM_PIN_LATCH1_LE	; bsf SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE

actually was assembler code like this:

bsf SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE

It was then changed to asm inline code in my C code like this:

bsf _LATA, SRAM_PIN_LATCH1_LE	; bsf SRAM_LAT_LATCH1_LE, SRAM_PIN_LATCH1_LE

with everything after the ; being a comment.

When used as a comment in my C code it was completely commented out with the two // - at least I think so. If it was not this way, the compiler should report an error, shouldn’t it?

I will try erasing the whole line to see what gives.

Best regards, ilmenator

ilmenator · November 1, 2006, 9:35pm

Now, I tried to leave out all the SRAM_Read and MIOS_BANKSTICK_Write function calls inside the loop. The result is still the same, after a certain period of time the core resets itself. I can almost definitely tell that it’s time dependent, because if I send out MIDI events inside of the FOR loops, then the counter does not get as far as if I just count and display…

I tried with this code:

for(card_h_addr=0x00; card_h_addr<0x7F; ++card_h_addr){
	  // inner loop increments the address lowbyte
	  for(card_l_addr=0x00; card_l_addr<0x7F; ++card_l_addr){

		MIOS_MIDI_TxBufferPut(0x91); 
		MIOS_MIDI_TxBufferPut(0x00);  
		MIOS_MIDI_TxBufferPut(card_l_addr);  

	  }

	  MIOS_MIDI_TxBufferPut(0x92); 
	  MIOS_MIDI_TxBufferPut(0x00);  
	  MIOS_MIDI_TxBufferPut(card_h_addr); 
	}

So, after around 2 seconds or so the problem arises. Does this ring any bells? Any OS things happening periodically every 2 seconds or so? I am not saying that it’s an OS thing, I am just trying to find some cause for this strange behavior…

Can somebody verify this behaviour?

Best regards, ilmenator

mess · November 1, 2006, 11:49pm

Hi Ilmenator,

I think the watchdog timer is causing this behaviour,

if the code inside tick() takes to long the watchdogtimer

will not be cleared so the core resets itself

quote from TK from another topic:

Watchdog timer: it’s configured in a way which doesn’t allow to enable/disable it via software. It’s always enabled, and it is serviced by MIOS within the mainloop (after USER_Tick has been called)

If USER_Tick stalls longer than 2 seconds, the PIC will be reset. If your routine needs longer (this is critical, >20 mS will cause a MIDI In buffer overrun if a bulk of data is received at the same time), than you have to issue a “clrwdt” instruction in order to service the watchdog, and to avoid a reset.

if it’s not possible to speed up your code you can clear the watchdog timer yourself:

__asm
clrwdt	
__endasm;

ilmenator · November 2, 2006, 10:00am

Thank you thank you thank you !! ;D

That’s exactly what’s going on, mess!

Now that I know my code works okay, I will try to make it faster using the block read commands. Thanks also to Audiocommander who reminded me of this.

Best regards, ilmenator