Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Extraneous FF_PRINTF #55

Closed
carlk3 opened this issue Nov 9, 2023 · 8 comments
Closed

[BUG] Extraneous FF_PRINTF #55

carlk3 opened this issue Nov 9, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@carlk3
Copy link
Contributor

carlk3 commented Nov 9, 2023

Describe the bug
A concise description of what the bug is. If possible, that is the code is not proprietary, please upload the code in a GitHub fork such that we can reproduce the bug.

Every time I mount a "disk", I get a message:

prvDetermineFatType: firstWord 0000FFF8

This comes from:

            /* FAT 32! */
            pxPartition->ucType = FF_T_FAT32;
            #if ( ffconfigFAT_CHECK != 0 )
                if( ( ( ulFirstWord & 0x0FFFFFF8 ) != 0x0FFFFFF8 ) &&
                    ( ( ulFirstWord & 0x0FFFFFF8 ) != 0x0FFFFFF0 ) )
                {
                    /* _HT_
                     * I had an SD-card which worked well in Linux/W32
                     * but FreeRTOS+FAT returned me this error
                     * So for me I left out this check (just issue a warning for now)
                     */
                    FF_PRINTF( "prvDetermineFatType: firstWord %08lX\n", ulFirstWord );
                    xError = FF_ERR_NONE; /* FF_ERR_IOMAN_NOT_FAT_FORMATTED; */
                }
            #endif /* ffconfigFAT_CHECK */
            xError = FF_ERR_NONE;

in Lab-Project-FreeRTOS-FAT\ff_ioman.c.

I don't know what it means.

I have four SD cards from three different vendors, and they all behave this way:

SandDisk Ultra:

Manufacturer ID: 0x3
OEM ID: SD
Product: SC16G
Revision: 8.0
Serial number: 0x9d49ce1d
Manufacturing date: 1/2021

SDHC/SDXC Card: hc_c_size: 30386
Sectors: 31116288
Capacity: 15193 MiB (15931 MB)

PNY Elite:

Manufacturer ID: 0x74
OEM ID: JE
Product: SDU1
Revision: 0.2
Serial number: 0x336e46
Manufacturing date: 1/2019

SDHC/SDXC Card: hc_c_size: 30199
Sectors: 30924800
Capacity: 15100 MiB (15833 MB)

SP 3D Nano:

Manufacturer ID: 0x0
OEM ID:
Product: USD
Revision: 1.0
Serial number: 0x301e
Manufacturing date: 8/2022

SDHC/SDXC Card: hc_c_size: 60947
Sectors: 62410752
Capacity: 30474 MiB (31954 MB)

Target

  • Development board: [e.g. HiFive11 RevB]
  • Instruction Set Architecture: [e.g. RV32IMAC]
  • IDE and version: [e.g. Freedom Studio 4.12.0.2019-08-2]
  • Toolchain and version: [e.g. riscv64-unknown-elf-gcc-8.3.0-2019.08.0]

Raspberry Pi Pico
Lab-Project-FreeRTOS-FAT @ 25129e5

Host

  • Host OS: [e.g. MacOS]
  • Version: [e.g. Mojave 10.14.6]

FreeRTOS-Kernel @ 1b2b090

To Reproduce

  • Use project ... and configure with ...
  • Run on ... and could observe ...

Simply FF_Mount one of these kinds of cards.

Expected behavior
A concise description of what you expected to happen.

I expected no FF_PRINT unless some kind of debug flag was set.

Screenshots
If applicable, add screenshots to help explain your problem.

Wireshark logs
To help us identify the issue and/or reproduce it, please attach Wireshark logs if applicable.

Additional context
Add any other context about the problem here.
e.g. code snippet to reproduce the issue.
e.g. stack trace, memory dump, debugger log, and many etc.

@carlk3 carlk3 added the bug Something isn't working label Nov 9, 2023
@cookpate
Copy link
Member

How were the SD cards formatted? Judging by the sector count, they should be FAT32, but the start-of-partition magic indicates FAT12. The other branches inside #if ( ffconfigFAT_CHECK != 0 ) blocks in this function have an FF_PRINTF, so I disagree that this message is extraneous.

@carlk3
Copy link
Contributor Author

carlk3 commented Nov 14, 2023

