Avis sur Code

Salut à vous, je débute avec l'Arduino et j'ai besoin d'un avis sur un petit bout de code que je vient de faire pour controler avec trois petit BP une matrice de de 8 Led bicouleur avec un 595.
Deux des BP servent à sélectionner la LED sur laquelle le troisiéme BP va changer la LED.

J'ai pas mal galéré pour arriver à mes fins, étant pas encore habitué à toutes les fonctions du langage arduino j'ai bidouillé ça:

/*La sélection de la couleur se fait par un boutons LED, et deux autres boutons de naviguer de LED en LED
Le potentiométre joue sur le delais de multiplexage d'affichage des LED
*/

const int latchPin = 8;             //595 pin 12
const int clockPin = 12;          //595 pin 11
const int dataPin = 11;           //595 pin 14
//595 pin 16 connected to 5VDC
//595 pin 8 connected to GND
const int BPG = 4;
const int BPLED = 3;
const int BPD = 2;

int LEDA[4]={0,17,33,49};
int LEDB[4]={0,18,34,50};
int LEDC[4]={0,20,36,52};
int LEDD[4]={0,24,40,56};
int LEDE[4]={0,65,129,193};
int LEDF[4]={0,66,130,194};
int LEDG[4]={0,68,132,196};
int LEDH[4]={0,72,136,200};
int LEDDelay;
int Etat_BPG = 0;
int Last_BPG = 0;
int Etat_BPLED = 0;
int Last_BPLED = 0;
int Etat_BPD = 0;
int Last_BPD = 0;
int Direction;
int ledA;
int ledB;
int ledC;
int ledD;
int ledE;
int ledF;
int ledG;
int ledH;

void setup() {
Serial.begin(9600);
pinMode(latchPin, OUTPUT);
pinMode(clockPin, OUTPUT);
pinMode(dataPin, OUTPUT);
pinMode(BPG, INPUT);
digitalWrite(BPG, HIGH) ;
pinMode(BPD, INPUT);
digitalWrite(BPD, HIGH) ;
pinMode(BPLED, INPUT);
digitalWrite(BPLED, HIGH) ;
}

void loop() {

Etat_BPLED = digitalRead(BPLED);
Etat_BPD = digitalRead(BPD);
Etat_BPG = digitalRead(BPG);

if (Etat_BPG!=Last_BPG){
	 if (Etat_BPG == HIGH){
		 Direction --;
		}
	 if(Direction == -1){
		 Direction=7;
		}
	else{}
	}

if (Etat_BPD!=Last_BPD){
		if (Etat_BPD == HIGH){
		 Direction ++;
		}
		if(Direction == 8){
		 Direction=0;
		}
	else{}
	}

switch (Direction){
		case 0:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledA ++;
				 if (ledA == 4){
					 ledA=0;
					}
					else{}	
				}
			}
		break;	
		case 1:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledB ++;
				 if (ledB == 4){
					 ledB=0;
					}
					else{}		
				}
			}
		break;
		case 2:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledC ++;
				 if (ledC == 4){
					 ledC=0;
					}
					else{}		
				}
			}
		break;
		case 3:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledD ++;
				 if (ledD == 4){
					 ledD=0;
					}
					else{}	
				}
			}
		break;	
		case 4:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledE ++;
				 if (ledE == 4){
					 ledE=0;
					}
					else{}		
				}
			}
		break;
		case 5:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledF ++;
				 if (ledF == 4){
					 ledF=0;
					}
					else{}	
				}
			}
		break;	
		case 6:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledG ++;
				 if (ledG == 4){
					 ledG=0;
					}
					else{}		
				}
			}
		break;	
		case 7:			
			if (Etat_BPLED!=Last_BPLED){
				if (Etat_BPLED == HIGH) {
				 ledH ++;
				 if (ledH == 4){
					 ledH=0;
					}
					else{}	
				}
			}
		break;			
	}

int LED[4]={LEDA[ledA]|LEDE[ledE],LEDB[ledB]|LEDF[ledF],LEDC[ledC]|LEDG[ledG],LEDD[ledD]|LEDH[ledH]};

  for (int x = 0; x <= 3 ; x++){
     digitalWrite(latchPin, HIGH);
     digitalWrite(latchPin, LOW); 
     shiftOut(dataPin, clockPin, MSBFIRST, LED[x]);     
	 digitalWrite(latchPin, HIGH);
	 LEDDelay = analogRead(0);
	 delay(LEDDelay);
	}
	
Last_BPLED = Etat_BPLED;
Last_BPG = Etat_BPG;
Last_BPD = Etat_BPD;
Serial.println(LEDDelay);
}

