logo elektroda
logo elektroda
X
logo elektroda
Adblock/uBlockOrigin/AdGuard mogą powodować znikanie niektórych postów z powodu nowej reguły.

[AVR][ATMEGA32][C] - Prośba o krytykę pierwszego programu AVR.

Radioamator Staszek 30 Lis 2012 22:14 2787 12
  • #1 11595116
    Radioamator Staszek
    Poziom 11  
    Witaj,
    Chciałbym prosić o przejrzenie poniższego kodu pseudo sumatora opartego na 4 przyciskach i wyświetlaczach LED. Jest to jeden z moich pierwszych programów napisanych na uP. Bardzo zależy mi na wychwyceniu błędów które mogą zaszkodzić mi w dalszej nauce, wskazania co można zrobić lepiej, czego nie używać itp. Piszą program korzystałem z kursu zamieszczonego w Elektronice Praktycznej w 2005 roku i swojej podstawowej wiedzy na temat języka C/C++. Procesor Atmega 32,16MHz Wyprowadzenia segmentów PORTC, Multipleksowanie PORTA, Przyciski PORTD.

    Program:
    Kod: C / C++
    Zaloguj się, aby zobaczyć kod
  • #2 11595596
    piotrva
    VIP Zasłużony dla elektroda
    A więc:
    1.dramat (tu i wszystkie tego typu kwiatuszki), używaj zamiast tego przesunięć bitowych i operacji logicznych...
    2. Cała funkcja wyświetlaj - kolejny dramat... raz notacja segmentów, dwa takie rzeczy robi się za pomocą tablicy z kodami znaków...
    3. funckja licz2tab - miliony niepotrzebnych obliczeń - takie rzeczy robimy w pętli, która dzieli i zapisuje modulo
    4. sprawdzanie przycisków - matko, a co jak ktoś 2 wciśnie? program wtedy zgłupieje... Tu też OPERACJE BITOWE i MASKOWANIE BITÓW
    5. jeśli już stosujemy zalecane ISR to niech chociaż nazwa przerwania też będzie z nowego zestawu nazw
    6. Ogólnie styl - masakra. Gdzie komentarze? A wcięcia to jak po eksplozji - daję konia z rzędem temu kto się połapie w 5 minut w czasie rzeczywistym....
    Podsumowanie: cud że to coś w ogóle działa, gdybym oceniał dałbym 3 (na zachętę)
  • #3 11595626
    dondu
    Moderator na urlopie...
    Oj, Piotrze widzę, że skoro autor prosił o krytykę, a nie uwagi, to przyłożyłeś się do roboty :D :D :D

    @radioamator Staszek
    Jeżeli ten program jest w całości Twój to widać że się starasz. Uwagi piotrva są istotne dla całości ja dodam tylko źródło wektorów przerwań: http://www.nongnu.org/avr-libc/user-manual/group__avr__interrupts.html
  • #4 11595988
    Radioamator Staszek
    Poziom 11  
    Zdrowa krytyka :)
    Dziękuje za wszystko :)

    Jeśli mogę przeprosić za wcięcia, to stało się tak po przekopiowaniu z eclipse.

    Muszę się przyzwyczaić do zapisów z przesuwaniem 1-jedynek. Nie wiem dlaczego, ale prościej jest mi czytać zapisy szesnastkowe.

    Możecie pokazać jak taka funkcja z wyborem znaku u mnie "wyświetlaj" powinna wyglądać i dlaczego u mnie jest to nie do przyjęcia?

    Z obsługą przycisków masz absolutną rację, aż się prosi poprawić.
  • #5 11596149
    piotrva
    VIP Zasłużony dla elektroda
    Dane zamiast do switcha pakujesz do tablicy np. uint8_t cyfry[]={0x81,0xF3,.....};
    i potem wywołujesz: PORTC=cyfry[liczba_do_wyswietlenia];
    A co do zapisów szesnastkowych, to na prawdę się łatwiej Ci czyta?
    To jakie bity ustawiam(ustalić w czasie <= 2s):
    TCCR0=0x67;
    Za pomocą przesunięć możesz też segmenty zapisywać, np. (1<<SEG_A)
  • #6 11596485
    Radioamator Staszek
    Poziom 11  
    z timerem akurat masz racje, bo wtedy ustawiałem za pomocą tabelki w serwisówce, ale jak pisałem funkcję wyswietlaj, to w głowie miałem jak cyfra ma wygladać i odpowiednio wstawiałem sobie zera i przed oczyma widziałem liczbę szesnastkową. A gdybym miał to rozpisywać za pomocą !(1<<xx) to raz że zajęłoby mi to dużo czasu, a dwa że musiałbym zrobić #define <segment> <liczba> (A,B,C itc.) co jest zupełnie nie potrzebne i moim zdaniem ściemnia tylko czytelność kodu.

    w tym wypadku nie przekonują mnie przesunięcia bitowe.
    Czy uważasz że tworzenie tablicy, zamiast switch'a jest czytelniejsze? Które z rozwiązań zajmuje mniej pamięci?
  • #7 11596523
    Piotr Piechota
    Poziom 22  
    Wersja z tablicą będzie szybsza, nieco mniejsza i czytelniejsza. Dla oszczędności RAM'u tablice warto zapisać we FLASH'u.
  • Pomocny post
    #8 11596664
    piotrva
    VIP Zasłużony dla elektroda
    No moim zdaniem np. zapisanie
    
    uint8_t cyfry[]={
    (1<<SA)|(1<<SB)|(1<<SC)|(1<<SD)|(1<<SE)|(1<<SF),
    ....
    };
    

    jest sto razy czytelniejsze od Twojego 0x81. Tu od razu widzę że zapalam segmenty ABCDEF.
    Prosiłeś o krytykę i rady - więc je daję, napisałem już niejeden kod i wiem, że nawet napisanie raz a porządnie definicji na początku programu zajmuje może z 5 minut, a potem przy większych projektach skraca czas pracy o parę godzin.
    Bo co powiedzmy, jak będziesz przenosił projekt na PCB i nie będziesz łączył pinów po kolei, bo łatwiej ścieżki będzie poprowadzić powiedzmy PC0 - dp, PC1 - B, PC2 - G, ....
    BUM, znowu przeliczanie, "widzenie" lub nie bitów...
    A jak masz definicje - zmiana 7-dmiu liczb w parę sekund.
    ---
    Zrobisz jak będziesz chciał, ale mówię Ci jakie są dobre nawyki - gwarantuję że nabierając takowe dobre nawyki źle na tym nie wyjdziesz.
  • #9 11596836
    Radioamator Staszek
    Poziom 11  
    Wezmę twoje porady na poważnie. Chce teraz napisać program wykorzystując przetwornik A/C. Postaram się od początku pisać tak jak wskazałeś. Szkoda że w twoich kursach nie ma C, bo domniemam że zestaw EvB 4.3 z którego korzystam jest twojego autorstwa.
    Dzięki serdeczne za porady i wskazówki.
    Uważam że temat można zamknąć.
  • #10 11597022
    piotrva
    VIP Zasłużony dla elektroda
    EvB4.3 nie jest moim dziełem, aczkolwiek współpracuję z firmą And-Tech od pewnego czasu i będę miał pewien wpływ na wygląd i funkcjonalność nowej wersji tego zestawu.
    A co do kursów, to pomysł ten wisi nad nami (nad właścicielem f-my And-Tech i mną) od pewnego czasu, ale ciągle nie mogę znaleźć wystarczająco dużo czasu, żeby na spokojnie to wszystko spisać (bo tu na potrzeby kursu musiałbym napisać parę bibliotek od podstaw, a nie korzystać z gotowych i sprawdzonych rozwiązań). Ale kto wie - być może w te wakacje coś powstanie ;-)
  • #11 11603222
    Konto nie istnieje
    Konto nie istnieje  
  • #12 11606573
    miszcz310
    Poziom 24  
    @U.P.
    EEeee. Ale to teraz te SA, SB itd. to nie są te same co te w nawiasach (w cytowaniu). Bo jak są te same to wyjdzie lipa straszna...
  • #13 11607537
    piotrva
    VIP Zasłużony dla elektroda
    Nie te same. W moim wypadku do SA, SB wpisujemy numer pinu np:
    
    #define SA PC1
    #define SB PC4
    //...
    

    a w przykładzie kolegi od razu całe przesunięcie, czyli np.:
    
    #define SA (1<<PC1)
    #define SB (1<<PC4)
    //...
    
REKLAMA