Could You Criticize my AVR C Code Please?

[phil_jp1]

Hi everyone,

I wrote a firmware for watering system PLC-like controller and would be really glad if you could criticize my code.
My C programming skills is still at the level: "What soldering iron I should buy to program this microcontroller", and I need someone, more experienced embedded programmer to point out my major mistakes, so that I can avoid those mistakes in my future projects. Also in about a year, when my skills would be a bit better, I am going to start a major open-source software-hardware project, so this review will help not only me, but many other people.

I know that this is electronics forum, and many people here is on the same or even lower level, but there is also some people here who have a lot of experience in embedded systems and can really help in this case.

Anyways, here's my project:
https://github.com/jumperone/Watering-Sys-Ctrl

You can watch content of all the files online, or you can download it to your local machine. Well, it's git repository, nothing unusual.

p.s. The controller has 10 discrete optically-isolated 12V DC inputs, two optically-isolated triac drivers with zero-cross detection (MOC3062), one relay output and 5 non-isolated LED outputs.
Currently(prototype) it based around ATmega168 controller, but that might change. And this would be non-commercial one-off device. I also hope to write some tutorial on its hardware.

Thanks in advance,
Phil

[Psi]

I had a quick browse and it looked pretty well written.

Better than some of my code

[TerminalJack505]

I looks really good to me.  I've seen worse.  Even from so-called professional software developers.

You might want to document the MCU's pin assignment and fuse programming. 

Here's an example from one of my projects:

Code: [Select]

//
// This project uses an ATmega 324PV-10AU MCU.
//
// Pin assignments...
//
// Inputs:
//
//    Front Panel Button Pins (Active low.)
//    =====================================
//
//    20 BTN0 (PC1/PCINT17) Time Set.   Internal pull-up resistor enabled.
//    19 BTN1 (PC0/PCINT16) Speed.          "         "      "       "
//    16 BTN2 (PD7/PCINT31) Intensity.      "         "      "       "
//    15 BTN3 (PD6/PCINT30) Down.           "         "      "       "
//    13 BTN4 (PD4/PCINT28) Up.             "         "      "       "
//
//    Rear Panel DIP Switch Pins (Active low.)  Internal pull-up enabled on each.
//    ===========================================================================
//
//    21 CFG0 (PC2/PCINT18) 12 Hour Clock (on) / 24 Hour Clock (off)
//    22 CFG1 (PC3/PCINT19) Dual Intensity Enable (on) / Disable (off)
//    23 CFG2 (PC4/PCINT20) Hourly Chime Enable (on) / Disable (off)
//    24 CFG3 (PC5/PCINT21) Not used at this time.
//
//    Note that when these pins are LOW the switch is in the ON position.
//
//    Other Input Pins
//    ================
//
//    30 AMB (PA7/ADC7) Ambient Light Detector.
//    12 LPM (PD3/INT1) Low Power Mode (High indicates battery operation.)
//
// Outputs:
//
//    High-side Driver Control Pins (Active low.)
//    ===========================================
//
//    42 HSC0 (PB2)  High-side Control Channel 0.
//    41 HSC1 (PB1)  High-side Control Channel 1.
//    36 HSC2 (PA1)  High-side Control Channel 2.
//    44 HSC3 (PB4)  High-side Control Channel 3.
//    43 HSC4 (PB3)  High-side Control Channel 4.
//    37 HSC5 (PA0)  High-side Control Channel 5.
//
//    Low-side Enable Pins (Active low.)
//    ==================================
//
//    35 LS0 (PA2)  Low-Side Enable Channel 0.
//    34 LS1 (PA3)  Low-Side Enable Channel 1.
//    33 LS2 (PA4)  Low-Side Enable Channel 2.
//     9 LS3 (PD0)  Low-Side Enable Channel 3.
//    10 LS4 (PD1)  Low-Side Enable Channel 4.
//    11 LS5 (PD2)  Low-Side Enable Channel 5.
//
// Other Outputs:
//
//    14 BUZZ (PD5/OC1A) Pulse Width Modulated Output.
//
// Other Pins:
//
//     4 Reset No programming required.
//     1 MOSI  (PB5) Set as input w/ pull-up resistor enabled.
//     2 MISO  (PB6) "        "        "        "        "
//     3 SCK   (PB7) "        "        "        "        "
//     6 GND
//    18 GND
//    28 GND
//    39 GND
//     5 VCC
//    17 VCC
//    38 VCC
//    27 AVCC
//    25 TOSC1 Handled by the RTC.
//    26 TOSC2 Handled by the RTC.
//     7 XTAL1 Not connected.
//     8 XTAL2  "       "
//    29 AREF   "       "
//    40 PB0   Set as input w/ pull-up resistor enabled.  Not Connected.
//    32 PA5           "          "       "         "            "
//    31 PA6           "          "       "         "            "
//
//    Note that the ISP pins are soley dedicated to ISP.
//
// Fuse programming:
//
//    lfuse = 0xe2 (11100010)
//
//      Divide clock by 8 bit unprogrammed.
//      Clock output bit unprogrammed.
//      Default startup time.
//      Default clock source (internal RC oscillator.)
//
//    hfuse = 0xd1 (11010001)
//
//      On chip debugger is disabled. *
//      JTAG is disabled.
//      SPI programming is enabled.
//      Watchdog timer alway on feature is off.
//      EEPROM memory is preserved through a chip erase.
//      Default boot size.
//      Select Reset Vector bit (BOOTRST) is unprogrammed.
//
//      * Disabling the on chip debugger is important!  This feature, if
//        enabled, will use a substantial amount of power even while in
//        sleep mode.
//
//    efuse = 0xff (11111111)
//
//      Brown-out detector is disabled.
//

