Code organization for modularity and testability

[krish2487]

Hello folks,
I am back again seeking help from the wisdomous. :-)
This is less of a technical help about testing itself and more of a code organization issue.
I m working on a project with an STM32 using the libopencm3 HAL and ceedling as the test suite.

I m keeping the issue brief and just quoting a small section to prove the point, obviously there are other such modules in the code.

Diving right into the issue, the main function calls a function "Init", which is present in its own module "Init". Init calls a couple of functions from the IO module - IOInit and IOBlink which initialize the GPIO and blink the GPIO in a specific pattern.

The form of IOInit and IOBlink are as follows

Code: [Select]

IOInit(GPIO)
IOBlink(PATTERN)

where GPIO and PATTERN are things like STATUS_LEDS, POWERUP respectively, which can be a enum or a #define depending on how I choose to construct them.

So the first problem is here, what would make sense here in the definition of the arguments for the IOInit and IOBlink?? If I want some argument that is verbose here but gets resolved later in a lower level function call? How would I define the argument?? as uint8_t, enum, #define?

So they can look something like

Code: [Select]

IOInit(GPIO)
{
//Something like this
if (GPIO == STATUS_LED)
{
InitIO(STATUS_LED_PORT, OUTPUT, STATUS_LED);
}
else (GPIO == MODE_IO)
{
//Do something else..
}
...
}

IOBlink(PATTERN)
{
if (PATTERN == POWERUP)
{
//GPIOToggle(Pin, delay, repetitions)
GPIOToggle(STATUS_LED, 500, 4);
}
         else if (PATTERN == ERROR)
          // Do something else
}
Now, IOInit and IOBlink call their lower level functions in the "DriverIO" module which sits right on top of the HAL. The DriverIO has the corresponding
functions - InitIO and GPIOToggle which might look something like this

Code: [Select]

InitIO(PORT, Mode, Pins)
{
rcc_periph_clock_enable(PORT);
gpio_set_mode(PORT, GPIO_MODE_OUTPUT_2_MHZ, Mode, Pins);
//Need to find a way to resolve PORT, Mode and Pins into a HAL compatible format
}

GPIOToggle(Pin, delay, repetitions)
{
for (uint8_t i = 0; i<= repetitions, i++)
{
gpio_toggle(STATUS_LED_PORT, Pin);
delay_ms(delay);
}
}
Now there are a few obvious glaring things that I see can be and should be improved.

All the three layers, Init, IO and DriverIO need a common include that contains the definitions of STATUS_LED_PORT and STATUS_LED - This smells bad.
The idea is to make each layer more modular and self contained, which is defeated if I have to include a BSP.h or something like that in all the three layers.

2a. DriverIO cannot be mocked and tested - since it is sitting right on top the HAL. - one way of addressing this, from what I read here and can understand is to add a "shim layer" or a "wrapper" which basically sits between the DriverIO and HAL and I can simply include only the header to the "wrapper" in DriverIO, mock the wrapper and test DriverIO.

2b. And there is yet another issue with the shim layer. The low level HAL used typedef enums for arguments, which the shim layer and the above layers will not have access to. So how would the translation functions in the shim layer look like ??

For ex - the clock enable function in libopenCM3 HAL looks like this

void rcc_periph_clock_enable(enum rcc_periph_clken clken);
How would it look like in the shim layer?? considering that the DriverIO wont know what the enum rcc_periph_clken looks like??

The overall architecture felt okay while designing.. but it really falling apart during the implementation. In principle, the main only calls the state functions, each state function only call the middle layer functions where the bulk of the control logic is present, the middle layer calls the driver layer functions which sit on the HAL.
I apologize for the long post, but I have spent a substantial amount of time working on this for the design and architecture phase that I am unable to think straight and coherently. I am suffering from some analysis paralysis. It might be a very simply solution but I am unable to see it straight.

Again, I apologize if I am not making much sense.. I have been staring at this for quite some days now and I would assume it wont make much sense (if any) to others. I have tried to make it understandable but that might just be my lack of sleep talking. Please do let me know if there is a specific section that I can explain in a better fashion and make it clear.

Any help is sincerely appreciated.

[ajb]

This is a pretty big question, so there's not really a simple answer.  What you're really asking about is decoupling, which is the practice of ensuring that different code modules or layers are not too intertwined with each other, or in more pragmatic terms, ensuring that it's relatively simple to disconnect one module from another and connect it to something different.  This is important for testing, as you discuss here, as well as code reuse.  There are several ways of doing this, depending on the environment and the overall application structure and what your goals are. 

