memcpy_PF – possibly bugged function

Log in or register to post comments
Go To Last Post
13 posts / 0 new
Author
Message
TFrancuz
TFrancuz's picture
Level: Resident
Joined: Fri. Jan 10, 2003
Posts: 584 View posts
#1
Posted by TFrancuz: Mon. Sep 23, 2013 - 09:26 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

memcpy_PF "“ possibly bugged function
The function does't work properly on at least XMEGA128A1 device. According to description the function should take a far pointer to FLASH location and copy len bytes into the SRAM location. The problem is as stated below:

memcpy_PF(&samplebuffer[0][0], srcaddr, BUFFER_SIZE):
00000112  MOVW R30,R20		Copy register pair 
00000113  MOVW R26,R24		Copy register pair 
00000114  RJMP PC+0x0003		Relative jump 
00000115  LPM R0,Z+		Load program memory and postincrement 
00000116  ST X+,R0		Store indirect and postincrement 
00000117  SUBI R18,0x01		Subtract immediate 
00000118  SBCI R19,0x00		Subtract immediate with carry 
00000119  BRCC PC-0x04		Branch if carry cleared 
0000011A  RET 		Subroutine return

The memcpy_PF uses LPM instruction instead of ELPM and doesn't update RAMPZ register. So it effectively truncates source address to 16-bits and reads only first 64 kB of FLASH.
I didn't checked rest of *_PF functions but they should be revised too.

Tags:

Tools, Compilers and General Programming
Top
Gold Markclawson
clawson's picture
Level: Moderator
Joined: Mon. Jul 18, 2005
Posts: 105110 View posts
Location: (using avr-gcc in) Finchingfield, Essex, England
Posted by clawson: Mon. Sep 23, 2013 - 09:36 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Which build of AVR-LibC is this and from what source?

BTW when I Google "memcpy_PF ELPM" the first hit is:

http://savannah.nongnu.org/patch...

that's an interesting thread. It even refers back to Freaks:

https://www.avrfreaks.net/index.p...

Top
TFrancuz
TFrancuz's picture
Level: Resident
Joined: Fri. Jan 10, 2003
Posts: 584 View posts
Posted by TFrancuz: Mon. Sep 23, 2013 - 10:07 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I use:

Atmel Studio 6 (Version: 6.1.2730 - Service Pack 2)
Installed Packages: Atmel AVR (8 bit) GNU Toolchain - 3.4.2.1002
AVR Toolchain 8 Bit
Version: AVR8_Toolchain_Version:3.4.2.992 GCC_VERSION:4.7.2

The threads you are referenced to are interesting, but 2-years old, and for two years the problem exists, making all _PF functions unusable.

Top
wek
wek's picture
Level: Raving Lunatic
Joined: Fri. Dec 16, 2005
Posts: 3270 View posts
Location: Bratislava, Slovakia
Posted by wek: Tue. Sep 24, 2013 - 05:32 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

TFrancuz wrote:
The threads you are referenced to are interesting, but 2-years old, and for two years the problem exists, making all _PF functions unusable.
Still valid, including the workaround mentioned there: add the sources of _PF functions to your project. Or build a simple library containing only the _PF functions and link them explicitly. Both should override the default, badly built libraries.

If you just wanted to point out how much Atmel cares of the community by following (and then contributing back to) the trackers of projects they are exploiting, then, well, this much.

JW

Top
TFrancuz
TFrancuz's picture
Level: Resident
Joined: Fri. Jan 10, 2003
Posts: 584 View posts
Posted by TFrancuz: Wed. Sep 25, 2013 - 12:58 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The problem is not the badly _PF functions – I corrected them within minutes. The problem is, that in documentation there is no information that these functions just don’t work. I spend a couple of hours looking for a stupid mistake in library. If nobody wants to correct the functions, why not just remove them?