It might be a good idea to stick the relevant component datasheets in a subdirectory of the project as well if they aren't already saved with the schematic/layout files.

[vk6zgo]

On this forum,that's a bit like standing there with a "kick me" sign on your back!

It looks like you got all the nice guys,though!

[McMonster]

Just some strange code conventions. Like function and define naming and things in types.h. Is defining "u08" instead of just using "uint8_t" just for convenience?

Aside from that I haven't found any strange or ugly things in the code. But I'll look closer into this later, the PLC part seems interesting.

[westfw]

It looks nice.  Of course, I haven't read through it carefully enough to see whether all the logic is correct.
Does it fit in the m168?  (It looks the way people want their code to look before they discover that it doesn't fit and/or doesn't run fast enough...)

I did have one complaint.   Some of your command names are awful:

static const char gCmdCmd0[] PROGMEM = "rst";
static const char gCmdCmd1[] PROGMEM = "debug";
static const char gCmdCmd2[] PROGMEM = "tptbos";
static const char gCmdCmd3[] PROGMEM = "wptbos";
static const char gCmdCmd4[] PROGMEM = "lldt";
static const char gCmdCmd5[] PROGMEM = "shft";
static const char gCmdCmd6[] PROGMEM = "err";
static const char gCmdCmd7[] PROGMEM = "rmerr";

[Mechatrommer]

want critics? not actually critics but it depends on your style... looking at https://github.com/jumperone/Watering-Sys-Ctrl/blob/master/io_pin_handler.c
i saw many #ifdef else {} etc etc replicated of the same everywhere, only a suggestion... you can use macro for that. type once change all. if they are slightly different version (even with operators) maybe you can do macro with arguments since it will direct replace the arguments... eg

Code: [Select]

#define func(myop) PORTA myop PORTB
later...
func(|=);
if you still dont know you can use \ as linefeed for multiline macro. i'll check another files when i have time, so far only this. YMMV

You might want to document the MCU's pin assignment and fuse programming. 

i did that but rethinking about it, when changing pin in hardware i need to update all those, at most i've documented 16 pins, even the 8 pins documentation update is a PITA. i'll suggest this when everythings are finalized, ie the products distributed to market, no more code to ammend and ready to go into the drawer for its eternal life.

for now at earlier stage, i'll only document the pin such as this

Code: [Select]

// AVR IO pin requirement
// ----------------------
// 3  IO: clk, fq, data (rst (gnd))
// 2 ADC: "full range" and "fine" frequency adjustment pot
// out -> sina or sinb, comparator
no specific which pin which, just generic design requirement.

