Strukturarray wird überschrieben bzw. kann nicht vollständig beschrieben werden.

[Martin S. (marten83)]

Hallo Zusammen,

ich grüble schon seit geraumer Zeit an einem Problem mit einem Array aus 
Strukturen.
Und zwar benötige ich eine Art Lookup Table die zur Programmlaufzeit mit 
Daten gefüllt wird.

typedef struct AIS_MMSI_table
{
  uint8_t occupied:1;
  uint32_t MMSI;
  uint16_t section;
}AIS_MMSI_lookup_t;

static AIS_MMSI_lookup_t AIS_data_section[MAXVESSELS];

static uint8_t list_depth = 0;

Diese Liste wird in folgender Routine beschrieben Routine:

/*
'AIS_search_data_section_put' 
compares MMSI with those in the list and returns memory adress of the SRAM
  -unoccupied sections are filled as early as possible
  -list is expanded if new MMSI is recognised
  -list can grow up to 'MAXVESSELS'
  -'0xFFFF' (= ERROR) will be returned in case of
   no MMSI match and full list */
uint16_t
AIS_search_data_section_put (const uint32_t comp_MMSI)
{
  /*local variables*/
  volatile uint8_t out = 0;
  volatile uint8_t match = 0;
  volatile uint16_t section = 0;
  volatile uint8_t newsection = 0;
  volatile uint8_t search_sec = 0;
  volatile uint8_t temp_section = 0;

  char puffer1 [20];

  /*loop until MMSI found or new section occupied*/
  do
  {
    /*check if section is occupied*/
    if (AIS_data_section [search_sec].occupied != 0)
    {
      utoa (AIS_data_section [search_sec].occupied, puffer1, 10);
      glcd_print (27, (search_sec + 2), &puffer1 [0]);
      
        ultoa (comp_MMSI, puffer1, 10);
        glcd_print (0, 3, &puffer1 [0]);
        
        ultoa (AIS_data_section [search_sec].MMSI, puffer1, 10);
        glcd_print (0, 4, &puffer1 [0]);


      /*store memory adress if MMSI matches*/
      if (AIS_data_section [search_sec].MMSI == comp_MMSI)
      {        
        section = AIS_data_section [search_sec].section;
        match = 1;
        out = 1;
      }
    }
    /*reserve first unoccupied section to store new AIS data
      in case of no MMSI match*/
    else 
    {
      if (newsection == 0)
      {
        section = search_sec * 0x40;
        temp_section = search_sec;
        newsection = 1;
      }
    }

    /*increment section to compare next MMSI...*/
    if (search_sec < list_depth)
    {      
      search_sec++;
    }
    /*...or expand list if necessary and leave loop*/
    else
    {
      if (search_sec == list_depth)
      {
        if (list_depth < MAXVESSELS)
        {
          list_depth++;
        }
      }
      out = 1;
    }
  }
  while (out == 0);

  /*store new data in list*/
  if (match == 0 && newsection == 1)
  {
    AIS_data_section [temp_section].occupied = 1;
    AIS_data_section [temp_section].MMSI = comp_MMSI;
    AIS_data_section [temp_section].section = section;
    
    ultoa (AIS_data_section [temp_section].MMSI, puffer1, 10);
    glcd_print (30, (temp_section + 2), &puffer1 [0]);
  }

  /*ERROR if MMSI not found and list full*/
  if (match == 0 && newsection == 0)
  {
    section = 0xFFFF;
  }

  return section;
}/*AIS_search_data_section_put*/

Mein Problem ist nun, dass das Array zwar im SRAM richtig platziert wird 
(durch Deklaration 'static', ansonsten werden Teile des Arrays 
überschrieben), aber die Routine beschreibt nur maximal 6 Arrayelemente, 
obwohl mit 'MAXVESSELS = 25' 25 Elemente erzeugt worden sind.
Lasse ich die Deklaration 'static' weg, klappt das Beschreiben aller 
Elemente, aber die ersten Elemente werden (wahrscheinlich vom Stack) 
teilweise wieder überschrieben.

