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.

Sign up for GitHub

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

Jump to bottom

Added API allowing user to choose settings at runtime - replacement of PR#49 #51

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Added API allowing user to choose settings at runtime - replacement of PR#49 #51

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cec9a7b
Adding in FF_SDDiskInitWithSettings optional initialization to suppor…
phelter Aug 8, 2023
d4d2c9b
FAT-#50: Fixing duplicate definition of st_atime, st_mtime, st_ctime …
phelter Aug 8, 2023
365e29b
Fixing formatting with diff provided using uncrustify.
phelter Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ff_stdio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1137,9 +1137,9 @@ int ff_stat( const char * pcName,

#if ( ffconfigTIME_SUPPORT == 1 )
{
pxStatBuffer->st_atime = prvFileTime( &( xDirEntry.xAccessedTime ) );
pxStatBuffer->st_mtime = prvFileTime( &( xDirEntry.xModifiedTime ) );
pxStatBuffer->st_ctime = prvFileTime( &( xDirEntry.xCreateTime ) );
pxStatBuffer->ff_atime = prvFileTime( &( xDirEntry.xAccessedTime ) );
pxStatBuffer->ff_mtime = prvFileTime( &( xDirEntry.xModifiedTime ) );
pxStatBuffer->ff_ctime = prvFileTime( &( xDirEntry.xCreateTime ) );
Comment on lines +1140 to +1142
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Would this break existing users code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please have a look at my earlier response?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only break existing user code if they are accessing the values inside the pxStatBuffer in their code in some way associated with st_[a|m|c]time. It can be mitigated by providing users a description of the breaking change in this Lab-Project based repo.

The other option is to possibly investigate replacing the entire struct for Linux based examples with the linux stat structure instead of using home-grown version, and ensuring compatibility between the FF_Stat and linux stat is maintained - see #50 (comment).

}
#endif
}
Expand Down
6 changes: 3 additions & 3 deletions include/ff_stdio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,9 @@

/* Note time_t must be used here otherwise will have bugs when 2032 (uint32_t second clock rolls over) */
#if ( ffconfigTIME_SUPPORT == 1 )
time_t st_atime;
time_t st_mtime;
time_t st_ctime;
time_t ff_atime;
time_t ff_mtime;
time_t ff_ctime;
Comment on lines +92 to +94
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a backward compatible change? An in what happens to users using these fields? Or are these internal?

Copy link
Contributor

@phelter phelter Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not backwards compatible - This is to fix #50 . What is allowed and not allowed in terms of breaking compatibility for Lab projects?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phelter asked:

What is allowed and not allowed in terms of breaking compatibility for Lab projects?

There is not an official guideline about "breaking compatibility". I'd like to break as little as possible, and avoid complex or tricky constructions like the use of #undef.

I just compiled a big Xilinx project, which contains an FTP-server and a compression module, all with ffconfigTIME_SUPPORT defined.

I must say that the code rarely refers to the one of the 3 fields (st_[amc]time).

Another way to void compilation errors would be :

#if( DOWNWARD_COMPATIBLE )
	#define st_atime    ff_atime
	#define st_mtime    ff_mtime
	#define st_ctime    ff_ctime
#endif

but that might cause problems in applications that also use stdio.h

Note that the compiler error is easy to understand:

gzip.cpp:746: error: 'struct stat_type' has no member named 'st_mtime'

Can you add some comments about the compatibility break?
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If compatibility with many different flavors of environments what I'd suggest is:

  1. Avoid #defines in your code - if you must then prefix them with something like FREERTOS_PLUS_FAT_<NAME> to ensure there is a low risk of them conflicting with other #defines in other libraries.
  2. Own your naming convention and typenames for your project. In this instance FF_STAT shares the linux names and portion of that struct. If you intend on supporting Linux AND other libraries - need to own the structure. FWIW this struct also doesn't meet the FreeRTOS naming criteria of adding an x prefix.
  3. Do not copy for the sake of copying other environment's structures. If you intend your API to be used as a wrapper for others then own the structure definition and do the translation to the other platforms.

@htibosch - are you referring to adding comments in the code about compatibility issues? If so I'd suggest the code is not the place for that, but release notes between versions is a better place to provide that type of information - i.e. provide search/replace information on compatibility breaks. Happy to provide a sed script if you like. Something like:

sed -e 's/st_(a|m|c)time/ff_$1time/g' *.c

#endif /* ffconfigTIME_SUPPORT */
} FF_Stat_t;

Expand Down
8 changes: 8 additions & 0 deletions portable/ATSAM4E/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,14 @@ void FF_SDDiskFlush( FF_Disk_t * pxDisk )
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

/* Initialise the SDIO driver and mount an SD card */
FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
Expand Down
8 changes: 8 additions & 0 deletions portable/ATSAM4E/ff_sddisk_r.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ void FF_SDDiskFlush( FF_Disk_t * pxDisk )
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