For the purposes of testing, it's first important to decide what you actually want to test.  If you're trying to test everything above literally just the hardware, that means your test harness needs to emulate the hardware, which is complicated and a lot of work to get to the point where it's accurate enough to be useful, so if something depends on the hardware it's better to just test it ON the hardware. 

It's more sensible to think in terms of the state you want your code to impose on the hardware and abstract at that level for software testing.  For example, instead of creating a test harness that emulates "void rcc_periph_clock_enable(enum rcc_periph_clken clken);", you can wrap that in your own function called "initPeriphClock()" or something, and mock that instead.  Or better yet, you create an "initGPIO()" function and if anything your test mock just ensures that the init function has been called before you call "IOBlink".  The same goes for more complicated peripherals, for example, there's really limited value in trying to test a basic "SPI_Send()" function given how much work that would be, so just mock that function for testing, and then develop the real version of that function with all of the HAL calls and/or register poking as part of your board bringup, preferably in the absence of the more complicated application code that will eventually use those functions, at least to start with. 

More generally, there's kind of no escaping the fact that in order to code against a module you need to have access to the data structures or enums or whatever it needs.  This comes down to deciding the correct boundaries between modules which is a complicated question, and it's also worth not getting too carried away in decoupling.  If you want chunks of this code to eventually become submodules you can reuse in other projects or on other platforms then it makes sense, but if you don't ever see yourself using module A without module B, then there's little point in trying to separate them. 

[krish2487]

@ajb
Thank you!
You hit the nail on the head. I must have a pretty bad job of explaining what I was looking for but you did indeed address it correctly.

My efforts presently are to, indeed, decouple the systems and to be able to test.

Let me see if I can explain it a bit better here. My goals are three fold

1. Decouple the different module at design time to make them, as far as possible, independant of the modules above or below them. To basically abstract out the system into black boxes that can be swapped without much disturbance.

2. Testability - when I say testability, I do not mean the hardware. That is a pretty hard and rather pointless exercise, as i do not know any runtime constraints that will alter the behavior. I was leaning towards testing the coverage, logic, control flows and error handling - which are handled in the layers above the hardware. these aspects do not, as far as I know, depends on the hardware and can be tested fairly easily.

3. Seperation of concerns - This goes hand in hand with the first point. Except going through the motions will help me identify the points where the firmware in presently, tightly coupled so to speak, and then decide how best to attempt to decouple them.

As it stands, it is a personal project of mine, that I am using as an excuse,  to learn how to do this design activity correctly.

The points you make have a lot of merit and give me food for thought. I ll have to see how best to redo the modules to integrate the suggestions you gave!

Thank you again!! :-)

[RoGeorge]

I would avoid hard to read names.  For example, is the IOBlink  about "IO Blink", or maybe it is about "IOB link".

ioBlink, IO_Blink, blinkIO etc. would be easyer to read and to tell its meaning.

[krish2487]

@RoGeorge
Sure, the example was given to state the problem. :-)
In the actual firmware, I have tried to name them with an underscore whereever possible.

[JOEBOBSICLE]

Use C++ interfaces ( see polymorphism and pure virtual functions)

I wouldn't fret about testing vendor supplied SDKs or an RTOS. Focus on testing any algorithms and complex business logic. Otherwise you're spending your life testing and not coding

[MIS42N]

All the three layers, Init, IO and DriverIO need a common include that contains the definitions of STATUS_LED_PORT and STATUS_LED - This smells bad.
The idea is to make each layer more modular and self contained, which is defeated if I have to include a BSP.h or something like that in all the three layers.

To me, this smells good. There should be one definition of something and it be included wherever needed. Define something twice and change it in one place = chaos. If you want to see what is in the .h file when compiling, there will be a directive to make it happen. Wherever anything is shared, its definition should be written once and included. If it looks messy then put in comments to clarify.

[ajb]

Interfaces, polymorphism, etc can certainly make some things easier to program, but for a lot of embedded applications it's easy enough to do what you need in plain old C.

If you have a library that needs to interact with the hardware (or some other aspect of a project-specific implementation) at certain points, it's easy enough to declare the hardware-specific functions in the library but leave them to be defined somewhere else in the project outside of the library.  So for example if you have a protocol library that needs to interact with a UART, you can have the library declare "void protocol_uartWrite(unsigned char data);", "void protocol_uartClearFlags(void);", etc, and then leave those functions undefined in the library so that they can be defined somewhere in the project but outside of the library.  The linker can then resolve all of the calls to "protocol_uartWrite()" in the library to the definition in the project at link time. 