edit: or maybe from this

Code: [Select]

//    Front Panel Button Pins (Active low.)
//    =====================================
//    20 BTN0 (PC1/PCINT17) Time Set.   Internal pull-up resistor enabled

to this

Code: [Select]

//    Front Panel Button Pins (Active low.)
//    =====================================
//    MYBTN1 (details refer to myAtTiny13 datasheet)

#define MYBTN1 PORTA
YMMV or YSMV (your style)

[madires]

You're inflating the firmware size by beautifying the source code too much. That also slows down the program. As long as you got enough ressources left it's ok, but it might become a problem.

For example:

Code: [Select]

u08 EEPROM_ReadByte(const u08 *addr)
{
    return eeprom_read_byte(addr);
}

No added functionality, just push/pulling stack and burning CPU cycles.

Another one:

Code: [Select]

bool PLC_GetManualControlStatus(void)
{
    return gManualModeEnabled;
}

A function to return a global variable? Make global variables really global, not just inside the source file where that function is located.

About RAM usage:

Code: [Select]

void PLC_GlueLogic(void)
{
    tTime timeTemp;
   
    /* 10ms timer variables */
    static tTime hundredMsTimer = 0U;
    static tTime hundredMsTimerTmp = 0U;
   
    static u08 statusLedBlinkTimer = 0U;
    /* true after single GlueLogic() cycle, after
manual mode has been enabled */
    static bool manualModeStatus = false;
    /* Used to disable tank pump when level hits LOW to wait until
level will rise a bit */
    static bool tankPumpIsOn = true;
    static bool tankPumpStateOld = false;
    static tTime tankLowTimer = 0U;
    static tTime tankLowTimerTmp = 0U;
    static bool tankIsFull = false;
    /* Used in semi-automatic and SH filling mode */
    static bool wellPumpIsOn = false;
    static bool wellPumpStateOld = false;
    /* If false, wait for water to reach well_hi until
well pump can be switched on */
    static bool wellLevelStatus = false;
    ...

If you run into RAM problems combine bit-sized variables. A "bool" is one byte, but you can fit 8 boolean variables into one byte using bitmasks.

Code: [Select]

#define PUMP_IS_ON         0b0000001
#define PUMP_STATE_OLD     0b0000010
...

uint_8 tankBits;

To set "true": tankBits |= PUMP_IS_ON
To set "false": tankBits &= ~PUMP_IS_ON
To check if the value is true:  tankBits & PUMP_IS_ON

[grumpydoc]

Sorry only had a quick look.

Code: [Select]

You're inflating the firmware size by beautifying the source code too much
For example: [...]

No added functionality, just push/pulling stack and burning CPU cycles.
Largely agree with madires - if you are going to do this then use inline functions (put the function definition in the header marked "static inline") - obviously only do this if your compiler supports inline functions.

A function to return a global variable?

Actually that's good style because it allows the global variable to change, or even disappear in future. It also allows checks, asserts pre/post conditions to be added for debug/development code However if you are going to do this do not have the variable as global - move it into the source file where the function definitions live and make it "static" in that file.

A "bool" is one byte, but you can fit 8 boolean variables into one byte using bitmasks.

True, but, good compilers can pack bitfields in structures so if yours can use this feature. The compiler is much less likely to make mistakes doing this than you are

Other observations:
Don't have variables or functions which are the same as standard library ones, but behave differently - thinking of "errno" here especially. Experienced programmers expect such things to behave, well, as expected

You are a bit cavalier with data types - eg using StrToUInt32 and then casting to an unsigned 8 bit value without checking that the value fits. You might "know" that the string is "0"-"255" but the day someone decides it needs to be "0"-"65535" without checking everywhere it is used might be in for some head scratching.

Code: [Select]

void SetPlcDebugLevel(const u08 level)There's not too much to be gained by making a simple argument to a function const because assignments to it will not affect anything outside the function anyway. It's not wrong though.

Code: [Select]

static tTime hundredMsTimerTmp = 0U;Technically this is a type violation - should be

Code: [Select]

static tTime hundredMsTimerTmp = (tTime)0for uber pedantic correctness

[adam1213]

Its great to see someone looking to improve their code quality

Good points:
 * Use of functions and multiple files with none extremely long.
 * consistent style
 * use of "static" for functions where appropriate to limit scope.

One way in which you can improve your code is to replace comments with functions or better variable names and function names.

for example:

Code: [Select]

/**
  * @brief  Initialize timers
  * @param  None
  * @retval None
  */
static void TimersInit(void)Perhaps you could name your function "InitializeTimers" and remove the entire comment. The comment provides no information that is not in the function deceleration.

Code: [Select]

    /* Reset WDT */
    MCUSR &= ~(1<<WDRF);
    wdt_disable();could be written "resetWatchDogTimer()" with a function to do just that. This will use more lines. However the code will be more maintainable. For example with a separate function to reset the watchdog the lines of code to do it won't get separated. (some IDE's have an extract function button which makes it easy to achieve this)


