review tetx mcu semaine 36

[alex31]

bug dangereux :

0/ Réservation de la mémoire pour les threads (partout où la macro THD_WORKING_AREA est utilisée) :

128 n'est pas assez en général, utilise 512 comme valeur par défaut.

[alex31]

1/ organisation des fichiers :

Mieux séparer les programmes, il y a un programme pour le MCU et un pour PC linux, il faut donc deux sous répertoires avec pour chaque programme un makefile, alors que pour le moment tout est mélangé avec un seul makefile pour générer les deux programmes.

Si il y a des fichiers qui sont communs aux deux programme, il faut les mettre dans un troisième répertoire appelé common_lib par exemple.

Si l'on veut pouvoir tout compiler (MCU et appli PC) en même temps, il faut un makefile à la racine qui ne fasse que lancer un make dans chaque sous répertoire.

[alex31]

2/ main.c (MCU)

comme déjà signalé, le style n'est pas cohérent, et le code est rendu illisible par le manque d'espace ou trop d'espaces

BUG IMPORTANT : utilisation des fonctions de l'API réservées aux zones protégées (API I-Lock) dans un thread normal.

Ce type d'erreur aurait du être trouvé par le système de vérification dynamique de ChibiOS, mais le fichier de configuration de chibios chconf.h n'a pas été renseigné pour que ces vérifications soient faites :

les macros CH_DBG_SYSTEM_STATE_CHECK, CH_DBG_ENABLE_CHECKS, CH_DBG_ENABLE_ASSERTS devraient être définies à la valeur TRUE

Ceci est un bug qui va tôt ou tard planter le MCU et doit être corrigé en urgence.

[alex31]

3/ TrameAntennaConstructor.c

---

static inline int checkElevation(double el){ return (el<MAX_EL_ANGLE && el>MIN_EL_ANGLE); }

cette fonction devrait retourner un bool au lieu d'un int

---

if( !checkElevation(el) )return;

comme déjà signalé avant les vacances, les branches d'un test conditionnel ne doivent pas être sur la même ligne de code que le test

---

//security if( *X>=MAX_ENCODER_X || *X<=MIN_ENCODER_X || *Y>=MAX_ENCODER_Y || *Y<=MIN_ENCODER_Y ){

en général, quand on défini des valeur MIN et MAX, ce sont les valeurs MIN et MAX autorisées, ce sont donc des valeurs valides, et le test doit être fait avec strictement inférieur (ou supérieur) et non supérieur(inférieur) ou égal pour déterminer l'invalidité.

---

int n=0; //SYN *(message+(n++))=HEADER_ANTENNA[0]; //MODE 0 to 9 A to F *(message+(n++))=CHAR_ZERO+puissance + (puissance/10)*( CHAR_A_OFFSET ) ; *(message+(n++))=CHAR_ZERO+mode;

ça ma l'air bien compliqué tout ça ...

pas besoin de variable n, il suffit de postincrémenter le pointeur message :

//SYN *message++=HEADER_ANTENNA[0]; //MODE 0 to 9 A to F *message++=CHAR_ZERO+puissance + (puissance/10)*( CHAR_A_OFFSET ) ; *message++=CHAR_ZERO+mode;

ensuite, pas besoin du buffer intermédiaire Pos_send, autant utiliser snprintf pour qu'il écrive directement dans message, puis incrémenter message de 4.

---

computeXYencoderFromAzEl calcule deux entier 16 bits signés, mais ensuite ces valeurs signées sont castées en valeur non signées, ça me parait dangereux.

Si jamais X ou Y sont négatifs, ça va faire des choses bizarres ! -> Comportement attendu : -42 => FFD6 (complément à 2) Testé et validé en fonctionnement.

---

[alex31]

4/ TrameAntennaConstructor.h

---

organisation : il y a un mélange de macro et de déclaration de fonctions, il vaut mieux commencer par les macro, puis les enum, et enfin les fonctions.

---

important : il faut protéger par des parenthèses les macro qui pourraient buguer en fonction de la précédence des opérateurs :

#define MIN_ENCODER_X -MAX_ENCODER_X
#define MAX_ENCODER_Y 31435
#define MIN_ENCODER_Y -MAX_ENCODER_Y

#define ENCODER_X_STEP_BY_ANGLE 6000./17
#define ENCODER_Y_STEP_BY_ANGLE 20000./51

doit être

#define MIN_ENCODER_X (-MAX_ENCODER_X)
#define MAX_ENCODER_Y 31435
#define MIN_ENCODER_Y (-MAX_ENCODER_Y)

#define ENCODER_X_STEP_BY_ANGLE (6000./17)
#define ENCODER_Y_STEP_BY_ANGLE (20000./51)

[alex31]

5/ messages.c

---

