Code in den Atomic Block verschleppt

[Andreas R. (andreasr)]

Dies ist die aufgewärmte Version des Threads 
Beitrag "Schwerer Bug in AVR-GCC 4.1.1" der gesperrt ist. Bislang 
hat die ganz unten vorgeschlagene Lösung mit _asm__ __volatile_ ( "" : 
: "memory" (x)) immer funktioniert. Nun habe ich trotz der clobber Liste 
das Problem.

Folgender Code:

volatile uint8_t* pI2CBuff = &txbuffer[i*4];
        // Folgende Anweisung verhindert, dass die Berechnung von pI2CBuff in den ATOMIC_BLOCK verzögert wird!
        // https://www.mikrocontroller.net/topic/65923
        __asm__ __volatile__ ( "" : : "memory" (pI2CBuff));
        ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
        {
          *pI2CBuff++ = tmp[0];
          *pI2CBuff++ = tmp[1];
          *pI2CBuff++ = tmp[2];
          *pI2CBuff++ = tmp[3];
        }

Was macht der Compiler (avrgcc 5.4.0, Optimierung -Os) daraus?

 16e:  2f b7         in  r18, 0x3f  ; 63
 170:  f8 94         cli
 172:  f7 01         movw  r30, r14
 174:  ee 0f         add  r30, r30
 176:  ff 1f         adc  r31, r31
 178:  ee 0f         add  r30, r30
 17a:  ff 1f         adc  r31, r31
 17c:  ef 59         subi  r30, 0x9F  ; 159
 17e:  ff 4f         sbci  r31, 0xFF  ; 255
 180:  00 83         st  Z, r16
 182:  19 83         std  Y+1, r17  ; 0x01
 184:  8a 83         std  Y+2, r24  ; 0x02
 186:  9b 83         std  Y+3, r25  ; 0x03
 188:  2f bf         out  0x3f, r18  ; 63

Das heißt er berechnet die Adresse des Pointers pI2CBuff doch erst im 
Atomic Block, d.h. nach dem cli. Weiß jemand wie man das verhindern 
kann???

[Falk B. (falk)]

Andreas R. schrieb:
> Das heißt er berechnet die Adresse des Pointers pI2CBuff doch erst im
> Atomic Block, d.h. nach dem cli. Weiß jemand wie man das verhindern
> kann???

Den Pointer selber volatile definieren, nicht nur das Ziel, auf welches 
er zeigt.

volatile uint8_t* volatile pI2CBuff = &txbuffer[i*4];

[Falk B. (falk)]

Beitrag "Re: Schwerer Bug in AVR-GCC 4.1.1"

[Andreas R. (andreasr)]

> volatile uint8_t* volatile pI2CBuff = &txbuffer[i*4];


das führt aber dazu dass der Pointer selbst nicht mehr in den Registern 
gehalten werden kann was zu noch ineffizienterem Code führt:

      in  r18, 0x3f  ; 63
 1ae:  f8 94         cli
 1b0:  e9 81         ldd  r30, Y+1  ; 0x01
 1b2:  fa 81         ldd  r31, Y+2  ; 0x02
 1b4:  af 01         movw  r20, r30
 1b6:  4f 5f         subi  r20, 0xFF  ; 255
 1b8:  5f 4f         sbci  r21, 0xFF  ; 255
 1ba:  5a 83         std  Y+2, r21  ; 0x02
 1bc:  49 83         std  Y+1, r20  ; 0x01
 1be:  00 83         st  Z, r16
 1c0:  e9 81         ldd  r30, Y+1  ; 0x01
 1c2:  fa 81         ldd  r31, Y+2  ; 0x02
 1c4:  af 01         movw  r20, r30
 1c6:  4f 5f         subi  r20, 0xFF  ; 255
 1c8:  5f 4f         sbci  r21, 0xFF  ; 255
 1ca:  5a 83         std  Y+2, r21  ; 0x02
 1cc:  49 83         std  Y+1, r20  ; 0x01
 1ce:  10 83         st  Z, r17
 1d0:  e9 81         ldd  r30, Y+1  ; 0x01
 1d2:  fa 81         ldd  r31, Y+2  ; 0x02
 1d4:  af 01         movw  r20, r30
 1d6:  4f 5f         subi  r20, 0xFF  ; 255
 1d8:  5f 4f         sbci  r21, 0xFF  ; 255
 1da:  5a 83         std  Y+2, r21  ; 0x02
 1dc:  49 83         std  Y+1, r20  ; 0x01
 1de:  80 83         st  Z, r24
 1e0:  e9 81         ldd  r30, Y+1  ; 0x01
 1e2:  fa 81         ldd  r31, Y+2  ; 0x02
 1e4:  af 01         movw  r20, r30
 1e6:  4f 5f         subi  r20, 0xFF  ; 255
 1e8:  5f 4f         sbci  r21, 0xFF  ; 255
 1ea:  5a 83         std  Y+2, r21  ; 0x02
 1ec:  49 83         std  Y+1, r20  ; 0x01
 1ee:  90 83         st  Z, r25
 1f0:  2f bf         out  0x3f, r18  ; 63

[Oliver S. (oliverso)]

Falk B. schrieb:
> Den Pointer selber volatile definieren, nicht nur das Ziel, auf welches
> er zeigt.
>
> volatile uint8_t* volatile pI2CBuff = &txbuffer[i*4];