They were formatted using this routine:

static FF_Error_t prvPartitionAndFormatDisk(FF_Disk_t *pxDisk) {
    FF_PartitionParameters_t xPartition;
    FF_Error_t xError;

    /* Media cannot be used until it has been partitioned.  In this
    case a single partition is to be created that fills all available space – so
    by clearing the xPartition structure to zero. */
    memset(&xPartition, 0x00, sizeof(xPartition));
    xPartition.ulSectorCount = pxDisk->ulNumberOfSectors;

    switch (xPartition.ulSectorCount) {
        // For alignment:
        // See https://www.partitionwizard.com/help/align-partition.html
        case 15519744:  // 8GB (7.4 GiB) SD card
            xPartition.ulHiddenSectors = 2048;
            break;
        case 31205376:  // 16GB (14.9 GiB) SD card
            xPartition.ulHiddenSectors = 67584;
            break;
        case 62325760:  // 32GB (29.72 GiB) SD card
            xPartition.ulHiddenSectors = 8192;
            break;
        case 124702720:  // 64GB (59.46 GiB) SD card
            xPartition.ulHiddenSectors = 32768;
            break;
        case 249733120:  // 128GB (119.08 GiB) SD card
            xPartition.ulHiddenSectors = 2048;
            break;
    }
    //	xPartition.xPrimaryCount = PRIMARY_PARTITIONS;
    //	xPartition.eSizeType = eSizeIsQuota;

    /* Perform the partitioning. */
    xError = FF_Partition(pxDisk, &xPartition);

    /* Print out the result of the partition operation. */
    FF_PRINTF("FF_Partition: %s\n", FF_GetErrMessage(xError));

    /* Was the disk partitioned successfully? */
    if (FF_isERR(xError) == pdFALSE) {
        /* The disk was partitioned successfully.  Format the first partition.
         */
        xError = FF_Format(pxDisk, 0, pdFALSE, pdFALSE);

        /* Print out the result of the format operation. */
        FF_PRINTF("FF_Format: %s\n", FF_GetErrMessage(xError));
    }

    return xError;
}

It produces this output:

FF_Partition:
FF_ParseExtended: Read sector 4096
No more extended partitions
FF_Format: Secs 31110144 Rsvd 32 Hidden 6144 Root 0 Data 31110112
FF_Format: SecCluster 64 DatSec 31100416 DataClus 485944 pxSet->ulClusterBeginLBA 15872
FF_Format: Clearing entire FAT (2 x 3797 sectors):
FF_Format: Clearing done
FF_Format: Clearing root directory at 00003E00: 64 sectors
FF_Format:

and a subsequent FF_SDDiskMount produces:

FF_ParseExtended: Read sector 4096
No more extended partitions
prvDetermineFatType: firstWord 0000FFF8

@htibosch
Copy link
Contributor

Hi @carlk3,

Expected behavior

I expected no FF_PRINT unless some kind of debug flag was set.

Is that all you're expecting?

The first word of the first cluster contains one of the following values:

 FAT12 : 0x00000FF8
 FAT16 : 0x0000FFF8
 FAT32 : 0x0FFFFFF8

These values should be set as in this list, but they're not used to determine the FAT type. The FAT type is determined only by the cluster count:

FAT12 :       ..  4084
FAT16 : 4085  .. 65524
FAT32 : 65525 .. 

The ff_printf warning is done because the driver finds an irregularity:

  1. The cluster count says: I'm a FAT32
  2. The first word of the first cluster says: I'm a FAT16.

The _HT_ comment says: please treat it as a FAT32 and you will probably safe, saying that Linux would also accept the SD-card. So I think that the warning is well placed.

The question is why FF_Format() has written the confusing information:

    switch( pxSet->ucFATType )
    {
        case FF_T_FAT16:
            FF_putShort( pxSet->pucSectorBuffer, 0, 0xFFF8U ); /* First FAT entry. */
            FF_putShort( pxSet->pucSectorBuffer, 2, 0xFFFFU ); /* RESERVED alloc. */
            break;

        case FF_T_FAT32:
            FF_putLong( pxSet->pucSectorBuffer, 0, 0x0FFFFFF8U ); /* FAT32 FAT sig. */
            ...
    }

