Equivalent to Interfaces in Embedded C / Code organization

Question

I'm developing embedded C code on EFM32 Cortex M3 processors, and after a few months the code is beginning to get crazy... By this I mean that we changed the hardware so we get different versions, on which we changed some components, moved some IOs, have different states for theses at start up...

So I'm trying to clean it up a bit:

Where I had some very big files organised like this:

/*============================================================================*/
/* File       : BSP_gpio.h                                                   */
/* Processor  : EFM32GG980F1024                                              */
/*----------------------------------------------------------------------------*/
/* Description : gpio driver                                                  */
/*----------------------------------------------------------------------------*/

#ifndef BSP_GPIO_H
#define BSP_GPIO_H

#ifdef EXTERN
#undef EXTERN
#endif

#ifdef BSP_GPIO_C
#define EXTERN
#else
#define EXTERN extern
#endif

typedef enum
{
  GPIO_INIT   = 0,
  GPIO_WAKEUP = 1,
  GPIO_SLEEP  = 2,  
} GPIO_MODE;

/*      Definition / conts        */

#define PIO_PIN_HIGH        1
#define PIO_PIN_LOW         0

#ifdef HW_v1

... hundreds of lines...

#elif defined HW_v2

... hundreds of lines...

#endif 
#endif

I try to separate the different versions in separated files and try something like this:

#ifdef HW_v1

    #include "BSP_gpio_HW1.h"

#elif defined HW_v2

    #include "BSP_gpio_HW2.h"

#endif

With the same type of header for each "subfile" (until the enum). The aim is that in every other ".c" file we include the "BSP_gpio.h" and it automatically includes the file corresponding to the hardware used.

The first problem is that the compilation depends on where I include the subfile. For example, I have a function "void BSP_GPIO_set(GPIO_MODE mode)" which uses the enum "GPIO_MODE" and is different in the two hardware versions (because the state of the IOs is not the same on the two hardwares). If I include the subfile before the declaration, it doesn't know the type "GPIO_MODE" and makes a compilation error, even if I #include "BSP_gpio.h" in the subfiles. So I just put this at the end of the file and it works, even if I don't really like it...

The second problem appears when I have a variable declared as extern which I want to use in both subfiles and in others C files. Lets say I put this line just before "#ifdef HW_v1":

EXTERN int numberOfToggles;

The "EXTERN" word is nothing in my "BSP_gpio.c" file as I define BSP_GPIO_C at the beginning of it, and is the keyword extern in every other file where I include "BSP_gpio.h". When I build my project, it compiles but I have a linker error : "Duplicate definitions for numberOfToggles in BSP_gpio.o and BSP_gpio_HW2.o" and I can't find a solution for that.

I'm ready to change the architecture of my project if anyone has a proper solution for that!