Commands.c:  there are a few improvements that could be made associated with this file. The file defines a global array of commands which is only used in main and passed into CMDHouseKeeping. As only a single file deals with this array it really should be in that file. Letting main access it as a global variable then pass it off is giving main more information than required which should be avoided. (http://en.wikipedia.org/wiki/Principle_of_Least_Knowledge). I was initially surprised to find CMDHouseKeeping in uary_cmd.c. Perhaps the function could have a name to indicate that it relates to handling uart commands. The current name of the function suggests that commands are  going to do something independent of serial commands coming in but this is not the case.


uart_cmd.c: the code and comments for CMDParseArgs could be improved.
 * why are you passing in the length of a string and using it for loops rather than simply looping until you reach null. (If you are new to c / pointers then its perfectly fine to do it this way)
 *

Code: [Select]

if ( !((IsAlNum(str[i]) != false) || (str[i] == ' ')) ){   * you could improve this a few ways.

Code: [Select]

IsAlNum(str[i]) != false is odd. Why not just write

Code: [Select]

IsAlNum(str[i]).
   * the code would look much nicer with a function. e.g.

Code: [Select]

if (!isCharacterValidForUseWithCommands(str[i]))

Code: [Select]

static int isCharacterValidForUseWithCommands(char c)
{
    return (IsAlNum(c) || c == ' ');
}

Minor point: why is there such a big comment indicating return. If you want to know the locations of returns so much perhaps you could avoid multiple returns.

Part of writing good code is doing it (as you are). If you are interested in a book on it you may find the book "Clean Code: A Handbook of Agile Software Craftsmanship" by Robert C. Martin useful, (I did).

----
as for those pointing out firmware sizes:
 1. a super faster program may take longer to write if you trade off style. Though some codes are likely correct and there probably are ways to save space. Efficiency isn't always needed though is always nice.
 2. it would be interesting to see what sort of things the compiler can optimise / can't. (for example consider improving function names vs having more functions. Having more functions may reduce the output size unless the compiler optimise this)

---
btw a better forum topic would be asking for feedback rather than for criticism.

"I am going to start a major open-source software-hardware project, so this review will help not only me, but many other people."
An important aspect of code quality (especially for projects with other contributors) is determining points likely to change and making them easy to change.

[phil_jp1]

Thanks everyone for your replies!
It's hard to answer to everyone, but I'll try to cover most of the stuff:

You might want to document the MCU's pin assignment and fuse programming.

The pins are largely self-documented in io_map_... files. But more info on active high/low, etc. is a good idea. And fuses, yes, that was on my todo list.

On this forum,that's a bit like standing there with a "kick me" sign on your back!

Better this, than to destroy some expensive equipment or harm someone with my poorly written code.
So in the end, I guess, it was a good idea with a "Kick me" sign 

Just some strange code conventions. Like function and define naming and things in types.h. Is defining "u08" instead of just using "uint8_t" just for convenience?

Yes, it is mostly for convenience and to have a consistent code on different platforms and compilers. Also writing u08 instead of uint8_t even with autocomplete function is always easier. )

But I'll look closer into this later, the PLC part seems interesting.

Well it's pretty far from PLC, but some things are alike.

Does it fit in the m168?  (It looks the way people want their code to look before they discover that it doesn't fit and/or doesn't run fast enough...)

I did have one complaint.   Some of your command names are awful:

Yes, it does fit just fine. I was writing this program with MCU's limited resources in mind.
Currently it takes:

Code: [Select]

Program Memory Usage : 12044 bytes   73.5 % Full
Data Memory Usage : 345 bytes   33.7 % Full
About command names - those might change in near future. I've used such names during debug stage to be able to enter them pretty fast and frequently, like "rst" instead of "reset".

I saw many #ifdef else {} etc etc replicated of the same everywhere, only a suggestion... you can use macro for that. type once change all. if they are slightly different version (even with operators) maybe you can do macro with arguments since it will direct replace the arguments... eg

I haven't thought about passing operators as a parameter to macro function. Got to try that.

if you still dont know you can use \ as linefeed for multiline macro. i'll check another files

Yep, check another files

You're inflating the firmware size by beautifying the source code too much.
For example:

Code: [Select]

u08 EEPROM_ReadByte(const u08 *addr)
{
    return eeprom_read_byte(addr);
}

Well not really beautifying. It's been added for an abstraction. What if I would like to use external EEPROM? But to use inline functions in this case is a good idea, although I haven't used them because the current compiler is smart enough to make it as an inline function. I guess I just need to state than explicitly.

Another one:

Code: [Select]

bool PLC_GetManualControlStatus(void)
{
    return gManualModeEnabled;
}
A function to return a global variable? Make global variables really global, not just inside the source file where that function is located.

The reason is simple. The variable itself is not global, I made it static to restrict its scope.
Why not make that variable global, but have getter, setter and function that returns its value? Because the type of that variable is "home-made" bool, which is not standard type for C lang., therefore to avoid setting that variable to something other than true(1) or false(0) I've used getter and setter.

If you run into RAM problems combine bit-sized variables. A "bool" is one byte, but you can fit 8 boolean variables into one byte using bitmasks.

Thought about using bitfields, but I was not even close to RAM size problems.

Other observations:
Don't have variables or functions which are the same as standard library ones, but behave differently - thinking of "errno" here especially. Experienced programmers expect such things to behave, well, as expected

You are a bit cavalier with data types - eg using StrToUInt32 and then casting to an unsigned 8 bit value without checking that the value fits. You might "know" that the string is "0"-"255" but the day someone decides it needs to be "0"-"65535" without checking everywhere it is used might be in for some head scratching.

Good point about standard functions/variables behavior! I'll change that.
About conversion from 32bit to 8bit, I haven't written StrToUInt8 function yet, and that's why I temporarily substituted it with (u08)StrToUInt32 - which was wrong, of course. I should've at least written "TODO:" comment there, to return and fix that in future.

There's not too much to be gained by making a simple argument to a function const because assignments to it will not affect anything outside the function anyway.

It's a pretty good practice, IMHO, because you can accidentally change that parameter in the beginning of the function and the rest of the function would use that wrong value. It allows to avoid some simple mistakes, like typos.


I think my post is already too long.. )

