Suggested improvement to <avr/pgmspace.h>

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Suggested improvement to <avr/pgmspace.h>

Marko Mäkelä
Hi all,

I am trying to revive my AVR hobby again, this time using the avr-libc,
mainly to introduce my kids to low-level programming. I am using a
couple of Arduino boards with the ATmega328P.

I started with a simple "hello world" program that outputs a
NUL-terminated string to the UART:

static void msg(const char* PROGMEM msg)
{
  char c;
  while ((c = pgm_read_byte_postinc (&msg))) {
    UDR0 = c;
    loop_until_bit_is_set(UCSR0A, UDRE0);
  }
}

For optimal AVR implementation, this would require some inline assembly:

static char pgm_read_byte_postinc (const char** PROGMEM s)
{
#ifdef __AVR_HAVE_LPMX__
  char c;
  asm volatile ("lpm %0, %a1+" "\n\t" : "=r" (c), "+z" (*s));
  return c;
#else
  return pgm_read_byte((*s)++);
#endif
}

I would like to have pre/post-increment/decrement variants of all
pgm_read_* functions in <avr/pgmspace.h> that are natively supported on
some AVR platform. I wonder what would be the practical way to achieve
this. Contribute a patch myself? I hereby contribute the above idea to
the public domain.

Best regards,

        Marko

_______________________________________________
AVR-libc-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Suggested improvement to <avr/pgmspace.h>

Georg-Johann Lay-2
On 02.01.2017 11:58, Marko Mäkelä wrote:

> Hi all,
>
> I am trying to revive my AVR hobby again, this time using the avr-libc,
> mainly to introduce my kids to low-level programming. I am using a
> couple of Arduino boards with the ATmega328P.
>
> I started with a simple "hello world" program that outputs a
> NUL-terminated string to the UART:
>
> static void msg(const char* PROGMEM msg)
> {
>  char c;
>  while ((c = pgm_read_byte_postinc (&msg))) {
>    UDR0 = c;
>    loop_until_bit_is_set(UCSR0A, UDRE0);
>  }
> }

You can use the __flash address space instead of PROGMEM + pgm_read, cf.

https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

In your case, the function would read:

static void msg (const __flash char *msg)
{
   char c;
   while ((c = *msg++))
   {
      UDR0 = c;
      loop_until_bit_is_set (UCSR0A, UDRE0);
   }
}

The __flash address space is implemented by the compiler as a GNU
extension to the C language, and hence -std=gnu99 (or higher) is
needed. There is no guarantee for an optimal code being generated
for that example (as applies to any other C code).

Also notice that PROGMEM is just a variable attribute; its only
effect is to locate data to .progmem.data for variable definitions
which are attributed progmem. Specifying it with prototypes or
declarations is void.

The advantage is that less typing is needed and the feature is more
trqansparent w.r.t. code that only addresses generic address space,
e.g. to deflate to .rodata (which is located in RAM for avr) you
only need to

#define __flash /* empty */


Johann


> For optimal AVR implementation, this would require some inline assembly:
>
> static char pgm_read_byte_postinc (const char** PROGMEM s)
> {
> #ifdef __AVR_HAVE_LPMX__
>  char c;
>  asm volatile ("lpm %0, %a1+" "\n\t" : "=r" (c), "+z" (*s));
>  return c;
> #else
>  return pgm_read_byte((*s)++);
> #endif
> }
>
> I would like to have pre/post-increment/decrement variants of all
> pgm_read_* functions in <avr/pgmspace.h> that are natively supported on
> some AVR platform. I wonder what would be the practical way to achieve
> this. Contribute a patch myself? I hereby contribute the above idea to
> the public domain.
>
> Best regards,
>
>     Marko
>
> _______________________________________________
> AVR-libc-dev mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
>


_______________________________________________
AVR-libc-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Suggested improvement to <avr/pgmspace.h>

Marko Mäkelä
On Mon, Jan 02, 2017 at 05:27:19PM +0100, Georg-Johann Lay wrote:

>On 02.01.2017 11:58, Marko Mäkelä wrote:
>You can use the __flash address space instead of PROGMEM + pgm_read,
>cf.
>
>https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
>
>In your case, the function would read:
>
>static void msg (const __flash char *msg)
>{
>  char c;
>  while ((c = *msg++))
>  {
>     UDR0 = c;
>     loop_until_bit_is_set (UCSR0A, UDRE0);
>  }
>}
>
>The __flash address space is implemented by the compiler as a GNU
>extension to the C language, and hence -std=gnu99 (or higher) is
>needed.

Thank you, this is much cleaner, and like you pointed out, this allows
portable code to be written when using typedef or #define. I guess
<avr/pgmspace.h> now only exists for backward compatibility with old
code?

I would like to follow up with some avr-gcc or avr-g++ remarks (sorry
for the off-topic discussion).

It looks like avr-g++ does not accept the __flash qualifier. Are there
plans to support it?

>There is no guarantee for an optimal code being generated for that
>example (as applies to any other C code).

It turns out that the compiled code is identical with my asm-accelerated
version. The entry into the loop looks like this:

  ae: 85 91       lpm r24, Z+
  b0: 81 11       cpse r24, r1
  b2: f7 cf       rjmp .-18     ; 0xa2 <main+0x12>
  b4: f3 cf       rjmp .-26     ; 0x9c <main+0xc>

The above is also emitted for -mmcu=attiny2313. With -mmcu=at90s2313,
the "mov" from the temporary register r0 could be omitted:

  48: c8 95       lpm
  4a: 80 2d       mov r24, r0
  4c: 31 96       adiw r30, 0x01 ; 1
  4e: 81 11       cpse r24, r1
  50: f8 cf       rjmp .-16     ; 0x42 <__SREG__+0x3>
  52: f4 cf       rjmp .-24     ; 0x3c <main+0x8>

Best regards,

        Marko

_______________________________________________
AVR-libc-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Suggested improvement to <avr/pgmspace.h>

George Spelvin
In reply to this post by Marko Mäkelä
> I started with a simple "hello world" program that outputs a
> NUL-terminated string to the UART:
>
> static void msg(const char* PROGMEM msg)
> {
>   char c;
>   while ((c = pgm_read_byte_postinc (&msg))) {
>     UDR0 = c;
>     loop_until_bit_is_set(UCSR0A, UDRE0);
>   }
> }
>
> For optimal AVR implementation, this would require some inline assembly:

You could also try using a compiler feature and skipping the pgm_read
function entirely.

static void msg(const char __flash *msg)
{
  char c;
  while ((c = *msg++)) {
    UDR0 = c;
    loop_until_bit_is_set(UCSR0A, UDRE0);
  }
}

Of course, GCC is generating terrible code for me on this (not only
is it not using Z+, it's copying the pointer to a different register to
increment it and using a subi/sbci sequence to do it), but it's definitely
more legible source code.

I also wonder if we could do something like:

static inline char pgm_read_byte(char const * PROGMEM p)
{
        char c;
        asm("lpm %0,%1" : "=r" (c) : "m<>" (*p));
        return c;
}

which would tell GCC how to make "pgm_read_byte(msg++)" use
a postincrement addressing mode, without having to add another
function.


(We should also update inttypes.h and pgmspace.h to use
__int24/__uint24 for far addresses.)

_______________________________________________
AVR-libc-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
Loading...