The above code is clear: 0x0FFFFFF8 will be written in case a FAT32 will be created.

@htibosch
Copy link
Contributor

Getting deeper into the code, I see a conflicting assignment:

-    ulFirstWord = ( uint32_t ) FF_getShort( pxBuffer->pucBuffer, 0x0000 );
+    ulFirstWord = ( uint32_t ) FF_getLong( pxBuffer->pucBuffer, 0x0000 );

That could very well be the cause of the warning that you see?

cookpate added a commit to cookpate/Lab-Project-FreeRTOS-FAT that referenced this issue Nov 14, 2023
Only the first two bytes were considered when reading the end-of-chain value from the FAT table. Extend to 4 bytes and relax requirement for first 3 bits.
Make requirement for FAT12 more strict because all valid, non-reserved cluster bits must be set.
@carlk3
Copy link
Contributor Author

carlk3 commented Nov 14, 2023

Getting deeper into the code, I see a conflicting assignment:

-    ulFirstWord = ( uint32_t ) FF_getShort( pxBuffer->pucBuffer, 0x0000 );
+    ulFirstWord = ( uint32_t ) FF_getLong( pxBuffer->pucBuffer, 0x0000 );

That could very well be the cause of the warning that you see?

Ah ha! Yes, I do believe you've found it.

By the way, this question has made me take a closer look at the format routine I'm using (#55 (comment), also at https://github.com/carlk3/FreeRTOS-FAT-CLI-for-RPi-Pico/blob/a4d25f75c41d71cecae8245c5a6ce0e6cb450517/src/FreeRTOS%2BFAT%2BCLI/src/ff_utils.c#L27). It has been years since I looked at it, and now I'm questioning some things. Is it OK to leave xPartition.xPrimaryCount at 0? I think my reasoning was that a "primary partition" is one that is bootable, and that I don't plan to boot an OS from this SD card.

Also, re: xPartition.ulHiddenSectors: I made some kind of attempt there at aligning the partition with the SD card "allocation unit":

AU (Allocation Unit): is a physical boundary of the card and consists of one or more blocks and its
size depends on each card. The maximum AU size is defined for memory capacity. Furthermore AU
is the minimal unit in which the card guarantees its performance for devices which complies with
Speed Class Specification. The information about the size and the Speed Class are stored in the
SD Status.

  • SD Card Association; Physical Layer Specification Version 3.01

Using the SD Memory Card Formatter, I see that it puts:

Partition Starting Offset 4,194,304 bytes

I would like my routine to do something similar. The AU_SIZE can be got from SD Status ACMD13 (CMD55 followed with CMD13), and is 4 MiB in this case. If I can determine that, then all I need to do is add a number ulHiddenSectors to get the start of the partition to an allocation unit boundary, right? How do I determine that number? Is it just AU_SIZE (in sectors; in this case 8192) minus some fixed number of sectors (maybe 2)?

@carlk3
Copy link
Contributor Author

carlk3 commented Nov 14, 2023

The AU_SIZE can be got from SD Status ACMD13 (CMD55 followed with CMD13), and is 4 MiB in this case. If I can determine that, then all I need to do is add a number ulHiddenSectors to get the start of the partition to an allocation unit boundary, right? How do I determine that number?

Researching this, I find that in the BIOS parameter block, number of "hidden" sectors is "the number of sectors between the actual start of the physical disc and the start of the volume". I think that means I wouldn't need to subtract anything; I would just use 8192 to get Partition Starting Offset 4,194,304 bytes. Is that what FF_Partition means by

typedef struct _FF_PartitionParameters
{
/...
/* The number of sectors to keep free. */
uint32_t ulHiddenSectors;
/...
} FF_PartitionParameters;

?

@carlk3
Copy link
Contributor Author

carlk3 commented Nov 28, 2023

Answering some of my own questions:

...
By the way, this question has made me take a closer look at the format routine I'm using (#55 (comment), also at https://github.com/carlk3/FreeRTOS-FAT-CLI-for-RPi-Pico/blob/a4d25f75c41d71cecae8245c5a6ce0e6cb450517/src/FreeRTOS%2BFAT%2BCLI/src/ff_utils.c#L27). It has been years since I looked at it, and now I'm questioning some things. Is it OK to leave xPartition.xPrimaryCount at 0? I think my reasoning was that a "primary partition" is one that is bootable, and that I don't plan to boot an OS from this SD card.

