Using functions to improve my code.

Please excuse any syntax errors I may make in writing this post. I am learning sketch as I go along, but want to learn the correct way, that is concise code which executes correctly and keeps the execution 'loop time' as short as possible. To this end I have started an online tutorial on c++ and, whilst the Arduino runs a version of C I am now aware that there are many subtle differences.

I am not asking for someone to correct my code, merely to flag up if I am doing something stupid or longwinded when there may be an elegant solution to hand. To this end I will try to write my 'code' in plain english (sorry Cockneys!) to make it's meaning clearer than pigeon code.

The project involves taking 10 samples of an analog input which represents electrical current, averaging them, and returning the result to void loop(). The data flow I intend is the following:

1 loop calls a function 'intervalTime' which creates an interval of about 100ms between samples by subtracting millisLast from millisNow and calling a second function 'takeSample' when interval>100ms.
intervalTime then makes itself zero, and makes millisLast=millis().

2 TakeSample reads the analog input when IntervalTime > 100ms, and makes variable Total=Total + Sample.

3 TakeSample then increments variable Count = Count + 1 (shorthand Count++ ?) and returns the value of Count to loop().

4 Loop sees when Count >= 10, when it assigns variable Current a value of Total/count.

The expected behaviour is that Current is updated about once per second with the average of 10 samples, and the processing time to do this is reasonably constant. This will be part of a larger program which will decide various outputs based on the value of Current, and at this stage I don't know how much extra code I'll be adding as the project develops from 'bare bones functionality' to 'works to the user's advantage and meets their needs'.

I realise that using interrupts may be better, but I've not got to that part in the tutorial yet...using what I (almost) know!

GM

Please post your code as it is now

Thanks HeliBob, though the rudimentary code I have may not be any use; it's more approval or otherwise of the method I am using to achieve the desired result.
I'll post it later today if time allows...

UKHeliBob:
Please post your code as it is now

It's unbelievable that requests for help come in like this.

I really can't figure it out.

:angry:

Glorymill:
Thanks HeliBob, though the rudimentary code I have may not be any use; it's more approval or otherwise of the method I am using to achieve the desired result.
I'll post it later today if time allows...

Without seeing your code it is impossible to provide much advice.

For instance, how and where are your variables declared, are you passing values to functions and if so how, are you returning variables from functions and if so how or are all variables global to avoid the need to pass them to and from functions, have you used millis() in a way that blocks code execution ?

Can you see how it is difficult without seeing the code

UKHeliBob is correct; it's much easier to make suggestions if we can see your code. (Please use code tags when posting it. See the "Using this Forum" post at the top of this Forum.) As a first point starting place, you might read this.

When you start writing functions, they should be:

Cohesive. That is, if you cannot explain what a function does in one or two sentences, it's probably
too complex. Too often students try to write the function equivalent of a Swiss Army Knife.
"If I add this flag variable, I can make the function do this AND that!" Usually, such
efforts result in functions that do multiple things badly, are difficult to maintain, and
are less portable to other applications. Simple is usually better.

Coupling. Avoid functions that require that some other function be called before this one is called. That
is, try to make them "stand alone" as much as possible. You can't de-couple all tasks (e.g.,
a file has to be opened before it can be read), but avoid it as much as possible.

Document. You should have a comment at the top of the header that explains what the function
does, the arguments that need to be passed to it, and the return value from the
function. If the function is based on some complex algorithm, you might consider adding
a URL where the algorithm is explained in detail.

Glorymill:
To this end I have started an online tutorial on c++ and, whilst the Arduino runs a version of C I am now aware that there are many subtle differences.

Just to prevent future confusion, Arduino uses standard C++. The only thing they do special is to try to resolve forward references automatically.

There are also many platform-specific features that come standard when you use the Arduino IDE, like the use of the Setup() and Loop() functions and the many pre-defined functions and macros that simplify hardware access, but those are all implemented in standard C++ (and you don't have to used them if you don't want to).

like the use of the Setup() and Loop() functions

Please, if you are going to give advice make sure that is accurate. The functions are called setup() and loop(). I am sure that you know that so why not use the proper names

Have a look at Planning and Implementing a Program