[alm]

You're inflating the firmware size by beautifying the source code too much. That also slows down the program. As long as you got enough ressources left it's ok, but it might become a problem.

For example:

Code: [Select]

u08 EEPROM_ReadByte(const u08 *addr)
{
    return eeprom_read_byte(addr);
}

No added functionality, just push/pulling stack and burning CPU cycles.

I would expect any decent compiler to optimize this. Although avr-gcc is usually used with -Os (optimize for size), so this may affect inlineing of functions.

Largely agree with madires - if you are going to do this then use inline functions (put the function definition in the header marked "static inline") - obviously only do this if your compiler supports inline functions.

Don't most compilers ignore the inline keyword these days, since compilers are much better at determining what function to inline? Same with register and other ancient keywords from the time when optimizers were much more primitive.

Yes, it is mostly for convenience and to have a consistent code on different platforms and compilers. Also writing u08 instead of uint8_t even with autocomplete function is always easier. )

stdint.h (uint8_t) is standard C99, so should be widely supported. I recommend against renaming standard identifiers for no good reason (saving a few characters), it makes the code harder to read (I would have to dig through headers to figure out that u08 is really uint8_t), maintain and reuse.

[Mechatrommer]

Largely agree with madires - if you are going to do this then use inline functions (put the function definition in the header marked "static inline") - obviously only do this if your compiler supports inline functions.

