AS7 (Native) from Studio4 (WinAVR ) imported project not working

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

Generally, I've had no problems in porting from Studio4 to AS7, making the occasional tweek and the code works.

I'm importing a bootloader - and it imports and compiles, and generally works ( the communications ), but the uploaded code is clearly not getting programmed correctly.

Is there anything specific that I should be looking for to resolve this?

 

This topic has a solution.
Last Edited: Tue. May 16, 2017 - 11:15 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

rowifi wrote:
but the uploaded code is clearly not getting programmed correctly.
Tell us more about how you are determining that to be the case.

 

Remember that ISP programming can work both ways: write and read. So to check if things are programmed as expected you can always program and then read back to a second file and then compare what was written with what is there.

 

My guess, if the bootloader code is "tight" and right up near the end of the AVR is that the compiler in AS7 might be building it a little larger than the 4.3.3 in WinAVR and it could be "spilling" out of the end of the device.

 

A read back after programming should reveal if this is the case.

 

Also useful to look at the address of the last record in the file. Does it exceed the device size?

 

If this guess is right then one option is to use "toolchain flavours" in AS7 to tell it to use the WinAVR version of the compiler to build this particular code.

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

Cliff, he might also mean that the bootloader works in parts, since it communicates fine with the host app - but the bootloader does not program the application code it gets from the host correctly.

 

@rowifi: Can you make it clear to us if it is the bootloader code that is not programmed correctly, or if it is the application code?

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

 

 

It's the application code that appears not to be getting flashed correctly.

It communicates fine, but after the first page load the flashed data appears wrong.

It's not a normal bootloader because it communicates over a CAN bus, and decodes a lightly encrypted hex file before flashing the true code.

 

I'm loathed to have to install WInAvr and mess with flavours because it 'should' work with the latest toolset and native compiler. Ideally I would like to fix it of course to understand what's going on.

 

The pages are checked after flashing for corrrectness with a simple check and that must be working otherwise the boot reports errors.

 

So everything in the code says it's working - it just isn't putting the right code in. On the surface, it appears that the decrypt method is failing - which is my next thing to check, but I've also attached the flashing code in case there's a glaring error. 

As a side fact - very occasionally while reflashing a device ( using the Studio4 versions ) some pages will fail to program and require one or more attempts. When this happens, often the page program fault occurs every so many pages like there's some timing issue going on. It's rare, but it does happen.

 

TIA

 

 

 

 



void write_page ( unsigned int pageno)
{

// Disable interrupts

		sreg = SREG;
		cli();


        boot_spm_busy_wait();       // Wait until the memory is written.

		temp_address = (unsigned long)(pageno*128);

	    /* store data in temp buffer and write to flash */
	    for( i = 0; i < SPM_PAGESIZE; i += 2 )
	    {
		/* swap the bytes for little endian format */
		data = page_data[ i ];
		data |= ( page_data[ i + 1 ] << 8 );
		boot_page_fill( temp_address, data ); // call asm routine to load temporary flash buffer
		temp_address += 2; // select next word in temporary buffer
	    }


	    /* write to the flash */
	    boot_page_write( temp_address - SPM_PAGESIZE );
	    boot_spm_busy_wait();	
	    boot_rww_enable();				//Re-enable the RWW section

		// Re-Enable Interrupts as they were

		SREG = sreg;

}


void erase_page( unsigned int pageno)
{

// Disable interrupts

		sreg = SREG;
		cli();

		   temp_address = (unsigned long)(pageno*128);

			boot_page_erase(temp_address);	// Perform page erase
			boot_spm_busy_wait();		// Wait until the memory is erased.

		// Re-Enable Interrupts as they were

		SREG = sreg;
}

 

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

Well, the decoding is wrong because the compiler completely ignores the counter++ line.

What's the best way to fix that?

 

		case ID_PAGEDATA00:
		case ID_PAGEDATA01:
		case ID_PAGEDATA02:
		case ID_PAGEDATA03:
		case ID_PAGEDATA04:
		case ID_PAGEDATA05:
		case ID_PAGEDATA06:		
		case ID_PAGEDATA07:
		case ID_PAGEDATA08:
		case ID_PAGEDATA09:
		case ID_PAGEDATA10:
		case ID_PAGEDATA11:
		case ID_PAGEDATA12:
		case ID_PAGEDATA13:
		case ID_PAGEDATA14:
		case ID_PAGEDATA15:


// fill buffer with received data

			counter = counter++; // increment counter at each block
			counter ^= shifter;

 

 

The counter ++ line gets ignored by the compiler

 

		switch (msg.identifier)
