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

[AniruddhaKanhere]

Description

Thank you for your changes @phelter - I am creating this PR on your behalf so that we can review this.

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

This should replace #49.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[AniruddhaKanhere]

@phelter and @htibosch can you please look at this PR?

[AniruddhaKanhere]

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

[phelter]

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?

[htibosch]

@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

[phelter]

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

[AniruddhaKanhere]

Same here. Would this break existing users code?

[htibosch]

Can you please have a look at my earlier response?

[phelter]

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).

[AniruddhaKanhere]

To fix the formatting, would you please apply this patch @phelter?
formattingDiff.patch

[AniruddhaKanhere]

@phelter and @htibosch,

We have been discussing this internally as this is a breaking change. I shall share what we discussed and the conclusions drawn here:

• What is the expectation around breaking changes in labs projects?

  • The labs projects are experimental and are there for community's benefit and review. While we do not make breaking changes for non-critical issues like to make the API look better, the expectation is that the code can and will change if the reason is justified and the code has a bug.

• What about this PR? Are we going to accept it?

  • Since there is a conflict of definitions with a standard file, it seems prudent that we accept this change even though it would break existing users code. However, we shall tag the commit just before this PR is merged so that folks who do not want to take in this breaking change can do that by checking out the tag.

[htibosch]

Thank you @phelter for this PR, I approve it as it is.
I checked my projects and in fact the struct is rarely accessed, easy enough to change it.