Was kann ich da machen?

Vielen Dank!

MfG, Marten83

[Klaus (Gast)]

Also auf meinem 0815 Controller läuft der Code super. Und er kann sogar 
Kaffee kochen.

Und was kann dein Controller?

[Karl H. (kbuchegg)]

Dein Code ist (unnötigerweise) ziemlich kompliziert aufgebaut. Ich bin 
noch nicht wirklich davon überzeugt, dass da alles mit rechten Dingen 
zugeht.

Was sagt deine Tool-Chain? Wieviel SRAM hast du noch frei?
(Wenn ich mal einen AVR als Prozessor und WinAvr als Toolchain 
voraussetze)

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Welchen Controller verwendest du?
Wie groß/voll ist denn dein RAM?
7*25 = 175 Bytes RAM für AIS_data_section[MAXVESSELS]
und
83*5 = 415 Bytes für char AISBUFFER [AIS_BUFCOUNT][83];
Das sind nur 2 augenfällige Arrays, wer weiß, was sonst noch so 
vergraben ist... :-/

/* global */
static uint8_t list_depth = 0;
:
:
  /*local variables*/
  volatile uint8_t out = 0;
  volatile uint8_t match = 0;
  volatile uint16_t section = 0;

Du solltest dir das Thema Modifier (insbesondere static und volatile) 
nochmal anschauen...

[Martin S. (marten83)]

...entschuldigung, natürlich habe ich vergessen die Umgebung klar zu 
machen.

Also der µC ist ein Mega644p mit 4kB SRAM wovon (laut AVR STudio) 1,4kB 
(~35%) belegt sind.

Jetzt frage ich mich was eine TOOLCHAIN ist?

[Karl H. (kbuchegg)]

So wie der Code momentan aussieht, könnte man den Speicherverbrauch 
eines AIS_MMSI_lookup_t ganz leicht von 7 Bytes auf 4 Bytes drücken. 
Dein Array würde dann nur noch ~die Hälfte Speicher verbrauchen.

* Das occupied Flag braucht keiner. Offenbar gibt es eine ungültige
  section-Nummer, nämlich 0xFFFF. Marikert man unbenutze Einträge mit
  einer derartigen section-Nummer, erfüllt das den gleichen Zweck wie
  das occupied Flag. (Was ist dir eigentlich dabei eingefallen, das als
  1-Bit Variable zu machen? Das spart keinen Speicher, weil du keine
  weiteren Bit-Variablen hast, zwingt aber den Compiler dazu, die
  Zugriffe auf das Flag komplizierter als notwendig zu machen)

* Offenbar gibt es einen fixen Zusammenhand zwischen dem Index des
  Eintrags und der Section Nummer.

   section = search_sec * 0x40;

  Wozu also die section-Nummer selbst speichern? Hat man bei einer
  Abfrage durch Suchen eine MMSI gefunden, kann man die zugehörige
  section-Nummer auch dort ganz leicht aus dem Index errechnen.
  -> die section-Nummer braucht in Wirklichkeit kein Mensch.


Beides zusammen führt dazu, dass du nur noch ein Array von MMSI hast, 
pro Eintrag also nur noch einen uint32_t

[Martin S. (marten83)]

Hmm, also, das mit den Modifiern ist mir schon klar. Da ich noch nicht 
genau weiss, worin mein Problem liegt habe ich einige Dinge probiert und 
die noch nicht wieder entfernt, aber danke für den Hinweis.

Das 'occupied' bit benötige ich schon um zu wissen, ob der 
Speicherbereich wieder frei ist.
Das mit der 'Section' ist allerdings ein guter Hinweis, daran habe ich 
nicht gedacht.
Naja, die Optimierung kommt ja noch, erstmal soll die Einsortierung 
klappen.

Das alles erklärt für mich, aufgrund der Tatsache, dass ich noch genug 
SRAM zur Verfügung habe, nicht das Verhalten.

