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?
but the uploaded code is clearly not getting programmed correctly.
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.
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?
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;
}
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
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
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?
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
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.
Because it seems wrong to me.
BTW the LSS file is a much easier to read disassembly than that atrocious (all caps) nonsense from the Atmel debugger
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.
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
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: https://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! 
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
EDIT: This post was written while rowifi submitted the post just above.
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.
*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.
Yes, I see.
Clearly explained - amid my hasty code.
Thanks
Do this setting at once. Repeat for all new projects you start.
counter = counter++;
This leads to undefined behaviour - see: http://c-faq.com/expr/ieqipluspl...
counter = counter++;
Ever heard the word "undefined"? Have no idea what a C compiler would make of such nonsense.
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!