search

Gixy31 / ESP32-PoolMaster

ESP32 version of PoolMaster from Loic74650
53 stars 19 forks source link

Bug possible en utilisant la lib ADS1115 #11

Closed shibida closed 1 year ago

shibida commented 1 year ago

A cet endroit là : https://github.com/Gixy31/ESP32-PoolMaster/blob/79e7cb5b6e01c8f416d057ba21296b8ecd581c4a/src/Loops.cpp#L78

Tu ne te sers pas du résultat de la fonction adc.update(). Juste en dessous tu viens lire adc.ready(), qui peut entre temps, avoir terminé, et donc tu peux lire potentiellement des valeurs qui sont fausses.

Je pense que si tu mets adc.update dans ton if, ça corrige éventuellement un bug.

Pour ma part je vais fork et utiliser une librairie plus standard pour ADS, d'autant plus que la seule fonction qui n'y est pas est les valeurs moyennes que tu reimplementes un peu plus loin. Peut être une autre fois je me servirait des signaux ready pour faire du vrai asynchrone avec les interruptions.

Si tu veux tu prendras la pool request.

Gixy31 commented 1 year ago

??? Il faut lire la doc de la librairie : adc.update est un void, donc ne produit pas de résultat. Cette fonction doit être appelée à un rythme supérieur à la vitesse de conversion et sert à relancer les conversions entre chaque canal. Lorsque toutes les conversions sont terminées, c'est adc.ready qui le confirme. Donc ce code est correct et parfaitement conforme à ce qu'il faut faire tel que décrit dans la doc.

Gixy31 commented 1 year ago

Et si tu déplaces adc.update() dans le if, adc.ready() ne sera jamais true et donc...

shibida commented 1 year ago

Oui, je suis d'accord avec toi si on lit la doc ! Mais regarde mieux le code de la lib. dans le .h l133 ou même dans le code cpp c'est un bool en fait.

De plus la variable "done" n'est protégée nulle part ...

Le risque est faible mais il est bien là.

Gixy31 commented 1 year ago

Je ne comprends pas de quel risque tu parles...

Le boolean done de la classe est private donc ne risque rien. Ensuite c'est bien update() qui relance une à une les conversions sur les canaux sélectionnés. Si tu appelles ready() sans avoir fait update(), tu n'auras jamais rien. Donc je persiste : On fait update() plus vite que la vitesse de conversion, ce qui permet de relancer une conversion après que chaque canal soit fait. Lorsque toutes les conversions sont faites, done passe à true et ready() (qui n'est que la version publique de done) permet de venir récupérer les valeurs et on repart pour un cycle.

Sans trop approfondir, je pense qu'on pourrait se contenter de if adc.update() puisque'en effet update() renvoie done et ne pas utiliser ready(). Mais en tous cas déplacer update() derrière le test de ready() ne peut pas fonctionner.

Cette fonction update() permet justement de ne pas se préoccuper de la fin de toutes les conversions avant de lire les valeurs, alors que les autres librairies, même en asynchrone, obligent à gérer soi-même la boucle sur les 4 canaux.

shibida commented 1 year ago

Bon ok désolé du bruit, j'ai mélangé le done de cette classe avec une autre lib qui utilise les meme nom de variable.

Bref pas de bug mais une doc pas à jour ...