Sounds like you should consider building some BSLs(board support libraries) for different versions of your boards. — user3528438, May 05 '15 at 13:47
Take a look at [How do I use `extern` to share variables between source files in C?](http://stackoverflow.com/questions/1433204/how-do-i-use-extern-to-share-variables-between-source-files-in-c/1433387#1433387). It's obvious you know most of what's in there, but it might suggest something new to you, particularly the end section where it discusses '`extern` vs no `extern`' and initializing variables, etc. — Jonathan Leffler, May 05 '15 at 14:19

John Bollinger · Answer 1 · 2015-05-05T14:08:26.360

The first problem is that the compilation depends on where I include the subfile. For example, I have a function "void BSP_GPIO_set(GPIO_MODE mode)" which uses the enum "GPIO_MODE" and is different in the two hardware versions [...]. If I include the subfile before the declaration, it doesn't know the type "GPIO_MODE" and makes a compilation error, even if I #include "BSP_gpio.h" in the subfiles. So I just put this at the end of the file and it works, even if I don't really like it...

I'm not sure how "The first problem is [...]" goes together with "it works". I guess your complaint is about coding style that is falling out from your refactoring.

Anyway, yes, C is sensitive to the order in which declarations appear, thus it matters where you put your #include directives. Personally, I prefer to take the approach that each header file H should #include all the other headers providing macros and declarations that H needs and does not itself provide. With standard guards against multiple inclusion, that eases many (but not all) of the issues around header order and position.

It may be that factoring out even more or finer pieces of your headers is necessary to be able to put #include directives only at the top of each one.

The second problem appears when I have a variable declared as extern which I want to use in both subfiles and in others c files. [...] When I build my project, it compiles but I have a linker error : "Duplicate definitions for numberOfToggles in BSP_gpio.o and BSP_gpio_HW2.o" and I can't find a solution for that.

A global variable may have any number of compatible declarations in any number of compilation units, but it must have exactly one definition. The extern declarations in your header(s) are perfectly fine as long as they do not have initializers for the globals. Each global should then have a non-extern declaration in exactly one .c source file to serve as a definition. Bonus points for using an initializer in the definition.

Yeah you're right , I'm just saying that I don't really like the fact that i'm obliged to put this at the end of my file. Normally I put every include on top of the file, with standard guard and it works, but not in this case. I know that a global variable should have only one definition. The problem is that with this "trick" of defining EXTERN as nothing where BSP_GPIO_C is defined and as the keyword **extern** in other files, plus the "factorisation" of the BSP_gpio.h file, and I cannot find a good code organisation where everything works. — Thib, May 05 '15 at 14:59

score 1 · Answer 2 · edited May 23 '17 at 11:59

1

To me it looks like you don't make a clear distinction between interface and implementation. Is there a good reason to have hardware dependencies in your header file? Is there a good reason to have a GPIO driver as opposed to a function specific driver?

In my opinion, hardware-dependencies should be hidden in the implementation (i.e. .c file) and not made public via header file. For example a Signalisation driver for LEDs. There, the header file could look something like this:

#ifndef DRIVER_SIGNAL_H
#define DRIVER_SIGNAL_H

typedef enum
{
  SIGNAL_STARTED,
  SIGNAL_ERROR,
  SIGNAL_WARNING,
} signal_id_t;

void signal_show(signal_id_t signal_id);

#endif

If you follow this approach you will mostly only have to change the implementations of driver code and not the header files between different hardware versions.

Apart from that, I agree with the answers of Ludin and John Bollinger.

edited May 23 '17 at 11:59

Community

1
1

answered May 05 '15 at 14:51

moktor

1,011
1
8
12

I'm ok with your example. In fact in my subfiles I have the same prototype 'void BSP_GPIO_set(GPIO_MODE mode)'. The difference is in the implementation of this function where, for example, the pin 1 of port A should be high at initialisation for the HW v1, whereas it should be low for the HW v2. Including the good header just redirect the compiler to the good implementation of the function, depending on the hw. So let's say I stay with one prototype 'void BSP_GPIO_set(GPIO_MODE mode)' in the "BSP_gpio.h" file. How do you change the implementation of this function depending on the hardware? – Thib May 05 '15 at 15:17
You could create a copy of the project in your IDE and just add a different source file. – moktor May 05 '15 at 15:41

score 0 · Answer 3 · answered May 05 '15 at 14:33

0

The problem seems to be 100% related to lack of version control and not really to programming.

Instead of going out of your mind with compiler switches, simply change the code as it should be for the latest version of the hardware. As you go, create a "tag" in your version control system for each hardware revision change. Should you ever need to use old hardware, simply go back to the version you used then.

Or create a branch of your current up-to-date files, take the "hardware routing" file from the old tag and dump it among the up-to-date files.

answered May 05 '15 at 14:33

Lundin

195,001
40
254
396

Yes, I guess you're right. Unfortunately I didn't start this program and instead of using versioning they started to used preprocessor directive... I'm just trying to get it working without being so messy. Anyway thank you for your answer. – Thib May 05 '15 at 15:07
1

@ThibautGenet Never too late to fix that. – Lundin May 06 '15 at 06:10

score 0 · Accepted Answer · answered May 05 '15 at 16:28

I modified my code so it get clearer, based on your answer. I have now only one BSP_gpio.h file where I have my definitions and all the prototypes:

/*============================================================================*/
/* File       : BSP_gpio.h                                                       */
/* Processor  : EFM32GG980F1024                                               */
/*----------------------------------------------------------------------------*/
/* Description : gpio driver                                                  */
/*----------------------------------------------------------------------------*/

#ifndef BSP_GPIO_H
#define BSP_GPIO_H

#ifdef EXTERN
#undef EXTERN
#endif

#ifdef BSP_GPIO_C
#define EXTERN
#else
#define EXTERN extern
#endif

typedef enum
{
  GPIO_INIT   = 0,
  GPIO_WAKEUP = 1,
  GPIO_SLEEP  = 2,  
} GPIO_MODE;

/*      Definition / conts        */

#define PIO_PIN_HIGH        1
#define PIO_PIN_LOW         0

/*      Variables                 */

EXTERN UINT8 numberOfToggles;

/*      Functions                 */
void BSP_GPIO_set_interupt(UINT8 port, UINT8 pin, BOOL falling);
void BSP_GPIO_set(GPIO_MODE mode)    

#endif

And I have now three .c files, BSP_gpio.c where I implement everything which is common to both hardwares:

/*============================================================================*/
/* File       : BSP_gpio.c                                                       */
/* Processor  : EFM32GG980F1024                                               */
/*----------------------------------------------------------------------------*/

#define BSP_GPIO_C

/*----------------------------------------------------------------------------*/
/*          Includes                                                          */
/*----------------------------------------------------------------------------*/
#include "BSP_type.h"
#include "BSP_gpio.h"

void BSP_GPIO_set_interupt(UINT8 port, UINT8 pin, BOOL falling)
{
    //implementation
}

, and BSP_gpio_HW1(/2).c files where I implement the void BSP_GPIO_set(GPIO_MODE mode) function surrounded by preprocessor instructions so it is implemented only once:

#ifdef HW_v1(/2)
#include "BSP_gpio.h"

/*============================================================================*/
/*                           BSP_gpio_set                                     */
/*============================================================================*/
void BSP_GPIO_set(GPIO_MODE mode)
{
//Implementation which depends on the hardware
}
#endif

So this answers my first remark/question where I wasn't really satisfied of my code, but I still have the duplicate definitions problem which I don't understand because BSP_GPIO_Cis only defined in the BSP_gpio.c file and not in the BSP_gpio_HW1(/2).c files.

Any idea on that? Again thank you for your help!

I don't know if I should be happy or angry, cause I cleaned and rebuilt my project many (many) times, tried to change options in the linker, rebuild the project etc. And I got each single time the duplicate definition error. As a desperate move, I changed the type to UINT16 and the name of the variable and... magic happened, I got no error... So I came back to UINT8 and the previous name and no error again. I really don't get it, but now it works (I'm working on IAR 6.6, with the Cortex M compiler edition 7.2, which so far was working well). So everything is fine now. Thank you for your help! — Thib, May 06 '15 at 09:23

Equivalent to Interfaces in Embedded C / Code organization

4 Answers4