0

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?

  1. 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?

Improve this question
16
  • Principally: Include files as local as possible, so at first consider the .cpp file. If the LED types, functions or variables are part of the public interface of your modbus 'library', though, then it should be included in the header. However: Modbus is independent from any LED stuff, the LED are controlled via some communication channel anyway. So would rather invert the relationship: Using modbus from LED, i. e. include modbus.h in led.cpp. That improves reusability of your modbus implementation. Commented Jun 23, 2021 at 6:57
  • Is modbus.h a public header or just used by modbus.cpp. If it is not a public header or if the led defines should not be exposed to users of the modbus component then led.h should be included only in modbus.cpp (and vice versa). Commented Jun 23, 2021 at 6:58
  • In C++ I'd even abstract the modbus away and hide behind a common interface such that LED could use modbus, ethernet, serial or whatever else you have available. That can be done with C as well, just is less convenient to achieve, so you maybe don't want to go that far... Commented Jun 23, 2021 at 7:00
  • Can you clarify what do you mean public header? modbus.h and modbus.cpp are the files I have created for the modbus wrapper that I have also wrote. I have all my modbus_read, modbus_write functions there as well as some extra modbus stuff. When certain modbus data is received, I want to blink and led hence I need the led funcitonality in my modbus file Commented Jun 23, 2021 at 7:01
  • @kaylum 'and vice versa' – I'd recommend only vice versa, see my first comment. If modbus is aware of LED, it is like TCP is aware of HTTP... Commented Jun 23, 2021 at 7:03

2 Answers 2

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

8 Comments

@Aconcagua And the code is C++. Hence my confusion. Anyway, the specific syntax isn't important here, correct program design is language agnostic.
Oh, right, there's Serial.println(...), which I overlooked. Looks like Arduino, though Serial could be a variable or constant of some struct with a function pointer named println, so that could be C as well...
@Aconcagua C doesn't have constructors or templates...
You won ;) – While the mb lacking an initialiser would yet compile as C, but run into UB for the function pointer not having been set, the template is definite. Your eyes are sharper than mine, I needed to do a text search for < to discover ;)
Bah, I'm so blind :( 'my modbus.cpp' in the text right under my nose. I edited the question tags, it's just too obvious.
|
0

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

Improve this answer

8 Comments

Thank you sir! Very helpful. Neither. The extern declations should go into leds.h. Now that makes sense to me all of a sudden.. Instead of doing extern rgbcolor blue; in every source file I want to use this colour, I can just do extern rgbcolor blue in my leds.h and when i include it in my other .cpp files, it will automatically be there...
"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." This is subjective though. While they may not need that info in their translation unit's global namespace, the application programmer does need to know what dependencies a certain module comes with.
@Lundin Considering a static library including all headers might reveal internals that aren't necessary nor desirable to know about. Would you then write separate headers just exposing the really public interface?
@Aconcagua If those headers are properly written without spaghetti globals and properly named identifiers, there's no harm done. The argument that wins the day for me personally is that when you include some 3rd part lib in your project, build and get "mysterious linker error: 1", you really want to know which .c files that are missing and you were supposed to link. Now if you don't have access to the .c files where the includes are (because they are actually lib, dll, obj or whatever), then no can do. If the includes were in the public header, well then you'd find out in seconds.
@Lundin Well, one would know how the corresponding source file was named. But the more important information is still lacking: Into which library has this source file been compiled into? Properly documenting the dependencies of that library might be of greater value in this case.
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.