/* Initialise the SDIO driver and mount an SD card */
FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
Expand Down
8 changes: 8 additions & 0 deletions portable/STM32F4xx/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,14 @@ static void vGPIO_SD_Init( SD_HandleTypeDef * xSDHandle )
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
FF_Error_t xFFError;
Expand Down
8 changes: 8 additions & 0 deletions portable/STM32F7xx/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,14 @@ static void vGPIO_SD_Init( SD_HandleTypeDef * xSDHandle )
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
FF_Error_t xFFError;
Expand Down
26 changes: 12 additions & 14 deletions portable/Zynq.2019.3/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,20 +432,18 @@ static CacheMemoryInfo_t * pucGetSDIOCacheMemory( BaseType_t xPartition )
}
/*-----------------------------------------------------------*/

/* Initialise the SDIO driver and mount an SD card */
BaseType_t xMountFailIgnore = 0;
FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
const FFInitSettings_t defaultSettings = {};

/* _HT_ : the function FF_SDDiskInit() used to mount partition-0.
* It would be nice if it has a parameter indicating the partition
* number.
* As for now, the partion can be set with a global variable 'xDiskPartition'.
*/
BaseType_t xDiskPartition = 0;
return FF_SDDiskInitWithSettings( pcName, &defaultSettings );
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInit( const char * pcName )
/* Initialise the SDIO driver and mount an SD card */
FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
FF_Error_t xFFError;
BaseType_t xPartitionNumber = xDiskPartition;
FF_CreationParameters_t xParameters;
FF_Disk_t * pxDisk = NULL;
CacheMemoryInfo_t * pxCacheMem = NULL;
Expand All @@ -456,13 +454,13 @@ FF_Disk_t * FF_SDDiskInit( const char * pcName )

do
{
if( pucGetSDIOCacheMemory( xPartitionNumber ) == NULL )
if( pucGetSDIOCacheMemory( pxSettings->xDiskPartition ) == NULL )
{
FF_PRINTF( "FF_SDDiskInit: Cached memory failed\n" );
break;
}

pxCacheMem = pxCacheMemories[ xPartitionNumber ];
pxCacheMem = pxCacheMemories[ pxSettings->xDiskPartition ];

if( pxSDCardInstance == NULL )
{
Expand Down Expand Up @@ -551,14 +549,14 @@ FF_Disk_t * FF_SDDiskInit( const char * pcName )
}

pxDisk->xStatus.bIsInitialised = pdTRUE;
pxDisk->xStatus.bPartitionNumber = xPartitionNumber;
pxDisk->xStatus.bPartitionNumber = pxSettings->xDiskPartition;

if( FF_SDDiskMount( pxDisk ) == 0 )
{
/* _HT_ Suppose that the partition is not yet
* formatted, it might be desireable to have a valid
* i/o manager. */
if( xMountFailIgnore == 0 )
if( pxSettings->xMountFailIgnore == 0 )
{
FF_SDDiskDelete( pxDisk );
pxDisk = NULL;
Expand Down
8 changes: 8 additions & 0 deletions portable/Zynq/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,14 @@ static struct xCACHE_MEMORY_INFO * pucGetSDIOCacheMemory()
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

/* Initialise the SDIO driver and mount an SD card */
FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
Expand Down
11 changes: 10 additions & 1 deletion portable/common/ff_sddisk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,27 @@

#define __SDDISK_H__

#include "ff_headers.h"

#ifdef __cplusplus
extern "C" {
#endif

#include "ff_headers.h"
/* @brief Initialization settings for more granular control on init. */
typedef struct FFInitSettings_s
{
BaseType_t xMountFailIgnore; /**< Ignore failure when mounting */
BaseType_t xDiskPartition; /**< Default disk partition number */
} FFInitSettings_t;


/* Return non-zero if the SD-card is present.
* The parameter 'pxDisk' may be null, unless device locking is necessary. */
BaseType_t FF_SDDiskDetect( FF_Disk_t * pxDisk );

/* Create a RAM disk, supplying enough memory to hold N sectors of 512 bytes each */
FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName, const FFInitSettings_t * pxSettings );

FF_Disk_t * FF_SDDiskInit( const char * pcName );

BaseType_t FF_SDDiskReinit( FF_Disk_t * pxDisk );
Expand Down
8 changes: 8 additions & 0 deletions portable/linux/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@ void FF_SDDiskFlush( FF_Disk_t * pxDisk )
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
FF_Disk_t * pxDisk = NULL;
Expand Down
9 changes: 8 additions & 1 deletion portable/lpc18xx/ff_sddisk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,14 @@ void FF_SDDiskFlush( FF_Disk_t * pxDisk )
}
/*-----------------------------------------------------------*/

/* Initialise the SDIO driver and mount an SD card */
FF_Disk_t * FF_SDDiskInitWithSettings( const char * pcName , const FFInitSettings_t * pxSettings)
{
( void ) pxSettings; /* Unused */

return FF_SDDiskInit( pcName );
}
/*-----------------------------------------------------------*/

FF_Disk_t * FF_SDDiskInit( const char * pcName )
{
FF_Error_t xFFError;
Expand Down
Loading