Ich benutze Übrigens die neuste Winavr Version und AVR Studio 4.16.

[Karl H. (kbuchegg)]

Und die Sache mit der list_depth würde ich mir auch noch einmal 
gründlich überlegen.
So wie deine Eintragsfunktion (und wahrscheinlich auch die 
Löschfunktion) aufgebaut ist, kann es vorkommen, dass im Array 
unbenutzte Einträge und benutzte Einträge sich abwechseln. Wenn du also 
7 benutzte Einträge im Array hast, ist dadurch nicht gesagt, dass diese 
im Array an den Indexpositionen 0 bis 6 liegen. list_depth hat damit 
überhaupt keine Aussagekraft, welche Indizes gültig sind und welche 
nicht sondern nur darüber wieviele Einträge im Array benutzt sind. Wenn 
du also nicht den Fall hast, dass du diese Anzahl tausendemale im 
Programm benötigst, ist es nur eine Variable die du mitziehen musst, die 
dir aber keine wirkliche Information gibt: Die Anzahl unbenutzer 
Arrayeinträge kann man auch ganz leicht in einer Schleife ermitteln. Der 
Nachteil ist, dass das dann zwar ein wenig länger dauert. Der Vorteil 
ist aber, dass die so ermittelte Anzahl niemals falsch sein kann, was 
bei einer zusätzlichen Variable nicht gegeben ist. Dort muss man dann 
nämlich darauf aufpassen, diese Variable immer und überall korrekt 
nachzuziehen.

[Karl H. (kbuchegg)]

Martin Stolper schrieb:

> Das 'occupied' bit benötige ich schon um zu wissen, ob der
> Speicherbereich wieder frei ist.

Da du einen eigenen Wert für 'ungültige Nummer' hast, brauchst du ihn 
eben nicht!

> Naja, die Optimierung kommt ja noch, erstmal soll die Einsortierung
> klappen.

Der Fehler muss nicht unbedingt in dieser Funktion liegen.

[gast (Gast)]

toolchain ~entwicklungsumgebung  == AVRStudio bei dir
aber sein etwas wirres herrumgewerfe von valatile und static is echt 
irre ^^

und merke :
wenn der compiler da was wegoptimiert ohne volatile ...
hast du grundlegend was falsch programmiert  :)

[Martin S. (marten83)]

