Bytes einsammeln, Überlauf Problem?

[Hansi (Gast)]

Nabend,

irgendwie habe ich gerade schwerwiegende Denkprobleme ( sollte mal 
lieber Feierabend machen.. )

Kann ich hier einen Speicherüberlauf produzieren, wenn mein Speicher 
"inStream" 1024 Bytes groß ist?
Oder habe ich das korrekt abgefangen?

uint8_t *usartReadRingBuff      ( uint8_t *stream )    
{
  static uint16_t index = 0;
  
  uint16_t c = usartGetChar();
  
  if ( c & USART_NO_DATA )
  {
    return NULL;
  }
  
  if ( c > USART_NO_DATA)
  {
    index = 0; 
    return NULL;
  }

  static uint8_t last = 0;
   
  if ( last == '\r' && c == '\n' )
  {
    if ( (index - 1) > sizeof(inStream ) )
    {
      index = 0;
      return NULL;
    }
    else
    {
      *( stream + ( index - 1 ) ) = '\0';
      index = 0; 
    
      return stream;      
    }
  }last = c;  
  
  if ( index > sizeof(inStream) )
  {
    index = 0;
  }
  else
  {
    *( stream + index++ ) = (uint8_t)c;  
  }
  
  return NULL;
}

[guest (Gast)]

Die Größe von "inStream" ist völlig Banane, Du greifst eh nicht darauf 
zu.
Wesentlich interessante wäre, wo der Funktionsparameter hinzeigt.
Und je nachdem was ein "sizeof(int)" liefert, knallt es dann unter 
Umständen immer noch.

[Stefan F. (Gast)]

Die wichtigste Info fehlt: sizeof(inStream)

Das liefert Dir nämlich leider nicht die Größe des zuvor reservierten 
Speichers, sondern die Größe des Zeigers.

[Hmmmm (Gast)]

Hansi schrieb:
> if ( index > sizeof(inStream) )


index  darf nicht grösser als

(sizeof(inStream)) - 1)

werden.

[Hansi (Gast)]

Stefanus F. schrieb:
> Die wichtigste Info fehlt: sizeof(inStream)
>
> Das liefert Dir nämlich leider nicht die Größe des zuvor reservierten
> Speichers, sondern die Größe des Zeigers.

Wenn ich auf dem Rechner

int main()
{
  uint8_t buff[] = "Test1234";

  printf("%d" , sizeof(buff));

    return 0;
}

Gibt er mir exakt die Größe des Array´s aus.

[Stefan F. (Gast)]

Hansi schrieb:
> Stefanus F. schrieb:
>> Das liefert Dir nämlich leider nicht die Größe des zuvor reservierten
>> Speichers, sondern die Größe des Zeigers.
>
> Wenn ich auf dem Rechner
>

> int main()
> {
>   uint8_t buff[] = "Test1234";
> 
>   printf("%d" , sizeof(buff));
> 
>     return 0;
> }
>

> Gibt er mir exakt die Größe des Array´s aus.

Ja, weil du dort die Größe des Array abfragst.
Aber in deiner Funktion oben hast du die Größe des Zeigers abgefragt.

[Floppy (Gast)]

Hansi schrieb:

Deine Funktion hat "RingBuff" im Namen, hat aber mit einem Ringbuffer 
nichts am Hut. Das finde ich jetzt nicht so besonders gut.

Was glaubst du wird passieren, wenn "index == sizeof(inStream)" wird?
>
>   if ( index > sizeof(inStream) )
>   {
>     index = 0;
>   }
>   else
>   {
>     *( stream + index++ ) = (uint8_t)c;
>   }

[Stefan F. (Gast)]

Du musst deine Funktion so ändern, dass sie zwei Aufrufparameter hat: 
Einen Zeiger auf den Puffer und einen Integer mit dessen Größe.

[Floppy (Gast)]

@Stefanus
Wo ist oben? Ich sehe nirgends ein sizeof(stream).

[Stefan F. (Gast)]

@Floppy
Zwei Troll-Versuche genügen. Lass es gut sein und geht woanders hin.

[Hansi (Gast)]

Stefanus F. schrieb:
> Du musst deine Funktion so ändern, dass sie zwei Aufrufparameter
> hat: Einen Zeiger auf den Puffer und einen Integer mit dessen Größe

Ich glaube ich habe die Bezeichnungen ein wenig unglücklich gewählt..
inStream ist mein Speicher ( inStream[1024]
Und der Funktion übergebe ich den Zeiger auf den Bereich. Also die Größe 
müsste passen?

[Floppy (Gast)]

Stefanus F. schrieb:
> @Floppy
> Zwei Troll-Versuche genügen. Lass es gut sein und geht woanders hin.

Oh, da ist ja jemand sehr von sich überzeugt!

Stefanus F. schrieb:

> Aber in deiner Funktion oben hast du die Größe des Zeigers abgefragt.

Zeig doch mal, wo das sein soll.

[Stefan F. (Gast)]

Wie gesagt liefert "sizeof(inStream)" nicht die von Dir erwarteten 1024, 
sondern je nach Computer wahrscheinlich 2 oder 4.

[Walter S. (avatar)]

Stefanus F. schrieb:
> Zwei Troll-Versuche genügen. Lass es gut sein und geht woanders hin.

schau dir noch Mal den Parameter von sizeof genau an!

[Hansi (Gast)]

Stefanus F. schrieb:
> Wie gesagt liefert "sizeof(inStream)" nicht die von Dir erwarteten
> 1024, sondern je nach Computer wahrscheinlich 2 oder 4.

Okay nehmen wir an es würde da jetzt noch die 1024 stehen, würde es denn 
so wie es ist funktionieren ohne Speicher Überlauf?

[Stefan F. (Gast)]

Hansi schrieb:
> Okay nehmen wir an es würde da jetzt noch die 1024 stehen, würde
> es denn so wie es ist funktionieren ohne Speicher Überlauf?

Es würde nicht funktionieren weil die letzte Index Position die Nummer 
1023 hat (die Zählung beginnt bei 0).

[Hansi (Gast)]

Okay, also Reservierter_Speicher-1, habe ich das richtig verstanden?

[Stefan F. (Gast)]

Hansi schrieb:
> Okay, also Reservierter_Speicher-1, habe ich das richtig verstanden?

Du kannst den gesamten reservierten Speicher belegen.

Die erste Speicherzelle ist *( stream + 0 )
Die letzte Speicherzelle ist *( stream + 1023 )

Du kannst übrigens auch "stream[index] = (uint8_t) c;" schreiben, 
bewirkt das Gleiche.

[Hansi (Gast)]

Jetzt verstehe ich es nicht mehr.. Wie müsste denn jetzt die if Abfrage 
aussehen? Du verunsicherst mich gerade..

[Experte (Gast)]

Hallo Hansi,

du hast grundsätzliche Konzepte von C offensichtlich nicht verstanden.
Auch dein Funktionsname usartReadRingBuff zeigt, dass du keinen 
Gendanken an
Softwaredesign/Abstraktion oder Generik/Wiederverwendbarkeit 
verschwendest.

Mein Tip:
Nimm dir eine Standard-Ringbuffer-Implementierung und such dir ein 
Beispiel wo man sieht, wie diese anzuwenden ist.

[Hansi (Gast)]

Experte schrieb:
> Hallo Hansi,
>
> du hast grundsätzliche Konzepte von C offensichtlich nicht verstanden.
> Auch dein Funktionsname usartReadRingBuff zeigt, dass du keinen
> Gendanken an Softwaredesign/Abstraktion oder
> Generik/Wiederverwendbarkeit verschwendest.
>
> Mein Tip: Nimm dir eine Standard-Ringbuffer-Implementierung und such dir
> ein Beispiel wo man sieht, wie diese anzuwenden ist.

Finde deine gefundenen Worte echt nett. Du gehörst in die Politik. Die 
Namenswahl lässt zum Teil zu wünschen übrig. Das stimmt wohl.

Die Funktion heißt so, weil sie mir Bytes aus einen anderen Ringpuffer 
raus saugt.

[Stefan F. (Gast)]

Hansi schrieb:
> Jetzt verstehe ich es nicht mehr..
> Wie müsste denn jetzt die if Abfrage
> aussehen? Du verunsicherst mich gerade..

Das kann mal nicht so einfach beantworten, da du mehrere Fehler gemacht 
hat. Erkläre erstmal deinen Anwendungsfall.

Wir müssen das nämlich in einen der folgenden Varianten umbauen:

- Ringpuffer
- Zeilenpuffer
- Mehrere Zeilenpuffer

Und wir müssen noch unterschieden, ob Du Text oder Binärdaten empfangen 
willst. Das hast du nämlich alles wild durcheinander gewürfelt.

Wie willst du den Puffer auslesen? Zeilenweise, Zeichenweise oder 
Byte-Für-Byte?

Lies das, bevor du antwortest: 
https://www.mikrocontroller.net/articles/FIFO

Bis morgen...

[guest (Gast)]

Stefanus F. schrieb:
> Aber in deiner Funktion oben hast du die Größe des Zeigers abgefragt.

Woher willst Du wissen, ob inStream an dieser Stelle als Zeiger 
aufgefasst wird? Aus dem geposteten Codefragment geht das jedenfalls 
nicht hervor. Wenn da "sizeof(stream)" (ohne "in" und mit kleinem 
's')stehen würde hättest Du definitiv Recht, aber so? Wozu er den 
Bereich dann allerdings noch als Funktionsparameter braucht, ist genauso 
unklar.

[Hansi (Gast)]

Ich benutze hier aus dem Forum die UART Lib. Dort ist ein Ringpuffer 2n 
implementiert.

Da ich auf diesen nicht direkt von meinem Source drauf zu greifen kann, 
habe ich mir diese Funktion gebaut.
Diese liest solange ein bis die Zeichenfolge "\r\n" eintrifft.
Die ganzen anderen Bytes werden auch als die gehändelt. Es gehen also 
außer diese beiden Zeichen keine anderen lesbaren Zeichen ein.

Der Speicher für die ganzen gelesen Zeichen aus dem externen Ringpuffer 
landen in meinem Array. Wenn die erwartete Zeichenfolge wie beschrieben 
eingetroffen ist, gebe ich einen Zeiger auf dessen Anfang zurück. Wenn 
nicht "null"

[Stefan F. (Gast)]

Hansi schrieb:
> Diese liest solange ein bis die Zeichenfolge "\r\n" eintrifft.
> Die ganzen anderen Bytes werden auch als die gehändelt. Es gehen also
> außer diese beiden Zeichen keine anderen lesbaren Zeichen ein.

Mach mal langsam. Das versteht kein Mensch.

Ein kleiner Tipp: Wenn du Zeilenweise aus einem bestehenden Ringpuffer 
lesen willst, dann implementiere den stdin Stream mittels 
FDEV_SETUP_STREAM und benutze die Standard C Funktionen 
(https://www.nongnu.org/avr-libc/user-manual/group__avr__stdio.html) zum 
Auslesen, zum Beispiel gets().

Von meiner HelloWorld Vorlage kannst du abgucken: 
http://stefanfrings.de/avr_hello_world/index.html

[Experte (Gast)]

Eigentlich brauchst du einen Zustandsautomaten, der solange Zeichen in 
einen Buffer einliest, bis \r und dann \n empfangen wurde und dann \r 
und \n abschneidet, den String \0-terminiert und zurückgibt.

Wenn der Buffer voll ist bevor \r und \n empfangen wurde, müssen solange 
alle Zeichen verworfen werden bis \r und \n empfangen wurde.
Danach geht es von vorne los.

Der Zustandsautomat kann meinetwegen aus einem Uart-Ringbuffer 
zeichenweise(!) auslesen. Er kann dann meinetwegen die einzelnen Strings 
wieder in einen Ringbuffer schreiben, der String-Objekte aufnehmen kann.

Oder auch anders, ich kenne ja seinen Anwendungsfall nicht. Aber dieses 
Zusammengemansche verschiedener Konzepte und Anfoerderungen ohne jede 
Abstraktion/Generik ist einfach nur schlecht.

[Floppy (Gast)]

Hansi, lass dich nicht verwirren. Wenn "inStream" ein Array von 1024 
Bytes ist, dann liefert "sizeof(inStream)" die 1024 zurück. Wer etwas 
anderes meint, sollte die Weihnachtszeit nutzen um ein paar Lektionen 
C/C++ zu wiederholen.

Wenn du die Funktion ausschließlich mit "inStream" nutzt, dann ist der 
Übergabeparameter "*stream" hier überflüssig und verwirrend. Wenn du die 
Funktion aber mit verschiedenen Buffern "inStream1, inStream2, .." 
nutzen möchtest, dann solltest du jeweils die Größe der Buffer beim 
Aufruf mitübergeben, und diesen Parameter dann anstelle von 
"sizeof(inStream)" benutzen.

Wenn der Buffer 1024 Bytes gross ist, dann darf "index" maximal 1023 
sein sonst schreibst du hinter den Buffer. Es gibt 2 Stellen in deiner 
Funktion die du ändern solltest.

Hansi schrieb:

>

>   if ( last == '\r' && c == '\n' )
>   {
>     if ( (index - 1) > sizeof(inStream ) )   <-- ALT
      if ( index > sizeof(inStream ) )         <-- NEU
Hier darf index == 1024 sein, da ja nur auf "index -1" geschrieben wird (\r durch \0 im Buffer ersetzt)
>     {
>       index = 0;
>       return NULL;
>     }
>     else
>     {
>       *( stream + ( index - 1 ) ) = '\0';
>       index = 0;
> 
>       return stream;
>     }
>   }last = c;
> 
>   if ( index > sizeof(inStream) )     <-- ALT
    if ( index >= sizeof(inStream) )    <-- NEU
Hier ist index == 1024 schon im verbotenen Bereich
>   {
>     index = 0;
>   }
>   else
>   {
>     *( stream + index++ ) = (uint8_t)c;
>   }
> 
>   return NULL;
> }
>

[Stefan F. (Gast)]

Ich muss mich entschuldigen, ich habe

> sizeof(inStream)    (=1024)

mit

>sizeof(stream)       (=2 oder 4)

verwechselt. Wenn man der Funktion einen Zeiger auf (irgendeinen) Puffer 
übergibt, macht es wenig Sinn, innerhalb der Funktion die Größe des 
konkreten inStream Puffers zu benutzen. Denn der übergebene und 
tatsächlich befüllte Puffer könnte ja ein anderer sein (als inStream), 
uns somit auch eine andere Größe haben.

Diese Unstimmigkeit hat mich verwirrt. Ich habe dachte es sei nur ein 
offensichtlicher Rechtschreibfehler (daher mein Trollvorwurf and 
Floppy), aber es war wohl ein grober Designfehler.

Entweder verzichtest du auf den Funktionsparameter, dann ist die 
Funktion fest an den einen inStream Puffer und seine Größe gebunden, 
oder du übergibst einen beliebigen Puffer als Parameter, brauchst dann 
aber einen zweiten Parameter, der dessen Größe angibt.

Die Größe des Pointers kannst du jedenfalls nicht gebrauchen.

Ich stimme Floppy und Walter S also nun doch zu.