static uint8_t crcTable[256]={};
void crcInit(void){
    uint8_t  remainder;
    //Compute the remainder of each possible dividend.
    for (int dividend = 0; dividend < 256; ++dividend)

Il y a deux fois 256, ce n'est pas une bonne pratique. à la deuxième utilisation, on devrait utiliser un sizeof pour s'affranchir d'une erreur si un jour la taille du tableau est changée

---

pour tout ce qui est taille de message, il faut utiliser le type standardisé size_t et non int

int encodePayload(uint8_t* payload,uint8_t* msg,int payload_length)

devrait être

size_t encodePayload(uint8_t* payload,uint8_t* msg,size_t payload_length)

---

int writeMessage(int(*writer)(uint8_t*,int),SerialPayload* payload){

la signature pointeur sur fonction devrait être definie par un typedef, ça serait plus lisible

typedef int(*read_write_callback_t)(uint8_t*,int);
int writeMessage(read_write_callback_t writer, SerialPayload* payload) 
int readMessage(read_write_callback_t reader, uint8_t* message)

---

[alex31]

6/ GpsTimeHandler.c

static void setFormatNMEAmessage(void){
    const  int8_t id_message=0x08;
    const int16_t pay_length=9;

y a t'il un intérêt à ce que le type de message et la longueur du message soient des variables signées ?

en fait il y a ensuite dans le code un bitshift sur la valeur pay_length, et la norme du langage C interdit de faire un décalage sur un nombre négatif : c'est un undefined behavior du langage C et doit être proscrit.

https://en.wikipedia.org/wiki/Undefined_behavior

---

static void setFormatNMEAmessage(void){
    const  int8_t id_message=0x08;
    const int16_t pay_length=9;

    char set_RMC[16];
...
         sdWrite(&SD1,(uint8_t *)set_RMC,n);

pourquoi ne pas déclarer uint8_t set_RMC[16]; au lieu de le déclarer tableau de char et caster ensuite ?

---

Bug dangereux :

Il y a plusieurs occurence de ce pattern dans le code :

ARGS empty_args;
writeLogToFifo(fifo_log_arg,ID_MSG_ALERT_WRONG_GPS_MESSAGE,
                                empty_args);

empty_args est dans ce cas une variable non initialisée, il peut y avoir n'importe quoi dedans !!

il faut employer : ARGS empty_args = {0}; pour forcer l'initialisation de la variable ) à 0;

[alex31]

7/ messages .h : voir le point 4

8/ AntennaThreads.h : voir point 4

define TIMEOUT_1PPS TIME_MS2I(1005)

Cette macro n'est toujours pas commentée comme je l'avais demandé semaine 25...

D'une manière générale, toutes les valeurs définies par des macros doivent être commentées pour permettre au repreneur du code de comprendre la conception et les raisons derrière tes choix.

[alex31]

9/ AntennaThreads.c :

plusieurs bugs dangereux :

writeLogToFifo(fifo_log_arg,ID_MSG_ALERT_NO_1PPS,
                (ARGS){});

Attention, cette construction ne garanti pas que la valeur ARGS soit initialée, il faut utiliser (ARGS){0}

---

chThdCreateStatic(waAntennaTxThread, sizeof(waAntennaTxThread), NORMALPRIO, antennaTxThread,
               (void*)&(ThreadsArgs){log, order, traj });

### Bug très dangereux (je t'avais déjà signalé de te mefier de ce type de bug) :

Tu crée une variable de type ThreadsArgs dans une fonction, cette variable est donc crée sur la pile, sa durée de vie est donc la durée de vie de la fonction englobante (startAntennaThreads) Quand on sort de startAntennaThreads, la pile est rendue au systeme, et cette variable n'a plus d'existance en mémoire.

Hors, tu prends l'adresse de cette variable, et tu la passe à un thread dont la durée de vie dépasse celle de la fonction startAntennaThreads.

C'est ce qu'on appelle un dangling pointer, et c'est une race de bug très dangereuse.

https://en.wikipedia.org/wiki/Dangling_pointer

Pour t'en sortir, plusieurs solutions :

A/ facile, mais non ré-entrant :

tu déclares une variable globale privée à la fonction dans AntennaThreads.c au lieu d'une variable automatique dans le corps de la fonction :

void startAntennaThreads(objects_fifo_t* log, objects_fifo_t* order,
                     Trajectory* traj){
        static ThreadsArgs ta {log, order, traj };
    //init port
    //SD3 = Antenna (PB10 = Tx, PB11 = Rx)
    palSetLineMode(ANTENNA_PIN_RX, PAL_MODE_ALTERNATE(7));
    palSetLineMode(ANTENNA_PIN_TX, PAL_MODE_ALTERNATE(7));
    sdStart(&SD3, &antennaSerialConfig);
    //1pps Ext interuption
    palSetLineMode(     PIN_1PPS, PAL_MODE_INPUT_PULLDOWN );
    palEnableLineEventI(PIN_1PPS, PAL_EVENT_MODE_RISING_EDGE);

    //Creates threads
    chThdCreateStatic(waAntennaRxThread, sizeof(waAntennaRxThread), NORMALPRIO, antennaRxThread,
                                                           (void*)log);
    chThdCreateStatic(waAntennaTxThread, sizeof(waAntennaTxThread), NORMALPRIO, antennaTxThread,
               (void*)&ta);

}

B/ ré-entrant, mais nécessite de la mémoire dynamique :

Allouer de la mémoire sur le tas (utilisation de malloc) dans la fonction qui lance le thread, et de faire un free sur cette mémoire dans la fonction point d'entrée du thread une fois la copie des champs de la structure faite.

C/ plus élégant et ré-entrant (c'est cette solution que tu dois mettre en oeuvre) :

Conserver l'utilisation d'une variable crée sur la pile, mais s'assurer que la variable allouée sur la pile vive le temps que le thread l'utilise en utilisant un moyen de syncro entre la fonction qui lance le thread et le thread. Dans ce cas, un sémaphore binaire est le moyen de synchro adapté, il faut l'ajouter à la structure que tu passes en paramètre :

typedef struct {
  uint32_t ledBlinkDuration;
  binary_semaphore_t sem; // sémaphore binaire pour protéger la structure
} ProtectedStackArg;

static THD_WORKING_AREA(waBlinker, 512);    // déclaration de la pile du thread blinker
static void blinker (void *arg)         // fonction d'entrée du thread blinker
{
  ProtectedStackArg *psa = (ProtectedStackArg *) arg;
  chRegSetThreadName("blinker");        // on nomme le thread

  // on copie le paramètre avant que le pointeur psa devienne un dangling pointer une fois
  // la pile de la fonction appelante rendue au système
  const uint32_t duration = psa->ledBlinkDuration;

  DebugTrace ("father : signal sem");
  chBSemSignal(&psa->sem); // après cet appel, l'appelant peut sortir, nous nous engageons à ne plus
               // utiliser le pointeur psa

  while (true) {                // boucle infinie
    palToggleLine(LINE_C00_LED1);       // clignotement de la led 
    chThdSleepMilliseconds(duration);       
  }
}

void launch (void)
{

  halInit();
  chSysInit();

  ProtectedStackArg psa = {
    .ledBlinkDuration=500,
    .sem= _BSEMAPHORE_DATA(psa.sem, 1) // initialisation du semaphore binaire
  };

  chThdCreateStatic(waBlinker, sizeof(waBlinker),
            NORMALPRIO, &blinker, (void *) &psa); // lancement du thread 
  DebugTrace ("son : wait until sem is signaled");
  chBSemWait(&psa.sem); // on attend jusqu'à ce que le thread lancé ai terminé d'utiliser la structure
  DebugTrace ("son : signaled sem : continue");
}

[alex31]

10/ AntennaFunctions.c

Pourquoi utiliser sdAsynchronous[Read/Write] au lieu de sd[Read/Write] ? L'utilisation de l'API asynchrones peut générer des problèmes compliqués et ne doit être faite que si il y a une bonne raison de ne pas utiliser l'API synchrone.

[alex31]

11/ PcSerialthreads.c (voir point 0)

msg_t state = chFifoReceiveObjectI(fifo_log_arg,&msg);

voir point 2 sur l'utilisation de l'API I-Lock

De plus, vu que chFifoReceiveObjectI n'est pas bloquant, tu doit ajouter un chThdSleepMilliseconds(2); pour ne pas que ton thread monopilise tout le cpu.

En utilisant chFifoReceiveObjectITimeout(fifo_log_arg,&msg, TIME_INFINITE); ton thread bloquera tant qu'un message n'arrive pas et tu peux supprimer ce chThdSleepMilliseconds(2);

---

pcRxThread :

le message ping est réellement utilisé, ou c'est une scorie d'un test pendant le développement ?

La fonction readMessage devrait être bloquante, et dans ce cas, le chThdSleepMilliseconds(25); ne sert à rien.

---

D'une manière générale (dans tous les .c) , mélange de l'API Pad et de l'API Line, ça fait pas très serieux, ce n'est pas prioritaire, mais si tout était converti avec l'API Line, ça serait plus clair.

---

//Creates threads
chThdCreateStatic(waPcRxThread, sizeof(waPcRxThread), NORMALPRIO, pcRxThread,
               (void*)&(ThreadsArgs){log, order, traj });

Même Bug très dangereux de dangling pointer que le point 9

[alex31]

reopen accidentally closed issue