Don't most compilers ignore the inline keyword these days

well sometime, even for a big bloated function, i would like to make it inline, no matter what, stupid decision if you want to say, but thats my choice. i usually very particular about this for performance critical function which will call 1000s or millions of time of another big function, hence i want it inlined. if compiler decides to ignore the inline keyword, its not a good idea for me. automatic inlining is good, but we should be able to force or disable it.

I haven't thought about passing operators as a parameter to macro function. Got to try that.

i tried and ensure that before i made the post. in avrstudio4 it seems workable, i never did it since i never encounter such conditional switch "topology".

[andyturk]

I haven't thought about passing operators as a parameter to macro function. Got to try that.

If you're getting to the point where you're using the preprocessor for that kind of thing, you really ought to consider stepping up to C++ and using templates (assuming you have a C++ compiler). With templates, you can parameterize a function (or a class) with an operator that you write yourself. It all boils down to the same thing you'd do with the preprocessor, but you get type checking with C++. Here's a reference.

The preprocessor is a tool from the 1970s. It made sense back then because the C compilers of the day weren't very smart. Compilers have come a long way in 30 years and usually do a better job optimizing code than most humans. So if you can write a particular piece of code in straight "C" without using the preprocessor, then that's probably the best way to do it.

If you're writing C++, this extends even to constant definitions. I prefer

Code: [Select]

enum {
  IO_PIN_MASK = 0x0080,
  ...
};
to

Code: [Select]

#define IO_PIN_MASK 0x0080
because with the former, the debugger knows that IO_PIN_MASK is a constant. With the latter, the debugger never sees the definition and you'll have to find it yourself.

[mrflibble]

Overall it looks pretty damn good! And certainly way above the open source hobby project quality out there.

You might want to document the MCU's pin assignment and fuse programming. 

Agreed. You may want to include something that's easier to read than the current io_map stuff. Here's a doxygen comment snippet of a recent project:

Code: [Select]

  *
  * <h2>Pinouts</h2>
  * Pin           | Function        | Color     | Description
  * --------------|-----------------|-----------|----------------
  * PA.2          | USART2.TX       | Yellow    | stdout, printf() to PL2303 @ 115200 baud
  * PA.3          | USART2.RX       | Red       | stdin, currently unused
  * PC.2          | ADC3.IN12       | Green     | TSL1402 AO1, analog output, sampled by ADC3
  * PC.14         | dat out         | Red       | TSL1402 CLK
  * PC.15         | clk out         | Yellow    | TSL1402 SI1
  * 3V            | VCC             | Red       | 3.0 Volt supply for TSL1402 (saturation level = 2.8 Volt typical)
  * 5V            | Optional supply | White     | 5.0 Volt, currently unused
  * GND           | GND             | Black     | Ground
  *
  * \todo 5 Volt operation.
  *

See attachment for the resulting table in the html documentation.

As for the "use an entire function to return a simple value" thing ... I'd keep that. As you already pointed out, abstraction is a good thing. To explicitely inline or not to explicitely inline, that the question be. I would expect even a half-decent compiler to properly inline those even without the "inline/__inline" compiler hints.

I suspect that as the years go by and compilers improve, it becomes more a matter of style and less a matter of technical requirement to add the explicit inline. So I guess that depends 1) on your compiler and 2) your target audience.

However if you are going to do this do not have the variable as global - move it into the source file where the function definitions live and make it "static" in that file.

Yeah, that.

I see in the commit history you already kicked out the u08's in favor of uint8_t. So all things considered, looks good.

And thanks for providing the inspiration to improve the documentation of my own stuff. Just added doxygen + graphviz to the stm32f4 eclipse toolchain yesterday.

[westfw]

move it into the source file where the function definitions live and make it "static" in that file.

Of course, that will do a pretty good job at defeating compile-time inlining as a possible optimization.
This sort of thing is one of the basics of "object oriented programming" ("data hiding", etc), but it is depressingly expensive for the case where the derivation of the value being returned is trivial and the architecture is slow.  On an AVR, I figure that putting the value in an accessor function is a least three times slower than simply using a global variable.  On an architecture where the instruction rate is many times the memory access rate (like your desktop), the results are different.  Probably.

[grumpydoc]

Of course, that will do a pretty good job at defeating compile-time inlining as a possible optimization.

Yes, you're right if you're trying to control access to a piece of global data. In this case it's difficult in C because you have to see the variable as well as the function definition to inline the function and you can't do that everywhere without the variable having global scope.

This is one case where C++ can give you the data encapsulation plus the efficiency. The trouble with C++, of course, is that there is a temptation to use all of its features, not just the useful ones and code readability can go down as a result

[Mechatrommer]

software engineering theories vs. real steel machine. from robot's point of view there is no such thing as encapsulation and inheritance or such things its all about registers and nothing else. the thing is just a compromise between human's ability and limitation and machine's performance, you pick. YSMV.

[phil_jp1]

Don't have variables or functions which are the same as standard library ones, but behave differently - thinking of "errno" here especially. Experienced programmers expect such things to behave, well, as expected

Changed errno to gErrno before I figure out something better.

However if you are going to do this do not have the variable as global - move it into the source file where the function definitions live and make it "static" in that file.

I had only three truly global variables: errno, and two variables for "PLC" inputs and outputs. Other variables are defined as static in source files, but with "g" prefix to differentiate function's local variables from file-scope "global" ones.
Now I rearranged things a bit: gManualModeInputs is gone and gManualModeOutputs from logic.c is "static". So now I only have a single truly global variable - gErrno.

You may want to include something that's easier to read than the current io_map stuff. Here's a doxygen comment snippet of a recent project..

Thanks for the tip! I am not comfortable with doxygen yet, and didn't know that it can generate really nice-looking tables. Already added similar table to io_map_v1.0.h

Just added doxygen + graphviz to the stm32f4 eclipse toolchain yesterday.

Didn't know about graphviz either. Looks like really useful piece of software.

On an AVR, I figure that putting the value in an accessor function is a least three times slower than simply using a global variable.

Don't really care about efficiency in this particular case, because the control loop in this application could be relatively slow and it'll be perfectly fine. But reliability is crucial in this sort of applications where small mistake could damage an expensive equipment.
And usually in critical applications if ten-something extra cycles for accessing a global variable is slowing system too much, it is better to move to a bit faster MCU or increase system clock frequency(if possible). Because we're talking about smaller number of hard-to-find bugs + less time for testing vs. greater development and testing time + a bit lower HW cost.

[mrflibble]

Thanks for the tip! I am not comfortable with doxygen yet, and didn't know that it can generate really nice-looking tables. Already added similar table to io_map_v1.0.h

Yeah, the tables are a neat feature IMO. Just in case, said feature is called markdown and for that you need doxygen 1.8.0 or later. I took a look at your new io_map_v1.0.h, and two minor points ..

1) make sure you use a ---|---|--- instead of a ------ ruler.
2) for best results put a bit of whitespace between the comment '*' and your table text entries.

If you don't do the former you will not get a nice table. ;-) And if you don't do the latter the table entries in your first column are going to start with a '*' for some silly reason. IMO no biggie since I always have the space there anyways since I find it more readable.

Like so:

Code: [Select]

/**
  * <h2>Pinouts</h2>
  * Pin      | Function    | Description
  * ---------|-------------|-------------
  * B5       | IN_1        | Discrete Input #1
  * B4       | IN_2        | Discrete Input #2
  * B3       | IN_3        | Discrete Input #3
  * B1       | IN_4        | Discrete Input #4
  * B2       | IN_5        | Discrete Input #5
  * D7       | IN_6        | Discrete Input #6
  * B0       | IN_7        | Discrete Input #7
  * ... etc


Didn't know about graphviz either. Looks like really useful piece of software.

I especially like the include dependency graph and class hierarchy graphs. In principle all the information is already in your source code, but a simple picture showing it all works a lot faster for me.