Pytania techniczne dotyczące kodu [bramka ZigBee]

[klew]

Set initial caption ustawi w cloud nazwę, jeśli nie było tam nic wcześniej ustawione.
Potem już zmiana nie działa

[vajera]

@klew - poniżej efekt moich wielotygodniowych walk z brakiem pamięci:

1. Problem stał się zauważalny po wprowadzeniu newGUI, ale co do zasady istniał już wcześniej, tylko wtedy było więcej RAMu do dyspozycji.
   ㅤ
2. Moje środowisko testowe wygląda następująco:
   • bramka Z2S (ESP32 C6),
   • minimum 7 urządzeń Zigbee dodanych do bramki,
   • 15 - 20 kanałów Supla (binarne, przekaźniki, liczniki elektryczne, HVAC).
   ㅤ
3. Pierwsze uruchomienie zwykle przebiega bezproblemowo - bramka loguje się do sieci WiFi, następnie do serwera Supla, uruchamia się stos Zigbeee, bramka wchodzi w tryb parowania.
   ㅤ
4. Problem pojawia się w bardzo konkretnej sytuacji - gdy z jakiegokolwiek powodu Supla straci połączenie z serwerem:
   • utrata połączenia z WiFi,
   • jakakolwiek zmiana w Cloud (np. zmiana nazwy, funkcji przekaźnika itp.), czyli coś co dzieje się dosyć często ,
   • awaria serwera Supla.
   ㅤ
5. W momencie nawiązania ponownego połączenia (sytuacja dotyczy szyfrowanego połączenia z serwerem) bardzo często pojawia się w logach błąd modułu ssl_client dotyczący braku pamięci, co skutkuje brakiem połączenia bramki z serwerem Supla.
   ㅤ
6. Wielu użytkowników może tego nigdy nie zauważyć z kilku powodów:
   • bramka ma zaledwie kilka kanałów Supla - wtedy problem zwykle nie występuje - a przynajmniej nie od razu, gdyż ma on tendencję do kumulowania się,
   • w przypadku braku połączenia z siecią po około 5 minutach dochodzi automatycznie do ponownego uruchomienia bramki - patrz pkt. 3 - tylko, że w tej sytuacji część danych nie trafi do Supla a użytkownik może odnieść wrażenie, że bramka się często zawiesza.
   ㅤ
7. W przypadku ESP32 C5, który ma nieco mniej pamięci RAM niż C6, bramka nie była praktycznie w stanie normalnie pracować. Oczywiście C5 ma pamięć PSRAM, ale nie udało mi się zmusić klienta SSL do korzystania z niej.
   ㅤ
8. Zacząłem już dopuszczać do świadomości fakt, że dla bardziej rozbudowanych bramek jedyną opcją będzie S3+C6, ale wciąż jeszcze próbowałem znaleźć jakiekolwiek rozwiązanie, równoległe optymalizując kod, gdzie się da.
   ㅤ
9. Najgorsze było to, że bramka ma pod dostatkiem pamięci, np. Free Heap: 63764 B | Minimal Free Heap: 1912 B | HeapSize: 361260 B | MaxAllocHeap: 19444 B, ale w momencie ponownego nawiązania połączenia SSL robi się wąskie gardło

Tyle opisu przypadku, a teraz pora na poszerzoną diagnostykę i opis leczenia

Przeglądając bezkresne zakamarki Internetu trafiłem kilka dni temu na taki oto wpis:
https://www.reddit.com/r/esp32/comments ... oide_and_a

Pozwolę sobie zacytować kluczowy fragment:

After beating my head against a heap memory leak for about a week, I learned something today -- WiFiClientSecure doesn't like being created and destroyed.

If you have an ESP32 program which makes recurring HTTP/HTTPS calls, don't make multiple WiFiClientSecure objects or destroy the client after each use -- save a pointer to one global object in setup() and just reuse it. Same applies to WiFiClient, though the heap space "lost" is much lower.

On an ESP32-WROOM board I was seeing a panic reboot every 8 hours, with the single global "client" object I've gone 10+ hours so far with no decrease in free heap!

Pomyślałem sobie, że nie zaszkodzi wypróbować ten pomysł, chociaż szczerze mówiąc miałem obawy, czy dam radę zmodyfikować kod SuplaDevice w tym zakresie i jednocześnie nic nie popsuć

Znalazłem plik \SuplaDevice\src\supla\arduino_esp_platform.cpp , a w nim taki fragment kodu:

Code: Select all

~ArduinoEspClient() {
    if (wifiClient) {
      wifiClient->stop();
      delete wifiClient;
    }
  }
...........

void stop() override {
    if (wifiClient) {
      wifiClient->stop();
      delete wifiClient;
      wifiClient = nullptr;
	log_i("Oops!...I Did Again!"); //to moje 
    }
  }
...................

if (sslEnabled) {
      clientSec = new WiFiClientSecure();
      wifiClient = clientSec;
.........

To oznacza, że w każdej sytuacji opisanej powyżej w pkt. 4 SuplaDevice dynamicznie usuwa i tworzy obiekt WiFiClientSecure

Zmodyfikowałem, więc swoją lokalną kopię tego pliku - póki co jest to oczywiście wersja robocza:

Code: Select all

static WiFiClientSecure myWiFiClientSecure;

..........................................................................

class ArduinoEspClient : public Client {
 public:
  ~ArduinoEspClient() {
    if (wifiClient) {
      //wifiClient->stop();
      //delete wifiClient;
    }
  }
......................................................................

void stop() override {
    if (wifiClient) {
      //wifiClient->stop();
      //delete wifiClient;
      //wifiClient = nullptr;
    }
  }
 ..................................................................
 
if (sslEnabled) {
      clientSec = &myWiFiClientSecure;//new WiFiClientSecure();
      wifiClient = clientSec;

Powiem szczerze, że nie zakładałem, że to w ogóle zadziała - jest to w sumie proteza a la MacGyver , ale ku mojemu zaskoczeniu kod się skompilował bramka ruszyła i...działa.

Testowa bramka pracuje już prawie dwie godziny, w tym czasie wielokrotnie rozłączałem router w telefonie - po jego ponownym włączeniu bramka nawiązywała połączenie SSL bez najmniejszego problemu, parametry wolnej pamięci RAM pozostają bez zmian.

@klew - pytanie do Ciebie - czy widzisz przestrzeń na pull request, czy raczej mam to zostawić jako lokalną modyfikację biblioteki SuplaDevice?

[klew]

PR jak najbardziej, ale sprawdź to najpierw na wszystkich boardach ESP32

Tam ogólnie już tego rodzaju problemy kiedyś były i to kasowanie i tworzenie było chyba dodane, aby zaradzić memory leakom, które następowały przy kolejnych połączeniach ssl.

Ogólnie trzeba by się przekopać przez historię na github i poszukać kiedy/gdzie/dlaczego to niszczenie i tworzenie zostało dodane.

Osobiście uważam, że jak się klienta niszczy to powinien zwolnić pamięć. Jeśli tego nie robi, to poprawić to u źródła, czyli poszukać przyczyny w boardach ESP i tam poprawić. Lub chociaż tam błąd zgłosić.
Chyba, że coś powinno być wykonane przed zniszczeniem obiektu, ale to znowu trzeba poszukać.

[klew]

Przejrzałem trochę kod i wrzuciłem jakąś propozycję zmiany, choć nie testowałem

Także proszę o testy. Fajnie by było gdyby zebrać jakiś feedback dla ESP8266 oraz kilku wariantów ESP32.

Ogólnie ten leak, o którym wcześniej pisałem, dotyczył ESP8266. Workaroundem na leakowanie przy reconnect było tam całkowite wyłączenie stosu Wi-Fi i włączenie go od nowa...

Aktualnie w kodzie wyrzuciłem kasowanie clienta przy metodzie stop() i nie tworzę nowego przy nawiązywaniu połączenia, gdy obiekt już istnieje.

Wydaje mi się, że powinno wszystko działać.Jedyna problematyczna sytuacja dotyczy scenariusza, gdy ktoś na tym samym kliencie spróbuje nawiązać raz szyfrowane i raz nieszyfrowane połączenie. Także takich rzeczy proszę nie robić ;P

[vajera]

@Klew - wracam do tematu Hvac i czujnika binarnego

Przy pierwszym dodaniu urządzenia wykonuje się następujący kod:

Code: Select all

void addZ2SDeviceHvac(ZigbeeGateway * gateway, zbg_device_params_t *device, uint8_t free_slot, uint8_t trv_thermometer_slot) {
  
  auto Supla_Z2S_HvacBase = new Supla::Control::HvacBaseEE();

  Supla_Z2S_HvacBase->setMainThermometerChannelNo(z2s_channels_table[trv_thermometer_slot].Supla_channel);
  Supla_Z2S_HvacBase->setBinarySensorChannelNo(Supla_Z2S_HvacBase->getChannel()->getChannelNumber());

Czyli zgodnie z Twoimi wytycznymi ustawiam kanał czujnika binarnego na identyczny z kanałem Hvac.

Przy każdym kolejnym uruchomieniu bramki wykonuje się funkcja initZ2SDeviceHvac, a w niej:

Code: Select all

auto Supla_Z2S_HvacBase = new Supla::Control::HvacBaseEE(Supla_Z2S_TRVInterface);

Code: Select all

Supla_Z2S_HvacBase->getChannel()->setChannelNumber(z2s_channels_table[channel_number_slot].Supla_channel);

Teraz moje pytanie - czy również w tej funkcji powinna pojawić się następująca linia kodu?

Code: Select all

Supla_Z2S_HvacBase->setBinarySensorChannelNo(z2s_channels_table[channel_number_slot].Supla_channel);

Jeżeli tak, to co w sytuacji, gdy w międzyczasie użytkownik zmienił kanał czujnika binarnego na inny - to się ustawi w onLoad...()?

[klew]

@Klew - wracam do tematu Hvac i czujnika binarnego
...
Jeżeli tak, to co w sytuacji, gdy w międzyczasie użytkownik zmienił kanał czujnika binarnego na inny - to się ustawi w onLoad...()?

Ogólnie wszystkie defaulty powinieneś ustawić za każdym razem na klasie. Także ten binary i termometr raczej powinny się w konstruktorze inicjalizować.
Jeśli zmieniasz numery kanałów (hvac, binary, etc) to to też trzeba ręcznie poprawić - przed suplową inicjalizacją elementów.

Nie wiem też, w którym momencie tworzysz te obiekty - powinny być utworzone przed "SuplaDevice.begin()", albo w trakcie jakiegoś "onLoadConfig" na obiekcie "konfiguratora urządzenia"

Jeśli dodajesz Elementy w runtime (np. przy parowaniu) i chcesz aby działały bez resetu bramki, to też trzeba ręcznie wywołać kilka operacji. Warto też dodać tam wymuszony zapis do "state storage", bo dodanie nowego elementu, zmienia strukturę storage i jeśli zrobisz reset bramki, to odczyt starego storage może się nie udać i stan kanałów przepadnie.

Przy bramkach koniecznie trzeba też używać "enableChannelNumbers()" na instancji storage. Bez tego musiałbyś zachować identyczną kolejność tworzenia Elementów za każdym razem.

[vajera]

@Klew - wracam do tematu Hvac i czujnika binarnego
...
Jeżeli tak, to co w sytuacji, gdy w międzyczasie użytkownik zmienił kanał czujnika binarnego na inny - to się ustawi w onLoad...()?

Ogólnie wszystkie defaulty powinieneś ustawić za każdym razem na klasie. Także ten binary i termometr raczej powinny się w konstruktorze inicjalizować.
Jeśli zmieniasz numery kanałów (hvac, binary, etc) to to też trzeba ręcznie poprawić - przed suplową inicjalizacją elementów.

To może tutaj jest pies pogrzebany.

Nie wiem też, w którym momencie tworzysz te obiekty - powinny być utworzone przed "SuplaDevice.begin()", albo w trakcie jakiegoś "onLoadConfig" na obiekcie "konfiguratora urządzenia"

Tworzone są prawilnie - przed begin();

Jeśli dodajesz Elementy w runtime (np. przy parowaniu) i chcesz aby działały bez resetu bramki, to też trzeba ręcznie wywołać kilka operacji. Warto też dodać tam wymuszony zapis do "state storage", bo dodanie nowego elementu, zmienia strukturę storage i jeśli zrobisz reset bramki, to odczyt starego storage może się nie udać i stan kanałów przepadnie.

Przy bramkach koniecznie trzeba też używać "enableChannelNumbers()" na instancji storage. Bez tego musiałbyś zachować identyczną kolejność tworzenia Elementów za każdym razem.

Może kiedyś podejdę do tematu dodawania urządzeń w locie, ale póki co ten mechanizm z resetem nie jest chyba zbyt uciążliwy. Chyba, że ktoś zacznie stawiać bramki na 30 termometrów

Dziękuję za uwagę o zapisie do state storage, natomiast o tej funkcji enableChannelNumbers() nigdy wcześniej nie słyszałem - od dawna jest w SD? Pytanie, czy jak ją teraz włączę, to stan kanałów w istniejących bramkach przepadnie?

[klew]

Dziękuję za uwagę o zapisie do state storage, natomiast o tej funkcji enableChannelNumbers() nigdy wcześniej nie słyszałem - od dawna jest w SD? Pytanie, czy jak ją teraz włączę, to stan kanałów w istniejących bramkach przepadnie?

Stan przepadnie. Pytanie czego stan trzymasz? Bo w zasadzie to te urządzenia ZigBee powinny same stan przechowywać.

[vajera]

Dziękuję za uwagę o zapisie do state storage, natomiast o tej funkcji enableChannelNumbers() nigdy wcześniej nie słyszałem - od dawna jest w SD? Pytanie, czy jak ją teraz włączę, to stan kanałów w istniejących bramkach przepadnie?

Stan przepadnie. Pytanie czego stan trzymasz? Bo w zasadzie to te urządzenia ZigBee powinny same stan przechowywać.

Przepadnie, ale jednorazowo, prawda?
Aktualnie to ten switch Enable gateway notifications oraz czujniki binarne, o ile użytkownik nie włączył mechanizmu timeout

[klew]

Przepadnie, ale jednorazowo, prawda?
Aktualnie to ten switch Enable gateway notifications oraz czujniki binarne, o ile użytkownik nie włączył mechanizmu timeout

Tak, jednorazowo.