Biblioteka MQTT i dziwny kod w C

Podziel się większym kontekstem tego kodu.

Nie możesz po prostu poprawić tego kodu. Ja, implementujące mqtt, po prostu powyginalem jakaś niedorobioną bibliotekę i będzie git. Przynajmniej w zakresie w jakim tego potrzebowalem.

jp

W poprzedniej wiadomości był link do repozytorium na GitHubie. Tam jest całość.

On 08.08.2022 19:14, Atlantis wrote: [...]

Ewidentny błąd – pokazuje dlaczego należy kompilować z -Wall (oraz ewentualnie -pedantic) i nie ignorować ostrzeżeń. Aczkolwiek w przytoczonym kontekście nie ma znaczenia – zmienna llen ma zasięg lokalny ograniczony do wnętrza if-a i poza wywołaniem MQTTReadPacket() nigdzie nie jest tam później używana.

Nie jestem pewny co oznaczają „te kawałki kodu”, ale w ramach testu proponuję odnaleźć ten kontekst:

if(MQTTAvailable()) { BYTE llen; WORD len = MQTTReadPacket(&llen); WORD msgId = 0; BYTE *payload;

i zaraz po deklaracjach zmiennych dopisać llen = len.

No i proponuję też zamienić switch(rxBF[1]) { //MQTTBuffer na switch(MQTTBuffer[1]) { //MQTTBuffer

Przy próbie kompilacji z -Wall kompilator (xc32-gcc) wywala kilka błędów w bibliotece GCC, ale dotyczą one takich rzeczy jak niewykorzystane zmienne albo niejawne rzutowanie z char* na BYTE* w argumencie funkcji. Do tej konkretnej konstrukcji z pustą listą parametrów się akurat nie czepia.

Chodziło mi o inne miejsce - od linii 787. Tam zmienna llen jest przekazywana do funkcji MQTTReadPacket w taki sam sposób, a potem bierze udział w wyliczaniu indeksów do MQTTBuffer. Chociaż z drugiej strony wszystkie z tych operacji to sumowanie. Zmienna llen jest zmienną lokalną, a wiec jest domyślnie inicjalizowana wartością 0. Jeśli przekazanie jej przez wskaźnik do funkcji o pustej liście parametrów nie ma żadnego wpływu na jej wartość, to nie będzie też miało na późniejsze wyliczenia. Czyżby pozostałość po jakichś wcześniejszych wersjach kodu, gdzie faktycznie w parametrze był wskaźnik? A potem autor to przepisał, na wersję bezparametrową i zamiast dać void wyczyścił listę parametrów, nie poprawiając wcześniejszych wywołań?

To była pierwsza rzecz jaką sprawdziłem. Wychodzi na to, że:

1) MQTTReadPacket() wołane bezpośrednio przed tym switchem zwraca 2. 2) MQTTBuffer[01] w tym miejscu zwraca wartość 0x02. Zawsze, niezależnie od tego czy dane do logowania były prawidłowe, czy nie. Postanowiłem więc sprawdzić co mamy w MQTTBuffer[0] i sprawa się rozjaśniła - mamy tam 0x20. Razem te dwa bajty stanowią więc prawidłowy nagłówek wiadomości CONNACK. Dalej powinny iść jeszcze dwa bajty, z których ostatni stanowi reurn code informujący o stanie autoryzacji. Serwer istotnie wysyła całą wiadomość - sprawdziłem tcpdump i wiresharkiem. 3) Spróbowałem więc po prostu sprawdzać MQTTBuffer[3] ale niestety - nie znajduję tam return code. Wygląda to faktycznie tak, jakby MQTTReadPacket w tym miejscu odczytywało tylko dwa pierwsze bajty, co zgadzałoby się z wartością zwracaną przez funkcję.

Atlantis snipped-for-privacy@wp.pl napisał(a):

Odwrotnie. Nie jest inicjalizowana bo nie jest statyczna. Inicjalizacja domyślna zmiennych odbywa się raz, przed uruchomieniem main(). Zmienne automatyczne, alokowane na stosie, mają de facto przypadkowe wartości.

On 09.08.2022 08:36, Atlantis wrote: [...]

Tak, na to to właśnie dla mnie wygląda. Widać to zresztą po niektórych komentarzach w kodzie.