I was going by the suggestion at FF_Partition: "A single partition that fills all available space on the media can be created by simply leaving the structure's xSizes and xPrimaryCount members at zero." However, this gives me an extended partition, which I don't think is what I want since I just want one partition. Comments?

Also, re: xPartition.ulHiddenSectors: I made some kind of attempt there at aligning the partition with the SD card "allocation unit" [AU_SIZE, or "segment"]:
...
Using the SD Memory Card Formatter, I see that it puts:

Partition Starting Offset 4,194,304 bytes

I would like my routine to do something similar. The AU_SIZE can be got from SD Status ACMD13 (CMD55 followed with CMD13), and is 4 MiB in this case. If I can determine that, then all I need to do is add a number ulHiddenSectors to get the start of the partition to an allocation unit boundary, right? How do I determine that number? Is it just AU_SIZE (in sectors; in this case 8192) minus some fixed number of sectors (maybe 2)?
...
Researching this, I find that in the BIOS parameter block, number of "hidden" sectors is "the number of sectors between the actual start of the physical disc and the start of the volume". I think that means I wouldn't need to subtract anything; I would just use 8192 to get Partition Starting Offset 4,194,304 bytes.

With one primary partition, this seems to be the case: xPartition.ulHiddenSectors is the number of sectors between the actual start of the physical disc and the start of the volume. With the extended partition I got an Partition Starting Offset at 10240 sectors when I specified xPartition.ulHiddenSectors = 8192.

So, this is my revised routine:

static FF_Error_t prvPartitionAndFormatDisk(FF_Disk_t *pxDisk) {
    FF_PartitionParameters_t xPartition;
    FF_Error_t xError;

    /* Media cannot be used until it has been partitioned.  In this
    case a single partition is to be created that fills all available space – so
    by clearing the xPartition structure to zero. */
    memset(&xPartition, 0x00, sizeof(xPartition));

    /* A single partition that fills all available space on the media 
    can be created by simply leaving the structure's 
    xSizes and xPrimaryCount members at zero.*/    

    xPartition.ulSectorCount = pxDisk->ulNumberOfSectors;
    xPartition.xPrimaryCount = 1; // Instead of using extended partitions

    /* Attempt to align partition to SD card segment */
    size_t au_size_bytes;
    bool ok = sd_allocation_unit(pxDisk->pvTag, &au_size_bytes);
    if (!ok || !au_size_bytes)
        au_size_bytes = 4194304; // Default to 4 MiB
    xPartition.ulHiddenSectors = au_size_bytes / _block_size;

    /* Perform the partitioning. */
    xError = FF_Partition(pxDisk, &xPartition);

    /* Print out the result of the partition operation. */
    FF_PRINTF("FF_Partition: %s\n", FF_GetErrMessage(xError));

    /* Was the disk partitioned successfully? */
    if (FF_isERR(xError) == pdFALSE) {
        /* The disk was partitioned successfully.  Format the first partition.
         */
        xError = FF_FormatDisk(pxDisk, 0, pdFALSE, pdFALSE, "FreeRTOSFAT" );
        /* Print out the result of the format operation. */
        FF_PRINTF("FF_Format: %s\n", FF_GetErrMessage(xError));
    }

    return xError;
}

Skptak pushed a commit that referenced this issue Nov 30, 2023
* Fix FAT32 format check #55
Only the first two bytes were considered when reading the end-of-chain value from the FAT table. Extend to 4 bytes and relax requirement for first 3 bits.
Make requirement for FAT12 more strict because all valid, non-reserved cluster bits must be set.
* Fix inverted FAT12 logic
* Add default heap selection to CMakeListst.txt
---------
Co-authored-by: Hein Tibosch <[email protected]>
Co-authored-by: Soren Ptak <[email protected]>
@Skptak
Copy link
Member

Skptak commented Nov 30, 2023

Marking this issue as closed as #56 has been merged
If you are facing any other problems or issues please feel free to open a post on the FreeRTOS Forums to discuss it!

@Skptak Skptak closed this as completed Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants