[newRPL] Filesystem related date/time issues

[Claudio L.]

(08-15-2016 12:13 PM)matthiaspaul Wrote:  Strangely enough, however, I saw some code for this chip (family) where 99 was special-cased for 1999 (but perhaps that was for an older chip revision, I don't know). It wouldn't matter much if the chip wouldn't be responsible for the calculation of the length of February and (probably also) the day-of-week. I didn't read this up in the reference manuals yet...

Because the chip uses only 2 digits, originally those digits could mean 00 = 1900 or 00 = 2000, at the user's choice. But not quite, because the original chip lacked the special 400-year leap year that happened in 2000. This is not a problem for us in 2016 obviously, it was only a problem on Feb 28, 2000 near midnight :-).
Later revisions of the chip implemented the leap year, but hardwired to be applied always. This means 00 = 2000, and user can no longer assume that 00 = 1900.
Other than that, this RTC just works fine, I assumed 00-99 = 2000-2099.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  

(08-15-2016 02:44 AM)Claudio L. Wrote:  The only reason is POSIX:
http://pubs.opengroup.org/onlinepubs/969...ime.h.html
I just took the struct tm and made it more compact.

I see. There were two reasons for that comment. I saw special -1 adjustments for it in three places (in the original unfolded code, that is), which could have been replaced by possibly fewer +1 adjustments on the other end. More important, for both entries, day and month, the FAT filesystem actually defines a value of 0 as valid to indicate "not set". Most code examples I have seen would display this with the date or month set to 0 (most often as "1980-00-00"), while others use it to suppress the display/usage of that value. The hardwired -1 in your code makes the month value underflow under that condition, which basically lumps this special case together with invalid month values beyond December. It very much depends on the higher level code if it would cope with this correctly or not.

Somebody could "split" this file system into a separate library and use it outside newRPL. In that case, they would expect the tm structure values returned to be in line with the POSIX standard struct tm, so I'm inclined to keep it that way. The 0 value to indicate not set I think it affects the whole 16-bit number: if the half-word is zero, then the date is not set, I don't think it was meant to be interpreted for individual fields. I can easily add support for that returning an error on those functions if the date is zero/invalid.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  So far, I have zero experience with this Samsung chip, so I don't know either.

I don't know if the Samsung chip ever had this issue (probably not), but interpreting Samsung's advise above very strictly, it could well mean that the whole bunch of regs should be reread *until* BCDSEC is not (no longer, that is) zero. On the other hand, their explanation regarding that "one second deviation that was mentioned" and the "there is no problem" indicate no such issue.

It's well possible (probably!) that the Samsung RTC registers are latched internally so that this is really a non-issue. However, in the past I have seen more than one RTC where the registers were not latched and the rollover took some time to be propagated internally - with such chips chances were that you still caught the old transient (year etc.) values on subsequent reads for some while and had to wait for the actual change to happen (complicated code) or just waited some arbitrary, but long enough time.

(Being somewhat shocked I saw code for that very Samsung chip doing just this, waiting for up to a whole second reading out the RTC. I don't have the link handy any more, though. I have also seen examples performing only one loop at max, though, so that former example might have been down to either "careless coding" or "working around some issue", I don't know. That's why I brought this up to our attention here.)

Don't be so shocked, I've seen weird stuff out there. I didn't actually test it but the way I interpreted the docs only one read is needed, all internal registers are updated at once and have no reason to suspect otherwise. It wouldn't be hard to test it though.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  Actually, I hope for the compiler to do that (well, folding code rather than inlining - speed is not really an issue here, size is) otherwise this coding style would be quite "expensive" on an embedded system.

I just checked the binaries, the compiler did not inline them.
You got me curious now. I'll count how many words these 3 functions take, then change them with your code and recompile, then count how many words it takes. My guess is by the time you add the tests/branching, additional argument, etc. it will be relatively close.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  I was tempted to introduce some dirty pointer indexing two bytes in front of the 16-bit lastaccess entry (and later just throw away/overwrite the bogus time value) building on the symmetry with the cases for the two 32-bit entries. That would remove all those special cases and considerably shrink the code - probably quite beyond to what the compiler could do. However, the easiest way to achieve this would be by some rearrangements in the FS_FILE struct so that negative indexing could be avoided, but I didn't want to touch that at this time.

I'm not sure it's worth the trouble. You'll save a few words but the code will be harder to follow for others. At this stage I don't want to obsess too much about code compactness or even speed. I'm more into portability and easy to read for others to help me (like you are doing right now). The main goal is: code correctness, proper error management, and general bug hunting (like that volatile you pointed out above).
Once newRPL 1.0 is released, I'll start aggressive optimizations all over the place (decimal math routines could use some assembler tricks, transcendental functions are particularly slow and in need of serious optimization).
But that's further down the road, if the code is clean enough it will be easier to optimize.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  Speaking about compiler optimization, without that added "volatile" there was some risk that a highly-optimizing compiler would have thrown out the RTC readout loop.

Yes, that was a bug, it should've been volatile, thanks.

(08-15-2016 12:13 PM)matthiaspaul Wrote:  I don't think it would be a good idea to propagate invalid values (except for one "neutral" value) up to the frontend (RPL), as users are expecting "mathematical correctness" here (perhaps even doing some calendar calculations on it and just relying on that the values are either well-formatted or clearly indicated as invalid). They probably don't expect date functions to return impossibly high days and months (even if that would happen only in case the filesystem stores invalid entries - ensuring that it hasn't, is, I think, outside the a user's responsibility and scope).
Being able to search for "month 13 or 14" might make sense on a lower level - for a lower level API function, perhaps. However, I'm not sure if that would still hold true on a calculator. Are you expecting people to write CHKDSK-like tools to be used on the calc?

I didn't mean to pass it on to the user, perhaps wasn't clear. lib74 commands would have to filter that information properly before passing to the user. But if I build a filer, it could internally sort using the illegal values with no problem (getting the values directly from the file system, not from the user-facing RPL commands).

(08-15-2016 12:13 PM)matthiaspaul Wrote:  I was living under the impression, though, that routines such as fsupdatedirentry() (discussed in the original thread) would deal with the raw 16-bit and 32-bit values as stored in the FS_FILE struct, not with the somewhat "higher level" tm struct as used by FSGetWriteTime(), FSGetCreatTime(), FSGetAccessDate(). Either way, regarding easier reversibility, then perhaps the -1 month alignment for POSIX should be abandoned, as you would have to revert that as well otherwise. This would also free the value 0 for "not set". Does some already existing high-level code depend on that POSIX alignment?

You are correct, the file system internally does not use the tm struct at all, and does not use or manipulate the dates in any way.
The tm struct is only for the external API.
So far nothing in newRPL *needs* the POSIX conformance, it just makes it easy to reuse the file system module in other projects too
I can find other ways to pass an "invalid date" error, more in line with the rest of the library's error reporting.