Jak dla mnie to ta funkcja to jakieś totalne, niedorobione gówno. Prowizorka znaczy się. Tutaj bym szukał błędu. Kody stanów w switch-u zakodowane na twardo, a nie za pomocą enumów/makr – jak mniemam chodzi o kody wymienione w enum-ie na początku. Stany zmieniane za pomocą m_state++ czy m_state=3 zamiast m_state=NAZWA_KOLEJNEGO_STANU – aż się prosi o kłopoty. Nie chce mi się analizować tego kodu, ale wydaje mi się, że nie wszystkie stany są obsługiwane.

Masz rację, moja pomyłka. W ramach eksperymentu usunąłem z kodu wszelkie przypadki zmiennych przekazywanych do MQTTReadPacket() przez wskaźnik na pustej liście parametrów. Takich sytuacji było dosłownie tylko kilka. Najbardziej istotna wydawała się sytuacja, gdzie taka zmienna llen była potem wykorzystywana do wyliczania indeksów tablicy MQTTBuffer. Usunąłem wszelkie odwołania do tej zmiennej, ustawiłem parametr MQTTReadPacket na void i skompilowałem program. Wszystko działa tak jak poprzednio - ani lepiej, ani gorzej. Wcześniej założyłem, że musi się tam dziać jakaś dziwna "ezoteryczna magia" wynikająca ze specyfiki języka i w jakiś niewidoczny w kodzie sposób odpowiednia wartość trafia jednak do zmiennej llen i wszystko działa jak powinno. Być może jednak po prostu w moim przypadki ten "if" nie jest nigdy wywoływany, więc program nie wchodził nigdy w tę gałąź i losowe wartości ze stosu trafiające do llen nie dawały o sobie znać.

Też się skłaniam ku tej hipotezie. Będę musiał przyjrzeć się temu bliżej, bo na razie wygląda na to, że ta funkcja nie jest kompatybilna z wiadomością CONNACK i po natrafieniu na nią nie czyta wszystkiego, a jedynie pobiera dwa pierwsze bajty. Autor nie zauważył tego, bo przez przypadek tak się złożyło, że w switch/case sprawdzał wartość komórki pamięci, która zawsze miała wartość 0x00, więc zawsze wyglądało to tak, jakby połączenie zostało autoryzowane. Najwyraźniej nie przetestował nigdy działania biblioteki z błędnym loginem/hasłem.

Generalnie i tak prościej będzie usunąć z tego błędy o trochę wyczyścić kod, niż pisać swoją własną bibliotekę albo portować inną pod stos TCP/IP MLA od Microchipa...

To prawdopodobnie nie będzie ostatnia rzecz, bo w Wiresharku widzę jeszcze jakieś retransmisje przy zamykaniu połączenia TCP po zakończonej transmisji, ale najpierw chciałem się uporać z problemem z autoryzacją,

Blad troche dziwaczny, bo w ogole nie widze definicji MQTTReadPacket z parametrem. Jak to zwykłe C, i parametr nie ma znaczenia, to nadal dziwaczne. Podejrzewam, ze cos tu przerabiali, i zrezygnowali z podawania adresu.

Ale nawet tam, gdzie tak piszą, to nie widzę aby uzywali tego llen, czyli błąd w zasadzie bez znaczenia. Dane będą w MQTTBuffer ?

A jak Atlantis zauwazyl - program bierze z rxBF, tylko juz tam nic nie zapisuje.

Wyglada na to, jakby kod byl w połowie wiekszych przeróbek.

ale o co im by mialo chodzic? Czemu mieliby uzywac llen - len niedobre?

J.

To jest właśnie jakaś zaszłość historyczna, o której sam wcześniej nie miałem pojęcia. W C++ funkcja z pustą listą parametrów równa się funkcji z parametrem void. Jednak w C działa to inaczej - pusta lista parametrów oznacza niesprecyzowaną liczbę parametrów. Do takiej funkcji możemy przekazać albo nic, albo cokolwiek i taki kod się skompiluje, jednak z wnętrza funkcji do tych parametrów i tak nie będziemy mieć dostępu. Liczyłem na to, że może jakaś ezoteryka języka pozwala na niejawne przekazanie czegoś w ten sposób jednak wychodzi na to, że ktoś po prostu zmieniał tę funkcję i po prostu usunął parametry zamiast przerobić funkcję na void.

