Type for each value and naming, anonymous structs inside struct

Here is little snippet from my types.h code:

/**
 * tick_t: is used to store and manipulate with time (tick count)
 * Unsigned, at least 32bits
 * 32bit number gives us 71 minutes resolution
 * 64bit number gives us 584'942 years resolution
 * Use OMFW_TIME_ISELAPSED(targetTime, currentTime) for time comparing
 */
typedef unsigned long tick_t;

/**
 * pos_t: Signed integer which holds position
 * In most cases even 16bit number is enough
 */
typedef int pos_t;

/**
 * num_t: Floating point number used to calculate 
 * Please note, if you want use fixed point or any custom
 * number, you should use operator overoading feature of C++
 * and also implement following functions for it in omfw/math.h
 */
typedef double num_t;

/**
 * step_t: Hold current step value
 * Only increased / decreased by one
 * May be signed, unsigned, floating point
 */
#ifdef OMFW_PRECOMPUTE_TABLE
typedef uint32_t step_t;
#else
typedef num_t step_t;
#endif

and here definition of struct in different file:

```
typedef struct {
num_t acceleration;
num_t speed;
num_t pulseLength;

int id; //index 0,1,2...
int32_t letter; //unicode letter

/** total resolution = motor resolution gearbox /
pos_t motorResolution;
num_t gearbox;

int pinSignal;
int pinDirection;
bool invertSignal;
bool invertDirection;
} MotorSettings;

typedef struct {

/** Motor settings */
MotorSettings set;

/** Maximum achievable limits
Computed from "in" variables /
struct {
num_t speed; //v-max; max steps/sec
num_t acceleration; //a-max; Max accceleration that can be used
} max;

/** Position data stored here */
struct {
pos_t resolution;
pos_t curPosition;
pos_t tarPosition;

//TODO impl - when motor lose step, increment this
pos_t curOffset;

dir_t curDirection;
dir_t tarDirection;
} pos;

/** Calculated and changed by program
* Used as indic

typedef unsigned long tick_t;

we are arguing with friend about that - he thinks that we should [...] make tick_t just 64bits [...] but I want to make this code run fast on 16bit microcontrollers too

Personally, I would never use the long or unsigned long types in production code, because they vary in size even between different 32-bit platforms. On some platforms, int is 32 and long is 32 and long long is 64; on other platforms, int is 32 and long is 64 and long long is 64. Rather than play a guessing game with my program's future portability requirements, I would write explicitly

#if OMFW_USE_32_BIT_TIMES
typedef uint32_t tick_t;
#else
typedef uint64_t tick_t;
#endif // OMFW_USE_32_BIT_TIMES

This is much more typing, but it conveys the intended semantics unambiguously: that is, by default I want 64-bit tick_t, but I also want to give the builder the option of using 32-bit tick_t in case it's helpful to him. In fact, this lets the builder pass -DOMFW_USE_32_BIT_TIMES on his 32-bit platform, or -DOMFW_USE_32_BIT_TIMES=0 on his 16-bit platform if he really wants to.


Do they have correct naming? I added _t prefix as uintx_t have.

FYI, that's a _t suffix (appended), not a prefix (prepended).

This is fine; but you are courting collisions with the standard library. For example, the standard library already defines time_t and off_t; if you get in the habit of using the exact same naming conventions as the standard library, then you might one day run into trouble when you need a name for an entity representing a "time" type or an "offset" type.

Therefore I would recommend at least namespacing the identifiers as omfw_time_t, omfw_offset_t, etc. (Also notice that I wrote out "offset" instead of "off", because it's clearer and the compiler certainly won't care that we're being a little more verbose.)

int pinSignal;
int pinDirection;

Surely pinDirection should be a bool. Also, in all cases where you're using int, I suggest being explicit about the expected range of the value: is an int16_t good enough? Could we get away with int8_t? I would definitely use unadorned int for computations, but here you're defining a storage format, which means it would be nice to clarify exactly how many bytes each field is expected to have.


Is it OK to have multiple anonymous structs inside main struct?

Yes. But I do think it's confusing to sometimes have a multi-level access path and sometimes not. For example, you access one field as

myMotor.cur.speed

but another as

myMotor.pos.curDirection

Why is it not myMotor.cur.direction? And why is myMotor.rt.currentStep not myMotor.cur.step or at least myMotor.rt.curStep, given that you're abbreviating current to cur on every other line of the program?

(Again, personally I would write out current and especially target; abbreviating them as cur and tar is more confusing than helpful IMHO. With current you have to worry about whether the proper abbreviation is cur or curr (I've seen both, particularly since curr is the same number of characters as prev and next and last). I've never seen target abbreviated as tar (although I have seen targ and tgt).

/**
 * num_t: Floating point number used to calculate 
 * Please note, if you want [to] use fixed point or any custom
 * number, you should use operator over[l]oading feature of C++
 * and also implement following functions for it in omfw/math.h
 */
typedef double num_t;

You don't explain which functions are "the following functions." Also, it might be more appropriate to write this as

#ifndef OMFW_NUM_T
#define OMFW_NUM_T double
#endif // OMFW_NUM_T

typedef OMFW_NUM_T num_t;

so that it can be specified by the user at build time as e.g. -DOMFW_NUM_T=float.

/**
 * step_t: Hold current step value
 * Only increased / decreased by one
 * May be signed, unsigned, floating point
 */
#ifdef OMFW_PRECOMPUTE_TABLE
typedef uint32_t step_t;
#else
typedef num_t step_t;
#endif

I'm suspicious of this code. Why is it okay for the step number to be a double (which is what num_t is)? At the very least, you should replace the word num_t with double here for clarity. But also, maintaining this code is going to be a nightmare without a very thorough test suite. For example, you'll have to be very careful never to use ++ or % with a step number, because those don't work with double.

Why not just let the step number be int and be done with it?

Incidentally, I don't know what the initialism omfw_ stands for, but you should be aware that to a native English speaker it bears an amusing resemblance to "omfg". :)

StackExchange Code Review Q#155103, answer score: 5