Voili voilou, je pense qu'il doit u avoir une voie plus simple approprié mais je ne sais pas vers laquelle me tourner, donc si vous pouvez m'aiguiller, je vous en remercie par avance ^^

PS voila le bouzin: Lien
PS II le orange ne ressort pas bien sur la vidéo mais il est bien visible en vrai....

Salut,

Bon, côté code, il semble que ça tourne, donc il n'y a pas grand chose à faire sinon optimiser.

Il y a des "else{}" qui trainenet un peu partout, tu peux les enlever, ils ne servent à rien.

Tu travailles avec des int. Il faut savoir que l'arduino est une plateforme 8 bits, et un int est en 16 bits. Le fait de travailler en 16 bits multiplie par 3 au moins le temps d'exécution. Si aucune de tes valeurs ne dépasse l'intervalle [0, 255], un conseille, déclare tes variables en byte (simple octet de 8 bits), ça tournera bien plus vite. Le type int ne sert que si tu as besoin de valeurs négatives, ce qui dans ton cas n'est pas utile.

si je prends ça (j'ai rajouté des commentaires) :

	 if (Etat_BPG == HIGH){
		 Direction --;
		}
	 if(Direction == -1){  // Dans ce cas, en effet, Direction devient négatif, mais il y a moyen de contourner cela.
		 Direction=7;
		}
	else{}  // ne sert à rien, ça encombre le code.
	}

Si toutes tes variables deviennent des octets non signés, ton code devient :

	 if (Etat_BPG){   // naturellement, la condition dans les parenthèses valide le test si elle est égale à HIGH ou true, donc pas nécessaire de l'indiquer.
		 Direction --;    // dans le cas d'un byte, pas de risque de se tromper si ça déborde : 0 - 1 = 255
		}
	 if(Direction == 255){  // voir commentaire plus haut.
		 Direction=7;
		}
	}

en simplifiant encore, on arrive à ça :

	 if (Etat_BPG) Direction --;
	 if (Direction == 255) Direction=7;

Salut à toi et merci de ton retour, bon donc je vais retravailler le code en suivant tes conseils qui ne sont pas passé dans l'oreille d'un sourd.

Par contre je ne comprends pas ton dernier bout de code, tu as oublié de mettre des {} ou on peut aussi traiter les if sans ces accolades?

Et autre questions, pour gérer une matrice à LED, il n'y a pas plus simple que ce que j'ai pondu? style un tableaux multidimensionnel comme ça :

{0,0,0,0}
{0,0,0,0}
{0,0,0,0}
{0,0,0,0}

Et comment faire changer les couleurs de LED ou la position de la LED à changer avec un tableaux de ce type? parce que j'ai cherché mais je n'ai rien trouvé de concret dans la langue de Moliére....bon j'ai peut étre pas cherché avec les bons termes, mais je ne suis pas encore vraiment familliarisé avec tout ça :blush:

encore merci à toi :slight_smile:

Pour les "if", tu n'es pas obligé de mettre les { si tu n'as qu'une seule instruction.

Bonjour,

J'ai repris ton code à la sauce skywodd (et un peu IOCC) :grin:

En gros j'ai :

  • remplacer les const int par des define,
  • formater le code de manière plus "aérienne",
  • passer en byte des int ne pouvant pas prendre une valeurs >255,
  • déclarer dans loop() les variables non globale,
  • supprimer les if inutile dans le switch qui pourrai être fait une seul fois avant,
  • utiliser des assignations avec retour et des incrémentations conditionné pour alléger les if,
  • fait des déclaration/initialisation multiple "en ligne" des divers variables global,
  • passer du type int au type unsigned int la variable LEDDelay,
  • renommer les variables selon la convention d'écriture "arduino".
/*
 * La sélection de la couleur se fait par un boutons LED, et deux autres boutons de naviguer de LED en LED
 * Le potentiométre joue sur le delais de multiplexage d'affichage des LED
 */

/* Pin Definition */
#define LATCH_PIN 8     //595 pin 12
#define CLOCK_PIN 12    //595 pin 11
#define DATA_PIN  11    //595 pin 14

//595 pin 16 connected to 5VDC
//595 pin 8 connected to GND
#define BPG   4
#define BPLED 3
#define BPD   2

byte tLedA[4] = {
  0,17,33,49};

byte tLedB[4] = {
  0,18,34,50};

byte tLedC[4] = {
  0,20,36,52};

byte tLedD[4] = {
  0,24,40,56};

byte tLedE[4] = {
  0,65,129,193};

