organizing code into modular, self contained blocks

Question

I am 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

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

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

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.

Answer 1

So the first problem is here, what would make sense here in the definition of the arguments for the IOInit and IOBlink??

For IOInit, it will need port, pin(s) and mask(s). Care must be taken if you will allow other unrelated functionality on the same port, or if the driver has exclusive control over it. Updating GPIO registers from multiple unrelated places in a program is a bad idea, since that can lead to race conditions and other such oddities.

In my experience, writing abstraction layers over simple GPIO tends to cause more harm than good.

In your case the GPIO driver should actually be a "LED blinking application module" which is somewhat higher level than GPIO, since it will also have to utilize timer and/or PWM hardware peripherals. So consider naming this something else.

Using an enum for different blink patterns is a good idea. This enum can then in turn be used by the driver internally as an index to a look-up table etc, where timing and patterns are specified. Unless you want the timer periods to be variable for some reason, then those will need to be passed along as parameters too.

Now, IOInit and IOBlink call their lower level functions in the "DriverIO" module which sits right on top of the HAL.

Why!? You then have some 3-4 abstraction layers for something utterly simple that needs no abstraction to begin with. This is very bad design!

1 layer makes sense, the one handling the LED patterns. The rest of the useless bloatware middle layers must go: they only take up resources and act as a source for bugs. You should access the registers directly from the LED pattern module. The ST so-called "HAL library" is harmful in general and should be avoided. This means that you have to redesign most of this.

DriverIO cannot be mocked and tested

Design your programs in the way that makes most sense in terms of functionality, readability, maintainability and speed. Do not design your programs to suit some TDD buzzword test suite template. You can by all means keep testing in mind when you design the code, but you shouldn't let it rule the design.

For example, rather than mocking, you could design a specific number of test functions that are delivered with the driver and have direct access to private members of the driver, but is only linked in debug build. This allows for more in-depth testing than mocking, which is really just a "black box" test.

Notably, flashing a LED after some pattern is utterly simple to test, you don't really need function mocking or anything, just the mandatory oscilloscope. Which is also the correct tool for benchmarking all embedded firmware.

---

Regarding TDD and testing in general, it is supposed to go like this:

Specification -> Program design -> Program implementation including tests -> Testing.

So for each requirement there is a module in the program, and for each such module you have a test. The test is there to see if the code lives up to the requirement it was written for, so that the product lives up to the specified functionality.

This means that you must have a specification to begin with. The program design should not be invented out of the blue. You should not implement functionality that you have no use for. Tests should test the requirements of the product, not just test random stuff in general or to suit some manner of "test suite".

But please note that program design is hard, it takes lots of experience and can only be studied to a certain extent, the rest you have to learn by doing. It is very normal that you've done a design, written the program, then in the end realize that you could have done the design in much better ways.