Dane powinny trafić do MQTTBuffer, jednak z tego co widzę funkcja MQTTReadPacket() w jej obecnej formie nie radzi sobie z wiadomością CONNACK. Odczytuje tylko dwa pierwsze bajty (nagłówek) ale już nie dwa kolejne, z czego nas interesuje ostatni - result code.

Na to wygląda. Udało mi się skontaktować z autorem wcześniejszej wersji tej biblioteki na PIC32. To znaczy wcześniejszej w stosunku do tego, co sam znalazłem na GitHubie. A i on prawdopodobnie opierał się na jakimś wcześniejszym kodzie. Przesłał mi jakąś swoją starą wersję kodu - nietestowaną i jak się okazało niedziałającą przez kilka bugów (m.in. powodujących zatrzaskiwanie się maszyny stanów albo błędy podczas czytania z socketa). Co jednak istotne, w tej wersji kodu funkcja MQTTReadPacket przyjmowała jeden parametr - wskaźnik na BYTE. Wychodzi się na to, że pusta lista parametrów powstała, gdy ktoś później przepisywał ten kod.

Wychodzi na to, że trzeba będzie teraz przyjrzeć się tej funkcji i ustalić, czemu nie parsuje prawidłowo CONNACT.

Rozróżnienie na len i llen bierze się ze specyfiki MQTT. W podstawowych wiadomościach (np. kontrolnych) mamy niewielką wartość len. Jednak gdy trzeba np. załączyć większy payload, załącza się jeszcze dodatkową informację. Podejrzewam, że w takich sytuacjach obydwie wartości mogą być potrzebne do manipulowania do manipulowania buforem. Z drugiej strony obecna wersja MQTTReadPacket posiada lokalną, statyczną tablice o nazwie lengthLength, która może być właśnie do tego wykorzystywana.

Sprawa ciągle nie daje mi spokoju, wiec przyjrzałem się jeszcze raz trochę bliżej działaniu funkcji MQTTReadPacket(). Nadal nie wiem jak rozwiązać problem, ale sprawa przynajmniej się trochę wyjaśniła.

W uproszczeniu korzystanie z tej funkcji wygląda następująco:

case MQTT_CONNECT_ACK WORD len = MQTTReadPacket(); if (len >= 2) { //Tutaj wykonują się główne operacje //Tylko tutaj może zostać zmieniony stan maszyny stanów } break;

Funkcja może być wywołana w tym miejscu wielokrotnie i dalej przejdziemy dopiero wtedy, gdy zwróci ona dwa lub więcej (wartość ta oznacza liczbę bajtów zapisanych do bufora). Sama funkcja MQTTReadPacket również posiada swoją własną maszynę stanów, której wartość jest zapisywana w lokalnej zmiennej statycznej. Od stanu tej maszyny zależy co funkcja zwróci.

Dopisałem do kodu kilka printf-ów, żeby zobaczyć co się tam dzieje. Wynik wygląda następująco:

Tutaj wywolujemy MQTTReadPacket() w MQTT_CONNECT_ACK m_state=0 <- stan po wywołaniu funkcji w MQTT_CONNECT_ACK len=2 <- wartość zwrócona

Tutaj wywołania w innych częściach programu m_state=2 m_state=3 m_state=0

Tutaj wywolujemy MQTTReadPacket() w MQTT_CONNECT_ACK m_state=2 <- stan po wywołaniu funkcji w MQTT_CONNECT_ACK len=0 <- wartość zwrócona

Tutaj wywolujemy MQTTReadPacket() w MQTT_CONNECT_ACK m_state=3 <- stan po wywołaniu funkcji w MQTT_CONNECT_ACK len=0 <- wartość zwrócona

Tutaj wywolujemy MQTTReadPacket() w MQTT_CONNECT_ACK m_state=0 <- stan po wywołaniu funkcji w MQTT_CONNECT_ACK len=2 <- wartość zwrócona

Tutaj wywołania w innych częściach programu m_state=2 m_state=3 m_state=0

Jak widać maszyna stanów przy kolejnych wywołaniach funkcji MQTTReadPacket() posiada wartości 0, 2 albo 3. Jeśli funkcja zostanie wywołana gdy jej maszyna będzie w stanie 0, to zwrócona zostanie wartość 2 - spowoduje to, że będziemy mogli wejść do ifa i wykonać operacje przewidziane dla MQTT_CONNECT_ACK. Jeśli jednak w momencie wywołania funkcji jej maszyna stanów będzie ustawiona na 2 albo 3, zwrócone zostanie zero. If się nie wywoła, a wiec nie opuścimy MQTT_CONNECT_ACK i wrócimy do tego samego miejsca w kolejnym obiegu pętli.

Co więcej - wywołania MQTTReadPacket w innych miejscach programu również biorą udział w tym cyklu.

Kolejny wniosek jest taki, że "magiczne liczby" w switch/case wewnątrz MQTTReadPacket to tak naprawdę kolejne etapy pracy maszyny stanów tej funkcji.

Więc główny problem polega teraz na tym, żeby w tym miejscu nie było zwracane 2, ale 4. Do MQTTBuffer powinny trafiać cztery bajty, a ostatnio z nich będzie kodem wyniku, przesyłanym w pakiecie CONNACK. Konieczna będzie dokładniejsza analiza tego, co właściwie ta funkcja robi. :)

On 09.08.2022 19:13, J.F wrote: [...]

Błąd człowieka, na który pozwala język C, w szczególności stare standardy. Od C11 (aka ISO/IEC 9899:2011) takie deklaracje zostały uznane za przestarzałe (chociaż nadal dopuszczalne).

Tu masz to opisane razem z cytatami ze standardu/ów:

Jest to moze i jakis błąd, ale jest szerszy - C pozwala na wywołanie funkcji z dowolną listą parametrow, bo inaczej printf nie moglby dzialac.

Co prawda powinno to zostac zdefinioniowane z uzyciem "..." ... no to moze i jest jakis błąd kompilatora, ze nie zgłasza rożnic w prototypie, deklaracji i wywolaniach ...

J.

J.F <jfox snipped-for-privacy@poczta.onet.pl> napisał(a):

Ale właśnie...

...printf z tego nie korzysta. On ma taką deklarację: int printf ( const char * format, ... ); Czyli przyjmuje jeden lub więcej parametrów i jest to po ludzku określone. Do niczego nie jest mu potrzebny ten dziwny mechanizm z pustą listą argumentów.

Ok, posiedziałem nad tym jeszcze przez kilka godzin i udało mi się znaleźć przyczynę. W wielkim skrócie maszyna stanów w funkcji MQTTReadPacket była źle napisana. Jeden if nie wykonywał się wcale, a część kodu w dalszej części była pomijana zupełnie z uwagi "break" umieszczony w złym miejscu. Efekt był taki, że kod przechodził dalej już po odebraniu nagłówka pakietu i nie pobierał pozostałych dwóch bajtów.

Biblioteka w takiej formie działała tylko dlatego, że w wyniku innego błędu kod zawsze zakładał, że autoryzacja przeszła poprawnie. ;) Z uwagi na wadliwość tej funkcji rzecz jasna nie miały szans działać jakiekolwiek inne funkcje związane z odbieraniem danych od brokera. Niewykluczone zresztą, że będę musiał przeprowadzić dodatkowe testy i poprawić jeszcze kilka błędów, zanim uda mi się uruchomić sprawdzanie wiadomości PUBLISHACK albo subskrybowanie tematu i odbieranie wiadomości przychodzących od brokera.

Wireskark pokazuje także, że mam trochę retransmisji TCP na porcie MQTT, więc temu też jeszcze będę musiał się przyjrzeć.

Na chwilę obecną działa jednak najważniejsze - wrzucanie danych na serwer. Jeśli ktoś byłby zainteresowany, to po posprzątaniu mogę się tym podzielić. O ile ktoś jeszcze używa PIC32 z bibliotekami MLA i będzie potrzebował obsługi MQTT. :)

Teraz, to się używa ESP8266/ESP32 do tego a nie jakieś PICe od migania ledami ;)

Uu pic32 od migania ledami, mocne.

Atlantis snipped-for-privacy@wp.pl napisał(a):

Ja nie potrzebuję, ale może po prostu wrzuć na githuba, to ktoś spoza pme też skorzysta i nie będzie się musiał męczyć z tamtym kodem.