Skocz do zawartości

Płatne zlecenia dla niepełnoletniego początkującego programisty C++

szerwi

Przez szerwi,
w Arduino i ESP

 Udostępnij

Pomocna odpowiedź

Elvis Lokalny Mentor

Elvis

Pomijając kwestię spacji i ogólnie formatowania, ta pętla pokazuje że nie rozumiesz jak działa for.

Pierwszy parametr powinien być instrukcją, a nie wyrażeniem. Napisałeś:

i>0

a to jest wyrażenie - zostanie obliczone i da wynik false, czyli 0, ale nic więcej.

W tym miejscu powinno być 

i=0

czyli instrukcja przypisania. W obecnej postaci pętla jest potencjalnie niepoprawna, bo tylko od kodu wcześniej zależy, czy i zostało zainicjalizowane poprawną wartością.

Niestety takie pisanie programów nie pomoże w znalezieniu dobrze płatnego zlecenia. Co gorsza, nawet na marnych studiach będzie ciężko taki program przepchnąć.

Proponuję więc zwinąć nieco własną ambicję i wrócić do nauki - zaczynając od angielskiego, a kończąc na C++.

szerwi Wynalazca

szerwi

  • Autor tematu
Pomijając kwestię spacji i ogólnie formatowania, ta pętla pokazuje że nie rozumiesz jak działa for.

Pierwszy parametr powinien być instrukcją, a nie wyrażeniem. Napisałeś:

i>0

a to jest wyrażenie - zostanie obliczone i da wynik false, czyli 0, ale nic więcej.

W tym miejscu powinno być 

i=0

czyli instrukcja przypisania. W obecnej postaci pętla jest potencjalnie niepoprawna, bo tylko od kodu wcześniej zależy, czy i zostało zainicjalizowane poprawną wartością.

Niestety takie pisanie programów nie pomoże w znalezieniu dobrze płatnego zlecenia. Co gorsza, nawet na marnych studiach będzie ciężko taki program przepchnąć.

Proponuję więc zwinąć nieco własną ambicję i wrócić do nauki - zaczynając od angielskiego, a kończąc na C++.

Aa, ok, dzięki 😉 Co do kwestii formatowania to to już zależy chyba od programisty czy chce dać spację czy nie i czy dla niego taki kod jest czytelny. Rozumiem, jeżeli jest praca w grupie to jakieś zasady co do formatowania trzeba mieć.

Elvis Lokalny Mentor

Elvis

To trochę jak z pisaniem wypracowań - można pisać brzydko i nieczytelnie, tłumacząc że to tylko dla nas. Jednak jeśli chcesz pisać programy lub wypracowania dla innych, forma ma znaczenie. Powstały całe książki i wiele poradników jak pisać, a jak nie. Skoro wrzuciłeś swoje dzieła na githuba i podałeś linka, rozumiem że chciałeś się nimi pochwalić. Oczywiście możesz je formatować jak lubisz, jednak polecam zaintersować się tym tematem - inaczej będą odbierane Twoje programy, jeśli będą ładnie wyglądały. Możesz poszukać reguł obowiązujących przy tworzeniu otwartego oprogramowania. To dobry początek.

StaryElektronik Odkrywca

StaryElektronik

Na oferia.pl mozesz sie zgłosic ze piszesz takie programy moze akurat ktos Cie wybierze. Druga sprawa jest taka czy dasz rade podolac wyzwaniu bo nie zawsze pisze sie podstawowe kody. Jak dojdzie Ci pisanie na komunikacke bezprzewodowa i akura nie bedziesz mial taki elementow to mozliwe ze Ci sie nie uda. Ale sprobowzc mozesz powodzenia 🙂

Wojcik98 Eksplorator

Wojcik98

szerwi, niekoniecznie. Chumanista prosił na razie o poprawki, a nie nowe funkcjonalności, a w tym zawiera się poprawienie aktualnego kodu, wykrycie błędów itd. Osobiście trochę mnie razi zakomentowany fragment "NEW" przy if(enforceSlope). System kontroli wersji jest też po to, żeby nie zakomentowywać niedziałającego kodu, tylko trzymać go w commitach i gałęziach. Podczas testowania na swoim komputerze sam tak robię, ale wysłałbym commita albo jeszcze bez tego, albo na inną gałąź, albo z opcją, żeby przy kompilacji definiowało się odpowiedni parametr i odpowiednia wersja by się kompilowała, coś takiego:

