forme : il serait bien que tu suives les coventions du camelCase que tu suis presque : les nom de types commencent par une majuscule au début, les noms de variable et de fonction commencent par une minuscule

ce qui est important, c'est d'être cohérent sur le style pour l'ensemble du projet par exemple : `void StartAntennaThreads(objects_fifo_t log, objects_fifo_t order, Trajectory* traj);

int readAntennaMessage(uint8_t* message); ` à deux ligne d'intervalle, tu n'utilises pas la même convention.

dans AntennaThread.h on trouve cette definition :

define TIMEOUT_1PPS TIME_MS2I(1005)

il faut que tu ajoutes un commentaire pour expliquer pourquoi tu associes 1005 millisecondes à un truc qui s'appelle 1PPS (1 Par seconde, on s'attends à trouver une valeur de 1000ms)

dans AntennaFunction.c :+1:

il y a la fonction int readAntennaMessage(uint8_t* message)

Une bonne pratique quand on passe un pointeur sur un buffer fourni par l'appelant qui doit être rempli par l'appelé est de passer la taille du buffer (façon strncpy que tu utilises d'ailleurs) . Dans le corps de la fonction on voit que la taille doit être ANTENNA_MESSAGE_LENGTH, mais rien n'interdit l'appelant de déclarer avec une autre taille.

quel est l'interêt de cette construction : while(n<ANTENNA_MESSAGE_LENGTH){ n+=sdAsynchronousRead(&SD3,&(buf[n]),ANTENNA_MESSAGE_LENGTH-n);

par rapport à un sdRead(&SD3, buf, ANTENNA_MESSAGE_LENGTH);

???

de plus, pourquoi passer par un buffer intermedaire buf plutôt que de remplir directement le buffer message passé en paramètre ?

AntennaThread.c :+1:

1/ Tu fais un truc compliqué avec des chThdSuspend/Resume en utilisant l'api asynchrone palSetPadCallback alors que tu pourrais rendre tout cela bien plus lisible en supprimant les chThdSuspend/Resume et en utilisant l'api synchrone palWaitPadTimout ou mieux palWaitLineTimout directement dans la boucle du thread.

http://chibios.sourceforge.net/docs3/hal/group___p_a_l.html#ga8193d226bb32543dd248fd1f8427f6e1

Tu utilises l'api Pad au lieu de l'api Line, bien plus robuste, et des numéros de PIn directement dans le code : palSetPadMode(GPIOB, 10, PAL_MODE_ALTERNATE(7)); palSetPadMode(GPIOB, 11, PAL_MODE_ALTERNATE(7)); //1pps Ext interuption palSetPadMode( GPIOB,0, PAL_MODE_INPUT_PULLDOWN ); palEnablePadEventI(GPIOB,0, PAL_EVENT_MODE_RISING_EDGE);

c'est pas lisible, et il n'y a pas un endroit dans le code où l'on a la table d'affectation des pins. Comme déjà dit précedement, il faut que tu utilises un fichier de description board.cfg, le script boardGen.pl qui te generera le board.h.

Ensuite, il est bien plus sur d'utiliser l'api Line que l'Api Pad : avec l'api Pad, si tu passes en premier paramètre un mauvais port, le compilateur ne pourra pas t'aider.

doc d'install, d'utilisation et script se trouvent ici : https://github.com/alex31/chibios_enac_various_common/tree/master/TOOLS

PcSerialFunctions.c :

soigne le style : if(incoming_message.id>=FIRST_ERROR_ID){ il manque quelques espaces : if (incoming_message.id >= FIRST_ERROR_ID) { dans les déclarations de fonction, une virgule doit être suivie d'un espace (comme en français)

La, tu récupères un message dans la queue, tu le modifies, et tu le renvoies dans la même queue, je comprends pas pourquoi, mais je suis presque sur que c'est pas ce qu'il faut faire.

Il faut que tu m'expliques ce que tu veux faire par là !!!

//Send to Antenna Executer
SimpleMessage* new_order=(SimpleMessage*)chFifoTakeObjectI(fifo_order);
new_order->arguments=incoming_message.arguments;
new_order->id=incoming_message.id;

chFifoSendObjectI(fifo_order,  (void*)new_order);
return 0;

PcSerialThreads.c :

dans l'appel if(state==MSG_OK){ //send message write_message(STM_PC_writer,(SerialPayload)msg);

tu passes le SerialPayload par valeur (le compilateur effectue une copie) au lieu de passer un pointeur sur l'objet, pourquoi ?

THD_WORKING_AREA(waPC_RxThread, 128); fait attention à la taille de la pile pour les threads : 128, c'est trop peux. Utilises 512 comme valeur par défaut, et plus si le thread alloue de la place sur la pile : s'il y a des tableaux déclarés en variables locales.

que font ces 10, 12, 20, 25 etc non commentés dans le code ? utilise des macros comme ça le nom de la macro documente la valeur littérale.

    if(count%20==0){

        ARGS ping;
        strncpy(ping.message_antenne,"TEST0MESSAGE",12);
        ping.message_antenne[4]+=(count/20)%10;

        WriteLogToFifo(fifo_log_arg,ID_MSG_LOG_PING,
            ping);
    }

    chThdSleepMilliseconds(25);

pareil pour toutes les valeurs littérales dans tou sles fichiers de ton projet.

PcSerialThreads.h :

Si tu veux declarer une fonction inline dans un .h, il faut qu'elle soit statique : inline int STM_PC_reader(uint8_t buff,int n){ return sdAsynchronousRead(&SD2,buff,n);} inline int STM_PC_writer(uint8_t buff,int n){ return sdAsynchronousWrite(&SD2,buff,n);}

devraientt être declarés

static inline

Sinon, si ton .h est inclus dans plusieurs.c, chaque .c va exporter la fonction et le linker va planter (avec raison) avec le message : définitions multiple de fonctions.

Courage, si tu as des questions, passe me voir.

Alexandre

FlorianLiehn / CodeMicrocontroleurAntenne

review semaine 25 #2

define TIMEOUT_1PPS TIME_MS2I(1005)