Fix - GFSK & nano crash

[HomeAutoUser]

1. Ich bin nun mal so frei und erstelle diesen PR weil beim senden mit dem Nano es zu einem crash kommt. CC1101 FSK Sendeprobleme (nur 15 Nachrichten?!) #209 (comment)
   (Das Problem mit dem ESP32 ist ein Anderes)
2. Bugfix GFSK wurde noch behoben. (NanoCul cc1101 - nanocc1101_3.5.1-dev+20210531.hex - problem #206)

• Allows other modulations than ASK / OOK and 2-FSK to be received.
• revised PROGVERS
• defined unused pin´s

[sidey79]

Mir ist das nicht klar, wieso das eine Auswirkung hat, wenn wir die PINs ganz am Anfang auf Eingang mit pullup stellen, damit die nicht umher floaten.
Mit der Anpassung bleibt der pin doch im undefinierten Zustand, wenn wir ohne cc1101 compilieren oder?

Wenn wir den Pin in einem anderen Modus brauchen, dann muss der doch später passend initialisert werden anstelle davon auszugehen, dass der Standardwert für uns passt.
Ebenso PIN 3 und 4, sind doch im Zweifelsfall undefiniert oder?

[HomeAutoUser]

enso PIN 3 und 4, sind doch im Zweifelsfall undefiniert oder?

Unter der Schleife werden diese gesetzt.

SIGNALDuino/src/signalduino.h

Lines 103 to 104 in 00089bb

	pinAsInput(PIN_RECEIVE);
	pinAsOutput(PIN_LED);

Mit der Anpassung bleibt der pin doch im undefinierten Zustand, wenn wir ohne cc1101 compilieren oder?

Dann musst du den cc1101 if deffen :-) denke ich mal.

Es gibt 2 Fälle.

1. ohne cc1101
2. mit cc1101 wo man bestimmte Pin´s nicht mit 'pinAsInputPullUp' versehen sollte.

ODER ...

der ganze Ablauf muss überarbeitet werden wenn ich das richtig einschätze und überblicke.

[devzero84]

Also generell heisst das ja, dass D4 bis D9 nun als Eingang mit Weak-Pullup geschaltet werden.
Das ist in Ordnung. D10 ist /CS (also Ausgang), D11 ist MOSI (also Ausgang), D12 ist MISO (also Eingang, kein Pullup notwendig) und D13 ist SCK, also auch Ausgang.

D10 bis D13 sollten wahrscheinlich von der Arduino Library im SPI Init richtig gesetzt werden.

EDIT: Ob die Lib das konfiguriert weiss ich zwar nicht, hab nicht reingeschaut, aber wenn nicht, koennte das tatsaechlich Probleme machen.

https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-7810-Automotive-Microcontrollers-ATmega328P_Datasheet.pdf - Seite 66

Wenn das eine Problem macht, koennte man das evtl. mit einem Oszi sehen (da der Pullup sichtbar das Signal veraendern sollte, wenn der CC1101 da nicht gegen arbeiten kann. Evtl. kommen daher die Wartezeiten, wo dann #212 reinhaut.

Wahrscheinlich ist das Problem mit beiden PRs geloest.

D2 ist GDO2, also Input, D3 ist GDO0, also Output, right?

Was ist mit D1? Der floatet?

Aber dennoch, ein floatender Eingang sollte funktional nichts machen, wenn er nicht abgefragt wird oder an einen Pin Change Interrupt gebunden ist - es kann aber EMV Probleme machen und ist daher zuvermeiden.

Dein Fix repariert aber das D10 bis D13 auf EIngang mit Pullup zu verbiegen. Ich hab im Datenblatt nicht gefunden, ob das was kaputt machen kann. Ich wuerde fast vermuten nein, sonst wuerde SPI auch gar nicht tun. Aber schaden tut das auf keinen Fall.

(Ich rede hier jetzt nur vom Nano und vergleichbar - auf 8 Bit ATmega Basis)

[HomeAutoUser]

Wir haben die unterschiedlichen Varianten uns angesehen und jeweils spezifiziert.

@sidey79 , was machen die Unittests welche fehl schlagen?

[elektron-bbs]

Hier ist uns noch ein Fehler unterlaufen, der Arduino Nano ohne CC1101 verwendet andere Pins.
So funktioniert es wieder:

    #else // Arduino Nano without CC1101
      for (uint8_t i=3;i<20;i++) { // not PIN_RECEIVE
        if ( i != PIN_SEND && i != PIN_LED ) {pinAsInputPullUp(i);}
      }
    #endif

[sidey79]

Die Sache mit Pin_Led leuchtet mir ein, da wir den nie initialisieren.

Pin_Send setzen wir später auf einen definierten Stand.
Ist dir bewusst, wieso das zum Problem führt?

[elektron-bbs]

Die Sache mit Pin_Led leuchtet mir ein, da wir den nie initialisieren.

Mhmm, pinAsOutput(PIN_LED); erfolgt gleich nach dem die Pullups der nicht angeschlossenen Pins eingeschaltet werden.

Pin_Send setzen wir später auf einen definierten Stand.
Ist dir bewusst, wieso das zum Problem führt?

Ich sehe in der output.h nicht so ganz durch, wie die Befehle umgesetzt werden.
Mir ist der Fehler aufgefallen, weil der SIGNALduino mit der alten Variante dauernd sendete.
Ich habe LEDs an PIN_SEND und PIN_RECEIVE. Die LED an PIN_SEND leuchtete dann dauernd.
pinAsOutput(PIN_SEND); scheint offensichtlich nicht mehr das zu tun, was es soll, wenn vorher der Pullup eingeschaltet wurde.

Ich habe auch nicht verstanden, warum ich {pinAsInputPullUp(i);} unbedingt in geschweifte Klammern setzen musste. Ohne funktionierte es nicht.

[devzero84]

Also ich habe mir das auch nochmal angesehen - mit dem Scope kann ich nicht sehen, dass die internen Pullups irgendeinen Einfluss auf das Signal haetten jedoch bringt es bei mir auch eine deutliche Verbesserung.

Ich habe ja als Fehlerursache die statische Varible in dem wait_Miso_rf() define in cc1101.h und entsprechend gefixt (den Pullrequest passe ich noch final an).

Um mal ein Gefuehl dafuer zu bekommen, habe ich den Code noch mit etwas mehr Debug versehen:

cc1101.c

void cc1101::sendFIFO(char *startpos, char *endpos) {
    cc1101_Select();                                // select CC1101
    sendSPI(CC1101_TXFIFO | CC1101_WRITE_BURST);    // send register address

    uint8_t val;
    for (char *i = startpos; i < endpos; i+=2) {
      val = hex2int(i[0]) * 16;
      val+= hex2int(i[1]);
      /* DBG_PRINTLN(val); // only to debug */
      sendSPI(val);    // send value
    }

    cc1101_Deselect();    //Wait for sending to finish (CC1101 will go to RX state automatically

    for(uint8_t i=0; i< 200;++i) {
      if( readReg(CC1101_MARCSTATE_REV00, CC1101_STATUS) != MarcStateTx)
        break;            //neither in RX nor TX, probably some error
      delay(1);
    }

    DBG_PRINTLN("waitMisoCnt");
    DBG_PRINTLN(waitMisoCnt);
    DBG_PRINTLN("waitMisoRfCnt");
    DBG_PRINTLN(waitMisoRfCnt);
}

In cc1101.h entsprechend das Miso wait gefixt (damit es stabil laeuft, aber mal waitMisoCnt und waitMisoRfCnt fuer Statistiken rein)

	#define wait_Miso()       { uint8_t miso_count = 255; while(isHigh(misoPin)) { waitMisoCnt++;   delay(1); if(miso_count == 0) return      ; miso_count--; } }    // wait until SPI MISO line goes low
	#define wait_Miso_rf()    { uint8_t miso_count = 255; while(isHigh(misoPin)) { waitMisoRfCnt++; delay(1); if(miso_count == 0) return false; miso_count--; } }    // wait until SPI MISO line goes low

Pullups abgeschaltet (also Dein Fix), 4 mal was gesendet:

send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
6
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
8
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
10
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
12
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;

Was man nun hier sieht: Bei jedem Senden wurde waitMisoRfCnt stetig um 2 erhoeht, d.h. es musste 2 mal auf MISO LOW gewartet werden, was vollkommen in Ordnung ist.

Jetzt machen wir das selbe Spiel wieder mit Pullups eingeschaltet:

send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
23
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
40
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
57
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;
send cmd detected 2
Adding xFSK message
rearrange beginptr
SN;locating data start:E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000; end:;
msg 1/1 part 0/0 repeats 1
waitMisoCnt
0
waitMisoRfCnt
74
SN;D=E3577F24FCBCEDA9F867EFFFFF1CA880DB034312560798100000;

Nun erhoeht es sich schon um 17 bei jedem send - also stoeren die Pullups vermutlich auf die Abfallzeit des Pegels von MISO.

Man sieht hier also deutlich, dass die Pullups abzuschalten richtig ist - dennoch: sind die eingeschaltet eigentlich nicht das Hauptproblem, dauert halt etwas laenger.

Ursaechlich fuer das Problem ist nach wie vor die statische Variable in den miso waits. Man muss bedenken, wenn 255 auf 0 ist, wird immer abgebrochen.
Also mit Pullups
255, 253, 251,...,203,201,0 kaputt

Ohne Pullups
255, 238, 221, ... 51, 34, 0 kaputt
Es dauert hier einfach nur laenger.

Initial wusste ich ja nicht, was los ist und schrieb im Titel, dass ich beobachtet habe, dass es bei mir nur mal 15 klappt etwa... 255/17 = na? genau, 15 :)

