Why are these constants declared in different ways?

Question

Here is the code that is declaring a lot of constants:

#define TD_DATA_PID               0  /* control pipe state for td_sie_done */
#define TD_STATUS_PID             2  /* control pipe state for td_sie_done */
#define TD_DONE_PID               4  /* control pipe state for td_sie_done */
#define TD_DELETE_PID           0xee /* pipe command for td_sie_done */
#define TD_SETUP_PID            0xd  /* also used for control pipe state for tf_sie_done */
#define TD_IN_PID               0x9
#define TD_OUT_PID              0x1
#define TD_SOF_PID              0x5
#define TD_PREAMBLE_PID         0xc
#define TD_NAK_PID              0xa
#define TD_STALL_PID            0xe
#define TD_DATA0_PID            0x3
#define TD_DATA1_PID            0xb

#define TD_NORMAL_TIMEOUT         0
#define TD_LONG_TIMEOUT           1

enum TD_CTRL_BITS
{
    TD_CTRL_ARM         = 0x01,
    TD_CTRL_ISOCH       = 0x10,
    TD_CTRL_SYNC_SOF    = 0x20,
    TD_CTRL_DTOGGLE     = 0x40,
    TD_CTRL_PREAMBLE    = 0x80
};


enum TD_STATUS_BITS
{
    TD_STATUS_ACK       = 0x01,
    TD_STATUS_ERROR     = 0x02,
    TD_STATUS_TIMEOUT   = 0x04,
    TD_STATUS_SEQ       = 0x08,
    TD_STATUS_SETUP     = 0x10,
    TD_STATUS_OVERFLOW  = 0x20,
    TD_STATUS_NAK       = 0x40,
    TD_STATUS_STALL     = 0x80
};

This code is for a USB OTG controller, low level embedded systems stuff and thus uses C rather than C++.

All the #define valus are used to assign value to a single one byte variable. The variable is packet ID, thus all the #define constants end with _PID. That is clear.

However, the part of enums is not clear.

Basically, there is a single byte long register that contains TD_CONTROL_BITS and may want to read/write it. Each bit effects behaviour of the hardware. The values given to each constant inside the enum is the bit that corresponds to that function e.g ARM bit is bit 0 and isochronous transfer bit is bit 2 e.t.c. This register is read and also written to.

Similar applies to TD_STATUS_BITS also. Each bit in the byte long status register provides independant information about the status of transfer. Thus, the constants TD_STATUS_ACK, TD_STATUS_ERROR e.t.c can be used as bit masks to read the status register. This register is only read back as it is updated by external hardware and only a value of 0 is written to reset it.

Now my question is, why not use #define constants for everything? What benefit does enum bring in this case? Also, in case of TD_CTRL_BITS we may want to logically OR bits together e.g TD_CTRL_ARM | TD_CTRL_DTOGGLE. I do not think that enums allow that.

Answer 1

This is a pretty standard coding convention and you'll see it many times in your career.

Using an enum is a way of associating a bunch of flags so that you can tell that they belong together. From the code it is clear which are control flags and which are status flags because it says so in the enum label. Some languages even enforce the flag type so that only control flags can be used with control parameters and ditto with status flags. C# does that, I don't think that C does, C++ might.

And more readable code is a good thing because it means fewer bugs and less maintenance overhead

Answer 2

There is no obvious difference in this case. You can either use enumeration constants or defines, there's no obvious right or wrong in this specific case.

Overall though, enumeration constants are dangerous for hardware-related programming, since they are required to always be of type int, which is always signed. You should avoid signed operands when doing things like bitwise arithmetic.

The same problem exists with your defined integer constant such as 0xee - they are also of signed int type. But with defines you have the option to suffix the integer constants, for example 0xeeu. Now you get unsigned int which is much more convenient and safe.

Some examples of where signed int constants would cause bugs:

• if(~TD_CTRL_ARM > 0) Bug, the operand has turned negative.
• TD_STATUS_STALL << 24 Bug, assuming 32 bit int. The code shifts data into the sign bits of an int, invoking undefined behavior.
• ~TD_CTRL_ARM >> n Bug, you may end up with either arithmetic shifts or logical shifts. Code is non-portable and may behave in unexpected ways.
• uint8_t data; ... if((~TD_CTRL_ARM & data) > 0). Bug, the signed left operand means that the implicitly promoted data will remain a signed type. The result may turn out negative even though the right operand was explicitly declared as unsigned. If the left operand had been unsigned int, the right operand would have become unsigned too and the code would have worked as expected.

And so on.

---

Advise/good practice:

• Only use signed types in embedded systems if they are explicitly needed for math etc.
• Never use enum for any form of bit representations or bitwise operations.
• Always suffix integer constants with U/u.
• Be explicit about type size and signedness: use stdint.h whenever possible.
• Be very careful whenever using small integer types in expressions. unsigned char, bool, unsigned short, uint8_t etc. They might get a change in signedness because of integer promotion.
• Actually learn and understand how implicit type promotions work in C. A frightening amount of C programmers don't know this. All C programmers should know how integer promotions, the usual arithmetic conversions (balancing) and type conversion during assignment work. If you don't know about these, you will write subtle and often severe bugs, repeatedly.
• Adopt a professional coding standard which is a safe subset of C, such as MISRA-C, and use static code analysis. Then you'll get all of the above for free.