If the application can have multiple instances of a protocol, you can create a protocol instance struct that holds whatever state the protocol instance requires and pass that to the hardware-specific functions, like "void protocol_uartWrite(struct protocol_inst_t * inst, unsigned char data);".  The hardware-specific function can then either choose which UART to access based on what struct it gets passed, or it can look in the protocol instance struct for a reference to the UART. 

For the latter method, it's a common pattern to include a void pointer in the protocol instance struct that the library can ignore but the implementation can use.  The void pointer can point to anything at all, so the library needs no knowledge whatsoever of the implementation data, and the implementation is free to link whatever data it requires and leave the library to hang on to it and pass it over when needed.

Code: [Select]

// in the library header
typedef struct protocol_inst_t {
    unsigned char * buffer;
    uint32_t someCounter;
    uint8_t whatever;
    void * impl_data;
} protocol_inst_t;

// in the file that holds the implementation
typedef struct protocol_impl_t {
    UART_t uart;
    const char * label;
} protocol_impl_t;

protocol_inst_t instance1;

protocol_impl_t instance1_impl = {
    .uart = UART4,
    .label = "instance 1",
};

// somewhere before the application initializes the protocol
instance1.impl_data = &instance1_impl;

// then in any of the hardware-specific implementations
void protocol_uartWrite(struct protocol_inst_t * inst, unsigned char data){
    UART_t uart_to_use = ((protocol_impl_t*)inst->impl_data)->uart;
    uart_to_use->data_reg = data;
}


There's an intermediate method where the library passes only the void pointer to the implementation functions which are statically linked, but this only makes sense if the implementation will never need to access the state held in the protocol struct, so it's of more limited use.

For some applications, especially ones where each instance of a library object will have substantially different behavior, it may be preferable to include function pointers in the protocol instance struct instead of using link-time resolution of a single set of implementation functions.  This incurs a bit of extra overhead via the need to use a function pointer versus a statically linked function, but it adds a lot of flexibility, and you can also use it for things like optional callbacks.

Code: [Select]

// in the library header
typedef struct protocol_inst_t {
    unsigned char * buffer;
    uint32_t someCounter;
    uint8_t whatever;
    void * impl_data; // this still may be useful
    struct {
        void (*uartWrite)(unsigned char data);
        void (*uartClearFlags)(void);
    } impl;
} protocol_inst_t;

// link in drivers before init
protocol_inst_t inst1 = {
    .impl = {
        .uartWrite = inst1_uartWrite,
        .uartClearFlags = inst1_uartClearFlags,
}

// wherever the library needs to make a call to the implementation
inst->impl.uartWrite(someData);
This latter method is what you'll see in complex things like LWIP, where it's expected to be able to run the same protocol on different interfaces that require completely different driver functions.  It starts to look very much like methods more familiar from object-oriented languages, but with sort of reversed syntax.  If you need to be able to instantiate a module then you'll also end up with the module's public functions all taking a pointer to protocol_inst_t so they have access to the state of a given instance, and the declarations end up looking very much like extension methods in those languages that have them, except you can't call them like member functions.

All of these methods are perfectly valid in different scenarios.  In general the first one makes sense when there is only ever going to be one instance of a module's functionality, the second when you might have multiple instances with very similar behavior or implementation needs, and the third when you need the most flexibility.  All of them, when done properly and under the right circumstances, can help write well-organized and well-decoupled code, and all can be used to facilitate testing as well.

[krish2487]

All the three layers, Init, IO and DriverIO need a common include that contains the definitions of STATUS_LED_PORT and STATUS_LED - This smells bad.
The idea is to make each layer more modular and self contained, which is defeated if I have to include a BSP.h or something like that in all the three layers.

To me, this smells good. There should be one definition of something and it be included wherever needed. Define something twice and change it in one place = chaos. If you want to see what is in the .h file when compiling, there will be a directive to make it happen. Wherever anything is shared, its definition should be written once and included. If it looks messy then put in comments to clarify.

Thank you!! That is a new perspective that did not occur to me. :-) . It does make sense to look at it as a single place for the board specific definition instead of being scattered all around.

Interfaces, polymorphism, etc can certainly make some things easier to program, but for a lot of embedded applications it's easy enough to do what you need in plain old C.

If you have a library that needs to interact with the hardware (or some other aspect of a project-specific implementation) at certain points, it's easy enough to declare the hardware-specific functions in the library but leave them to be defined somewhere else in the project outside of the library.  So for example if you have a protocol library that needs to interact with a UART, you can have the library declare "void protocol_uartWrite(unsigned char data);", "void protocol_uartClearFlags(void);", etc, and then leave those functions undefined in the library so that they can be defined somewhere in the project but outside of the library.  The linker can then resolve all of the calls to "protocol_uartWrite()" in the library to the definition in the project at link time. 

If the application can have multiple instances of a protocol, you can create a protocol instance struct that holds whatever state the protocol instance requires and pass that to the hardware-specific functions, like "void protocol_uartWrite(struct protocol_inst_t * inst, unsigned char data);".  The hardware-specific function can then either choose which UART to access based on what struct it gets passed, or it can look in the protocol instance struct for a reference to the UART. 

For the latter method, it's a common pattern to include a void pointer in the protocol instance struct that the library can ignore but the implementation can use.  The void pointer can point to anything at all, so the library needs no knowledge whatsoever of the implementation data, and the implementation is free to link whatever data it requires and leave the library to hang on to it and pass it over when needed.

Code: [Select]

// in the library header
typedef struct protocol_inst_t {
    unsigned char * buffer;
    uint32_t someCounter;
    uint8_t whatever;
    void * impl_data;
} protocol_inst_t;

// in the file that holds the implementation
typedef struct protocol_impl_t {
    UART_t uart;
    const char * label;
} protocol_impl_t;

protocol_inst_t instance1;

protocol_impl_t instance1_impl = {
    .uart = UART4,
    .label = "instance 1",
};

// somewhere before the application initializes the protocol
instance1.impl_data = &instance1_impl;

// then in any of the hardware-specific implementations
void protocol_uartWrite(struct protocol_inst_t * inst, unsigned char data){
    UART_t uart_to_use = ((protocol_impl_t*)inst->impl_data)->uart;
    uart_to_use->data_reg = data;
}


There's an intermediate method where the library passes only the void pointer to the implementation functions which are statically linked, but this only makes sense if the implementation will never need to access the state held in the protocol struct, so it's of more limited use.

For some applications, especially ones where each instance of a library object will have substantially different behavior, it may be preferable to include function pointers in the protocol instance struct instead of using link-time resolution of a single set of implementation functions.  This incurs a bit of extra overhead via the need to use a function pointer versus a statically linked function, but it adds a lot of flexibility, and you can also use it for things like optional callbacks.

Code: [Select]

// in the library header
typedef struct protocol_inst_t {
    unsigned char * buffer;
    uint32_t someCounter;
    uint8_t whatever;
    void * impl_data; // this still may be useful
    struct {
        void (*uartWrite)(unsigned char data);
        void (*uartClearFlags)(void);
    } impl;
} protocol_inst_t;

// link in drivers before init
protocol_inst_t inst1 = {
    .impl = {
        .uartWrite = inst1_uartWrite,
        .uartClearFlags = inst1_uartClearFlags,
}

// wherever the library needs to make a call to the implementation
inst->impl.uartWrite(someData);
This latter method is what you'll see in complex things like LWIP, where it's expected to be able to run the same protocol on different interfaces that require completely different driver functions.  It starts to look very much like methods more familiar from object-oriented languages, but with sort of reversed syntax.  If you need to be able to instantiate a module then you'll also end up with the module's public functions all taking a pointer to protocol_inst_t so they have access to the state of a given instance, and the declarations end up looking very much like extension methods in those languages that have them, except you can't call them like member functions.

All of these methods are perfectly valid in different scenarios.  In general the first one makes sense when there is only ever going to be one instance of a module's functionality, the second when you might have multiple instances with very similar behavior or implementation needs, and the third when you need the most flexibility.  All of them, when done properly and under the right circumstances, can help write well-organized and well-decoupled code, and all can be used to facilitate testing as well.

Thank you @ajb, thats a lot of reading and experimenting materia you have given here. Oddly enough, I was considering opaque pointers a while ago for the implementation but it was way overkill for this project.. atleast for now. Your examples look like they are a little short of being such an implementation or a variation of it.... but much more easier to implement I think. Atleast that is what I am understanding from them.. you are not encapsulating the entire interface and providing a void pointer.. but you are a little shy of it. Please do correct me if my understanding is wrong.