Correct way to share global variables between the files

Question

I have a wrapper around neopixel led library. I have named files leds.cpp and leds.h

leds.cpp:

#include "leds.h"

RgbColor nice_purple(153,153,255);
RgbColor orange(255,165,0);
RgbColor bright_purple(204,0,102);
RgbColor cyan(0, colorSaturation, colorSaturation);
RgbColor red(colorSaturation, 0, 0);
RgbColor green(0, colorSaturation, 0);
RgbColor blue(0, 0, colorSaturation);
RgbColor yellow(colorSaturation,colorSaturation,0);
RgbColor white(colorSaturation);
RgbColor black(0);
RgbColor purple(colorSaturation, 0, colorSaturation);
NeoPixelBus<NeoGrbFeature, NeoEsp32Rmt0800KbpsMethod> strip(PixelCount, PixelPin);

void toggle_led_strip(RgbColor colour,int number){
      for(int i=0;i<=number;i++){
        strip.SetPixelColor(0, colour);
        strip.SetPixelColor(1, colour);
        strip.SetPixelColor(2, colour);
        strip.SetPixelColor(3, colour);
        strip.SetPixelColor(4, colour);
        strip.SetPixelColor(5, colour);
        strip.SetPixelColor(6, colour);
        strip.SetPixelColor(7, colour);
        strip.Show();
        delay(500);
        strip.SetPixelColor(0, black);
        strip.SetPixelColor(1, black);
        strip.SetPixelColor(2, black);
        strip.SetPixelColor(3, black);
        strip.SetPixelColor(4, black);
        strip.SetPixelColor(5, black);
        strip.SetPixelColor(6, black);
        strip.SetPixelColor(7, black);
        strip.Show();
        delay(500);
      }
    }


void toggle_led_strip_on(RgbColor colour){
        strip.SetPixelColor(0, colour);
        strip.SetPixelColor(1, colour);
        strip.SetPixelColor(2, colour);
        strip.SetPixelColor(3, colour);
        strip.SetPixelColor(4, colour);
        strip.SetPixelColor(5, colour);
        strip.SetPixelColor(6, colour);
        strip.SetPixelColor(7, colour);
        strip.Show();   
      }


      void toggle_led_strip_off(){
        strip.SetPixelColor(0, black);
        strip.SetPixelColor(1, black);
        strip.SetPixelColor(2, black);
        strip.SetPixelColor(3, black);
        strip.SetPixelColor(4, black);
        strip.SetPixelColor(5, black);
        strip.SetPixelColor(6, black);
        strip.SetPixelColor(7, black);
        strip.Show();     
      }


void clear_LED_strip(int number){
  for (int i=0;i<number;i++)
         strip.SetPixelColor(i, black);
        //delay(1);
          strip.Show();
}

void LED_strip_ON(RgbColor colour,int number){
    for (int i=0;i<number;i++)
        strip.SetPixelColor(i, colour);
        strip.Show();

}

and leds.h:

#ifndef LEDS_H
#define LEDS_H

#include "NeoPixelBus.h"
#include <Adafruit_I2CDevice.h>
#define PixelCount  8 // this example assumes 4 pixels, making it smaller will cause a failure
#define PixelPin  27  // make sure to set this to the correct pin, ignored for Esp8266
#define colorSaturation 250


void toggle_led_strip(RgbColor colour,int number);
void toggle_led_strip_on(RgbColor colour);
void toggle_led_strip_off();
void clear_LED_strip(int number);
void LED_strip_ON(RgbColor colour,int number);


#endif

Now I want to use these functions in multiple other files and there are few questions that I want to clarify.

1. I have modbus.cpp and modbus.h files and one of the functions that I have written in modbus.cpp involves toggling an LED strip I need to include the leds.h in this file to be able to use these functions.

Do I do #include "leds.h" in my modbus.h or modbus.cpp? Is there any difference?

2. All led functions take colour as parameter. The colour variable type is of RgbColor. All the colours are defined in leds.cpp. I must then declare the colours that I will be using in my modbus as extern such as extern RgbColor blue;. And again, my concern whether it should be declared in modbus.h or modbus.cpp.

I appreciate any suggestion. Thanks in advance.

My modbus.cpp involves functions such as:

#include "modbus_custom.h"



enum MODBUS_REGISTERS{
  MAIN_REG = 10,
  QUANTITY_REG = 11,
  SLAVE_ID_REG = 12,
  SERIAL_LEN_REG = 13,
  SERIAL1_REG = 14,
  SERIAL2_REG = 15,
  SERIAL3_REG = 16,
  SERIAL4_REG = 17,
  SERIAL5_REG = 18,
  SERIAL6_REG = 19,
  SERIAL7_REG = 20,
  SERIAL8_REG = 21,
  SERIAL9_REG = 22,
  SERIAL10_REG = 23,
  NUMBER_TO_PICK_REG = 24,
};



ModbusRTU mb; // class object 

void Read_modbus_Pildymas(){
    switch(mb.Hreg(MAIN_REG)){
      case 1:
        Serial.println("Received start of pildymas code");
        mb.Hreg(MAIN_REG, 0);   //ALWAYS REPLY WITH 0 IN THE REGISTER
        break;
      case 2:
        Serial.println("Received 2 code");
        toggle_led_strip(blue, 2);
        
    }
}

and my modbus.h looks like:

#ifndef MODBUS_CUSTOM_H
#define MODBUS_CUSTOM_H

#include <ModbusRTU.h>
#include "definitions.h"
#include "leds.h"



#define RXD2 16
#define TXD2 17
#define DIR 15
#define REG_Pildymas 10
#define REG_Picking 100