Top
wek
wek's picture
Level: Raving Lunatic
Joined: Fri. Dec 16, 2005
Posts: 3270 View posts
Location: Bratislava, Slovakia
Posted by wek: Wed. Sep 25, 2013 - 01:17 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As explained in the linked tracker item (and in the duplicate https://savannah.nongnu.org/bugs... ), it's not that the functions are bad, but the build system. It's upon the avr-libc maintainters to change that - and, upon Atmel's engineers to change their own build system, if they use a different one from the original.

The avr-libc documentation is, together with the library, open source. Please feel free to contribute to it. Again, Atmel's version is just upon Atmel. Feel free to submit a bug report to them.

JW

Top
Gold Markclawson
clawson's picture
Level: Moderator
Joined: Mon. Jul 18, 2005
Posts: 105110 View posts
Location: (using avr-gcc in) Finchingfield, Essex, England
Posted by clawson: Wed. Sep 25, 2013 - 01:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

and, upon Atmel's engineers to change their own build system

Which may well be the issue here. What happens if you build the same thing on a Linux system with a cleanly pulled and built copy of the toolchain?

Or how about using SprinterSB's builds of avr-gcc/AVR-LibC for Windows to compare?

Top
wek
wek's picture
Level: Raving Lunatic
Joined: Fri. Dec 16, 2005
Posts: 3270 View posts
Location: Bratislava, Slovakia
Posted by wek: Thu. Sep 26, 2013 - 07:30 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
Or how about using SprinterSB's builds of avr-gcc/AVR-LibC for Windows to compare?

They of course work OK in this respect - see below.

clawson wrote:
Quote:

and, upon Atmel's engineers to change their own build system

Which may well be the issue here.

Indeed.

http://svn.savannah.nongnu.org/v...
http://lists.nongnu.org/archive/...

The extra irony is in that it was no other than EW of Atmel who provided the fix.

I can't say nothing but repeat once again my rants about Atmel being selfish and straight stupid when ignoring the established OS development infrastructure.

JW

Top
Gold Markclawson
clawson's picture
Level: Moderator
Joined: Mon. Jul 18, 2005
Posts: 105110 View posts
Location: (using avr-gcc in) Finchingfield, Essex, England
Posted by clawson: Thu. Sep 26, 2013 - 08:53 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I wonder if Atmel even know about their failure to build this library code correctly? Threads here do not raise entries in their own Bugzilla system. I guess we just have to hope that some eagle eyed Atmel employee glances at this thread and makes sure it is reported to their bug tracker.

Top
SprinterSB
SprinterSB's picture
Level: Raving Lunatic
Joined: Thu. Dec 21, 2006
Posts: 2115 View posts
Location: Saar-Lor-Lux
Posted by SprinterSB: Thu. Sep 26, 2013 - 04:37 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Some notes:

  • Do not use the address operator & with _PF functions. & will only yield a 16-bit address because pointers in avr-gcc are 16 bits wide.
  • If you use a local copy, fix the constraint "p" in inline asm; it's nonsense. Use "i" instead.
  • The _PF functions will break programs on ATxmega because they don't reset RAMPZ to 0. Thus, reading from RAM might return garbage because the high byte of the address is incorrect. Cf. the avr-gcc documentation:

    gcc(dot)gnu(dot)org/onlinedocs/gcc/AVR-Options.html

avrfreaks does not support Opera. Profile inactive.

Top
wek
wek's picture
Level: Raving Lunatic
Joined: Fri. Dec 16, 2005
Posts: 3270 View posts
Location: Bratislava, Slovakia
Posted by wek: Thu. Sep 26, 2013 - 06:40 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

SprinterSB wrote:
[*]If you use a local copy, fix the constraint "p" in inline asm; it's nonsense. Use "i" instead.
Only in the local copy? Shouldn't it be changed in the library, too?
[EDIT] I didn't notice yesterday night when I wrote this reply, but - what are you talking about? The memcpy_PF() (and kin) are .S and not inline...? [/EDIT]
SprinterSB wrote:
[*]The _PF functions will break programs on ATxmega because they don't reset RAMPZ to 0. Thus, reading from RAM might return garbage because the high byte of the address is incorrect.
Isn't it the case with the "basic" pgm_read_xxx_far() too? Is this the case with all ATxmega, in other words, is the __AVR_XMEGA__ macro appropriate to detect this?
[EDIT]Now I see they are OK in this respect... and use the __AVR_HAVE_RAMPD__ macro - and they restore the RAMPZ rather than zero. So, do you say, it could be zeroed? It would then be better to do it in that way, sparing one SFR read...?[/EDIT]

SprinterSB wrote:
Cf. the avr-gcc documentation:
gcc(dot)gnu(dot)org/onlinedocs/gcc/AVR-Options.html
[/list]
OT, but once you mentioned, could you please have a look at this:
Quote:
Linker relaxation must be turned on so that the linker will generate the stubs correctly an all situaltion. See the compiler option -mrelax and the linler option --relax. There are corner cases where the linker is supposed to generate stubs but aborts without relaxation and without a helpful error message.
Isn't the "corner case" mentioned here a linker bug already fixed?

Top
SprinterSB
SprinterSB's picture
Level: Raving Lunatic
Joined: Thu. Dec 21, 2006
Posts: 2115 View posts
Location: Saar-Lor-Lux
Posted by SprinterSB: Fri. Sep 27, 2013 - 10:12 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My bad, the "p" constraint occurs in avr/pgmspace.h:pgm_get_far_address(). It's filed as #36609 against AVR-Libc.

For devices with > 64k RAM (device das RAMPD), avr-gcc assumes that RAMPD/X/Y/Z is zero: Insns accessing RAM won't set RAMP registers before the access or reset them afterwards. Code that clobbers / sets RAMP is very likely to break. The only safe example of changing RAMP is within (inline) assemller that restores RAMP afterwards. RAMP registers work rather like GPRs, not like SFRs.

For the relax thing better ask binutils people, mayne the comment in the avr-gcc docs is outdated and no more holds for new binutils.

avrfreaks does not support Opera. Profile inactive.

Top
wek
wek's picture
Level: Raving Lunatic
Joined: Fri. Dec 16, 2005
Posts: 3270 View posts
Location: Bratislava, Slovakia
Posted by wek: Fri. Oct 4, 2013 - 08:01 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

SprinterSB wrote:
My bad, the "p" constraint occurs in avr/pgmspace.h:pgm_get_far_address(). It's filed as #36609 against AVR-Libc.
Can you please explain, why not "p" and why yes "i"? Thanks.

SprinterSB wrote:
For devices with > 64k RAM (device das RAMPD), avr-gcc assumes that RAMPD/X/Y/Z is zero:
Is there no report of this problem in the avr-libc's tracker so far?

SprinterSB wrote:
For the relax thing better ask binutils people, mayne the comment in the avr-gcc docs is outdated and no more holds for new binutils.
The stubs generation/handling is entirely an AVR-specific issue, thus I doubt the binutils people will have a relevant comment on this. I say, the above bug was the reason, and as it's fixed, please remove the warning.

JW

Top