Real Time Clock - Fixes and Improvements

GoogleCodeExporter commented 8 years ago

Rather than the standard "steps to repro, expected output, etc.", this bug 
report will just "dive in" to details...

Problems occur when using the feature documented in the Propeller GCC Library 
page at http://propgcc.googlecode.com/hg/doc/Library.html#RTC:

*** Begin Quote ***
There is also a mechanism to use another cog to update the cycle counter, which 
will remove the requirement for frequent calling of the clock functions. If the 
global variable _default_ticks_updated is set to a non-zero value, then the 
default RTC assumes that another cog is keeping the 64 bit counter 
_default_ticks up to date, and it does not set it from the cycle counter.
*** End Quote ***

This RTC feature (as coded) has two basic problems:

1) It is not thread safe, and will generate spurious results at times
2) It is not easy to use:
  A) The cog tending function is not in the library, it is a "main()" function
  B) A roll-your-own solution requires people to cut/paste declarations from the code

What version of the product are you using? On what operating system?

propgcc-0.2.2

Please provide any additional information below.

Attached please find several files:

rtc.c - a fixed lib/sys/propeller/rtc.c
rtc_cog.c - a fixed lib/sys/propeller/rtc_cog.c
rtc.h - a fixed lib/include/sys/rtc.h
c_sample.c - a sample program showing how to use the fixed rtc_cog.c

Also included are .diff files that show the differences from the 0.2.2 versions.

Discussion:

The rtc.c file suffered from thread-unsafe practices.  If two threads are going 
to access _default_ticks, then it needs to be locked as each thread updates it. 
 All the fixes in rtc.c are to add the necessary locking, except...  The 
update_ticks function is now global (named _default_update_ticks), not static 
local.  This is due to the streamlining of rtc_cog.c.

About rtc_cog.c: The issue here is that IMHO, a sizable majority of its users 
will want to just say "OK, I want a stable time, but I don't want to think 
about it.  Use the cog if you need to, but just make the time works reliably 
and properly".  Therefore, the library should have a simple routine that a 
typical user can call to simply direct the RTC to do exactly that.  The new 
"void _rtc_start_tender_cog()" routine does exactly that.  Please add rtc_cog.o 
to the library.