Der Sinn  der Variable 'list_depth' liegt darin, dass ich in einer 
anderen Routine die darin enthaltenen Informationen auswerte und nicht 
das ganze Array durchsuchen muss.
Später sollen bis zu 255 Elemente möglich sein und die sollen nicht 
durchsucht werden wenn sie leer sind und am ende kein belegter Platz 
mehr ist (siehe uint16_t
AIS_search_data_section_get (void).

[Karl H. (kbuchegg)]

Wie sieht eigentlich dein Testprogramm aus?

[Martin S. (marten83)]

Wegen der Modifier.
Wie gesagt, ich weiss wann man sie benutzt, aber ich wollte Fehler 
ausschliessen auch wenn ich in diesem Falle weiss, dass ich sie da nicht 
brauche.

[Karl H. (kbuchegg)]

Martin Stolper schrieb:
> Der Sinn  der Variable 'list_depth' liegt darin, dass ich in einer
> anderen Routine die darin enthaltenen Informationen auswerte und nicht
> das ganze Array durchsuchen muss.

Frage:
Kann es sein, dass sich in deinem Array benutzte und unbenutzte Einträge 
abwechseln?

Wenn ja: list_depth liefert dir keine (oder nahezu keine) Information. 
Was soll die Aussagekraft von list_depth sein? Wieviele Einträge belegt 
sind. Schön. Nur welche sind es? Die von 0 bis list_depth? Mööp - 
falsch. Die benutzten Einträge können über das ganze Array verstreut 
sein.

Wenn nein: Warum machst du dann die Einfügeprozedur so kompliziert, wenn 
dir list_depth sowieso den Index des nächsten freien Array-Elements 
gibt?

[Karl H. (kbuchegg)]

So würde ich das machen

typedef uint32_t MMSI_t;

typedef struct AIS_MMSI_table
{
  MMSI_t MMSI;
}AIS_MMSI_lookup_t;

static AIS_MMSI_lookup_t AIS_data_section[MAXVESSELS];

#define SECTION_FACTOR  0x40
#define INVALID_MMSI    ((MMSI_t)-1)

/* ----------------------------------- */
void AIS_init( void )
{
  int8_t i;

  for( i = 0; i < MAXVESSELS; ++i )
    AIS_data_section[i].MMSI = INVALID_MMSI;
}

/* ----------------------------------- */
static int8_t AIS_search_for_index( MMSI_t comp_MMSI )
{
  int8_t i;
  for( i = 0; i < MAXVESSELS; ++i ) {
    if( AIS_data_section[i].MMSI == comp_MMSI )
      return i;
  }

  return -1;
}

/* ----------------------------------- */
uint16_t AIS_search_data_section_put( MMSI_t comp_MMSI )
{
  // suche nach der MMSI
  int8_t index = AIS_search_for_index( comp_MMSI );

  if( index == -1 ) {
    // nicht gefunden, versuche neuen Slot zu allokieren
    for( index  = 0; index  < MAXVESSELS; ++index  ) {
      if( AIS_data_section[index ].MMSI == INVALID_MMSI )
        break;
    }

    // kein Slot mehr frei?
    if( index == MAXVESSELS )
      return 0xFFFF;

    AIS_data_section[index ].MMSI = comp_MMSI;
  }

  return ((uint16_t)index) * SECTION_FACTOR;
}

/* ----------------------------------- */
void AIS_remove_section( MMSI_t comp_MMSI )
{
  int8_t index = AIS_search_for_index( comp_MMSI );

  if( index != -1 )
    AIS_data_section[index].MMSI = INVALID_MMSI;
}

/* ----------------------------------- */
uint8_t AIS_count( void )
{
  int8_t i;
  uint8_t cnt = 0;

  for( i = 0; i < MAXVESSELS; ++i ) {
    if( AIS_data_section[i].MMSI != INVALID_MMSI )
      cnt++;
  }

  return cnt;
}

Weniger Speicherverbrauch, einfachere Funktionen

[Simon K. (simon)]

AIS_data_section[i].MMSI = 0xFFFFFFFF;

Nicht 0xFFFFFFFFUL?

[Martin S. (marten83)]

Ich erkläre nochmal genauer was passieren soll:
Vorweg:der Verfall der Daten ist noch nicht existent, deswegen soll hier 
erstmal nur einsortiert werden.

Also, ich habe eine Struktur mit Daten welche aus einem Stream des USART 
gefüllt wird (classA_position_report). Diese Daten sollen in ein 
externes SRAM gespeichert werden.
Um nicht das gesamte SRAM auf der Suche einer bestimmten MMSI zu 
durchforsten (Performance) lege ich also eine Liste an in der die 
wichtigsten Daten stehen (MMSI, belegt und Speicherort (habe ich jetzt 
schon geändert)).
Diese Liste wird belegt und die entsprechenden Daten aus dem USART in 
das externe SRAM gelegt.
Kommen nun neue Daten mit anderer MMSI sollen auch neue Speicherbereiche 
belegt werden.
Kommen neue Daten mit bekannter MMSI soll der externe Speicherbereich 
der entsprechenden MMSI überschrieben werden.
Später kommt noch hinzu, dass die Daten nach einer bestimmten Zeitspanne 
veralten und der Speicherbereich wieder freigegeben wird.

[Karl H. (kbuchegg)]

Simon K. schrieb:
>

> AIS_data_section[i].MMSI = 0xFFFFFFFF;
>

>
> Nicht 0xFFFFFFFFUL?

Habs nochmal geändert. Jetzt mit einem typedef für uint32_t und
(MMSI_t)-1
statt der 0xFFFF

Vorteil: wird der uint32_t irgendwann mal auf einen underen uint Type 
geändert, passt der Compiler die Kennung für 'ungültig' (= alle Bits 1) 
an.

[Oliver (Gast)]

>Mein Problem ist nun, dass das Array zwar im SRAM richtig platziert wird
>(durch Deklaration 'static', ansonsten werden Teile des Arrays
>überschrieben), aber die Routine beschreibt nur maximal 6 Arrayelemente,
>obwohl mit 'MAXVESSELS = 25' 25 Elemente erzeugt worden sind.
>Lasse ich die Deklaration 'static' weg, klappt das Beschreiben aller
>Elemente, aber die ersten Elemente werden (wahrscheinlich vom Stack)
>teilweise wieder überschrieben.

>Was kann ich da machen?

Debuggen. Schritt für Schritt. Wenn das SRAM tatsächlich nicht voll ist, 
sind da der Fehlerbeschreibung nach massive Programmfehler drin, und die 
sollten sich im Debugger einfach finden lassen. Wenn da nur 6 Einträge 
geschrieben werden, was passiert denn genau beim siebten?

Oliver

[Martin S. (marten83)]

OK, der Code überzeugt natürlich schon. Aber löst das mein Problem mit 
den maximal 6 Einträgen?
Mein code funktioniert nämlich (bis auf das genannte Problem) auch.
Ich werde den code jetzt mal testen, interessiert bin ich (falls 
erfolgreich), trotzdem an der Ursache damit mir sowas nicht nochmal 
passiert.

[Simon K. (simon)]

Karl heinz Buchegger schrieb:
> Simon K. schrieb:
>>

>> AIS_data_section[i].MMSI = 0xFFFFFFFF;
>>

>>
>> Nicht 0xFFFFFFFFUL?
>
> Habs nochmal geändert. Jetzt mit einem typedef für uint32_t und
> (MMSI_t)-1
> statt der 0xFFFF
>
> Vorteil: wird der uint32_t irgendwann mal auf einen underen uint Type
> geändert, passt der Compiler die Kennung für 'ungültig' (= alle Bits 1)
> an.

Das ist natürlich noch viel besser :D Fine Tuning würde ich das nennen 
:-)