#ifdef NEW_ENFORCE_SLOPE
...
#else
...
#endif

Można by jeszcze poprawić trochę nazewnictwo, żeby zmienne więcej mówiły o samych sobie.

Poza tym są jeszcze TODO, które bezpośrednio mówią, co jest do zrobienia.

  • Lubię! 1
Chumanista Kreator

Chumanista

Wojcik98, ten kawałek kodu to było "lepsze rozwiązanie" które nie działało (coś gdzieś w tej drabince jest nie tak raczej), poza tym wymagałoby jeszcze jednej zmiany gdzie indziej i w interfejsie, późno się robiło, wróciłem do tego co działa i zrobiłem commita.

  • Lubię! 1
  • Popularny post
jbanaszczyk Wynalazca

jbanaszczyk

  • Popularny post
szerwi, a co, zaszalejmy.

Tu masz repo: https://github.com/SteelLiras/OvenController

Zaproponuj jakie poprawki możesz wprowadzić i w jakiej cenie.

Masz za friko 🙂

Popatrzyłem na struct Thermistor. W kilku miejscach mam uwagi.

Metoda forceInit:

czytasz siedem razy po kolei analogRead(pin), z bardzo małym opóźnienieniem (tyle, co obrót pętli). Szansa, że cokolwiek się zmieni w odczycie przetworników jest nikła. Jakiś malutki delay między odczytami bym dał. Bez delaya - wyliczenie średniej z siedmiu takich samych liczb i potem populacja wyniku na cały bufor ma mały sens.

Metoda forceInit działa tak,
że odczytuje dane z przetwornika po kolei kilka razy, wylicza ich średnią, po czym populuje tę średnią po całej tablicy. Populowanie floata po tablicy intów - szkoda czasu, zresztą te dane były całkiem niezłe (nikt ich nie odrzucił), bym ostatnią pętlę wywalił.

Metoda update działa tak:

Jeśli jeden odczyt z przetwornika różni się o co najmniej 10 od poprzedniego, to go odrzucasz. I jeśli odrzucisz ich 10 po kolei, to restartujesz odczyty - wypełniasz buffer kolejnymi siedmioma odczytami, które prawie na pewno są bliskie temu, co przed chwilą uznałeś za błędne. Ja bym zmienił. Odrzuciłbym jeden odczyt. No może dwa. Kolejny bym wpisał do bufora i bufora nie reinicjował. Jeśli ten wpisany był jednak błędny, to kolejny (albo kolejne dwa) znowu odpadną (mimo, że poprawne) i następny zostanie dopisany do bufora i z nim będzie porównywany kolejny odczyt.

Po pierwsze - wyliczenie temperatury bazuje na średniej z siedmiu elementów, to ten jeden odstający nie będzie mieć wielkiego wpływu, po drugie (wydaje mi się, że) wynik wyliczenia temperatury na podstawie średniej z bufora wpada do regulatora PID, który powinien sobie z tym dać radę bez nadmiernych konsekwencji.

Metoda update:

używa magicznych liczb 10, z których każda ma inne znaczenie, a fu, const'y od tego są.

Metoda update działa tak,
że przesuwa bufor w kierunku początku i dopisuje nowy wynik na koniec. Aż się prosi bufor cykliczny:

static auto head = ++head % BUFSIZE; // BUFSIZE jest stałe, kompilator powinien sobie to dobrze zoptymalizować
buffer[ head ] = analogRead(pin);

Nawet nie trzeba trzymać początku i końca bufora, bo jest stała liczba elementów. Koszt implementacji nieduży, a nie trzeba bufora w kółko stale kopiować. Ostatnio odczytany element (do walidacji nowych odczytów) też łatwo wskazać: 

auto last = buffer[ head ];

Implementacja bufora cyklicznego nawet nie wpływa na sposób liczenia średniej. No chyba, że: a nie wolisz jakiejś średniej ważonej?

struct Thermistor: 

#define BUFFERSIZE 7

No po jest C++, żeby móc napisać wewnątrz klasy/struktury

static const uint8_t BUFFERSIZE = 7;

I nie definiuje się globalnego symbolu, tylko znacznie słabszy w skutkach lokalny dla klasy identyfikator, i kompilator ma szansę na optymalizację kodu, i kompilator ma szansę na kontrolę typów.