[elektron-bbs]

Hier müssen wahrscheinlich für ARDUINO_ATMEGA328P_MINICUL auch noch Ausnahmen rein.

    #define PIN_LED               4
    #define PIN_MARK433           A0

[sidey79]

Ich verstehe den Ansatz nicht, Pins in der Schleife auszunehmen.
Für mich sieht das wie eine Umgehung eines Problems aus.

Warum beim Radino überhaupt keine Pins auf einen definierten Stand gebracht werden verstehe ich nicht.

Was ich verstanden habe ist, dass die Pins die anderweitig definiert sind nicht besser nicht gesetzt werden, weil da offenbar etwas schief geht.

[HomeAutoUser]

Ich verstehe den Ansatz nicht, Pins in der Schleife auszunehmen.
Für mich sieht das wie eine Umgehung eines Problems aus.

Warum beim Radino überhaupt keine Pins auf einen definierten Stand gebracht werden verstehe ich nicht.

Was ich verstanden habe ist, dass die Pins die anderweitig definiert sind nicht besser nicht gesetzt werden, weil da offenbar etwas schief geht.

@sidey79
kannst du deine Bedenken genauer erläutern?

Wir nehmen keine Pins raus. Es werden alle Pins welche ungenutzt sind eindeutig gesetzt. Wenn wir alle setzen, so geht das schief wie @devzero84 schon erläuterte.

was willst du beim Radio anpassen? Bei dem uC sind andere Pins vom Standard genutzt (Vergleich zum Nano bsp.)

was denkst du geht da schief?

ich übernehme den Pr noch nicht um deine Fragen nicht zu umgehen ;)

[sidey79]

Ich sehe ja ein, dass hier etwas nicht funktioniert.
Warum auch immer.

1. für den ARDUINO_RADINOCC1101 wird meines Erachtens überhaupt kein Pin initialisiert.

2. dann fängt die Schleife je nach Variante bei Pin 3,4 oder 5 an.
   Die Startwerte sind hier fix und unabhängig von der Pin Nutzung.

Eine Schleife geht bis 20.
Hier habe ich mich gefragt, welche Pins 14-20 sind das? Gibt es die wirklich?

Den Ansatz, Pins anhand unserer Definitionen auszuschließen finde ich irgendwie wartbarer und leichter nachvollziehbar:

if ( i != PIN_SEND && i != PIN_LED ) {pinAsInputPullUp(i);}

[HomeAutoUser]

@sidey79 hier die Zusammenstellung mal verdeutlicht mit dem Hinweis, das beim Radino die PIN´s nur bedingt geändert werden können je nach Hardware. Deswegen haben wir die Schleifen fest nach den Pin´s aufgebaut.

Hier habe ich mich gefragt, welche Pins 14-20 sind das? Gibt es die wirklich?

Ja die gibt es.

[Hofyyy]

Wenn ihr hier durch seid und damit 2 issues geschlossen werden, kann ich den Firmwarestand dann gerne bei mir testen.

[sidey79]

@HomeAutoUser

Warum machen wir nicht eine schleife und dann ein continue je nach UC und gesetzter Variable?
Dann wir erst mal alles initalisiert außer es gibt Ausnahmen?

Ungefähr so. (Syntax ist ggf. nicht 100% korrekt)

for (uint8_t i=2 ; i<=23;i++) {        
  if (i==PIN_SEND) continue;
  #ifdef CMP_CC1101 
  if (i==MOSI || i==MISO || i==SCK) continue;
  #endif

  pinAsInputPullUp(i);      
} 

[HomeAutoUser]

@sidey79
du warst schneller mit der Nachfrage als ich die schon bereits überarbeitet Variante hochgeladen hatte :-)

[sidey79]

Hier würde ich break verwenden, sobald wir keine Pins mehr initialisieren wollen.
Sieht für mich insgesamt jetzt stimmig aus.

