PIC bearbeitet MAIN{} nicht richtig nach Programmänderung

[C. L. (calle)]

Hallo Leute,

Ich habe hier ein unglaubliches Phänomen!!

Kurz zur Ausgangssituation:
Ich habe ein PIC18F2685 der via RS232 mit einem Wiznet Wlanmodul 
kommuniziert und "GET" - Methoden entgegennimmt und darauf antwortet und 
LEDs steuert bzw. Taster einliest.
Sozusagen ein kl. Webserver bzw. er bedient die NET IO APP. (hier schon 
öfter drüber gesprochen).

Das funktioniert auch soweit erstmal.
Jetzt möchte ich das Programm erweitern und plötzlich wird die MAIN 
Routine nach dem flashen zwar angesprungen aber dann wieder unterbrochen 
bzw. bleibt irgendwie stehen.
Die Programmbearbeitung scheint einfach zu stehen.
Das merke ich an Blinkfolge einer LED zu Testzwecken.

Wackelkontakte kann ich ausschließen.
Das Board läuft mit einem Blinki Programm tagelang durch und ist sonst 
auch unauffällig!

Geflasht wird der PIC mit dem PIC Kit 2 per ICP.
Er hat 96Kb und es werden gerade mal 5% genutzt.

Ich habe das Gefühl, das oberhalb ca. 4650 Bytes ROM das Problem 
auftritt.
Der Compiler (Mikroe V6.61) meckert auch nicht. Auch kein DEMO Limit.
Er gibt nur einige Hinweise, das mit der Pointerübergabe was nicht 
stimmt.
Habe ich anders nicht hinbekommen, Programm läuft aber (wenn der Fehler 
nicht auftritt).
Hier die Compiler Warnungen:
 Implicit conversion of int to ptr
 Implicit conversion of pointer to int

Könnt Ihr mal mit drüberschauen?
Ist da ein Stack oder BANK Problem?

Ich stehe seit Tagen auf dem Schlauch!

Comp. Meldungen, Programm, die Hex. die bei mir nicht funst und 
Configbits anbei.

Habe auch einen anderen PIC ausprobiert und sogar einen anderen Typen.
Das gleiche Problem!

PS: Vlt. kann ja mal jemand die hex. in einen anderen PIC laden und 
untersuchen.
Wenn die LED an Port A1 einmal schnell und 6 mal langsamer blinkt, geht 
es bei Euch.

thx
CL

[Rufus Τ. F. (rufus)]

C. L. schrieb:
> Er gibt nur einige Hinweise, das mit der Pointerübergabe was nicht
> stimmt.

Das sind Hinweise darauf, daß da etwas sehr streng riecht.

> Habe ich anders nicht hinbekommen, Programm läuft aber (wenn der Fehler
> nicht auftritt).
> Hier die Compiler Warnungen:
>  Implicit conversion of int to ptr
>  Implicit conversion of pointer to int
> Könnt Ihr mal mit drüberschauen?


Die Fehlermeldungen enthalten eine Zeilennummer, Deine IDE sollte bei 
einem Doppelklick auf die zugehörige Codezeile springen.

Da solltest Du schon selbst nachsehen, welche Zeilen das verursachen.

[C. L. (calle)]

Hi!

Das mit den Zeilennummern ist klar, aber ich dachte, das sind Warnungen 
und keine Fehler. Das Programm wird ja compiliert und läuft auch. Nur 
wenn ich die Routine erweitere, dann gibts Probleme.
Deswegen habe ich nicht weiter in diese Richtung geforscht.
Kann das den trotzdem damit zusammenhängen?

CL

[C. L. (calle)]

Ich habe auch mal gecheckt ob ich zu viele Schachtelungen (nested calls) 
benutze. Deer PIC kann aber lt. Datenblatt 31 Stück. Da bin ich weit von 
entfernt.

CL

[Karl H. (kbuchegg)]

C. L. schrieb:
> Hi!
>
> Das mit den Zeilennummern ist klar, aber ich dachte, das sind Warnungen
> und keine Fehler.

