Projekcik w C++ do oceny.

Wątek przeniesiony 2021-02-05 09:56 z C/C++ przez cerrato.

0

Od pewnego czasu próbuję swoich sił w C++ choć może nietypowo bo jest to C++ na AVR ale akurat to jest miejsce, które motywuje mnie do działań.
Oczywiście AVR ma swoje poważne ograniczenia co rzutuje na sposób pisania aplikacji ale załóżmy, że nie biorąc tych ograniczeń bardzo mocno pod uwagę chciałem napisać projekt w miarę poprawnie. Środowisko, w którym to pisałem to Microchip Studio.
Projekt napisany jest obiektowo na "obiektach statycznych" / "singletonach" czy jak to tam inaczej się nazywa w C++. I tu właśnie czuję, że to się robi inaczej.
Obawy mam także o to czy poprawnie wykorzystuję pola typu const i definiowane wartości. No i operacje bitowe ze strzałeczkami, do których przyzwyczaić się nie potrafię.

Chciałbym poprosić C++`owych wyjadaczy o wskazanie najpoważniejszych "błędów" w konstrukcji kodu / aplikacji.

Kod źródłowy projektu jest dostępny pod adresem: https://bitbucket.org/xksi/priv_amp_drv_2021.02/src/master/
Dokładny opis "projekciku" zamieściłem: https://www.elektroda.pl/rtvforum/topic3774571.html

1

@Descendant: ty jestes dobry z C++. zerkniesz?

3

@katakrowa:
Tak na szybko:

  • Zaprzyjaźnij się z CMake do opisywania projektów i wrzucaj pliki CMake'a do repo, nie .sln.
  • Definicje klas z deklaracjami metod trzymaj w pliku .h/.hpp, definicje metod w .cpp (a nie jak w classMenu.cpp)
  • Nie musisz korzystać z this żeby odwołać się do własnych składników klasy
  • nie musisz używać pustego return żeby wyjść z funkcji zwracającej void jeśli dojdziesz do jej końca (chyba że chcesz wyjść przedwcześnie).
0

@GutekSan: dzięki.

nie musisz używać pustego return żeby wyjść z funkcji zwracającej void jeśli dojdziesz do jej końca (chyba że chcesz wyjść przedwcześnie).
To mi jeszcze pozostał strach z baaardzo starego kompilatora "Keil", który jak na końcu funkcji nie miał return to leciał w kodzie jak w assemblerze ... czyli do funkcji która była następna :-)

Nie musisz korzystać z this żeby odwołać się do własnych składników klasy
To już zauważyłem ale jestem formalistą :-)

No te definicje klas wiele tłumaczą... Pewnie jak będę je miał osobno to kolejność ich deklaracji nie będzie mieć znaczenia?

3
  • nazwy klas w konwencji smurfów
  • brak testów
1

@MarekR22: brak testów
Łatwo się pisze a w sumie ciekaw jestem w jaki sposób podszedł byś do pisania testów funkcji, których zadaniem jest dekodowanie kodu RC5 czy programowania potencjometru głośności. A to w sumie "ideowo" 90% tego programu.
Bez symulowania urządzenia albo zbudowania warstwy abstrakcji dla sprzętu słabo to widzę. Pisanie symulatora urządzenia raczej jest całkowicie bez sensu. 100 razy lepiej poświęcić czas na testy rzeczywistych urządzeń. Inna sprawa to to, że program i tak już zajmuje 85% dostępnej pamięci więc dodanie jakiejkolwiek kolejnej abstrakcji raczej by spowodowało, że nie zmieści się do uC.

Dekodowanie RC5

/*
  Biblioteka dekodująca kod RC-5 bez uzycia przerwań i timera.

  UWAGA - funkcja receiveRC5 jest kodem "blokującym" na czas dekodowania/odczytu sygnału z pilota
  więc w przypadku poolingu należy brać to pod uwagę.
  MTSz 2018
  Testowane jedynie przy FCPU = 8 000 000 Hz

*/

#define REMOTE_RC5_PORT         PIND
#define REMOTE_RC5_PIN_VALUE    bit_5_value

class classRemoteRC5{

  public:
    static const uint16_t SHORT_IMPULSE_TIME_MIN =    444 ;
    static const uint16_t SHORT_IMPULSE_TIME_MAX =   1333 ;
    static const uint16_t LONG_IMPULSE_TIME_MIN  =   1334 ;
    static const uint16_t LONG_IMPULSE_TIME_MAX  =   2222 ;
    static const uint16_t IMPULSE_TIMEOUT        =   2223 ;
    static const uint16_t IMPULSE_TIME_SHORT     =      1 ;
    static const uint16_t IMPULSE_TIME_LONG      =      2 ;
    static const uint16_t IMPULSE_TIME_ERROR     =   0xff ;
    static const uint16_t DECODE_ERROR           = 0xffff ;

    volatile uint16_t value ;
    volatile uint8_t  command ;
    volatile uint8_t  address ;
    volatile uint8_t  toggle ;

    bool isFrameBegin(){
      if ( ( REMOTE_RC5_PORT & REMOTE_RC5_PIN_VALUE ) == 0 ) return true ;
      return false ;
    }

    uint8_t getChangeTime() {
      uint16_t time = 0 ;
      uint8_t cuurPinValue = REMOTE_RC5_PORT & REMOTE_RC5_PIN_VALUE ;
      while ( ( REMOTE_RC5_PORT & REMOTE_RC5_PIN_VALUE ) == cuurPinValue ){
        _delay_us(55);
        time = time + 50 ;
        if ( time >= IMPULSE_TIMEOUT ) return IMPULSE_TIME_ERROR ; // timeout
      }
      if ( ( time > SHORT_IMPULSE_TIME_MIN ) && ( time < SHORT_IMPULSE_TIME_MAX ) ) return IMPULSE_TIME_SHORT ;
      if ( ( time > LONG_IMPULSE_TIME_MIN ) && ( time < LONG_IMPULSE_TIME_MAX ) ) return IMPULSE_TIME_LONG ;
      return IMPULSE_TIME_ERROR;
    }

    uint16_t readFrame() {
      //
      // getValue uruchamiamy po wykryciu pierwszego opadającego zbocza z TSOP.
      // musi być wywołana tak aby załapać się na SHORT_IMPULSE_TIME_MIN
      //
      uint8_t impulseType = 0 ;
      //////////////////////////////////////////////////////////////////////////////////////////////////////
      // teraz w zależności od długości przychodzących impulów będę
      // dekodował kolejne 12 bitów danych
      //
      uint8_t i = 0 ;
      uint8_t lastState = 0 ;
      uint8_t nextBit = 0 ;
      value = 0 ;
      for ( i=0; i<13; i++ ){
        lastState = ( REMOTE_RC5_PORT & REMOTE_RC5_PIN_VALUE ) ;
        impulseType = getChangeTime();
        if ( impulseType == IMPULSE_TIME_ERROR ) return DECODE_ERROR ;
        if ( lastState == 0 ){
          if ( impulseType == IMPULSE_TIME_SHORT ) nextBit = 1 ;
          if ( impulseType == IMPULSE_TIME_LONG )  nextBit = 0 ;
        }else{
          if ( impulseType == IMPULSE_TIME_SHORT ) nextBit = 0 ;
          if ( impulseType == IMPULSE_TIME_LONG )  nextBit = 1 ;
        }
        // ponieważ był impuls krótki i jestem na początku bitu to musze przesunąc się do połowy bitu
        if (( impulseType == IMPULSE_TIME_SHORT )&&(i<12)){
          impulseType = getChangeTime();
          // ponieważ byłem na początku bitu to spodziewam się impulsu krótkiego.
          if (( impulseType == IMPULSE_TIME_ERROR )||( impulseType == IMPULSE_TIME_LONG )) return DECODE_ERROR ;
        }
        value = value * 2 ;
        value = value + nextBit ;
      }
      _delay_us( LONG_IMPULSE_TIME_MAX );

      //
      // wynik odczytu zapisywany jest tekże w zmiennych globalnych:
      //
      command = ( value & 0b0000000000111111 ) ;
      address = ( value & 0b0000011111000000 ) / 64 ;
      toggle  = ( value & 0b0000100000000000 ) / 2048 ;
      return value;
    }
};

Programowanie potencjometru:

class classDigitalPotFM62429{

  public:

    static const int8_t MAX_ATTENTUATION = 84 ;
    static const int8_t MUTE_ATTENTUATION = 84 ;

    void set ( uint8_t attLevel, uint8_t channel, uint8_t both ) {
      
      attLevel = this->MUTE_ATTENTUATION - attLevel ;      
      
      if ( both > 1 || channel > 1 ) return ;          // są tylko 2 kanały
      if ( attLevel > this->MAX_ATTENTUATION ) attLevel = this->MUTE_ATTENTUATION ;  // blokada na maksymalne tłumienie.

      uint16_t dataToSend = 0 ;                        // Dane / bity do wysłania
                                                       // Przygotowujemy 11 bitów zgodnie ze specyfikacją układu
      dataToSend |= channel ;                          // numer kanału
      dataToSend |= ( both << 1 );                     // 0 = oba kanały, 1 = każdy kanał osobno
      if ( attLevel <= this->MAX_ATTENTUATION ) {
        dataToSend |= ( 21 - ( attLevel / 4 ) ) << 2 ; // ATT1
        dataToSend |= ( 3 -  ( attLevel % 4 ) ) << 7 ; // ATT2
      }
      dataToSend |= 0b11 << 9;                         // ostatnie bity = 11

      //
      // wysyłamy dane
      for ( uint8_t i = 0; i <= 10; i++ ) {
        _delay_us( 3 );
        PORT_VOL_DATA &= ~VOL_DATA ;   // DATA = 0
        _delay_us( 3 );
        PORT_VOL_CLK &= ~VOL_CLK ;     // CLK = 0
        _delay_us( 3 );
        if ( ( ( dataToSend >> i ) & 0x01 ) == 1 ) {
          PORT_VOL_DATA |= VOL_DATA ;  // DATA = 1
          } else {
          PORT_VOL_DATA &= ~VOL_DATA ; // DATA = 0
        }
        _delay_us( 3 );
        PORT_VOL_CLK |= VOL_CLK ;      // CLK = 1
      }
      _delay_us( 3 );
      PORT_VOL_DATA |= VOL_DATA ;      // DATA = 1
      _delay_us( 3 );
      PORT_VOL_CLK &= ~VOL_CLK ;       // CLK = 0
      _delay_us( 3 );

      return;
    }

};
2

Twoje wyznawanie "ciekaw jestem w jaki sposób podszedł byś do pisania testów funkcji" jest dla mnie zbyt pracochłonne, bo po prostu nie znam się na Arduino. Trudno mi rozpoznać co jest twoje, a co jest zdefiniowane przez platformę. Do tego brak wiedzy o RC5 utrudnia mi zrozumienie logiki kodu.

Pisanie testów do gotowego kodu jest trudne. Na tyle trudne, że ludzie piszą na ten temat książki.
Pisanie testów razem z kodem produkcyjnym (TDD, DDD) jest łatwe, wymaga jedynie dyscypliny.

Gdy piszę coś od zera razem z testami, to używam polimorfizmu statycznego lub dynamicznego (lub mieszanka obu), by oddzielić zewnętrzne API od mojego kodu.
W kodzie testowym zewnętrzne API zastępuje gmock-ami. Wówczas jestem wstanie odtworzyć dowolne zachowanie jakie może mi dostarczyć API i dobrze przetestować kod.

2
  1. Nie stosujesz nagłówków. Skazujesz się przez to na piekło bardzo uciążliwych w zarządzaniu zależności. Dzięki nagłówkom możesz mieć klasy/struktury zapowiadające i o wiele większą hermetyzację modułu.
  2. Nie stosujesz constexpr. Wyrażenia stałe można wyrzucić z Flash, deklarując je nie jako:
static const uint16_t FOO = 42;

a

constexpr static uint16_t FOO = 42;
  1. Brak stosowania const correctness. Zwróć uwagę że klasy często "piszą po rejestrach" a nie po swoich atrybutach. Stąd metody mogą często być const.
  2. Zmienne globalne... naprawdę? Niezbędne?
  3. Return z funkcji void. Powód?
  4. Deklaracja częstotliwości zegara w kodzie aplikacji. To jest makro zewnętrzne.
  5. Zbędny this przy odwoływaniu się do atrybutów metod klasy. Reguły rozwiązywania nazw, wyraźnie określają sposób poszukiwania nazwy. Są naprawdę jednostkowe przypadki gdy to jest niezbędne.
  6. Z jednej strony tworzysz statycznie klasę (a nie dodajesz przed nią static), a z drugiej zostawiasz możliwość budowania konstruktora. O ile pamiętam, avr-g++, budował tam zbędny kod. Jeśli klasa ma być statyczna (to dłuższa dyskusja czy i kiedy), to raczej coś takiego..
class Bar {
public:
    Bar() = delete;
    ~Bar() = delete;
    // ...
};
  1. Na pierwszy rzut oka, wydaje się że zbędnie dehermetyzujesz klasę. Te atrybuty publiczne. Czy to naprawdę niezbędne w każdym przypadku?

Przy programowaniu dla MCU w C++, ogromne benefity daje constexpr, szablony oraz polimorfizm statyczny (i nie tylko). Stąd warto z pojęciami się zapoznać.

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.