search

mikaelpatel / Cosa

An Object-Oriented Platform for Arduino/AVR
https://mikaelpatel.github.io/Cosa/
GNU Lesser General Public License v2.1
339 stars 76 forks source link

time_t internal representation #247

Closed SlashDevin closed 10 years ago

SlashDevin commented 10 years ago

I am trying to use the time_t struct for, well, time-related things. I was working on a GPS IOStream::Device, among other things, and I have had trouble with the BCD internal representation. I have a solution that I was considering for a pull request, but I though I'd discuss it first through an issue.

For one thing, the object does not track its current representation. A side-effect of calling 'operator clock_t' results in switching to a binary representation, and assumes that it started in BCD. A second call, of course, would yield nonsense.

I considered adding a flag, but that would break all the code that depends on sizeof. I'm wondering if this struct could simply operate on a binary representation and leave the to_bcd method for those cases where it's needed (e.g., writes to BCD devices). I'm not sure the BCD representation used on some devices should be forced on all generic time-related operations.

I also worked on a few other things, like running on different epochs (e.g., POSIX 1970) and quite a bit of performance improvement. This implementation is ready to submit, but I only have the DS1307 for regression testing. Most of the changes in XXX/Commands.cpp and examples/CosaXXX.ino I can submit, but not test. They are mostly removing to_bcd (before a write) or adding to_binary (after a read and before a time operation like clock_t cast).

What are your thoughts?

mikaelpatel commented 10 years ago

The time_t struct is basically the common struct of the time registers in the RTC devices DS1302, DS1307 and DS3231. The support functions take the value from the device, from BCD to binary and reverse for setting. The clock_t and zone handling are add-on's. Designed out of simplicity and very specific for these devices.

The general design, which I also understand you are suggesting, would result in a device specific time struct and an internal general time struct. The BCD and binary value could then be handled by the type system (static) if wanted or with a state variable.

In any case attempting to do something more general will result in a very large ripple effect with many hours of work and some increase in footprint. I currently do not see the need for this. More motivation is needed. Or I am simply missing the point.

Cheers! Mikael

SlashDevin commented 10 years ago

TL;DR - I've created a branch in my fork of Cosa, if you'd don't mind taking a look. The design changes are fairly modest and restricted to time_t. The results below are quite nice, but I'm still thinking a little more about multiple epochs.

TL - As you noted, time_t isn't really suited for general-purpose use. And I really hate to see multiple copies of weird code, like calendar math, so I started looking at fixing what was there... Before I got too deep into it, I thought I'd see if it was already on your list of things, or if you had encountered similar problems. Thanks for your response

> Designed out of simplicity and very specific for these devices.

Yes, I think this is what tripped me up. Time.hh resides in the Cosa directory and is used in other non-BCD contexts, like NTP and xxxCommands. To me, this implied a general-purpose usage.

However, when I went to use it in another non-BCD context (GPS), it did not handle 2-digit years correctly, and it would unexpectedly change internal representations. If you look at operator <<, it essentially uses an epoch year of 2000 and a pivot year of 0. But in the time_t(clock,zone) constructor, the epoch is January 1, 0! I was even more confused when I looked at CosaNTP.ino: it converts a received binary epoch 1900 to a BCD epoch 2000 for printing... and it works! I think there may be a leap year problem lurking in here (see the commit line note here ).

Oh well, this is the way software grows. :D

By the way, GPS devices provide times in UTC, GPS and time-of-week (offset from midnight Sunday). Leap seconds can be involved. And different messages use different units, so faithful conversions must be performed. Sigh.

My goals were to

In the process of modifying time_t, I added new methods for full_year, day_of_year and converting to days. It exposes the is_leap method (formerly static in the .CPP). It also assumes the internal representation is binary, but it retains BCD methods.

After some investigation, changes are limited to the just these 2 core files:

MCP7940N.cpp insert "now.to_binary()" at line 176  (printing)
             insert "now.to_bcd()" after line 181  (printing)
DS3231.cpp   insert "now.to_binary()" at line 176  (printing)
             insert "now.to_bcd()" after line 181  (printing)

And these examples required minor changes:

CosaLCDclock.ini     insert "now.to_binary()" at line 152
                     insert "now.to_binary()" at line 155
CosaDS18B20alarm.ino insert "now.to_binary()" at line 138
CosaDS1302.ino       insert "now.to_binary()" at line 97   (printing)
CosaDS1307.ino       insert "now.to_binary()" at line 112  (printing)
CosaDS3231.ino       insert "now.to_binary()" at line 94   (printing)
                     insert "now.to_binary()" at line 106
CosaMCP7940N.ino     insert "now.to_binary()" at line 61   (printing)
                     insert "now.to_bcd()" after line 61   (printing)
                     insert "now.to_binary()" at line 87
                     delete "now.to_bcd()" at line 99      (printing)
                     insert "alarm.to_bcd()" at line 111
                     insert "alarm.to_bcd()" at line 116
CosaTime.ino         insert "now.to_binary()" at line 51
CosaTimeLCD.ino      insert "now.to_binary()" at line 63
                     remove "bcd" format at lines 75-77 & 82-84  (printing)
Ethernet/CosaShellWebServer/Commands.cpp
                     remove "now.to_bcd()" at line 168
Ethernet/CosaTelnetShell/TelnetCommands.cpp
                         ditto
Sandbox/CosaShell/Commands.cpp
                         ditto

More than half of these changes are for printing. A few more are because the time_t struct is initialized with hard-coded BCD values, but you could remove the "0x" to make them decimal instead (e.g., CosaTime.ino/lines 45-50). I have included all these changes in my branch.

These files did not have to change:

CFFS.hh
CFFS.cpp
NTP.hh
NTP.cpp
DS1302.hh
DS1302.cpp
DS1307.hh
DS1307.cpp
DS3231.hh
MCP7940N.hh
CosaBenchmarkSizeOf.ino
CosaNTP.ino   (what?!?  :))
CFFSCommands.cpp
CosaCFFSshell.ino
Ethernet/CosaTelnetShell/Commands.cpp

Next, I gathered some stats:

Gee, the times aren't so great. After looking a little closer, most of the extra time has to do with properly handling the 2-digit year with respect to the NTP EPOCH_YEAR of 1900 and PIVOT_YEAR of 37: The days_per(year) loops have more iterations. Indeed, if I select an EPOCH_YEAR of 2000 and a PIVOT_YEAR of 0, I get

This is more of an apples-to-apples comparison, in that an EPOCH_YEAR of 2000 is closer to the current implementation. Not too shabby! The new code is about twice as fast for everything except NTP. Maybe I can address that as I think about mixing epochs.

Regards, Devin

mikaelpatel commented 10 years ago

Nice work. Seems like a lot of changes but still not as many as I first thought. The extra layer between the device and the more general time_t is really needed when looking at other time sources such as NTP, GPS, etc. The current RTC bases time_t (copy of device struct) will not hold.

I would encourage you to do a pull request after testing some more and I will merge it into the master. So far this looks very promising and I do really appreciate your effort with this. I simply do not have the time right now to dive into this.

Cheers! Mikael

mikaelpatel commented 10 years ago

Small fix with day number setting and some basic clean. Thanks for the execellent work!