FlorianLiehn / CodeMicrocontroleurAntenne

0 stars 0 forks source link

review de code #1

Open alex31 opened 6 years ago

alex31 commented 6 years ago

remarque sur le code :

dans tous les .c : les includes suivant sont inutiles (c'est pour lancer des tests de non regression internes à chibios):

include "rt_test_root.h"

include "oslib_test_root.h"

L'ideal serait d'utiliser un outil pour generer le board.h à partir d'un board.cfg

comme ça les pin auraient des noms pour rendre le code lisible et eviter des erreurs.

Le code et la doc du script de generation boardGen.pl : https://github.com/alex31/chibios_enac_various_common/tree/master/TOOLS

main.c :

*Tu utilises une Mailbox directement, il serait plus sur et facile d'utiliser une FifoObject FifoObject est l'association d'une Mailbox avec un MemoryPool (voir doc chibios)

*Les mailbox, et les structures que dont tu envoies le pointeur avec chThdCreateStatic, sont des variable locales au main, allouées sur la pile. Pour ce genre d'objects partagés, il vaudrait mieux que ça soit des variable globales au main, eventuellement déclarée static si tu veux réduire leur visibilité.

dans le cas présent, vu que le main est bloqué sur un while (true) { chThdSleepMilliseconds(500);}, ça va fonctionner car les variables automatiques ne vont jamais disparaitre, mais ce n'est pas une bonne pratique.

PcSerialThread.h :

les working area (espace mémoire des piles privées des thread ne doivent pas être déclarée dans le .h). elles sont privées au thread et il n'y a besoin que de les definir static dans le .c là ou tu lances le chCreateStatic

Pareil pour les fonctions point d'entrée dans le thread

Si tu veux lancer les thread depuis une fonction accessible depuis l'exterieur, crée une fonction pcSerialStart dans PcSerialThreads.h qui fera les chCreateStatic.

PcSerialThreads.c :

Il peux y avoir une erreur ici, si il n'y a pas d'erreur, il faudra que tu m'expliques exactement ce que tu veux faire :

union Payload_message payload={.message=(struct log_message)msg};

cette ligne fait que tu crées un nouveau message, et tu copies ((opération couteuse) le message pointé par msg. Donc c'est une duplication.

En fait cette remarque rejoint une remarque faite au debut : pourquoi tu n'utilises pas un FiFoObject comme je te l'ai conseillé jeudi ?

phase devrait probalement être un booleen (déclaré bool) et phase=~phase remplacé par un phase = !phase

au niveau du post : struct log_message test={ .order=ORDER_GOTO, .logs =log_test, };

    (void)chMBPostI(mailbox_log, (msg_t)&test);

Tu vas toujours poster l'adresse de la même structure test, donc tous tes message pointeront sur le même test ce qui fait que ta file de message ne va pas fonctionner, CF utilisation d'un FifoObject