Please revert #10002 FS library returning (size_t)-1

[TD-er]

Board

Any

Device Description

Hardware Configuration

Version

latest master (checkout manually)

IDE Name

PlatformIO

Operating System

Windows 11

Flash frequency

Any

PSRAM enabled

yes

Upload speed

115200

Description

The change introduced by this commit has all kinds of side-effects as position() is used in several other functions, some of which expect this to be a signed int.

If you really need to return an error code for this function, then all (!!!) position related code should expect a signed type.
Returning (size_t)-1 is always something that should be frowned upon.

For now, please revert this commit.

Sketch

-

Debug Message

-

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[drmpf]

As noted in #9992, (by me_no_dev) this change matches the Arduino File class in the Arduino SD library.
As it is now, if 0 is returned I don't know if it is safe to let the code proceed or not.
The Arduino File class returns -1 for position errors which is implicitly converted to uint32_t
The Arduino File class also expects uint32_t for seek( ).

I don't believe returning -1 necessitates changing all the other methods args to ints as that would be even more 'non-standard' compared to the Arduino SD library
Simplier to just let the user check the position return against -1 with implicit C conversions handling the comparision.

Looking at the C fn ftell(), it returns -1 on error so I suppose that was the template for the Arduino functions, but due to the word size, they opted to returned an unsigned to get the maximum available range on restricted word length CPU's

In any case, an experienced C programmer would expect the 'position' function to return -1 on error.
Returning 0 is just confusing and un-useable as an error return and an unnecessary variation from the Arduino 'standard'

Please re-apply the original pull request to provide a usable error return and to be compatible with the Arduino File class.

[TD-er]

Please re-apply the original pull request to provide a usable error return and to be compatible with the Arduino File class.

Well it isn't accepted and @me-no-dev also mentioned via chat that I should find other ways.

Don't get me wrong, I do completely understand that reporting an error state which could also be a valid state is not good either, but what we're ending up with now is that we need to check at numerous places whether we're dealing with either (size_t)-1, -1 or 0.
So IMHO this 'fix' makes things worse as you need to take implementation into account when handling file positions and not just can assume any negative value to be an error for example.

However the situation is now that we can't make a proper fix as it will break Arduino compatibility. (meaning turning all these into signed values whenever an error could be expected)
Also we can't work with the current implementation as it leaves checking for badly made design decisions in this code upto the caller and thus lots of code duplication and error-prone situations.

So which one is the worst?
It is like what we call in Dutch "choosing between being bitten by the cat or the dog".

Meaning I still feel strongly for reverting this commit for now and then re-do this but now with proper testing.
And if needed we can also try to convince the "powers that be" of the original Arduino to move over to changing the interface to properly using signed types whenever an error can be expected. However I think we both know this will take some time.

As how it is now, it is unusable as it simply does cause issues which are hard to track.
So I think we all agree something has to be done about it and having a clear difference between a valid return value and an int should be our main goal. But how it is now, we just moved this confusion to other code in multiple places.

TL;DR
If this is not reverted, then there should be a proper fix ASAP to deal with these issues as it is already affecting other functions in the same FS class right now and who knows what else in code using these functions.

[TD-er]

By the way just to make it clear how inconsistent it all is now...

size_t VFSFileImpl::position() const {
  if (_isDirectory || !_f) {
    return 0;
  }
  return ftell(_f);
}

[drmpf]

I did not manage to track down the .h file for ftell() but I assume it is the standard C declaration in which case it will return -1 on error,
so VSFileImpl::position will return either 0 or -1 on error
which is even more confusing.

[brentru]

Arduino to move over to changing the interface to properly using signed types whenever an error can be expected. However I think we both know this will take some time.

Yup, this will take some time to do "properly". For now, it is introducing bugs to FS.

Meaning I still feel strongly for reverting this commit for now

I agree, +1. @me-no-dev - what would be the reason to not revert this change?

[drmpf]

Perhaps in the mean time update the docs for VSFileImpl::position to say
returns 0 or ((size_t)-1) on error.

[me-no-dev]

what would be the reason to not revert this change?

The only reason why this was merged and will not be reverted is that this is how it works in upstream Arduino (in the SD Card class). I also do not like it and do not find it proper, but for the sake of being compatible, it had to be done.