[Karl H. (kbuchegg)]

Martin Stolper schrieb:
> OK, der Code überzeugt natürlich schon. Aber löst das mein Problem mit
> den maximal 6 Einträgen?

Das Problem ist, dass in deinem Code nicht ersichtlich ist, warum dieses 
Problem überhaupt existiert! Auf der anderen Seite ist dein Code 
ziemlich kompliziert mit der while Schleife und den vielen Flags die die 
while Schleife steuern.
Vergleich einmal mit meinem Code. Der ist gestreamlined. Keine Schleife 
ist länger als ein paar Zeilen und die Funktionalität kann leicht 
überblickt werden. Es gibt keine dubiosen Flags, die irgendwelche 
Zustände anzeigen und die man im Kopf behalten muss, wenn man den Code 
analysiert.

Das Problem kann auch ganz woanders in deinem Code stecken. Du siehst 
beim Testen immer nur die Symptome aber nie die Ursache.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

OT:
> gestreamlined
Sag doch bitte einfach:
begradigt, übersichtlicher oder durchschaubarer
Wir haben im Deutschen so schöne Übersetzungen für "gestreamlined" ;-)

[Martin S. (marten83)]

OK, ich danke für diesen Hinweis.
Vielleicht habe ich meine Engstirnigkeit diesbezüglich dadurch 
überwunden, denn ich habe schonmal rausgefunden, dass das was mit meinen 
USART Buffern zu tun hat.
Ich werde mich da jetzt nochmal dran begeben.

[Martin S. (marten83)]

Jetzt muss ich nochmal was fragen:

Ich benötige eine brauchbare Lösung um einen Ringpuffer aufzubauen (Da 
scheint wirklich mein Problem zu liegen).
Ich will eigentlich nicht den FIFO aus RN-Wissen.de verwenden, weil ich 
die ganzen Strings nicht kopieren und mich dann auch noch um den Pointer 
kümmern will.
Ich habe mir einen Ringpuffer aus x Arrays gebaut und beschreibe diese.
Anscheinend schafft die Routine es nur bis zum letzten Array und hängt 
dann fest. Wie kann ich sinnvoll das besetzen und freigeben der arrays 
bewerkstelligen?

[Karl H. (kbuchegg)]

Hab ich das richtig herausgelesen:

Du benötigst einen Ringbuffer, wobei die Bufferelemente Strings sind?

> Ich habe mir einen Ringpuffer aus x Arrays gebaut und beschreibe
> diese. Anscheinend schafft die Routine es nur bis zum letzten Array
> und hängt dann fest. Wie kann ich sinnvoll das besetzen und
> freigeben der arrays bewerkstelligen?

Am besten gar nicht. In einem µC möchtest du nach Möglichkeit keine 
dynmischen Allokierungen machen. Daher wird wohl auch der RN-Ringbuffer 
so aussehen, wie er aussieht (ich kenne die Implementierung nicht. Wenn 
ich deinen Text lesen denke ich das ist ein ganz normaler 
char-Ringbuffer)


Zeig mal was du bisher hast.

[Martin S. (marten83)]

Bisher habe ich das so gemacht:

#define LF 0x0a
#define CR 0x0d
#define AIS_BUFCOUNT  4 //max. 8
#define AIS_FREEBUF    0b00011111//((1 << AIS_BUFFER) -1)
#define BAUDRATE_AIS    38400UL  

char AISBUFFER [AIS_BUFCOUNT][83];

struct
{
  uint8_t hardware:1;
  uint8_t data:1;
  volatile uint8_t RX_complete;
  volatile uint8_t occupied;
  volatile uint8_t overflow:1;
}AIS_UART;
/*
'ISR'
reads UART data register and stores character in buffer
  -buffer area is changed if 'LF' is detected
  -'AIS_UART.overflow' bit is set if no free buffer available
   or actual buffer area runs out of space*/
ISR (USART0_RX_vect)
{
  /*local variables*/
  char AIS_chartemp;
  static uint8_t AIS_charcnt = 0;
  static uint8_t AIS_bufcnt = 0;

  /*cache data register*/
  AIS_chartemp = UDR0;

  /*check if actual buffer is unoccupied*/
  if ((AIS_UART.occupied && (1<<AIS_bufcnt)) == 0)
  {
    /*check if end of string is reached*/
    if (AIS_chartemp == LF)
    {  
      AIS_UART.occupied |= (1<<AIS_bufcnt);
      if (AIS_bufcnt < AIS_BUFCOUNT) AIS_bufcnt++;
        else AIS_bufcnt = 0;
      AIS_charcnt = 0;
      AIS_UART.RX_complete++;
      char buffer67 [10];
      utoa (AIS_bufcnt, buffer67, 16);
      glcd_print (0,11,&buffer67[0]);
    }
    else
    {
      /*store cache into buffer if end not reached*/
      if (AIS_charcnt <= 81)
      {
        AISBUFFER [AIS_bufcnt][AIS_charcnt] = AIS_chartemp;
        AIS_charcnt++;
      }
      else
      {
        AIS_UART.overflow = 1;
      }
    }
  }
  else
  {
    AIS_UART.overflow = 1;
  }
}/*ISR (USART0_RX_vect)*/

Da funktioniert aber, wie gesagt, was mit der Array indizierung nicht.
ais_occbuf wird am ende auf 0b00100000 bzw 0b01000000 gesetzt und 
AIS_bufcnt verharrt auf '4'.

[Martin S. (marten83)]

..achso. Hier wird der Speicher wieder frei gegeben:

/*Convert Payload 
of position report (1,2 or 3) into separated vessel information*/

void
ais_msg_process (void)
{
  static uint8_t AIS_getbuf = 0;    
      char buf34 [20];
    utoa (AIS_UART.occupied, buf34, 2);
    glcd_print (0,12,"          ");
    glcd_print (0, 12, &buf34 [0]);
  
  if (AIS_UART.RX_complete != 0)
  {    
    if ((AIS_UART.occupied && (1<<AIS_getbuf)) == 1)
    {
      if (strncmp (&AISBUFFER [AIS_getbuf][0], "!AI", 3) == 0)
      {
        AIS_UART.hardware = 1;
      
        if (strncmp (&AISBUFFER [AIS_getbuf][3], "VDM", 3) == 0)
        {
          if (NMEA0183_CRC_check (&AISBUFFER [AIS_getbuf][0]) == 1)
          {
            char *payload_ptr;
        
            payload_ptr = nmea0183_delim_search(&AISBUFFER [AIS_getbuf][0], 5);

            six_bit_ascii_convert (payload_ptr);

            switch (*payload_ptr)
            {
              case 1:
                  classA_position_report (payload_ptr);
                  AIS_store_dynamic_data ();
                break;

              case 2:
                  classA_position_report (payload_ptr);
                  AIS_store_dynamic_data ();
                break;

              case 3:
                  classA_position_report (payload_ptr);
                  AIS_store_dynamic_data ();
                break;
            }
          
            AIS_UART.data = 1;
          }
        }
        else
        {
          AIS_UART.data = 0;
        }
      }
      else
      {
        AIS_UART.hardware = 0;
      }

      AIS_UART.occupied &= ~(1<<AIS_getbuf);
      AIS_UART.RX_complete--;
      char bufg[10];
      utoa(AIS_getbuf, bufg, 10);
      glcd_print(0,10, &bufg[0]);
      utoa(AIS_UART.RX_complete, bufg, 10);
      glcd_print(0,9, &bufg[0]);
    }
    if (AIS_getbuf < AIS_BUFCOUNT) AIS_getbuf++;
      else AIS_getbuf = 0;
  }
}

[Karl H. (kbuchegg)]

Martin Stolper schrieb:

Hmm.
Du speicherst da keine sauberen C-Strings mit einem '\0' am Ende. Das 
kann dir in der Weiterverarbeitung der Strings mächtig Probleme machen.

> Da funktioniert aber, wie gesagt, was mit der Array indizierung nicht.
> ais_occbuf wird am ende auf 0b00100000 bzw 0b01000000 gesetzt und
> AIS_bufcnt verharrt auf '4'.

Wo ist die Auslesefunktion, die die occupied Bits wieder freigeben muss?

(Edit: Die Frage nach der Auslesfunktion hat sich erledigt)


PS: Wusstest du, dass man solche unabhängige Teile wunderbar auf dem PC 
debuggen kann. Da hat man wesentliche bessere Möglichkeiten zum 
Debuggen. Nur so nebenbei :-)

[Martin S. (marten83)]

Das weiss ich schon. Aber es ist ziemlich mühselig die USART Inputs 
nachzubilden. Oder gibts da eine einfache möglichkeit? Ich kenne nur 
zwei:
In einer Schleife das Register beschreiben und die Werte beim Debuggen 
direkt ins register tippen.
Ich wäre Dankbar für eine bessere Lösung.

[Karl H. (kbuchegg)]

Martin Stolper schrieb:
> Das weiss ich schon. Aber es ist ziemlich mühselig die USART Inputs
> nachzubilden.

Nicht wirklich.
Du hast eine Funktion, die einen character in den Buffer schreibt und du 
hast eine Funktion, die eine Message abholt (sofern eine da ist).

Du machst dir am PC eine main(), die (hausnummer) 5 Zeichen über die 
Funktion in den Buffer schreibt, dann einen LF und dann die get Funktion 
aufruft (die Auswertung da drinnen schliesst du kurz, hat sowieso da 
drinnen nichts verloren)

Das ganze machst du im main in einer Schleife und kannst so wunderbar 
debuggen.

Die occupied Bits würde ich als erstes rausschmeissen, genauso wie den 
complete Counter. Alles was du brauchst sind die beiden Zähler, die 
jetzt statische Variablen der Funktionen sind. Die wandern als erstes in 
deine UART Struktur und die komplette Auswertung ob Platz für neues 
vorhanden ist bzw. ob eine Msg vorliegt, wird nur über diese beiden 
Counter gemacht.

Mit solchen diversen (vielen) Flags und Countern schiesst man sich oft 
ein Eigentor. Sie machen den Code des öfteren einfach nur 
unübersichtlich.

[Karl H. (kbuchegg)]

Ach, das ist fies

    if ((AIS_UART.occupied && (1<<AIS_getbuf)) == 1)


Der Vergleich ist Müll.
Der kann nur dann wahr werden, wenn AIS_getbuf genau 0 ist. In allen 
anderen Fällen kommt da durch die Verundung nicht 1 raus (wohl aber 
etwas, was ungleich 0 ist).

Manchmal ist es in C äusserst kontraproduktiv, wenn man zu explizit ist. 
Boolsche Ergebnisse sind da ein häufiges Beispiel. Am besten fährt man 
hier mit der Devise: weniger ist mehr.

[Stefan E. (sternst)]

Karl heinz Buchegger schrieb:
> Ach, das ist fies
>
>     if ((AIS_UART.occupied && (1<<AIS_getbuf)) == 1)
>
>
> Der Vergleich ist Müll.
> Der kann nur dann wahr werden, wenn AIS_getbuf genau 0 ist. In allen
> anderen Fällen kommt da durch die Verundung nicht 1 raus (wohl aber
> etwas, was ungleich 0 ist).

Na, na, na. ;-)
Da steht ein "&&", kein "&".

So wie der Vergleich dort jetzt steht, könnte man auch schreiben:

if (AIS_UART.occupied && (AIS_getbuf<16))

Ob das wirklich so beabsichtigt ist?

[Karl H. (kbuchegg)]

Stefan Ernst schrieb:
> Karl heinz Buchegger schrieb:
>> Ach, das ist fies
>>
>>     if ((AIS_UART.occupied && (1<<AIS_getbuf)) == 1)
>>
>>
>> Der Vergleich ist Müll.
>> Der kann nur dann wahr werden, wenn AIS_getbuf genau 0 ist. In allen
>> anderen Fällen kommt da durch die Verundung nicht 1 raus (wohl aber
>> etwas, was ungleich 0 ist).
>
> Na, na, na. ;-)
> Da steht ein "&&", kein "&".

Ja richtig!
Jetzt sehe ich es auch.
Da sollte aber ein & stehen :-)

Also gleich 2 Fehler in einer Zeile :-)


Sagte ich schon, dass solche Flags oftmals nur für Verwirrung sorgen :-)


Edit: In der ISR derselbe Fehler noch einmal

  /*check if actual buffer is unoccupied*/
  if ((AIS_UART.occupied && (1<<AIS_bufcnt)) == 0)

Auch hier muss es & heissen.

[Martin S. (marten83)]

Hmm, da habe ich mal wieder Mist gebaut...
Das stimmt natürlich und ich habe das gleich geändert und getestet.
Und siehe da, es klappt!!!

Vielen Dank dafür!

Ich werde heute noch die meisten eurer Verbesserungsvorschläge in die 
Tat umsetzen (hoffentlich verfalle ich nicht wieder in einen Flag Wahn).

Ich muss hiermit auch gleich ein Lob an diese Forum aussprechen:
Die Antworten prasseln hier nur so auf einen ein und die 
Diskussionsfreudigkeit scheint sehr hoch zu sein.

Dann mach ich mich mal ans Werk!

MfG, Marten83