How to avoid globals in EEPROM structs for system settings?

Question

I'm struggling with getting system settings from EEPROM and trying to avoid having them as global variables and wondered what the prevailing wisdom is and if there's an accepted practice and / or elegant solution.

I'm getting system settings stored in an EEPROM via structures with some error checking and the sizeof operator in main.c along the lines of:

// EEPROM data structures
typedef struct system_tag
{
    uint8_t     buzzer_volume;
    uint8_t     led_brightness;
    uint8_t     data_field_3;
} system_t;

typedef struct counters_tag
{
    uint16_t    counter_1;
    uint16_t    counter_2;
    uint16_t    counter_3;
} counters_t;

typedef struct eeprom_tag
{
    system_t    system_data;
    uint8_t     system_crc;
    counters_t  counters;
    uint8_t     counters_crc;
} eeprom_t;

// Default values
static system_t system_data =
{
    .buzzer_volume = 50,
    .led_brightness = 50,
    .data_field_3 = 30
};

static counters_t counter =
{
    .counter_1 = 0,
    .counter_2 = 0,
    .counter_3 = 0
};

// Get system settings data from the EEPROM
if (EEPROM_check_ok(EEPROM_BASE_ADDRESS, sizeof(system_t)))
{
        eeprom_read_block(&system_data, (uint16_t *) EEPROM_BASE_ADDRESS, sizeof(system_t));
}

if (EEPROM_check_ok((EEPROM_BASE_ADDRESS + offsetof(eeprom_t, counters)), sizeof(counters_t)))
{
        eeprom_read_block(&counter, (uint16_t *) EEPROM_BASE_ADDRESS, sizeof(counters_t));
}

I'm then using the system settings data at the moment to set other variables in different modules. E.g. in another file, buzzer.c, I have a module static variable (in an effort to avoid globals) with accessor functions to try and give some encapsulation:

// Current volume setting of the buzzer
static uint8_t volume = 50;

void BUZZER_volume_set(uint8_t new_volume)
{
    volume = new_volume;
}

uint8_t BUZZER_volume_get(void)
{
    return (volume);
}

The problem I feel is I've now got unnecessary duplication of data, as when I pass the buzzer_volume from the system data to set the static volume variable in the buzzer module things could get out of synchronisation. Having the system settings as globals would be easy, but I know this is frowned upon.

Is there a more elegant way of doing this without using globals and still having some encapsulation?

Any suggestions would be gratefully received.

Your question doesn't indicate whether the data you read into those structs is used subsequent to initializing modules like the buzzer. In case they aren't, why not read the data onto the stack (function local), initialize what you must and move on? — Vinci, Apr 20 '19 at 16:43
@Vinci A good point. At the moment the EEPROM structs are used after the other modules' initialisation, but this is something I'm still playing with, certainly in light of Cliffords answer. — Richard Farrar, Apr 21 '19 at 15:52

score 3 · Accepted Answer · answered Apr 19 '19 at 17:20

General advice to avoiding globals (and why you need to do so) are given in Jack Ganssle's excelent article "A Pox on Globals". Essential reading.

One solution is simply to have accessor functions in main.c (or better a separate nvdata.c, to protect it from direct access by anything).

Rather then relying on a single initialisation function being called before any access to the data, I would suggest an "initialise on first use" semantic thus:

const system_t* getSystemData()
{
    static bool initialised = false ;
    if( !initialised )
    {
        eeprom_read_block( &system_data, 
                           (uint16_t*)EEPROM_BASE_ADDRESS, 
                           sizeof(system_t) ) ;
        initialised = true ;
    }

    return &system_data ;
}

void setSystemData( const system_t* new_system_data )
{
    system_data = *new_system_data ;

    eeprom_write_block( &system_data, 
                        (uint16_t*)EEPROM_BASE_ADDRESS, 
                        sizeof(system_t));
}

Then in buzzer.c:

uint8_t BUZZER_volume_get(void)
{
    return getSystemData()->buzzer_volume ;
}

void BUZZER_volume_set( uint8_t new_volume )
{
    system_t new_system_data = *getSystemData() ;
    new_system_data.buzzer_volume = new_volume ;
    setSystemData( &new_system_data ) ;
}

There are some issues with this - such as if your structures are large updating a single member can be expensive. That could be resolved however, but may not be an issue in your application.

Another issue is the writing back to the EEPROM on every change - that may cause unnecessary thrashing of the EEPROM and stall your program for significant periods if you have several sequential changes to the same structure. In that case a simple method is to have a separate commit operation:

void setSystemData( const system_t* new_system_data )
{
    system_data = *new_system_data ;
    system_data_commit_pending = true ;
}

void commitSystemData()
{
    if( system_data_commit_pending )
    {
        eeprom_write_block( &system_data, 
                            (uint16_t*)EEPROM_BASE_ADDRESS, 
                            sizeof(system_t));
    }
}

where you commit the data only when necessary or safe to do so - such as on a controlled shutdown or explicitly selected UI "save settings" operation for example.

A more sophisticated method is to set a timer on change and have the commit function called when the timer expires, each "set" would restart the timer, so the commit would only occur in "quiet" periods. This method is especially suited to a multi-threaded solution.

that's really helpful thanksl. I've read a lot of stuff on this website and have found it very informative, but this is the first time I've had the courage to post a question. Thanks ever so much for taking your time to craft such a detailed response, it's greatly appreciated. — Richard Farrar, Apr 21 '19 at 13:58

How to avoid globals in EEPROM structs for system settings?

1 Answers1