current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I have a program that accesses a certain io controller using memory mapped io. Currently I store the base address of this controller in a const variable. This leads to a code structure something like this:

gpio.h:

int gpioFunc (unsigned int pin, unsigned int func);

int setGpio (unsigned int pin, unsigned int val);

gpio.c:

#include "gpio.h"
unsigned volatile int* const gpioAddr = (unsigned int*) 0x20200000;

int gpioFunc (unsigned int pin, unsigned int func)
{
    //code that uses the variable
}

int setGpio (unsigned int pin, unsigned int val)
{
    //code that uses the variable
}

main.c:

#include "gpio.h"
void main ()
{
    gpioFunc (16,1);
    setGpio(16,0);
    hlt();
}

I have always been uncertain on the best practices in C as to where to put constants. Does this way make sense, or would it be better practice to put it somewhere else? If so, where?

share|improve this question

2 Answers 2

up vote 2 down vote

I recommend you to declare gpioAddr variable as static. That way the program can only access it from gpio.c and nowhere else (unless you somehow pass the address of the variable to somewhere, which is a poor design, IMO). Making it static also helps the compiler to optimize the variable and just generates code for reading from/writing to the particular memory address.

Also, 0x20200000 is a magic number, you should consider to define it somewhere appropriate.

Lastly, casting as (unsigned volatile int*) may be better for readability, although it makes no difference.

Edit: The appropriate place for defining constants depend on the context. If these definitions are needed somewhere else, you must put them in header file. Defining them in source file is more appropriate otherwise, as unnecessary information would be hidden. Unfortunately, #define preprocessor directives in C are in the global namespace, so appropriate prefixes should be added to the name regardless of the definition place.

share|improve this answer
    
I am aware that I should consider to define it somewhere appropriate. The question was where would be considered appropriate? –  handuel Mar 17 '14 at 19:06
    
It is the source file if it does not needed to know from somewhere else, i.e. including a header file, header file if otherwise. I think source file is more appropriate in this occasion. –  obareey Mar 17 '14 at 19:07
up vote 2 down vote

You should make a small driver layer, so that the main doesn't know about the low level details of what is going on - and here in that .c file that you have implemented the driver in, the constants goes as well.

Then you call your driver and all is good :)

share|improve this answer
    
I think this may what I have currently... how would this be different from my current implementation? Thank you for your help. –  handuel Mar 16 '14 at 12:18
    
No, this is not a driver. This is calling the bare metal layer from your main. You want to make a small driver layer, so that you get calls like "<driver_name>_get<data_pin_name>" and "<driver_name>_init". This also allows you to easily change the setup in the future, when you need to. –  Jesper Bangsholt Mar 16 '14 at 12:20

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.