extern uint16_t SLAVE_ID;
extern item_inside_struct item_inside;
extern int pildymas_flag;
extern char current_status[20];
extern int clear_data_flag;
extern int number_to_pick;
extern int screen_colour;
extern char DEVICE_ID[10];

extern RgbColor blue;
extern Adafruit_ST7735 tft;

void Read_modbus_Pildymas();
void modbus_respond_pildymas();
void modbus_respond_clear_data();


#endif

Trying Lundin method

In my leds.h I now have created an enum with all possible colours:

typedef enum
{
  nice_purple,
  orange,
  bright_purple,
  cyan,
  red,
  green,
  blue,
  yellow,
  white,
  black,
  purple,
} color_t;

in my leds.cpp:

I have created this static const RgbColor colors[11]:

static const RgbColor colors[11] =
{
  [nice_purple] = {153,153,255},
  [orange]  = {255,165,0},
  [bright_purple]  = {204,0,102},
  [cyan]  = {0, colorSaturation, colorSaturation},
  [red]  = {colorSaturation, 0, 0},
  [green]  = {0, colorSaturation, 0},
  [blue]  = {0, colorSaturation, 0},
  [yellow]  = {colorSaturation,colorSaturation,0},
  [white]  = {colorSaturation},
  [black]  = {0},
  [purple]  = {colorSaturation, 0, colorSaturation},
};



void toggle_led_strip(color_t colour,int number){
      for(int i=0;i<=number;i++){
        strip.SetPixelColor(0, colour);
        strip.SetPixelColor(1, colour);
        strip.SetPixelColor(2, colour);
        strip.SetPixelColor(3, colour);
        strip.SetPixelColor(4, colour);
        strip.SetPixelColor(5, colour);
        strip.SetPixelColor(6, colour);
        strip.SetPixelColor(7, colour);
        strip.Show();
        delay(100);
        strip.SetPixelColor(0, black);
        strip.SetPixelColor(1, black);
        strip.SetPixelColor(2, black);
        strip.SetPixelColor(3, black);
        strip.SetPixelColor(4, black);
        strip.SetPixelColor(5, black);
        strip.SetPixelColor(6, black);
        strip.SetPixelColor(7, black);
        strip.Show();
        delay(100);
      }
    }

I am not fully understanding the structure of this data, could you clarify for me please? From my understanding, I have created an array of 11 RgbColor elements.

[nice_purple] = {153,153,255},

Why are these curly brackets required? { 153,153,255 } and why use brackets here? [ nice_purple ]

In my main.c after including "leds.h"

I call the function as following:

toggle_led_strip(red,1);

I dont fully understand the use of enum in leds.h file. I assume each colour in my enum will have number assigned from 0 to whatever the last colour number is.

And then in my RgbColor array I simply declare the 0th element as {153,153,255}, 1th element as {255,165,0} and so on. Is this understanding correct?

Answer 1

Do I do #include "leds.h" in my modbus.h or modbus.cpp? Is there any difference?

This is a bit subjective and there are different styles.

• One style is that headers which are only used internally should be included from the .c/.cpp file. The benefit is reduced namespace clutter, since the application code including your header doesn't get additional headers included at the same time.
• Another style is that all headers used by a module should be included from the .h file, as a way to document to the application programmer which libraries that module uses. This is self-documenting code of all program dependencies ("coupling") existing in a certain module. Header files are regarded as public and .c files as private, so the application programmer shouldn't have to dig through the .c file to find out (it might not even be available). This method also makes it much easier to track down mysterious linker errors when someone is importing your module but forgetting to link some necessary .c files required.

I subscribe to the "always put all includes in the header" camp personally.

I must then declare the colours that I will be using in my modbus as extern such as extern RgbColor blue;. And again, my concern whether it should be declared in modbus.h or modbus.cpp.

This is completely wrong design. Those color definition in your .cpp file are private to that file (and should be made static). The caller doesn't need and shouldn't need to concern yourself with them. Ask yourself why a Modbus module should need to know how colors are defined... obviously, it shouldn't. It should know about Modbus and nothing else.

Rule of thumb: the presence of extern in C or C++ program is almost always an indication of bad design and spaghetti programming. There are a few rare exceptions, but generally you should never need to use it if your design is sound.

What you should do is to provide the caller with an enum in the leds header such as:

typedef enum
{
  GREEN,
  BLUE,
  ...
} color_t;

Your API should then use this enum:

void toggle_led_strip_on (color_t color);

And then in the leds.cpp file you should have something like (this is pseudo code, I'm not quite sure if you are using C or C++):

static const RgbColor colors[n] =
{
  [GREEN] = {0, colorSaturation, 0},
  [BLUE]  = {0, 0, colorSaturation},
  ...
};

And now you can use the enum passed by the caller as table lookup.

Answer 2

This answer will not discuss the overall interface design (i.e. whether it's good or not). The answer is based on the design already selected by OP.

Do I do #include "leds.h" in my modbus.h or modbus.cpp?

If nothing in modbus.h requires knowledge of the stuff in leds.h, you do the include in modbus.cpp. Otherwise, in modbus.h

Is there any difference?

When you do the include in modbus.h, all users of "modbus" will also know about "leds". If they don't need that information, there is no reason to give it to them.

Further, it may slow down compilation when you include unnecessary files in a compilation unit.

I must then declare the colours that I will be using in my modbus as extern such as extern RgbColor blue;. And again, my concern whether it should be declared in modbus.h or modbus.cpp.

Neither. The extern declations should go into leds.h