Regarding sys/rtc.h:  It now has declarations for the _default_ticks_updated 
variable, and the "void _default_update_ticks()" routine (as well as the 
aforementioned _rtc_start_tender_cog().  If a programmer doesn't want to give 
up a cog, but just wants to call a "hey - wake up and make sure the time being 
tracked is OK" function from some other cog that has the time and frequency to 
do so, why should the user have to know the gory details of the _default_ticks 
structure?  All the user needs to do is set _default_ticks_updated to 1, then 
call _default_update_ticks() on a regular basis.  (Note that this is exactly 
what _rtc_start_tender_cog() function does, albeit in a standalone cog.)

Adding to this argument is my belief that folks are not going to want to write 
some other way to keep track of the ticks within the _default_ticks structure.  
Indeed, if a particular application needs a different/better way to keep the 
clock (perhaps by using a DS1318 or some other time-keeping chip), these 
programmers are not going to be fussing with the _default_ticks counter; 
instead they are going to "install" their own diver by overwriting the 
_rtc_gettime and _rtc_settime pointers.  That's why I didn't bother suggesting 
adding the _default_ticks structure declaration to sys/rtc.h.

For a project I'm working on, I ported the DS1302.spin file to C, and fixed and 
added a bunch of stuff.  One thing that would be desirable for me (and I'm 
guessing others) is to add a battery-backed up DS1302 chip (it's a three-wire 
interface) to their system and have their C code have full access to standard 
"C" date/time routines.  The attached c_sample.c file contains an example of 
how to set this up.  The call to _rtc_start_tender_cog() causes the timekeeping 
to "just work", then the rtc_init() and rtc_c_library_init() calls from my 
DS1302 driver look up the time stored in the DS1302 and initializes the C 
runtime via a "settimeofday" call.

I plan on submitting the DS1302 C code to the Propeller Object Exchange as soon 
as PropGCC has the fixed gmtime() function (issue 27) and also has these fixes.

In summary, I hope you accept and integrate these changes.  As far as I can 
tell, the changes to rtc.c are mandatory, even if you don't want to change 
rtc_cog.c.  In this case, you have to add the locking to rtc_cog.c as well 
(yuck).  Please accept both of my fixes, and please accept the changes to 
sys/rtc.h as well.  Then, please make the following changes to the library 
documentation:

*** Start Quote ***
The interface to real time clocks is via two function pointers:

    #include <sys/time.h>
    int (*_rtc_gettime)(struct timeval *tv);
    int (*_rtc_settime)(const struct timeval *tv);
*** End Quote ***

This is wrong, no matter if you accept my proposed changes or not.  This should 
say sys/rtc.h, not sys/time.h.

*** Start Quote ***
There is a default clock that simply uses the built-in cycle counter. At a 
typical clock rate of 80MHz this counter overflows in 54 seconds. The default 
RTC driver uses a 64 bit tick counter to compensate for this, but it must be 
called more often than the counter overflows in order for it to keep the high 
word of the tick counter accurate.

There is also a mechanism to use another cog to update the cycle counter, which 
will remove the requirement for frequent calling of the clock functions. If the 
global variable _default_ticks_updated is set to a non-zero value, then the 
default RTC assumes that another cog is keeping the 64 bit counter 
_default_ticks up to date, and it does not set it from the cycle counter.
*** End Quote ***

Change this to:

*** Start Quote ***
There is a default clock that simply uses the built-in cycle counter. At a 
typical clock rate of 80MHz this counter overflows in 54 seconds. The default 
RTC driver uses a 64 bit tick counter to compensate for this, but a time 
retrieval function (such as time()) must be called more often than the counter 
overflows in order for it to keep the high word of the tick counter accurate.

There is also a mechanism to use another cog to update the cycle counter, which 
will remove the requirement for frequent calling of a clock functions (e.g. 
time()). To do this, call the _rtc_start_tender_cog() routine - it starts a cog 
whose entire purpose is to keep the clock up-to-date.

Alternatively, if you have spare time on one of your cogs, you can kick the 
built-in cycle counter yourself (without the expensive call to the time() 
routine which performs several 64-bit math operations).  To do this, set the 
global variable _default_ticks_updated to a non-zero value, and call the 
_update_default_ticks() routine on a regular basis.

If you have an alternate way to keep time (such as a hardware chip like the 
DS1318), replace the built-in drivers by assigning the global _rtc_gettime and 
_rtc_settime pointers to your own gettime() and settime() routines.
*** End Quote ***

Original issue reported on code.google.com by tjstefan...@charter.net on 3 Feb 2012 at 5:28

Attachments:

GoogleCodeExporter commented 8 years ago

Thanks for your ongoing enthusiasm. I will be getting in touch with you on this 
and other issues.

Original comment by jsden...@gmail.com on 3 Feb 2012 at 6:59

GoogleCodeExporter commented 8 years ago

I'll echo this -- thank you for your help and suggestions. Your bug reports are 
detailed and very very useful.

I'm looking at the time code again now, and I will incorporate your suggestions 
(or something functionally equivalent). The time code pre-dated the threads 
code, and obviously we overlooked some things when adding threads!

Eric

Original comment by ersm...@hfx.eastlink.ca on 4 Feb 2012 at 9:45

Changed state: Accepted

GoogleCodeExporter commented 8 years ago

I've checked in some fixes for the rtc. I did things slightly differently, 
mainly to accommodate XMM and XMMC modes, which do not support running C code 
in other cogs. So in rtc_cog.c I added a small PASM function to do the update. 
XMM and XMMC also don't support the atomic set/get operations, so I moved the 
rtc variables to hub memory and added _getAtomic64 and _putAtomic64 functions 
to get/set the variables. All cogs read the low word first, then the high word, 
in consecutive hub cycles; similarly they write low first then high in 
consecutive cycles. I think this should guarantee that all the cogs will see 
consistent values.

Original comment by ersm...@hfx.eastlink.ca on 7 Feb 2012 at 5:34

Changed state: Fixed

sammielove45 / propgcc

Real Time Clock - Fixes and Improvements #28