00003E54  LDD R24,Y+2		Load indirect with displacement 
00003E55  LDD R25,Y+3		Load indirect with displacement 
00003E56  LDD R26,Y+4		Load indirect with displacement 
00003E57  LDD R27,Y+5		Load indirect with displacement 
00003E58  CPI R24,0x13		Compare with immediate 
00003E59  LDI R30,0x02		Load immediate 
00003E5A  CPC R25,R30		Compare with carry 
00003E5B  CPC R26,R1		Compare with carry 
00003E5C  CPC R27,R1		Compare with carry 
00003E5D  BREQ PC+0x27		Branch if equal 
00003E5E  BRCC PC+0x1F		Branch if carry cleared 
00003E5F  SUBI R25,0x02		Subtract immediate 
00003E60  SBC R26,R1		Subtract with carry 
00003E61  SBC R27,R1		Subtract with carry 
00003E62  SBIW R24,0x10		Subtract immediate from word 
00003E63  CPC R26,R1		Compare with carry 
00003E64  CPC R27,R1		Compare with carry 
00003E65  BRCS PC+0x02		Branch if carry set 
00003E66  RJMP PC-0x00EB		Relative jump 
			counter ^= shifter;
00003E67  LDS R20,0x02AB		Load direct from data space 
00003E69  LDS R24,0x02AA		Load direct from data space 
00003E6B  EOR R20,R24		Exclusive OR 

 

 

 

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

Show more of the code - that is likely in a different location (optimization)

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

OK... What I did next was to make the two variables counter and shifter volatile...( just a test) and the disassembled code now shows this..

 

I DO see the addition of 1 to the counter ( starting at zero) , but its immediately over written by a zero again by the compiler writing both R25 and R24 back to the same location.

 

EDIT

global variables not in a function

volatile unsigned char shifter,counter;

 

			counter = counter++; // increment counter at each block
00003E66  LDS R24,0x02AB		Load direct from data space
00003E68  LDI R25,0x01		Load immediate
00003E69  ADD R25,R24		Add without carry
00003E6A  STS 0x02AB,R25		Store direct to data space
00003E6C  STS 0x02AB,R24		Store direct to data space
			counter ^= shifter;
00003E6E  LDS R25,0x02AA		Load direct from data space
00003E70  LDS R24,0x02AB		Load direct from data space
00003E72  EOR R24,R25		Exclusive OR
00003E73  STS 0x02AB,R24		Store direct to data space
00003E75  LDS R18,0x016E		Load direct from data space
00003E77  LDS R19,0x016F		Load direct from data space 

 

Last Edited: Tue. May 16, 2017 - 09:08 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

but its immediately over written by a zero again by the compiler writing both R25 and R24 back to the same location

What are you talking about?

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

Thanks Clawson.

Here's what I did. Evidently the compiler didn't originally do what I expected.

 

The solution was to  change to :

 

// fill buffer with received data

			counter = counter+ 1; // increment counter at each block
			counter ^= shifter;

I've still left the volatile statement in and the compiler does it correctly

			counter = counter+ 1; // increment counter at each block
00003E66  LDS R24,0x02AB		Load direct from data space 
00003E68  SUBI R24,0xFF		Subtract immediate 
00003E69  STS 0x02AB,R24		Store direct to data space 
			counter ^= shifter;
00003E6B  LDS R25,0x02AA		Load direct from data space 
00003E6D  LDS R24,0x02AB		Load direct from data space 
00003E6F  EOR R24,R25		Exclusive OR 
00003E70  STS 0x02AB,R24		Store direct to data space 
00003E72  LDS R18,0x016E		Load direct from data space 
00003E74  LDS R19,0x016F		Load direct from data space 

 

 

 

Removing the volatile tag .. the code still works .

		switch (msg.identifier)
00003E54  LDD R24,Y+2		Load indirect with displacement 
00003E55  LDD R25,Y+3		Load indirect with displacement 
00003E56  LDD R26,Y+4		Load indirect with displacement 
00003E57  LDD R27,Y+5		Load indirect with displacement 
00003E58  CPI R24,0x13		Compare with immediate 
00003E59  LDI R30,0x02		Load immediate 
00003E5A  CPC R25,R30		Compare with carry 
00003E5B  CPC R26,R1		Compare with carry 
00003E5C  CPC R27,R1		Compare with carry 
00003E5D  BREQ PC+0x28		Branch if equal 
00003E5E  BRCC PC+0x20		Branch if carry cleared 
00003E5F  SUBI R25,0x02		Subtract immediate 
00003E60  SBC R26,R1		Subtract with carry 
00003E61  SBC R27,R1		Subtract with carry 
00003E62  SBIW R24,0x10		Subtract immediate from word 
00003E63  CPC R26,R1		Compare with carry 
00003E64  CPC R27,R1		Compare with carry 
00003E65  BRCS PC+0x02		Branch if carry set 
00003E66  RJMP PC-0x00EB		Relative jump 
			counter ^= shifter;