In der professionellen Programmierung gilt:
Warnungen werden wie Fehler behandelt.

und das ist nicht ohne Grund so.

Der C-Standard spezifiziert, was definitiv als Fehler gewertet wird. Das 
muss von einem Compiler auch als Fehler gemeldet werden. Darüber hinaus 
dürfen Compiler auch noch Dinge warnen, die nicht spezifiziert sind. 
Derartige Warnungen, speziell wenn sie Pointer betreffen, sind oft 
schwerwiegend. Die Warnung des Compilers ist ernst zu nehmen, denn 
meistens hat er recht und da gibt es tatsächlich ein Problem.

> Das Programm wird ja compiliert und läuft auch. Nur
> wenn ich die Routine erweitere, dann gibts Probleme.

Ergo hat deine Erweiterung ein ernsthaftes Problem aufgedeckt, dass nur 
zufällig vorher nicht zugeschlagen hat.

> Kann das den trotzdem damit zusammenhängen?

Können tut alles.
Wer Warnungen ignoriert, hat es erst mal nicht besser verdient. Wenn 
dein Programm auf höchstem Warning Level ohne Meldung compiliert und 
dann immer noch fehlerhaftes Verhalten zeigt, dann wird es interessant.

[Daniel S. (daniel_s49)]

C. L. schrieb:
> aber ich dachte, das sind Warnungen und keine Fehler

Warnung heißt in dem Fall, dass es compiliert und der μc vermutlich auch 
irgendwas damit anfangen kann, aber es heißt nicht, dass es auch 
tatsächlich das tut, was du erwartest. Im Zweifel benutzt er halt quasi 
zufällige Werte aus dem RAM als Pointer und greift auf Speicheradressen 
zu, in denen er nichts verloren hat.

C. L. schrieb:
> Ist da ein Stack oder BANK Problem?

Du hast eigentlich keine groß verschachtelten Funktionsaufrufe, von 
daher würde ich das eher in den Hintergrund schieben.

----
also nachdem die Pointer-Warnungen weg sind:

Bleibt die Ausführung immer sofort stehen? Oder erst, wenn du ihm einen 
bestimmten Wert schickst? Oder immer nach der selben Zeit? Und woran 
merkst du das


Was mir sonst am Code auffällt:
- Variablen, die in Interrupts verändert werden, sollten häufig besser 
volatile sein. In diesem Fall zB die RecDataCounter
- void Zeit_ms(unsigned Zeit)  - vermutlich steht irgendwo, wie der 
Compiler das zu verarbeiten hat, aber ich würd da explizit noch einen 
Typen angeben.
- wenn du ihm "/?blue=22" schickst, ist nicht nur das if mit der 22 
true, sondern auch das mit der 2 ("/?blue=2" ist in "/?blue=22" 
enthalten). Damit durchläufst du zweimal die HTTP_Response() und die 
TCP_Close().
- Generell würde ich für das Filtern der Strings Reguläre Ausdrücke 
empfehlen. Alternativ erst auf die farbe filtern und dann den 
Zahl-String dahinter in einen int umwandeln (Stichwort zB atoi).
- Du vergleichst Strings mit strstr(). Ein String in C ist eine Kette 
von Zeichen, gefolgt von einer Null (\0). Er sucht also so lange, bis er 
eine Null in txt findet. (quickfix: txt[99] = 0; als erste Zeile in die 
ZeichenketteFinden())

[Karl H. (kbuchegg)]

Du hast eine Funktion

char* ZeichenketteFinden(char *txtFinden)

die liefert als Rückgabewert einen Zeiger.

Aber was liefert sie tatsächlich beim Return?

  if (*test != 0)  return 1; else return 0;

aha. Also doch keinen Zeiger, sondern einen Integer (oder einen unsigned 
char, wenn man unsigned char als 'kleinen Integer ohne Vorzeichen' 
auffasst).

Wie wird die Funktion verwendet?
zb so

           erg = ZeichenketteFinden("GET /?status=Kamin HTTP/1.1");