byte tLedF[4] = {
  0,66,130,194};

byte tLedG[4] = {
  0,68,132,196};

byte tLedH[4] = {
  0,72,136,200};

unsigned int ledDelay;
byte lastBpG = 0, lastBpD = 0, lastBpLed = 0, direction = 0, ledA = 0, ledB = 0, ledC = 0, ledD = 0, ledE = 0, ledF = 0, ledG = 0, ledH = 0;

void setup() {
  Serial.begin(9600);

  pinMode(LATCH_PIN, OUTPUT);
  pinMode(DATA_PIN, OUTPUT);
  pinMode(CLOCK_PIN, OUTPUT);

  pinMode(BPG, INPUT);
  digitalWrite(BPG, HIGH) ;

  pinMode(BPD, INPUT);
  digitalWrite(BPD, HIGH) ;

  pinMode(BPLED, INPUT);
  digitalWrite(BPLED, HIGH) ;
}

void loop() {

  byte etatBpLed = digitalRead(BPLED);
  byte etatBpD = digitalRead(BPD);
  byte etatBpG = digitalRead(BPG);

  if (etatBpG != lastBpG && etatBpG)
    if (direction-- == -1) direction = 7;

  if (etatBpD != lastBpD && etatBpD)
    if (direction++ == 8) direction = 0;

  if (etatBpLed != lastBpLed && etatBpLed)
    switch (direction){
    case 0:			
      if (ledA++ == 4) ledA = 0;
      break;	
    case 1:			
      if (ledB++ == 4) ledB = 0;		
      break;
    case 2:			
      if (ledC++ == 4) ledC = 0;
      break;
    case 3:			
      if (ledD++ == 4) ledD = 0;
      break;	
    case 4:			
      if (ledE++ == 4) ledE = 0;	
      break;
    case 5:				
      if (ledF++ == 4) ledF = 0;
      break;	
    case 6:			
      if (ledG++ == 4) ledG = 0;	
      break;	
    case 7:			
      if (ledH++ == 4) ledH = 0;
      break;			
    }

  byte tLed[4] = {
    tLedA[ledA] | tLedE[ledE], tLedB[ledB] | tLedF[ledF], tLedC[ledC] | tLedG[ledG], tLedD[ledD] | tLedH[ledH]   };

  for (byte i = 0; i < 4; i++){
    digitalWrite(LATCH_PIN, HIGH);
    digitalWrite(LATCH_PIN, LOW); 
    shiftOut(DATA_PIN, CLOCK_PIN, MSBFIRST, tLed[i]);     
    digitalWrite(LATCH_PIN, HIGH);

    delay(ledDelay = analogRead(0));
  }

  lastBpLed = etatBpLed;
  lastBpG = etatBpG;
  lastBpD = etatBpD;

  Serial.println(ledDelay);
}

J'ai un petit doute pour le delay(ledDelay = analogRead(0)); normalement ça marche comme ça ou sinon c'est avec des () autour, je sais plus ><

Salut à toi skywodd, et merci pour le code que tu as retravaillé, mais je n'en demandé pas autant ^^....
Par contre je viens de Up le sketch sur la carte, et ça marche mais y'a des soucis pour la navigation et le changement de couleur de LED, ça marche sur les deux premiéres LED puis aprés ça part un peu en sucette....mais bon pas grave je vais analyser ton code et le retravailler avec les conseils de Super_Cinci.... quoi qu'il en soit merci de votre aide, je m'en vais retravailler le bouzin qui n'est qu'une petite partie de mon projet :slight_smile:
PS le Leddelay avec le potentiométre c'était pour voir la meilleur valeur à mettre en delay, et je trouve que 4 est trés bien pour que toutes les LED scintillent pareil, parce qu'à 0 delay y'a des LED qui scintillent plus que d'autres....donc pas de soucis le potentiométre est supprimé ^^

@jmatgou Merci je savais pas...

Arioch:
(...) ça marche sur les deux premiéres LED puis aprés ça part un peu en sucette.... (...)

C'est pas un descriptif détaillé du problémes ça :grin:

euh...ben en fait c'est aléatoire donc je ne sais pas comment décrire la chose ^^....

A pars les initialisations à zero en plus que j'ai rajouté, les deux codes sont syntaxiquement identique, je vois pas ce qui pourrait donner un résultat aléatoire :~
Enlève les initialisations à zero en plus peut être que ...

byte lastBpG = 0, lastBpD = 0, lastBpLed = 0, direction, ledA ledB, ledC, ledD, ledE, ledF, ledG, ledH;

Mais bon je pense pas que ce soit ça ...