...R

Here is my code to date. I have tried to keep the loop() function short and readable by calling other functions from within it. The sketch (as it is) compiles and reports the following:-

Sketch uses 1670 bytes (5%) of program storage space. Maximum is 30720 bytes.
Global variables use 196 bytes (9%) of dynamic memory, leaving 1852 bytes for local variables. Maximum is 2048 bytes.

To my shame I realise that I should have done this before posting, apologies for that.

Thanks to UKhelibob, as a major part of this is modified from his blink without delay forum post.

I would appreciate comments that would help me to keep the code easily readable and modifiable, as this is by no means the finished article. (especially as it's not even complete!)

Intended mods are to add a WiFi adapter so that the user can read the total load via his mobile phone, and addition of a real time clock so that electrical priorities can be set via a weekly schedule.

GM

//The electrical supply to this flat trips at about 40A, and cannot be upgraded easily. This code is intended to shed low priority electrical
//loads in favour of keeping on the higher priority ones. It should take several readings of an analogue input at regular time intervals using 
//millis(). These readings (called samples here) represent the electrical loadings of all appliances except the heating (6kW electric boiler).
//Thus when appliance loads exceed approx. 4kW we will commence shutdown of the heating to keep total power load below 10kW (40A).

 
int sampleCount = 0;            //number of current readings taken. Used to calc average
int total = 0;                  //the cumulative total of several current samples
int count = 0;                  //the number of samples to average
int averageCurrent = 0;         //the result of averaging
unsigned long millisLast = 0;   //Time of last reading
unsigned long interval = 0;     //Duration since last reading taken
unsigned long sampleTime = 100; //Period between taking current readings in milliseconds


void setup()
{
  Serial.begin(115200);         //start Serial in case we need to print debugging info
  millisLast = millis();        //This is so that interval is relative to the time now
                                //Need to set up pinmode to allow analogue readings
                                //Need to set up pinmode for outputs to relays
}

void loop()
{
  interval = millis() - millisLast;   //time interval since last reading taken
  
  if (interval >= sampleTime)         //if true it's time to take a reading of current
   {
    takeSample();                     //run this function to return an average of several current readings
                                      //and return an averaged value as an integer 0 - 1023
   total = total + takeSample;        //Each new sample added to a starting total of zero
   count ++;                          //increment count value to use for average value later
   }

   if (count >= 10)
   {
    averageCurrent = total / count;   //used in the decision making code later
    total = 0;                        //reset ready for the next samples
    count = 0;                        //reset ready for the next samples too
   }

   loadReduction();                   //call of function to turn off electrical items based on the values of
                                      //averageCurrent and userPreferences and otherModifiers which haven't been coded yet!
}

int takeSample()                      //start of function to take a current sample from Arduino ADC 0-5vdc and
                                      //return it to the main loop as integer takeSample
     {
      
     }

void loadReduction()                  //function to energise relays through Arduino outputs, add hysteresis, and
                                      //take the decisions as to what stays on and what is turned off
     {
      
     }
  interval = millis() - millisLast;   //time interval since last reading taken
  if (interval >= sampleTime)         //if true it's time to take a reading of current

Personally I think this reads better as

if (currentTime - previousSampleTime >= sampleInterval)
  {
    //do stuff
    previousSampleTime = currentTime;
  }

With suitable adjustment of variable names and types of course

Be careful when returning variables from functions because if they are declared in the function they immediately go out of scope when the function ends. A variables with the same name as a function is also not a good idea.

int sampleCount = 0;            //number of current readings taken. Used to calc average
int total = 0;                  //the cumulative total of several current samples
int count = 0;                  //the number of samples to average
int averageCurrent = 0;         //the result of averaging

I don't know what you are summing and/or averaging but is a signed int data type appropriate for these variables ? Will they ever be that big or negative ?

Your takeSample function sounds like it'll be nothing more than a wrapper for analogRead, so it doesn't really add anything. If it took care of doing the ten readings and averaging them, it would make more sense.

loadReduction is the opposite - looks like it does everything, including cleaning the kitchen sink. It may be possible to keep it clean and simple by giving it some helper functions to abstract away the details.