00003E67  LDS R20,0x02AB		Load direct from data space 
00003E69  SUBI R20,0xFF		Subtract immediate 
00003E6A  LDS R24,0x02AA		Load direct from data space 
00003E6C  EOR R20,R24		Exclusive OR 
00003E6D  STS 0x02AB,R20		Store direct to data space 
00003E6F  LDS R18,0x016E		Load direct from data space 
00003E71  LDS R19,0x016F		Load direct from data space 
		page_data[n + (blockcount*8) ] = msg.dta[n] ^ counter;
00003E73  LDS R24,0x0150		Load direct from data space 
00003E75  MOVW R26,R28		Copy register pair 
00003E76  ADIW R26,0x07		Add immediate to word 
00003E77  LDI R22,0x08		Load immediate 
00003E78  MUL R24,R22		Multiply unsigned 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
			counter = counter++; // increment counter at each block
00003E66  LDS R24,0x02AB		Load direct from data space   ** GET MY VALUE 00  into R24**
00003E68  LDI R25,0x01		Load immediate                        ** GET THE INCREMENTER **
00003E69  ADD R25,R24		Add without carry                     ** DO THE ADD **
00003E6A  STS 0x02AB,R25		Store direct to data space   ** WRITE THE INCREMENT BACK**
00003E6C  STS 0x02AB,R24		Store direct to data space   ** WRITE BACK THE ZERO !!!  WTF *

What I am talking about. 

Am I interpreting this correctly? Because it seems wrong to me.

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

rowifi wrote:
Because it seems wrong to me.
That's what I asked you to explain. I read the code and it appears to be doing EXACTLY what the C told it to do so I'm interested to hear how you think it is "wrong".

 

BTW the LSS file is a much easier to read disassembly than that atrocious (all caps) nonsense from the Atmel debugger

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

rowifi wrote:
I've still left the volatile statement in and the compiler does it correctly

It is highly unlikely that there is a compiler bug. It is much more likely that the compiler has re-arranged the code due to optimization and that you are not understanding what is happening. (Don't take this wrong! This happens to many of us. I'm not trying to be derogatory.)

 

Do what Cliff says and show more code, and we might be able to help you tangle this out.

 

We see fairly often expectations of what the C language should be and do, but when dissected it turns out that those expectations are wrong. Not the compiler.

 

"Peepholing" a few lines of code and saying "I do not see the generated code where I expect it" is often a telltale sign.

 

Also...

 

counter = counter++;

That  is effectively the same as

counter = counter;
counter++;

If you just want to increment the counter then either

counter++;

or

counter = counter + 1;

or (the short form of the one just above)

counter += 1;

are ways to do that.

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

I respect what you're saying and extremely grateful for help and advice, but..

The FIX,( because it works ) was to change the line from  counter = counter++;   to counter = counter+1;

 

Stepping through the code at assembly level, showed me that the variable address was incremented - then immediately overwritten by a 0 as per the disassembled text with my notes.

The code then proceeded to do everything as expected but the decode was incorrect simply because the counter was NEVER permanently incremented.

 

Changing that one line of code - which as we all agree should generate the same functionality clearly made all the difference, as can be seen also in the disassembled code.

I am by no means a daily expert, but having a working bit of code and a non working bit of code by changing one line to a slightly different syntax indicates that the compiler isn't doing as expected.

 

I wasn't just looking at isolated code sections because I am aware that optimising can make it convoluted and difficult to see what goes on, however the proof of the pudding is in the eating, and I can make or break the code at will by one single line change.

 

Anyway - I am happy to have got it working - even if I don't understand the root cause.

I will post if I find anything obvious.

Cheers

 

 

 

 

 

 

 

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

JohanEkdahl wrote:
It is highly unlikely that there is a compiler bug. It is much more likely that the compiler has re-arranged the code due to optimization and that you are not understanding what is happening.

 

See: http://www.avrfreaks.net/comment... and the following replies

 

Don't take this wrong! This happens to many of us. I'm not trying to be derogatory.

Indeed! blush

 

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

Heres a test I just did.

 

test1 does the addition, test2 appears not to.

Seriously . Is it me?

 

*Edit*

A clue may be in the warning, whatever it means:

Warning        operation on 'counter' may be undefined [-Wsequence-point]    

 

 

 

 

unsigned char test1 ()
{

				counter = counter+1; // increment counter at each block
				counter ^= shifter;
				return (counter);

}

unsigned char test2 ()
{

	counter = counter++; // increment counter at each block
	counter ^= shifter;
	return (counter);

}

 

 

00007750 <test1>:
    7750:	80 91 b0 02 	lds	r24, 0x02B0	; 0x8002b0 <counter>
    7754:	8f 5f       	subi	r24, 0xFF	; 255
    7756:	90 91 af 02 	lds	r25, 0x02AF	; 0x8002af <shifter>
    775a:	89 27       	eor	r24, r25
    775c:	80 93 b0 02 	sts	0x02B0, r24	; 0x8002b0 <counter>
    7760:	08 95       	ret

00007762 <test2>:
    7762:	90 91 b0 02 	lds	r25, 0x02B0	; 0x8002b0 <counter>
    7766:	80 91 af 02 	lds	r24, 0x02AF	; 0x8002af <shifter>
    776a:	89 27       	eor	r24, r25
    776c:	80 93 b0 02 	sts	0x02B0, r24	; 0x8002b0 <counter>
    7770:	08 95       	ret

 

Last Edited: Tue. May 16, 2017 - 10:56 AM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

EDIT: This post was written while rowifi submitted the post just above.

 

rowifi wrote:
Anyway - I am happy to have got it working - even if I don't understand the root cause.

Dangerous attitude. Just you got it to work right there is no guarantee that it will still work if anything around the "fix" changes. Code, compiler settings, compiler version.

 

Having said that, I concur with the increment being done in the straight forward way will work as expected.


 

Re

counter = counter++;

it seems rather obvious why that does not do what you expect..

 

There are three steps involved here:

 

a. evaluate the value of counter (no, not counter++, only evaluate counter!). This has to be done first. The ++ is a post increment, i.e. it is to be done after the value to be used in the assignment has been determined.

 

b. Do the counter++ operation. Note that this is not the value to be used in the assignment - again, the ++ is a post-decrement.

 

c. Do the assignment.

 

As I said, step a has to be done first. If the two other steps are then done in the order b, c you will get the exact effect you experienced.

 

Say counter was 42 to begin with:

 

a. Get the value to use at the assignment. It's 42.

b. Do counter++. The new value of  counter now is 43.

c. Do the assignment, i.e. assign the value determined in a (42) to counter.

 

The fact you can not get away from is: The value for the assignment is taken before the ++ op. (The value to be used in the assignment is not the value of counter after counter++ has been done.) This is clearly by the C standards.

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

Last Edited: Tue. May 16, 2017 - 11:16 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

rowifi wrote:
*Edit* A clue may be in the warning, whatever it means: Warning        operation on 'counter' may be undefined [-Wsequence-point]    

 

Never, never, ever ignore warning from the compiler. Always take note of them. Always fix them.

 

In order to discipline oneself to adhere to this golden rule, one can set options to treat all warnings as errors. This will force them being fixed at once.

 

The warning essentially says what I tried to analyze and explain above.

 

The "sequence point" most likely refers to the assignment operation. It is a sequence point, meaning (loosely formulated) anything to the right of it has to be evaluated before the assignment is done. This, as per my previous post, will render the counter++ operation "meaningless". The compiler spots that the effect of it will immediately be cancelled out by the assignment.

 

Never, never, ever ignore warnings. Especially if the reason for ignoring one is not understanding it.

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

Yes, I see. 

Clearly explained - amid my hasty code. 

Thanks

 

 

 

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

Do this setting at once. Repeat for all new projects you start.

 

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

rowifi wrote:

	counter = counter++; 

 

This leads to undefined behaviour - see: http://c-faq.com/expr/ieqipluspl...

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
counter = counter++;

Ever heard the word "undefined"? Have no idea what a C compiler would make of such nonsense.

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

Ah! Brilliant, gentlemen! Those links are golden, awneil!

 

This explains why the code might have worked using an older compiler.

 

It also points to my detailed analysis/explanation being wrong - but the ultimate conclusion being right(ish) :-)

 

Again (to the OP): Welcome to the wonderful world of C - not always as obvious at it would seem!

"He used to carry his guitar in a gunny sack, or sit beneath the tree by the railroad track. Oh the engineers would see him sitting in the shade, Strumming with the rhythm that the drivers made. People passing by, they would stop and say, "Oh, my, what that little country boy could play!" [Chuck Berry]

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]