4
\$\begingroup\$

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 indications to calculate realtime data */
    struct {
        num_t speed; //v = steps/sec
        num_t acceleration; //a = steps/sec.sec
    } cur;

    /** Used for Realtime caclulations and timing
     * Should not be used in other parts of program than realtime */
    struct {
        //tick count to turn off and next tick
        //cannot overflow
        tick_t timeTurnOff;
        tick_t timeNextMove;

        num_t firstDelay; //c0; depending on TIME_RESOLUTION
        num_t minimalDelay; //clast; TIME_RESOLUTION / maxSpeed

        step_t stepsToAccelerate; //nmax
        step_t currentStep; //n
    } rt;

} Motor;

I am doing one little project that I would like to publish soon, but I have few questions about naming:

  • Is having multiple types for each value OK in way I have them? I hope yes, I tried keep it as simple as I knew, but we are arguing with friend about that - he thinks that we should add omfw_ prefix to them and make them more general (eg. make tick_t just 64bits), but I want to make this code run fast on 16bit microcontrollers too and having 32bit timing is working good.
  • Do they have correct naming? I added _t prefix as uintx_t have.
  • Is it OK to have multiple anonymous structs inside main struct? There is lot of data in here and I want to organize them. Should I declare then as normal structs, even when I dont want to serialize them or access as single ones (except MotorSettings)?
  • Do the structs and variables in them have correct naming?
  • And last one. I am not native English speaker, so if there are any errors in text or code, I would appreciate if you let me know. Thanks
\$\endgroup\$
1
5
\$\begingroup\$
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". :)

\$\endgroup\$
5
  • \$\begingroup\$ Many thanks for nice answer. omfw is short name for the library name, but I may try to change it to something better. is 2-letter prefix OK? like om_(sorry I won't say you name, it's reserved :-D) Also I will use full names like target and current as you stated. I will rename rt.currentStep to realtime.step, hope it's ok. The num_t is int when precomputed table is used and double when results are calculated like sqrt(motor.realtime.step)-sqrt(..step-1) so there is no need for int->double conversion. May I ask why ++ on doubles is bad? And thanks for correcting grammar. \$\endgroup\$
    – ttdado
    Feb 11 '17 at 20:52
  • \$\begingroup\$ If you needed fixed-size types, why would you write your own typedefs, instead of simply including stdint.h? Do you still need pre-C99 compatibility? \$\endgroup\$
    – Cody Gray
    Feb 12 '17 at 1:41
  • \$\begingroup\$ @ttdado: ++ on doubles isn't "bad", it just isn't part of the language. \$\endgroup\$ Feb 12 '17 at 22:38
  • \$\begingroup\$ @CodyGray: Not sure if your comment was directed at me or at ttdado's original post, but for the record, I'm assuming that he is getting uint64_t etc. out of stdint.h/inttypes.h. (If he's not, he should be.) But there's still value in using typedefs to distinguish typedef uint32_t tick_t from typedef uint32_t step_t and so on; it makes the code slightly more self-documenting. (It also makes overload resolution in C++ more like a mysterious adventure; but the OP is using C, so that's mostly irrelevant. ;)) \$\endgroup\$ Feb 12 '17 at 22:44
  • \$\begingroup\$ Well, it was a response to your answer, but I admit to having not read very carefully before posting it. In particular, I ignored the code blocks and read only the words, so I didn't see that you actually were using the standard length types. I just saw where you were recommending against using long, etc. in favor of a typedef. Your argument in the comment makes sense, I suppose, but I'm not a big fan of introducing a plethora of typedefs. That just makes the code harder to read and understand, in my opinion, because I have to go look up those typedefs. But maybe that's the C++ talking. \$\endgroup\$
    – Cody Gray
    Feb 13 '17 at 11:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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