Dann berechnet der den Pointer nicht nur einmal, sondern gleich viermal 
in dem Block. Und das zu Recht...

Andreas R. schrieb:
> Weiß jemand wie man das verhindern
> kann???

Neueren gcc verwenden? Mein kleines Testprogrämmchen mit gcc 10.1 hat 
das Problem nicht. Wobei es vermutlich sehr vom nicht gezeigten Code 
drumherum abhängt, was der gcc draus macht.

Ich würde es mal so probieren:

      volatile uint8_t* volatile pI2CBuffv = &txbuffer[i*4];
      volatile uint8_t* pI2CBuff = pI2CBuffv;

[Andreas R. (andreasr)]

Oliver S. schrieb:
> Ich würde es mal so probieren:
>

>       volatile uint8_t* volatile pI2CBuffv = &txbuffer[i*4];
>       volatile uint8_t* pI2CBuff = pI2CBuffv;
>

Das fixt das Problem. Danke!

Compiler will (kann) ich nicht updaten, ist mit Atmel (Microchip) Studio 
verbandelt.

[Andreas R. (andreasr)]

Oliver S. schrieb:
> Wobei es vermutlich sehr vom nicht gezeigten Code
> drumherum abhängt, was der gcc draus macht.


Der Code "drumherum" ist nicht so spannend; es wird ein ADC Wert und 
eine CRC 16 in einen 4 byte temp buffer geschrieben und dieser danach 
atomar in den Ausgangsbuffer des USI I2C Slaves (Martin Junghans 
[e-mail]) an eine bestimmte Stelle kopiert.

volatile uint8_t txbuffer[buffer_size];      // Transmission buffer to be read from the master

int __attribute__((OS_main)) main(void)
{
  InitIO();
  
    uint8_t i2cBit = (PIN & (1U << PIN_ADDR0)) ? 0 : 1;
  usiTwiSlaveInit(I2C_START_ADDR | i2cBit);  // TWI slave init
  
  InitADC();
  sei();
    while (1) 
    {
    for (uint8_t i = 0; i < 2; i++)
    {
      uint8_t flags;
      uint16_t sum;
      ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
      {
        flags = GmvData[i].Flags;
        GmvData[i].Flags = flags & ~1;
        sum = GmvData[i].Sum;
      }
      if ((flags & 3) == 3)
      {
        // new Data and GMV full
        uint16_t x = sum / GMV_SIZE;
        uint8_t tmp[4];
        tmp[0] = (x >> 0) & 0xff;
        tmp[1] = (x >> 8) & 0xff;
        x = 0xffff;
        x = _crc16_update(x, tmp[0]);
        x = _crc16_update(x, tmp[1]);
        tmp[2] = (x >> 0) & 0xff;
        tmp[3] = (x >> 8) & 0xff;
        volatile uint8_t* volatile pI2CBuffv = &txbuffer[i*4];
        volatile uint8_t* pI2CBuff = pI2CBuffv;
        ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
        {
          *pI2CBuff++ = tmp[0];
          *pI2CBuff++ = tmp[1];
          *pI2CBuff++ = tmp[2];
          *pI2CBuff++ = tmp[3];
        }
      }
    }
    }
}

[Falk B. (falk)]

Andreas R. schrieb:
> Der Code "drumherum" ist nicht so spannend;

Ich würde da aber auf JEDEN Fall einen sinnvollen Kommentar ergänzen, 
warum die beiden Pointer exakt so benutzt werden.

[Johann L. (gjlayde)]

Andreas R. schrieb:

>         __asm__ __volatile__ ( "" : : "memory" (pI2CBuff));

Das ist Käse:  Der Input-Operand hat Constraints "m", "e", "o", "r" und 
"y", ist also die Vereinigung all dieser Constraints.  Das ist kaum das, 
was du willst.

1) Wenn Speicher geklobbert werden soll, genügt ein Clobber auf memory:

__asm__ __volatile__ ("" : : : "memory");

2) Wenn nur ein bestimmtes Array-Object geclobbert werden soll, dann

__asm__ __volatile__ ("" : "+m" (*pI2CBuff));

was aber natürlich nicht funktioniert, wenn pI2CBuff nur eine Adresse 
ins Array ist.

3) In deinem Fall ist pI2CBuff aber nur eine einfache Variable, die in 
ein Register passt.  Ergo:

__asm__ __volatile__ ("" : "+r" (pI2CBuff));

Es geht nämlich nicht um den Inhalt des Arrays, sondern um die 
berechnete Adresse.

[Andreas R. (andreasr)]

Johann L. schrieb:

> 3) In deinem Fall ist pI2CBuff aber nur eine einfache Variable, die in
> ein Register passt.  Ergo:

__asm__ __volatile__ ("" : "+r" (pI2CBuff))

Genau das hab ich gesucht :-))
Vielen Dank!!!

 1e6:  2f b7         in  r18, 0x3f  ; 63
 1e8:  f8 94         cli
 1ea:  00 83         st  Z, r16
 1ec:  11 83         std  Z+1, r17  ; 0x01
 1ee:  82 83         std  Z+2, r24  ; 0x02
 1f0:  93 83         std  Z+3, r25  ; 0x03
 1f2:  2f bf         out  0x3f, r18  ; 63