C cast: Is this expression coded correctly?

Question

I've inherited some code that uses the elm-chaN fatfs. It creates Directories and files with either missing or wrong date-time stamps. I've traced the problem to what appears to be an incorrect cast.

I changed the cast and things seem to work. But I would like confirmation from more experienced programmers that the original cast was incorrectly constructed (or a compiler issue)

Below is the original code: the curTime stuct assigns uint16_t to "Year". The other members are uint8_t types. They need to be packed into 32b word (DWORD) following the DOS date/time bitmap format.

{
    DWORD tmr;

    /* Pack date and time into a DWORD variable */
    Calendar curTime = sdInterface->sdGetRTCTime();

    tmr = ((DWORD)(curTime.Year-1980)<<25)
            | ((DWORD)(curTime.Month) << 21)
            | ((DWORD)(curTime.DayOfMonth) << 16)
            | (curTime.Hours << 11)
            | (curTime.Minutes << 5)
            | (curTime.Seconds >> 1); //modified to truncate into two second intervals - jn

    return tmr;
}

Below is the modified code: I explicitly cast the Hours, Minutes and seconds to DWORD type. It is puzzling as to why the original author would cast Month, DayOfMonth but not the other uint8_t types.

DWORD get_fattime(void)
{
    DWORD tmr;

    /* Pack date and time into a DWORD variable */
    Calendar curTime = sdInterface->sdGetRTCTime();

    tmr = ((DWORD)(curTime.Year-1980)<<25)
            | ((DWORD)(curTime.Month) << 21)
            | ((DWORD)(curTime.DayOfMonth) << 16)
            | ((DWORD)(curTime.Hours) << 11)
            | ((DWORD)(curTime.Minutes) << 5)
            | ((DWORD)(curTime.Seconds) >> 1); //modified to truncate into two second intervals - jn

    return tmr;
}

the code seems to work. Would like a sanity check from more experienced programmers.

I am updating this post to provide the requested information: first is the struct Calendar curTime.

//
//! \brief Used in the RTC_C_initCalendar() function as the CalendarTime
//! parameter.
//
//*****************************************************************************
typedef struct Calendar {
    //! Seconds of minute between 0-59
    uint8_t Seconds;
    //! Minutes of hour between 0-59
    uint8_t Minutes;
    //! Hour of day between 0-23
    uint8_t Hours;
    //! Day of week between 0-6
    uint8_t DayOfWeek;
    //! Day of month between 1-31
    uint8_t DayOfMonth;
    //! Month between 1-12
    uint8_t Month;
    //! Year between 0-4095
    uint16_t Year;
} Calendar;

The image below shows the directory of the SD Card running the original code (without the DWORD cast on the Hour, Minute and Time uint8_t objects). Notice the missing date time stamp on two of the files. The directories are also missing their date time stamps.

And the last two images below shows the results of the code with DWORD applied to the uint9_t objects. Both directories and files now have date time stamps.

From the comments I received so far, I am leaning towards this being a compiler error. The code was developed on earlier version of the compiler. This is a new compiler CCS v9.

Answer 1

[This is not really an answer, but it's too elaborate for a comment.]

Please try this program:

#include <stdio.h>

int main()
{
    uint8_t b = 9;
    DWORD w1 = b << 6;
    DWORD w2 = (DWORD)b << 6;
    printf("%d %d\n", (int)w1, (int)w2);
}

The expected output of this program is

576 576

Shifting right by 6 bits is equivalent to multiplying by 64, and 9 × 64 = 576, so this makes sense.

If, on the other hand, you get the output

64 576

I believe this indicates a bug in your compiler. 64 is what you get if you take 9 and shift it left by 6 bits within an 8-bit field, meaning that you lose a bit off the left.

That (almost) makes sense, and it's what the answer posted by @jaz_n is getting at. However, your compiler is not supposed to shift anything left within an 8-bit field. When you write

x << 6

where x is type uint8_t, the first thing the compiler is supposed to do is promote x to a full-width integer type, then shift it left by 6 bits, in a field that's as wide as type int on your machine (which will obviously be either 16 or 32 bits, typically).

This explains why the three "extra" casts to DWORD should not be necessary. The fact that you had to add them suggests that your compiler may have a problem. If your compiler generates code that prints 64 576 for my test, this is additional evidence that your compiler is wrong.

Answer 2

C compilers have some option to compile in different mode(16-bit, 32-bit, 64-bit). From the code you provided above, it looks DWORD stands for 32-bit.

The original code may work fine if it is compiled in 16-bit mode, because in calculation 16-bit lengths is used by default without specific cast.

Please also keep in mind while you are migrating your code from one platform to another, or change the compile option from 16-bit to 32-bit or 64-bit some of your code may not work properly.

Answer 3

Minutes and Hours are 8 bit entities (uint8_t). In the first code snippet, the compiler did exactly what it was suppose to: shift Minutes and Hours to the left some number of bits. Those bits were lost and the residual bits were OR'd with the 32b DWORD. The bitmask failed because the most significant bits of Hours and Minutes were thrown away. In the second code snippet, Minutes and Hours are first cast into a 32b entity, now the shifted bits are preserved and the bitmask succeeded.

Seconds is shifted to the right with the intent to increase the granularity of the field (i.e. 2 Seconds per count) so technically you did not need to cast Seconds into a 32b entity.