Leider wird es uns ein paar Byte an Flash kosten :(

[HomeAutoUser]

Wir werden uns nie in deinen Kopf denken :D Code folgt morgen wenn dir break besser gefällt.

Flash, da haben wir uns nicht einen Überblick verschafft. Die Funktion war hier wichtiger.

[sidey79]

Break bricht die Schleife ab.
Continue führt ja nur zum Nächsten Durchlauf.

Die Funktion ist hier wichtiger ja.

[devzero84]

Waere es im Grunde nicht besser und einfacher explizit alle Pins erstmal als Input mit Pullups zu setzen und dann spaeter, je nach Controller, die richtigen Pins auf Ausgang bzw. Eingang ohne Pullup (also MISO als Eingang, die anderen SPI Ports als Ausgang, usw.)

Letztlich sind ja gerade die Pins, die verwendet werden, bekannt ueber die jeweiligen Defines.
Dann kann man ueber die entsprechenden Namen einfach die Pins richtig setzen. Der Code sollte dann hier auch nur einmal exisitieren (wenn die Defines fuer alle Controller gleich heissen, wenn nicht, sollte man hier ansetzen).

Auch wenn klar ist, was hier in den Schleifen passiert - es ist irgendwie unleserlich und in 2 Jahren versteht das niemand mehr, ohne sich tief reinzudenken.

Daher lieber sowas:
for(uint8_t i=0; i <= MAXIONUM /* muss man je nach plattform natuerlich haben, koennte aber auch ein define werden */; ++i)
{
pinAsInputPullUp(i);
}

#ifdef CC1101
pinAsOutput(MOSI);
pinAsInput(MISO);
#else
pinAsOutput(IRGENDEINANDERERPIN)
pinAsInput(UNDDERHIERALSEINGANG)
#endif
...

(Habe gerade nicht nachgeschaut, wie die Funktionen genau heissen oder welche Pins es genau gibt - geht ums Prinzip)
Sollte - wenn die defines passen - dann fuer alle Controller passen. Weiter verfeinern ueber #ifdefs geht natuerlich.

Generell: Code wird oefters gelesen, als geschrieben.

[elektron-bbs]

Waere es im Grunde nicht besser und einfacher explizit alle Pins erstmal als Input mit Pullups zu setzen und dann spaeter, je nach Controller, die richtigen Pins auf Ausgang bzw. Eingang ohne Pullup (also MISO als Eingang, die anderen SPI Ports als Ausgang, usw.)

So war es ja ursprünglich im Prinzip, aber das führte zu Fehlfunktionen.

[HomeAutoUser]

@elektron-bbs bitte mit testen.
Die gedachte Variante lässt den raddino stolpern. Da haben wir noch einen Fehler drin (ohne die Anpassung break)

[HomeAutoUser]

Ich denke fast, so könnte ein Schuh draus werden.

  for (uint8_t i=2 ; i<=23;i++) {
    if (i==PIN_LED) continue;
    if (i==PIN_RECEIVE) continue;
    if (i==PIN_SEND) continue;

    #ifdef CMP_CC1101 
      if (i==MOSI || i==MISO || i==SCK || i==SS) continue;
    #endif

    #if defined(ARDUINO_RADINOCC1101) || defined(ARDUINO_ATMEGA328P_MINICUL)
      if (i==PIN_MARK433) continue;

      #ifdef ARDUINO_RADINOCC1101
        if (i==SS || i==17) continue;
      #endif
    #endif
    pinAsInputPullUp(i);
  } 

Beim radiono muss die 17 noch zusätzlich definiert werden sonst rutscht die noch mit durch. Das kommt aufgrund der komischen Pin´s. Werde mal weiter durchprobieren sämtliche HW.

EDIT:

• radino läuft
• nano läuft
• pro mini läuft

[elektron-bbs]

Was ich diese verdammte Arduino-Pin-Nummerierung hasse :-(

Pin 17 = PB0 = RXLED - OK
keine Nummer laut Radino-PDF = PD5 = TXLED - ?

[HomeAutoUser]

Was lange .... wird gut :-)

@sidey79 nun wurde die Schleife nach erneutem Documentationsvergleich und Ergänzung der Quellen für "finish" beziffert.
Auf den Nano´s + pro mini´s + radino läuft es so wie gewünscht.

Wenn dieser PR übernommen wurde, wäre es sehr Hilfreich ein Release zu erstellen und das fehlerhafte Release https://github.com/RFD-FHEM/SIGNALDuino/releases/tag/3.5.0-dev%2B20201221 zurück zu ziehen. (bitte auf das Releasedatum achten und bitte dann die Versionsnummer aus der PROGVERS-Variable nehmen ---> somit kommt man nicht durcheinader)

[elektron-bbs]

So kommen wir aber nicht bis einschließlich Nr. 23. Ich denke, break darf erst erfolgen, wenn > 23.

[sidey79]

Deine Annahme stimmt. Wenn für Nr. 23 noch der PullUp geschaltet werden soll, dann muss da >23 stehen

[HomeAutoUser]

Da unterlief mir ein C&P Fehler weil der Magen knurrte :-D
fertig und korrigiert