3

I am writing code for a PIC 18F45K42 to read .wav files from an HCSD card. I am using MPLAB X IDE v5.20 and XC v2.05. I am using FATFs library to read data off the card.

I can read data off the card and get good results until it comes to combining the 4 bytes representing the .wav file size.

#define BYTE_TO_BINARY_PATTERN "%c%c%c%c%c%c%c%c"
#define BYTE_TO_BINARY(byte)  \
(byte & 0x80 ? '1' : '0'), \
(byte & 0x40 ? '1' : '0'), \
(byte & 0x20 ? '1' : '0'), \
(byte & 0x10 ? '1' : '0'), \
(byte & 0x08 ? '1' : '0'), \
(byte & 0x04 ? '1' : '0'), \
(byte & 0x02 ? '1' : '0'), \
(byte & 0x01 ? '1' : '0') 

UINT actualLength;
UINT br;
char contents[44]; // Buffer
uint32_t file_size;
char result;

printf("Checking media\n\r");
if( SD_SPI_IsMediaPresent() == false)
{
    printf("No media\n\r");
    return;
}

 if (f_mount(&drive,"0:",1) == FR_OK)
    {
    if (f_open(&file, "P.WAV", FA_READ) == FR_OK){

        result = f_read(&file, contents, 44, &br);
        if ( result == FR_OK){
            printf("%c%c%c%c", contents[0],contents[1],contents[2],contents[3]);
            printf("\n\r");
            printf("contents 4-7: %u %u %u %u", contents[4],contents[5],contents[6],contents[7]);
            printf("\n\r");
            printf(""BYTE_TO_BINARY_PATTERN, BYTE_TO_BINARY(contents[7]));
            printf(""BYTE_TO_BINARY_PATTERN, BYTE_TO_BINARY(contents[6]));
            printf(""BYTE_TO_BINARY_PATTERN, BYTE_TO_BINARY(contents[5]));
            printf(""BYTE_TO_BINARY_PATTERN, BYTE_TO_BINARY(contents[4]));
            file_size = (contents[7] << 24) | (contents[6] << 16) | (contents[5] << 8) | contents[4];
            int kb = file_size/1024;
            printf("\n\r");
            printf("Size: %u kB: %u", file_size, kb);
            printf("\n\r");
// . . . OMITTED . . .

I am am printing the binary to reassure myself that the data written from the card is correct. The output I get is

Checking media
RIFF
contents 4-7: 4 122 18 0
00000000000100100111101000000100
Size: 31254 kB: 0

That binary gives me the correct number for the file size, 1210884, so the size I am getting by trying to get by combining the bytes is wrong. I presume this is is because I get the following complier warnings:

main.c:108:42: warning: shift count >= width of type [-Wshift-count-overflow]
            file_size = (contents[7] << 24) | (contents[6] << 16) | (contents[5] << 8) | contents[4];
                                     ^  ~~
main.c:108:64: warning: shift count >= width of type [-Wshift-count-overflow]
            file_size = (contents[7] << 24) | (contents[6] << 16) | (contents[5] << 8) | contents[4];

I have searched on these warnings and tried a number of recommended fixes, but so far I haven't found anything that works for this situation. I hasten to add that my C knowledge is limited, and I am not 100% sure what that bit shift expression is doing. My naive understanding is that the rightmost bit of contents[7] should be shifted to bit 24 of the 32bit integer, contents[6] rightmost bit to bit 16 etc. and somehow, probably because of some kind of data type issue, this is not happening properly. But of course I don't know, and I'm not sure what limitations there are in the Microchip XC compilier.

Short of recommending that I finally take a good course in C, can someone suggest what's going wrong and how to get the right value by assembling the 4 file size bytes properly.

Many thanks for looking.

Lundin
  • 195,001
  • 40
  • 254
  • 396
Peter Wiley
  • 820
  • 7
  • 19
  • Lundin, that's correct. Pure hacking. I didn't mean to post the code with the `(unit32_t)`s included and I was just editing the post to remove them and then there were a couple of fantastically quick responses. – Peter Wiley Aug 23 '19 at 12:49
  • 1
    Eh well, that seems to be the bug, so...? – Lundin Aug 23 '19 at 12:51
  • Doing it this way 'file_size = ((unint32_t) contents[7] << 24) | (uint32_t) contents [6] <<16) |` does get rid of the warning, but, unless I am missing something pretty basic (and I may be) does not return the right number. – Peter Wiley Aug 23 '19 at 13:00

1 Answers1

5

Some basic things:

  • Never use the default types of C when programming embedded systems. Use stdint.h instead. The char type in particular is problematic, since it has implementation-defined signedness.
  • Doing arithmetic on small integer types is difficult on 8 bit MCUs. At a minimum, you must be aware of the Implicit type promotion rules.
  • 32 bit arithmetic is dreadfully slow on a PIC and should be avoided when possible. Probably not an option in this specific case.
  • PIC, being an 8-bitter, has 16 bit int.

With that cleared up, we can note that each of these shifts is wrong:

file_size = (contents[7] << 24) | (contents[6] << 16) | (contents[5] << 8) | contents[4];

contents[i] is of type char (or uint8_t if you fix the code to the appropriate type). This is a small integer type. As explained in the link above, it will get implicitly promoted to int through integer promotion. This is why the compiler is giving you a warning. You need to cast each operand to uint32 before shifting.

file_size = ( ((uint32_t)contents[7] & 0xFF) << 24) | 
            ( ((uint32_t)contents[6] & 0xFF) << 16) | 
            ( ((uint32_t)contents[5] & 0xFF) <<  8) | 
            ( ((uint32_t)contents[4] & 0xFF) <<  0) ;

(The byte masking with 0xFF is necessary in case char is signed and negative, in which case the conversion to uint32_t will "sign extend" the value. TL;DR just stay clear of char as advised above.)

Please note that this sorts data into the uint32_t according to some endian order (from 7 to 4, whatever that means in your case), which might be an issue, particularly if you drop the result in some data communication protocol. In theory, 8 bitters aren't using endianess, though in practice they do as soon as you have things like double accumulators or 16 bit index registers in the core. In such situations, PIC is Little Endian.

Also note that %u is assuming unsigned int, a 16 bit type on your PIC. To print a uint32_t you should use "%"PRIu32 from inttypes.h, though %lu will work too.

Lundin
  • 195,001
  • 40
  • 254
  • 396
  • 1
    And in your solution too - if contents is still an array of `char` this doesn't work if they are signed. – Antti Haapala -- Слава Україні Aug 23 '19 at 13:05
  • @AnttiHaapala Why not, they will get converted to uint32_t "in some implementation-defined manner", which quite likely means the binary representation of the 2's compl. number gets treated as unsigned. In fact I read the MPLAB compiler manual the other day so I'm pretty sure that's exactly what happens. – Lundin Aug 23 '19 at 13:08
  • Oh it's not even implementation-defined, but well-defined. 6.3.1.3/2. – Lundin Aug 23 '19 at 13:10
  • 1
    Signed integers will be converted to unsigned via modulo arithmetic, period. Definitely not what you want here. – Antti Haapala -- Слава Україні Aug 23 '19 at 13:12
  • @AnttiHaapala "Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type." Which is perfectly fine, given that the data inside the `char` was raw binary to begin with. – Lundin Aug 23 '19 at 13:14
  • @Lundin Thanks. I had tried your suggested solution a couple of times and assumed it didn't work, but I was fooling myself. Know why? The difference between `printf("Size: %u kB: %u", file_size, kb);` and `printf("Size: %lu kB: %u", file_size, kb);` The number was ok, it was just being printed out wrong. Oy. – Peter Wiley Aug 23 '19 at 13:23
  • @AnttiHaapala Right, the problem isn't the conversion as such but the lack of masking. Sorry, Friday brain, will fix. – Lundin Aug 23 '19 at 13:27
  • @PeterWiley Ah yeah, int is 16 bit on PIC, so %u wont work. The most correct form is `"Size: %" PRIu32 " kb..."` from inttypes.h. – Lundin Aug 23 '19 at 13:29
  • @PeterWiley Added a note about format specifiers at the end of the answer. – Lundin Aug 23 '19 at 13:35