i wtedy można napisać pętle jakoś tak (po co używać signed int, kiedy pętla ma być po unsigned byte):

for (uint8_t i = 0; i < BUFFERSIZE; i++) { ... }

Można jeszcze tak:

for (auto i = decltype(BUFFERSIZE){0}; i < BUFFERSIZE; ++i) { ... }

ale to nie dla wszystkich jest czytelne.

A nie wolisz ten BUFFERSIZE dać 8? By się lepiej w dzieleniu układało.

Metoda calculate:

Nie ma żadnego powodu, żeby sum było volatile.

Nie ma żadnego powodu, żeby sum było unsigned - sumujesz kilka sztuk signed, fajnie, że do longa, ale signed long jest bardziej na miejscu.

W C++ odchodzi się od castów (int), (float), na rzecz static_cast, static_cast. avr-gcc rozumie static_cast.

Temperaturę wyznaczasz lookupem po tablicy 1024 sztuk. Skoro przetwornik może dać jakieś dziwne wartości, to kolejne siedem sztuk dziwnych wartości też jest możliwe, to wypada dać ograniczenie:

auto idx = static_cast<int>(average + 0.5);
idx = constrain(idx, 0, 1023); // 1023 też jakiś static const, a nie magiczna liczba
temp = pgm_read_float_near(lookupTemp + idx);    // po co tam tyle nawiasów w kodzie?

Metoda calculate:

Do wyznaczenia indeksu użycie floatów jest zbędne. Pola average używasz:

w metodzie calculate - o tym zaraz

w metodzie forceInit - po to żeby wstawić average (float) do bufora (tablica intów). Równie dobrze można by użyć int, ale (przedtem wspomniane) - w ogóle tu jest zbędne.

No to pole averge ląduje w metodzie calculate (i wypada ze struktury).

A tak naprawdę arytmetyka na floatach tu jest zbędna i average w ogóle znika:

auto idx = (sum + BUFFERSIZE / 2) / BUFFERSIZE;
idx = constrain(idx, 0, 1023); // 1023 też jakiś static const, a nie liczba magiczna

----------------------------------------------------------

Disclaimer:

mam nadzieję, że długim code review nie naruszyłem dobrego samopoczucia. Ja takimi przeglądami zarabiam na życie, jakby ktoś chciał ...

Kod ogólnie jest fajnie napisany, nie ma w nim ani jednego rażącego błędu, podoba się.

Kod ogólnie jest napisany bardzo czytelnie - na podstawie krótkiej lektury byłem w stanie zrekonstruować jego działanie. No, żeby tak wszyscy pisali, toby świat lepiej działał.

Czytać dalej?

----------------------------------------------------------

A tak w ogóle - dzień dobry wszystkim.

To mój pierwszy post, choć czytam forbota od dawna.

Wróciłem do przedmiotu fascynacji z dawnych lat - mikrokontrolerów.

Witam wszystkich 🙂

  • Lubię! 3
Chumanista Kreator

Chumanista

jbanaszczyk, WOW, dziękuję bardzo!

Thermistor zostanie poprawiony, do tego i tak wymieniam je na termopary xD

Reset przy odrzuconych jest pozostałością po tym jak działało wyłączanie, ogólnie tu jeszcze będę debugował.

Co do bufora, #define, (int) i tak dalej to jest to podyktowane dość niepełną znajomością C++ szczerze mówiąc, poczytam doszkolę się, poprawię.

Floaty są bo

//TODO: Finalize float support for temperature points.

Ogólnie chcę zrobić docelowo wszystkie temperatury na floatach ale brakło ostatnio czasu.

W weekend będziemy kończyć z kumplem ten projekt, plan jest taki żeby do poniedziałku całkiem działało, także teraz nie ma jeszcze sensu czytać dalej bo się zmieni 🙂

  • Lubię! 1

Bądź aktywny - zaloguj się lub utwórz konto!

Tylko zarejestrowani użytkownicy mogą komentować zawartość tej strony

Utwórz konto w ~20 sekund!

Zarejestruj nowe konto, to proste!

Zarejestruj się »

Zaloguj się

Posiadasz własne konto? Użyj go!

Zaloguj się »
 Udostępnij
Przejdź do listy tematów
×
×
  • Utwórz nowe...