der Return Wert der Funktion wird an die Variable erg zugewiesen. Da die 
Funktion einen Pointer liefert, müsste erg dann auch eine Pointer 
Variable sein (dazu später mehr). Welchen Datentyp hat sie tatsächlich?

unsigned char erg;

Oops. Kein Pointer. Ein unsigned char.

Bereinige erst mal diesen Pallawatsch.

Die Funktion will ganz offensichtlich keinen Pointer returnieren und 
auch der Aufrufer will keinen Pointer haben. Also sollte die Funktion 
auch nicht so tun als ob.

unsigned char ZeichenketteFinden(char *txtFinden)
....

und dann ist das erst mal bereinigt und weh getan hat es auch nicht.

Und dann sehen wir weiter, ob das Problem immer noch besteht.

[C. L. (calle)]

OK, Update.

- Danke erstmal, das ihr Euch da so reinhängt -

Ich habe jetzt die Pointergeschichte umgestellt und kompiliert - 
Programm läuft auch. Die einzige Warnung ist die etwas abweichende 
Baudratenberechnung. Ich könnte zwar jetzt einen Baudratenquarz 
einlöten, aber das kann der Fehler nicht sein!

Anbei nochmal die .c Datei und die Compilermeldungen.
Habe nun nochmal mit den Anweisungen rumgespielt und den ROM Bytewert 
beachtet. Das Programm läuft einwandfrei wenn ich unter oder gleich 
4654 Byte bleibe. Eine Anweisung (2 Byte) mehr, und es tritt das Problem 
auf.
Sry, das mit den Zeigern ist noch etwas unroutiniert. ;-)

Was nun?

CL

[Karl H. (kbuchegg)]

Das hier

unsigned char *txt = "                                                                                                                                                                                                                                                                                                                                                                                                                                                                  ";

....

void RS232_RECV() iv 0x0008 ics ICS_AUTO
{
  txt[RecDataCounter] = RCREG;
  RecDataCounter += 1;
}

ist sehr mutig!

Definier doch deinen Speicherplatz für den empfangenen Text mit einer 
ordentlich definierten Größe!

#define TXT_LEN  50
unsigned char txt[TXT_LEN];

und überprüf auch, ob du beim Empfang nicht das Array überläufst!

void RS232_RECV() iv 0x0008 ics ICS_AUTO
{
  if( RecDataCounter < TXT_LEN ) {
    txt[RecDataCounter] = RCREG;
    RecDataCounter += 1;
  }
}

Genau solche Bufferüberläufe sind es, vor denen seit mehr als 30 Jahren 
jeder C Programmierer gewarnt wird und die C einen schlechten Ruf 
eingebracht haben. Kein Wunder, wenn immer wieder Jungspunde nachkommen, 
die alles über Bord werfen, was man je über halbwegs sicheres 
Programmieren gelernt hat.

Das hier in main

    if (RecDataCounter > 100)   // Irgendeine lange Anfrage kommt vom Internet (GET, POST oder HTML Befehle o.ä.)

ist sowieso erstklassiger Unsinn.
Der Browser schliesst jede 'Zeile' mit einem '\n' (also einem Carriage 
Return) ab. Das ist dein Trigger, dass du eine Zeile zur Auswertung hast 
und nicht wenn dein Buffer voll ist (ich hab nicht nachgezählt, ob dein 
originaler String überhaupt 100 Zeichen lang war.


Alles in allem muss da noch viel Arbeit rein gesteckt werden. So wie das 
jetzt ist, ist das größtenteils ziemlicher Unsinn.

[C. L. (calle)]

OK, weiß ich Bescheid.
Die von Dir angesprochenen Themen werde ich nochmal umstellen, 
allerdings erst morgen oder übermorgen. So lernt man dazu.

Der String der Get Methode ist in diesem Fall deutlich >100 aber das 
gehe ich auch an. Ich frage mich nur, wie ich eine ordentliche Länge 
definieren soll, wenn ich nicht weiß, wie lange die Zeichenkette maximal 
sein kann.
Wenn man keine IP sondern ein wie auch immer gearteten DNS String mit 
übergibt, kann es ja sein, das der Buffer trotzdem überläuft.
Oder meinst Du, nach "\n" suchen und den Buffer ORDENDLICH groß machen?

Melde mich morgen oder übermorgen dazu nochmal.

CL

[Luki (Gast)]

C. L. schrieb:
> Ich frage mich nur, wie ich eine ordentliche Länge
> definieren soll, wenn ich nicht weiß, wie lange die Zeichenkette maximal
> sein kann.
Normalerweise würde man das mit dynamischem Speicher machen, das geht 
auf einem  8bit PIC aber wahrscheinlich nicht so gut.

[Karl H. (kbuchegg)]

C. L. schrieb:
> OK, weiß ich Bescheid.
> Die von Dir angesprochenen Themen werde ich nochmal umstellen,
> allerdings erst morgen oder übermorgen. So lernt man dazu.
>
> Der String der Get Methode ist in diesem Fall deutlich >100 aber das
> gehe ich auch an. Ich frage mich nur, wie ich eine ordentliche Länge
> definieren soll, wenn ich nicht weiß, wie lange die Zeichenkette maximal
> sein kann.

Du musst eine Maximallänge festlegen.
Länger darf dann eben das nicht sein, was vom Browser kommt.

Was du aber auf keinen Fall machen darfst:
einfach ungeprüft alles in den bereitgestellten Speicher knüdeln. Denn 
wenn der bereit gestellte Speicher zu Ende ist, dann überschreibst du 
irgendwas im Speicher. Ab dann werden keinerlei Wetten mehr 
angenommen, was das Programm machen wird.

Wenn das was vom Browser kommt länger als der bereitgestellte Speicher 
ist, dann musst du dir was einfallen lassen, wie du damit umgehst. Aber 
einfach weiter in den Speicher schreiben ist keine Lösung.

Und sooo lange sind die Sachen vom Browser ja dann auch wieder nicht. 
Dich interessiert ja wahrscheinlich sowieso nur die Zeile, die mit 'GET' 
anfängt.

[Karl H. (kbuchegg)]

C. L. schrieb:

> Wenn man keine IP sondern ein wie auch immer gearteten DNS String mit
> übergibt, kann es ja sein, das der Buffer trotzdem überläuft.

Das darf eben nicht passieren.

> Oder meinst Du, nach "\n" suchen und den Buffer ORDENDLICH groß machen?

Nach dem \n suchst du erst gar nicht.
Das erledigt der Empfangsinterrupt, der sich das empfangene Zeichen 
ansieht und wenn es ein \n ist, dann signalisiert er mit einem Flag der 
Hauptroutine in main(), dass es eine Zeile zur Auswertung gibt und 
wahrscheinlich wird er die Zeile in einen 2-ten Buffer umkopieren, damit 
die Auwertung sich in Ruhe die empfangene Zeile ansehen kann, während 
über den Empfangsinterrupt die nächsten Zeichen eintrudeln.
Ausserdem wäre es gut, wenn da dann auch gleich ein String nach den C 
Regeln gebildet wird.
(Und die Doku zu strstr solltest du dir auch noch mal ansehen. Mit 
Pointern scheinst du auf Kriegsfuss zu stehen. Wenn der Suchstring NICHT 
enthalten ist, dann kriegst du einen NULL Pointer zurück. Den darfst du 
NICHT derefeenzieren. Brauchst du auch nicht. Denn man kann den 
Returnwert von strstr ja durchaus prüfen, ob das ein NULL Pointer ist)

unsigned char txt[INPUT_LEN];
unsigned char receivedLine[INPUT_LEN];
volatile unsigned char haveInput;

void RS232_RECV() iv 0x0008 ics ICS_AUTO
{
  unsigned char c = RCREG;

  if( c == '\n' ) {
    txt[RecDataCounter] = '\0';   // C String bilden
    strcpy( receivedLine, txt );
    RecDataCounter = 0;
    haveInput = 1;
  }

  else {
    if( RecDataCounter < INPUT_LEN - 1 )
      txt[RecDataCounter++] = c;
  }
}


.....
unsigned char ZeichenketteFinden(char *txtFinden)
{
  if( strstr(receivedLine, txtFinden) == NULL )
    return 0;

  return 1;
}

int main()
{
   .....

 while(1)
 {
    if (haveInput) 
    {
      haveInput = 0;

      if( ZeichenketteFinden("GET /?status=Kamin HTTP/1.1") )
      {
 ....

Das sollte erst mal die ärgsten Fehler beseitigen.  Wobei sich jetzt 
natürlich die Frage erhebt, wozu man die Funktion ZeichenketteFinden 
überhaupt noch benötigt. So wie ich das sehe wäre ein

 while(1)
 {
    if (haveInput) 
    {
      haveInput = 0;

      if( !strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") )
      {
....

genausogut zu lesen. Oder meinetwegen ein

     if( strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") != NULL )

was mehr oder weniger genau das gleiche aussagt.

[Karl H. (kbuchegg)]

Karl H. schrieb:


>

>  while(1)
>  {
>     if (haveInput)
>     {
>       haveInput = 0;
> 
>       if( !strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") )
>       {
> ....
>

Das Rücksetzen von haveInput auf 0 sollte man konsequenter Weise erst am 
Schluss, nachdem man die Zeile ausgewertet hat machen. Denn was du auf 
keinen Fall willst, dass ist, dass dir der Empfangsinterrupt die Zeile, 
die du gerade auswertest unter dem Hintern ändert.

  while(1)
  {
    if (haveInput)
    { 
      if( !strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") )
      {
        ....
      }
      if( !strstr(receivedLine, ".....
      {
        ....
      }
      ....

      haveInput = 0;
    }
  }

Sinniger Weise müsste man da eigentlich eine Queue implementieren, in 
der der Empfangsinterrupt Zeilen ablegen kann, solange die Hauptschleife 
noch mit der Auswertung beschäftigt ist.

Fällt mir gerade ein:
Was man natürlich auch machen könnte, dass ist dass im Empfangsinterrupt 
alle Zeilen, die nicht mit GET anfangen gleich verwirft. Sie werden ja 
für die Auswertung sowieso nicht gebraucht. Das verschafft dann der 
Hauptschleife ein wenig Zeit, in der im Empfangsinterrupt die 
Browseridentifikation und sonstige Mitteilungen durchlaufen und 
ignoriert werden, während die Hauptschleife die Antwort auf den letzten 
GET an den Mann bringt.

void RS232_RECV() iv 0x0008 ics ICS_AUTO
{
  unsigned char c = RCREG;

  if( c == '\n' ) {
    txt[RecDataCounter] = '\0';         // C String bilden
    if( !strncmp( txt, "GET", 3 ) ) {   // Alle Zeilen, die nicht mit GET
                                        // anfangen interessieren die
                                        // Auswertung nicht. D.h. die braucht
                                        // davon nichts wissen
      strcpy( receivedLine, txt );
      haveInput = 1;
    }
    RecDataCounter = 0;
  }

  else {
    if( RecDataCounter < INPUT_LEN - 1 && c != '\r' )
      txt[RecDataCounter++] = c;
  }
}

[Luki (Gast)]

Karl H. schrieb:
> if( !strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") )
>       {
> ....
> genausogut zu lesen. Oder meinetwegen ein
>  if( strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") != NULL )
Das ist jetzt aber nicht das gleiche, das in der oberen if ist true, 
wenn strstr() == NULL.

[Karl H. (kbuchegg)]

Luki schrieb:
> Karl H. schrieb:
>> if( !strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") )
>>       {
>> ....
>> genausogut zu lesen. Oder meinetwegen ein
>>  if( strstr(receivedLine, "GET /?status=Kamin HTTP/1.1") != NULL )
> Das ist jetzt aber nicht das gleiche, das in der oberen if ist true,
> wenn strstr() == NULL.

Danke für den Catch.
Das ! wegdenken.
Da war ich wohl in Gedanken noch zu sehr beim strcmp