Re Skywodd, ben en fait l'autre soir je devais étre fatigué.... :blush:

Y'avais que un soucis de comptage de +1 pour les changements couleur et LED, en gros c'est comme si il y avait eu 9 LED et 5 couleur avec ton code, donc vraiment pas grand chose, j'ai remis les bonnes valeurs et ça marche nikel...

Désolé, et merci :wink:

Arioch:
Y'avais que un soucis de comptage de +1 pour les changements couleur et LED, en gros c'est comme si il y avait eu 9 LED et 5 couleur avec ton code, donc vraiment pas grand chose, j'ai remis les bonnes valeurs et ça marche nikel...

Tu peut poster ton code modifié juste que je puisse voir ou j'avais fait l'erreur ?
On apprend de ses erreurs, au moins celle là je la ferait plus ^^

Arioch:
Désolé, et merci :wink:

Bin pour le coup c'est moi qui est foiré donc c'est plutôt à moi de m'excuser ^^

Re, ben y'avait pas grand chose, mais c'est bizarre parce que ça ne correspond pas aux valeurs de mon code ^^

/*
 * La sélection de la couleur se fait par un boutons LED, et deux autres boutons de naviguer de LED en LED
 * Le potentiométre joue sur le delais de multiplexage d'affichage des LED
 */

/* Pin Definition */
#define LATCH_PIN 8     //595 pin 12
#define CLOCK_PIN 12    //595 pin 11
#define DATA_PIN  11    //595 pin 14

//595 pin 16 connected to 5VDC
//595 pin 8 connected to GND
#define BPG   4
#define BPLED 3
#define BPD   2

byte tLedA[4] = {
  0,17,33,49};

byte tLedB[4] = {
  0,18,34,50};

byte tLedC[4] = {
  0,20,36,52};

byte tLedD[4] = {
  0,24,40,56};

byte tLedE[4] = {
  0,65,129,193};

byte tLedF[4] = {
  0,66,130,194};

byte tLedG[4] = {
  0,68,132,196};

byte tLedH[4] = {
  0,72,136,200};

unsigned int ledDelay;
byte lastBpG = 0, lastBpD = 0, lastBpLed = 0, direction, ledA, ledB, ledC, ledD, ledE, ledF, ledG, ledH;

void setup() {
  Serial.begin(9600);

  pinMode(LATCH_PIN, OUTPUT);
  pinMode(DATA_PIN, OUTPUT);
  pinMode(CLOCK_PIN, OUTPUT);

  pinMode(BPG, INPUT);
  digitalWrite(BPG, HIGH) ;

  pinMode(BPD, INPUT);
  digitalWrite(BPD, HIGH) ;

  pinMode(BPLED, INPUT);
  digitalWrite(BPLED, HIGH) ;
}

void loop() {

  byte etatBpLed = digitalRead(BPLED);
  byte etatBpD = digitalRead(BPD);
  byte etatBpG = digitalRead(BPG);

  if (etatBpG != lastBpG && etatBpG)
    if (direction-- == 0) direction = 7;

  if (etatBpD != lastBpD && etatBpD)
    if (direction++ == 7) direction = 0;

  if (etatBpLed != lastBpLed && etatBpLed)
    switch (direction){
    case 0:			
      if (ledA++ == 3) ledA = 0;
      break;	
    case 1:			
      if (ledB++ == 3) ledB = 0;		
      break;
    case 2:			
      if (ledC++ == 3) ledC = 0;
      break;
    case 3:			
      if (ledD++ == 3) ledD = 0;
      break;	
    case 4:			
      if (ledE++ == 3) ledE = 0;	
      break;
    case 5:				
      if (ledF++ == 3) ledF = 0;
      break;	
    case 6:			
      if (ledG++ == 3) ledG = 0;	
      break;	
    case 7:			
      if (ledH++ == 3) ledH = 0;
      break;			
    }

  byte tLed[4] = {
    tLedA[ledA] | tLedE[ledE], tLedB[ledB] | tLedF[ledF], tLedC[ledC] | tLedG[ledG], tLedD[ledD] | tLedH[ledH]   };

  for (byte i = 0; i < 4; i++){
    digitalWrite(LATCH_PIN, HIGH);
    digitalWrite(LATCH_PIN, LOW); 
    shiftOut(DATA_PIN, CLOCK_PIN, MSBFIRST, tLed[i]);     
    digitalWrite(LATCH_PIN, HIGH);

    delay(4);
  }

  lastBpLed = etatBpLed;
  lastBpG = etatBpG;
  lastBpD = etatBpD;

}

