Coding Elegance Advice

Projects General Guidance
1

I'm creating a Halloween project: a skull that turns as people walk by it. I'm using three PIR sensors at different angles to sense approaching people, and a servo to position the skull. I've written the code and breadboarded the circuit; everything works fine.

Even though it works, is there a more elegant way to write my positioning code? I'm an I&C hardware guy by training; coding is not my normal or natural skillset. I'd appreciate any advice or recommendations on how to clean up the positioning code, it seems clunky as written. I'm sure that there's a better and cleaner way to write it.

Here's the schematic:

Schematic, PIR-Servo wLEDs
Schematic, PIR-Servo wLEDs1472×1592 489 KB

Here's the code:

//Works 12/14 at 1134

#include <Servo.h>

Servo myServo;

//PIR pin assignment
const byte PIRL = 2;
const byte PIRC = 4;
const byte PIRR = 7;

//PIR sensor status variable, LOW when not sensing heat
bool Lstatus = LOW;
bool Cstatus = LOW;
bool Rstatus = LOW;

//Possible servo angles
int Angle[] = { 20, 55, 90, 125, 160 };

//Servo position
int Pos;

//Time variable for future project
unsigned long CurrentTime;

void setup() {

  myServo.attach(9);

  pinMode(PIRL, INPUT);
  pinMode(PIRC, INPUT);
  pinMode(PIRR, INPUT);

  //The three LEDs are for testing purposes only and will be removed in the final version
  pinMode(12, OUTPUT);
  pinMode(11, OUTPUT);
  pinMode(10, OUTPUT);

//Delay to let the PIR sensors get acclimated
  delay(2000);
}

void loop() {

  CurrentTime = millis();
  Sense();
  Position();
  Move();
}

void Sense() {
  Lstatus = digitalRead(PIRL);
  Cstatus = digitalRead(PIRC);
  Rstatus = digitalRead(PIRR);

  //LED if/else code is for testing purposes only and will be removed in the final version
  if (Lstatus == HIGH) {
    digitalWrite(12, HIGH);
  } else {
    digitalWrite(12, LOW);
  }

  if (Cstatus == HIGH) {
    digitalWrite(11, HIGH);
  } else {
    digitalWrite(11, LOW);
  }

  if (Rstatus == HIGH) {
    digitalWrite(10, HIGH);
  } else {
    digitalWrite(10, LOW);
  }
}
//There must be a better way to write the below code.  This seems clunky to me.
void Position() {
  if (Lstatus == LOW && Cstatus == LOW && Rstatus == LOW) {
    Pos = Angle[2];
  }

  if (Lstatus == HIGH && Cstatus == LOW && Rstatus == LOW) {
    Pos = Angle[0];
  }


  if (Lstatus == HIGH && Cstatus == HIGH && Rstatus == LOW) {
    Pos = Angle[1];
  }


  if (Lstatus == HIGH && Cstatus == HIGH && Rstatus == HIGH) {
    Pos = Angle[2];
  }


  if (Lstatus == LOW && Cstatus == HIGH && Rstatus == LOW) {
    Pos = Angle[2];
  }


  if (Lstatus == LOW && Cstatus == HIGH && Rstatus == HIGH) {
    Pos = Angle[3];
  }


  if (Lstatus == LOW && Cstatus == LOW && Rstatus == HIGH) {
    Pos = Angle[4];
  }
}

void Move() {
  myServo.write(Pos);
}

PS. Thank you to the many gurus here who helped me figure out how to write state code. Couldn't have done it without this forum.

2

Don't change that code! It's well organized as it is, details not checked.

3

Please tell us you're not using the Arduino as the +5V for the servo...please?

What's clunky about it? Like, takes too long to react, or jumps from point to point instead of a smooth transition...?

You could, instead of jumping between 30 degree points at at time, use some speed control and write to the servo across the ranges in between the 20, 55, 90 etc desired endpoints.

Also, from a practical perspective, if the servo isn't holding anything in place between calls to move the servo, it's a good idea to servo.attach(9);, move the servo, then servo.detach(); to save wear and tear on the servo and help prevent it jittering while it's idle. For this, you might find it useful to keep track of the last states of the PIR sensors, and only perform the Position(); and Move(); functions if something changed.

4

If OP does decide to change something, call the existing, working code myCode_v1 or something like that, add in a little Serial printing right after void setup() like

Serial.begin(115200);
Serial.println("myCode_v1");

and make any changes to the code in a copy/pasted new version altogether, so it's easy to roll back to the good one.

5

Of course.

But "clean" and "elegant" code are matters of opinion!

Remember to take a backup of your working code before you start changing it!

6

  • Looks good

  • Suggest you use some macros to help make things more understandable.
    Avoid, HIGH/LOW and avoid magic numbers.

Example:


#define ENABLED            true
#define DISABLED           false
 
#define LEDon              HIGH    //+5V---[220R]---[LED]---PIN
#define LEDoff             LOW
 
#define OPENED             HIGH    //+5V---[Internal 50k]---PIN---[Switch]---GND 
#define CLOSED             LOW

if(doorSwitch == OPENED)
{
digitalWrite( doorLED, LEDon);
}
else
{
digitalWrite( doorLED, LEDoff);
}

7

@hallowed31 For my breadboarding, I'm using the Arduino to power the servo. I have a 100 microF capacitor across the servo power wiring to minimize momentary power sag. For the final version, I might power the servo directly from a 5V power supply. The skull is light, so I'm not expecting a lot of torque on the servo.

By clunky I mean: not an efficient use of code lines. I figure there has to be a way to code it better, I just don't have enough coding experience to know how best to approach it. Operationally, the motion is jumpy. I'll have to figure out how to slow the servo motion down without disturbing the scan time. Future additions to this project will use more void loops to perform additional tasks, so a function can't bog down the whole program.

Didn't realize that I could attach and detach the servo. Good idea!

To all: I've learned the hard way to save my work. It's easy to get lost when revising code as to what worked, and what didn't. Apparently, once you compile and upload code, you can't get it back. Wish I knew that when I started...

PS A question about variables: int versus bool versus byte. For this project I'm not doing anything fancy. Is there anything inherently wrong about using bool for TRUE/FALSE inputs versus byte or int? Or using byte versus int for a 0-255 variable?

8

More use of arrays would certainly help. An array for the sensor pins. An array for the led pins.

And some way to turn the 3 sensor readings directly into one of the 5 angles.

There are 8 possible combinations of the 3 sensor readings...

I'm thinking a 3D array for the servo positions!

int Angle[2][2][2] = {
  {
    {20, 55},
    {90, 125}
  },
  {
    {160, xxx},
    { yyy, zzz}
  }
};

Need to figure out in detail...

9

A general rule is: don't think of using any controller board as a power supply, especially not for motors. Your out on thin ice if You do.

10

One possibility;

void setup() {
  // put your setup code here, to run once:
}

byte angleIndex;
int Angle[8] = { /* user-defined values */ };

void loop() {

  angleIndex = 0;  // reset the index value
  if (Lstatus == HIGH) bitSet(angleIndex, 0);
  if (Rstatus == HIGH) bitSet(angleIndex, 1);
  if (Cstatus == HIGH) bitSet(angleIndex, 2);
  Pos = angleArray[angleIndex];
}

Angle array size is increased to account for the 23 possibilities from the three PIR sensors.

Consider - an int is a 16-bit signed variable, a byte is 8-bit unsigned. Since the value won't exceed 255 a byte will work and save a byte of RAM.

11 
int Angle[2][2][2] = {{{
      20,  //PIRL==LOW PIRC==LOW PIRR==LOW
      55   //PIRL==LOW PIRC==LOW PIRR==HIGH
    },{
      90,  //PIRL==LOW PIRC==HIGH PIRR==LOW
      125  //PIRL==LOW PIRC==HIGH PIRR==HIGH
    }},{{
      160, //PIRL==HIGH PIRC==LOW PIRR==LOW
      xxx  //PIRL==HIGH PIRC==LOW PIRR==HIGH
    },{
      yyy, //PIRL==HIGH PIRC==HIGH PIRR==LOW
      zzz  //PIRL==HIGH PIRC==HIGH PIRR==HIGH
    }}};

@tampadavid I'll leave it to you to put the right angles in the right places in the 3D array. You only have 5 angles but there will be 8 values in the 3D array, so just put that centre angle in as a default for any unwanted positions.

Then this function would become extremely "elegant"!

void Position() {
  Pos = Angle[Lstatus][Cstatus][Rstatus];
}
12

You might as well sort this in prototyping. Were you planning on having it plugged into mains? If so, a 5V wall wart of 2A rating will probably do, but check the datasheet for all the devices it will be powering, multiply the sum of the amperage requirements by 1.5 at least. (for the servo, that's the stall current).

13

Hello tampadavid

Please specify.

14

visusal things:

  • let variables and function names start with low letters (see setup() and loop())
  • why these double line breaks in Position()?
  • line breaks in general, why are there so many empty lines - and not even consistent. there is a line feed after loop() but not after Sense()

code wise:

  • why do you hardcode LED pins and not using constants for the LED Pins?
  • I don't think you will need CurrentTime in global namespace, may be you don't need it at all.
  • Position() contains lot of repeated lines of code. If you can't solve this in another way, at least think about, whether you need single if statements or if you could use else if to finish the function earlier.
  • check your int variables ... ask yourself, is there any need of a signed two byte type needed for what you need?

learn something new:

  • learn about structures. For examples take the PIR Sensor. for the PIR sensor you need at least one pin and you store its state in the status variable. Sounds like a structure for me.

From functional point of view (just my opinion)

  • slow down the movement of the servo the get smoother movements
15

You could have written this as simply

digitalWrite(12, Lstatus);
16

I like coding the 3 switch variables into a single combined sensor input variable, and then using that for a LookUpTable.

It might be nice to allow for a do-not-care value in the table, and to also only do the updating if things change.

Completely untested snippets:

if(angleIndex != lastAngleIndex) { // change detection
  lastAngleIndex = angleindex;
  if(angleArray[angleIndex] != -1 ){ // ignore do-not-cares
     targetPos = angleArray[angleindex];
  }
}

if(Pos != targetPos && (millis() - lastNudgeMs > 100)) { // error detection and rate limiting
   lastNudgeMs = millis(); 
   Pos += (targetPos - Pos > 0 ? 1 : -1 ) ; // Adjust towards target
   myServo.write(Pos);
}
17

I would.  I was just trying to show the germ of the idea.  Enhancements left to OP's discretion.

18

When you have your release version of code, format it - elegantly - like this...

jack
jack1234×1288 189 KB

19

Thanks for all the advice. It will take me a week or two to digest it, write new code, and try it out. Arrays are a particular challenge - the Arduino starter kit book wasn't real clear about them. When I get this project done, I'll post my results and improved code.

@hallowed31: I do plan to use a 5V wall wart for the Arduino and a 12V wart for my LED strips (to be added), probably controlled through a MOSFET or relay card.

@noiasca: these are the kind of comments I was looking for. I wrote this post because I don't know what I don't know. Elegant may have been the wrong adjective, but its all I could think of. You all have been doing this for much longer than me, and any wisdom is appreciated.

@xfpd If I could make my code look like that, I would!

Many thanks to all, and a good Christmas too.

20

If Uno, 7-9 V for arduino if using barrel jack or VIN. 5v if powering Uno through 5V pin.