search

leomil72 / swRTC

swRTC is a library that implements a software-RTC on an Arduino board or an Atmel microcontroller
36 stars 16 forks source link

Add int setDeltaT and getDeltaT #1

Closed tochinet closed 11 years ago

tochinet commented 11 years ago

I really love swRTC, but I was annoyed that I couldn't read back the DeltaT correction. Also I felt that using floats only for that was a disservice to the library, because an int with tenths of seconds would fits more easily in integer only Arduino sketches.

However, my first try using the forked library seems to indicate an increase of compiled size instead of a decrease. Maybe I did forget some class specific stuff for overloaded methods ?

lestofante commented 11 years ago

I like your correction, but you are repeating the code of setDeltaT (float) into setDeltaT(int) and this IS BAD! a good solution is that your int function does that:

setDeltaT(int a){ setDeltaT(a/10.0); //force to be float, and divide by 10 }

that way correction in setDeltaT code are to be made in one point only. skecth size should aldo decrease.

BUT

because float are evil with precision, and because in the variable delta we take in account only for the first decimal, maybe is better to do the contrary: leave your setDeltaT as it is and change (and deprecate)

/@DEPRECATE! it is here only for backcompatibility. use setDeltaT(int a) setDeltaT(float a){ setDeltaT( int(a*10) ); //force to be int with decimal of second }

at we should encurage the use of your function because we are sure decimal is the right value... and this is not possible useing float.

2013/3/28 Tochinet notifications@github.com

I really love swRTC, but I was annoyed that I couldn't read back the DeltaT correction. Also I felt that using floats only for that was a disservice to the library, because an int with tenths of seconds would fits more easily in integer only Arduino sketches.

However, my first try using the forked library seems to indicate an increase of compiled size instead of a decrease. Maybe I did forget some

class specific stuff for overloaded methods ?

You can merge this Pull Request by running

git pull https://github.com/tochinet/swRTC master

Or view, comment on, or merge it at:

https://github.com/leomil72/swRTC/pull/1 Commit Summary

  • Update README.md
  • Update swRTC.h
  • Update swRTC.cpp
  • Update swRTC.cpp

File Changes

Patch Links:

tochinet commented 11 years ago

Agree. I didn't want to break it... And I think that the gcc anyway would throw away the version that is never used at compile time, so it may be better than usign teh same code in the end...

OK for the deprecation of float, makes sense.

BUT what I can't understand is why when going from float to int my sketch size actually increases !!! That is contrary to the logic.

lestofante commented 11 years ago

You are right about function not used, but those are method, I don't know if the same rule apply Il giorno 29/mar/2013 15:39, "Tochinet" notifications@github.com ha scritto:

Agree. I didn't want to break it... And I think that the gcc anyway would throw away the version that is never used at compile time, so it may be better than usign teh same code in the end...

OK for the deprecation of float, makes sense.

BUT what I can't understand is why when going from float to int my sketch size actually increases !!! That is contrary to the logic.

— Reply to this email directly or view it on GitHubhttps://github.com/leomil72/swRTC/pull/1#issuecomment-15643468 .

leomil72 commented 11 years ago

I've modified the code of the library including the new method getDeltaT. I've also reviewed the method setDeltaT. Now it accepts integers from -8,400 to +8,400 tenths of seconds. At the moment this method is still compatible with the old syntax that uses floats but I think that I'll drop it off the lib so I marked it as deprecated and I suggest, in the README file, to use the new one.