Arioch:
Re, ben y'avait pas grand chose, mais c'est bizarre parce que ça ne correspond pas aux valeurs de mon code ^^

if (direction++ == 8) direction = 0;

VS

if (direction++ == 7) direction = 0;

Je suis vraiment trop bête !! c'était pas variable++ mais ++variable !!

Voila le code corrigé :

/*
 * La sélection de la couleur se fait par un boutons LED, et deux autres boutons de naviguer de LED en LED
 * Le potentiomètre joue sur le delais de multiplexage d'affichage des LED
 */

/* Pin Definition */
#define LATCH_PIN 8     //595 pin 12
#define CLOCK_PIN 12    //595 pin 11
#define DATA_PIN  11    //595 pin 14

//595 pin 16 connected to 5VDC
//595 pin 8 connected to GND
#define BPG   4
#define BPLED 3
#define BPD   2

byte tLedA[4] = {
  0,17,33,49};

byte tLedB[4] = {
  0,18,34,50};

byte tLedC[4] = {
  0,20,36,52};

byte tLedD[4] = {
  0,24,40,56};

byte tLedE[4] = {
  0,65,129,193};

byte tLedF[4] = {
  0,66,130,194};

byte tLedG[4] = {
  0,68,132,196};

byte tLedH[4] = {
  0,72,136,200};

unsigned int ledDelay;
byte lastBpG = 0, lastBpD = 0, lastBpLed = 0, direction = 0, ledA = 0, ledB = 0, ledC = 0, ledD = 0, ledE = 0, ledF = 0, ledG = 0, ledH = 0;

void setup() {
  Serial.begin(9600);

  pinMode(LATCH_PIN, OUTPUT);
  pinMode(DATA_PIN, OUTPUT);
  pinMode(CLOCK_PIN, OUTPUT);

  pinMode(BPG, INPUT);
  digitalWrite(BPG, HIGH) ;

  pinMode(BPD, INPUT);
  digitalWrite(BPD, HIGH) ;

  pinMode(BPLED, INPUT);
  digitalWrite(BPLED, HIGH) ;
}

void loop() {

  byte etatBpLed = digitalRead(BPLED);
  byte etatBpD = digitalRead(BPD);
  byte etatBpG = digitalRead(BPG);

  if (etatBpG != lastBpG && etatBpG)
    if (--direction == -1) direction = 7;

  if (etatBpD != lastBpD && etatBpD)
    if (++direction == 8) direction = 0;

  if (etatBpLed != lastBpLed && etatBpLed)
    switch (direction){
    case 0:			
      if (++ledA == 4) ledA = 0;
      break;	
    case 1:			
      if (++ledB == 4) ledB = 0;		
      break;
    case 2:			
      if (++ledC == 4) ledC = 0;
      break;
    case 3:			
      if (++ledD == 4) ledD = 0;
      break;	
    case 4:			
      if (++ledE == 4) ledE = 0;	
      break;
    case 5:				
      if (++ledF == 4) ledF = 0;
      break;	
    case 6:			
      if (++ledG == 4) ledG = 0;	
      break;	
    case 7:			
      if (++ledH == 4) ledH = 0;
      break;			
    }

  byte tLed[4] = {
    tLedA[ledA] | tLedE[ledE], tLedB[ledB] | tLedF[ledF], tLedC[ledC] | tLedG[ledG], tLedD[ledD] | tLedH[ledH]   };

  for (byte i = 0; i < 4; i++){
    digitalWrite(LATCH_PIN, HIGH);
    digitalWrite(LATCH_PIN, LOW); 
    shiftOut(DATA_PIN, CLOCK_PIN, MSBFIRST, tLed[i]);     
    digitalWrite(LATCH_PIN, HIGH);

    delay(4);
  }

  lastBpLed = etatBpLed;
  lastBpG = etatBpG;
  lastBpD = etatBpD;
}

Et bien sur l'explication qui va avec :
Prenons int i = 5;
Si l'on fait Serial.println(i, DEC); il s'affiche 5, normal.
maintenant avec Serial.println(i++, DEC); il s'affiche toujours 5 car la valeur de i est passé à la fonction avant d'étre incrémenté.
Avec Serial.println(++i, DEC); i est incrémenté puis passé à la fonction, ici il s'afficherait 6.

Heureuse de base ...pour le coup je suis vraiment désolé, faire une si grosse boulette :.

Oula ne t'excuses surtout pas, tu me fais un code amélioré, moi j'apprends, en plus tu fais un fautes et j'apprends aussi de ta faute....que du bhonneur.

Merci à toi :slight_smile: