Help needed to clean up code, do it smarter.

Hi All,

I just finished my first actual working code. It is meant for an RC car, controlling lights and the use of disc braking (it was a Nitro with disc braking but now brushless, I just want the discs to kick in when braking hard).

It does what I want but because I am really new to this I know that the code is put together like bits and pieces and I would like some advice to clean it up.

Bottomline:
RC RX receiver, channel 2, 4 and 5. Channel 2 is the throttle, 4 is a switch (lights off, lights dim, high beam). 5 is a dimmer for a Roof light strip.

Some leds are being used as an indicator (neutral, forward).

Lights are obvious I guess. Off, dim, and highbeam
Rearlights, off, normal and breaking
Rooflight dimmable from 0 to full

Also, The discbrakes need to kick in when I want and as hard as I want. Hence the Potentiometer involved.

Like I said it works but do I use the right pins and how can I clean up the code? I feel that I can do the coding a lot smarter but again, I am very new to this.

Thanks.

#include <Servo.h>
int maxBreakpPin=A2;
int maxBreakRead;
int ch2Pin=10; // Throttle / Brake input
int ch5Pin=12; //Dimmer input
int ch4Pin=11; // Lightswitch input
int ch2Value; //Throttle Value
int ch5Value; //Dimmer Value
int ch4Value; //LightSwitch Value
float maxExtraBrake;
float RoofValue; //Intensity of Rooflight
int ServoPin=9;
int FrontPin=6; //PWM / Frontlights
int StripPin=3; //PWM / RoofLight
int ForwardPin=4;//NORM / Indicator forward throttle
int NeutralPin=7; //NORM / Indicator Neutral Position
int BrakeRearPin=5;//PWM / Rear and brake light
int ExtraBreakPin=2;//PWM / Indicator for additional disc braking

int Direction=0; //Tracer for reverse
Servo myServo;


void setup() {
  // put your setup code here, to run once:
  pinMode(maxBreakpPin, INPUT);
  pinMode(ch5Pin, INPUT); // Set our input pins as such
  pinMode(ch2Pin, INPUT);
  pinMode(ch4Pin, INPUT);
  pinMode(13, OUTPUT);
  pinMode (FrontPin, OUTPUT);
  pinMode (StripPin, OUTPUT);
  pinMode (ForwardPin, OUTPUT);
  pinMode (NeutralPin, OUTPUT);
  pinMode (BrakeRearPin, OUTPUT);
  pinMode (ExtraBreakPin, OUTPUT);
  pinMode (ServoPin,OUTPUT);
  myServo.attach(ServoPin);

  Serial.begin(9600); 
}

void loop() {
  maxBreakRead=analogRead(maxBreakpPin);
  maxExtraBrake=(180./1023.)*maxBreakRead;
 
  
  // put your main code here, to run repeatedly:
  ch2Value = pulseIn(ch2Pin, HIGH, 30000); // Read the pulse width of 
  ch4Value = pulseIn(ch4Pin, HIGH, 30000); // each channel
  ch5Value = pulseIn(ch5Pin, HIGH, 30000);
  
  if (ch2Value >= 1510){
    digitalWrite(ForwardPin, HIGH);
    } 
    else if (ch2Value < 1510)
    {digitalWrite(ForwardPin, LOW);}
  
  if ((ch2Value >=1375) && (ch2Value <= 1509)){
    digitalWrite(NeutralPin, HIGH);
    }
    else{ digitalWrite(NeutralPin, LOW);} 

if (ch4Value <= 1374){
  analogWrite(FrontPin,0);
    if (ch2Value <=1374){
    analogWrite(BrakeRearPin,255);   
    }
    else {
      analogWrite(BrakeRearPin,0);
    }
  }
 else if ((ch4Value >=1375) && (ch4Value <= 1875)){
  analogWrite(FrontPin,75);
  if (ch2Value <=1374){
    analogWrite(BrakeRearPin,255);   
    }
    else {
      analogWrite(BrakeRearPin,50);
    }
}
else if (ch4Value >= 1876){
  analogWrite(FrontPin,255);
  if (ch2Value <=1374){
    analogWrite(BrakeRearPin,255);   
    }
    else {
      analogWrite(BrakeRearPin,50);
    }
 }
if (ch2Value <=1050){
    analogWrite(ExtraBreakPin,255); 
    myServo.write(maxExtraBrake);
}
else {
    analogWrite(ExtraBreakPin,0);
    myServo.write(0);
}
  RoofValue=(255./991.)*(ch5Value-995.);
  if (RoofValue >=253){
    RoofValue=255;
  }
  else if (RoofValue <=2){
    RoofValue=0;
  }
  analogWrite(StripPin, RoofValue);
  Serial.println(maxExtraBrake);
  //Serial.println(ch5Value);

  //delay(2000); // I put this here just to make the terminal 
              // window happier
}

Oh, the tracer for reverse can be ignored at this point. It is something I need to wrap my head around.

The ESC in this car will go in reverse the second time I push the lever into reverse. I need some indicator to count how often I did that.

But That is for later.

I am more worried about all the if’s and the use of pins. :wink:

Up :wink:

My advice from many years of experience is never to mess with a working program!!!!! Go on to the next project with what you have learned for your first one.

Paul

Ok, but what have I learned? That is why I am asking here :wink:

It works but there should be a smarter way right? all those if's.

Oh and this project is just the start I will be amending and growing it as I go so I want a good basis.

(GPS, Gyro, WiFi and SDcard will be added I hope).

Then work from a copy of your program. Keep the original. When you get the next addition working, keep that. Never cut off your backup.

Paul

Hello Sareno,
Well done for getting your first project working ++Karma;

Ok, but what have I learned? That is why I am asking here :wink:

I think only you know that, this is your project, what do you know now that you didn't know at the start?

Now back to your original question:

How can I clean up the code? I feel that I can do the coding a lot smarter but again, I am very new to this.

The thing that stands out for me in your code is that you do everything in loop(); C/C++ is built around the idea of functions and you need to learn to use them. Look at your code, find things that work together to achieve one thing and put them into a function. Your loop(); should just be a list of calls to functions you have created and nothing else. Have a look at Using Nextion displays with Arduino to see how I approach this. The actual code is not not what I want you to look at, but the way I group things together into a function that does one thing. You could also look at the source code for the servo library you have used; while I have not looked it them myself I feel confident that you will see a series of functions that do particular tasks, that is the model to follow.

Do also follow what Paul said; work on a copy of your program, keep the original working code as back up in case you make a complete mess of it (and NEVER be afraid of making a mess of it!).

Hi Sareno

It takes time to get better. I am still learning every day and sometimes when I am in a rush, my code suffers.

There is a book I can recommend -

Clean Code a Book by Robert Cecil Martin

It is a world-renowned book that was recommended to me by many. I am also trying a new game on Steam called Screeps. It is meant to improve programming skills so I will update how I get on.

I checked your code and I have written worse. I would personally put more space between if statements and sometimes on big projects, I make comment titles like this:-

//------------------------------------------DECLARING VARIABLES-------------------------------------

<code>

//------------------------------------------DECLARING VARIABLE END----------------------------------

Breaking things down into functions also helps and using a good IDE (Personally I like Visual Studio Code)

Thank you for the replies. I did make a copy and I am working of of that copy, keeping the original to fall back to if needed.

I am fairly new to C so working with functions is new to me. In this case, all of the communication with the RX receiver has to be realtime. What I am afraid of is that the code/arduino is waiting for something that will delay the communication between my transmitter and receiver. It is a fast driving car. Control needs to be 100% realtime.

So, controlling lights is prio 2, 3, 4 etc. BUT applying the discbrake is prio 1.

How can I make sure that whatever the arduino is doing, the controls are being sent through immediately? Lights being prio 2 or less.

How can I make sure that whatever the arduino is doing, the controls are being sent through immediately? Lights being prio 2 or less.

Don't use delay();. Ever.
Don't use while(); unless you can be confident that the condition will become untrue very quickly.
Use for(); carefully and make sure it doesn't loop for long enough to slow anything else down.

Apart from that, you are worrying about nothing. Splitting code into functions won't cause what you are concerned about.

A comment on your comment about "C". I think every language I ever used had the capability of using functions or similar with different names. Even assembly languages. It's a way of thinking, not something specific to "C" or "C++".

Paul

Sareno:
I am fairly new to C

Arduino sketches are written in C++, not C, this might be important when Googling.

Sareno:
I did make a copy and I am working of of that copy, keeping the original to fall back to if needed.

Using Git or some other version-control system is a great idea for larger projects. It has saved me on multiple occasions when I accidentally destroyed or deleted something.

Pieter

Thanks again all.

Wat is GIT? (sorry, noob)