-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refracture DateTime Class #95
Refracture DateTime Class #95
Conversation
rework due to changed getter method names.
- rework due to renamed member function names
- rework due to renamed member functions.
It perhaps should be tested also on more modern boards including SAMD21-based Arduinos including Zero, the MKR series, and the Nano 33 IoT, plus the Renesas chip on the Uno Rev 4 and the ESP32-based boards. |
Thank you for bringing forward such an impressive and extensive revision to the library and the supporting repo. I feel sure I will find much to learn by going carefully through the body of work you have produced here. Will you allow the others of us some time to study it? This library has been in wide use for a long time and we want to proceed carefully, to avoid breaking existing code that depends upon it. I would like to make a case for keeping critical calculations in the source code of the DS3231 library rather than to rely on functions in a "time.h" library, for the following reasons. There is a trade-off between code-efficiency and code-clarity. To what extent does the DS3231 library become less clear as a result of making it possibly a bit more compact? I would like to describe a value I see in keeping certain calculations in the library, even when similar calculations may be available in a dependency. What do I mean by that term, "dependency"? When we use functions from a "time.h" header, our code becomes dependent on that header. The header is then a dependency. One concern is that critical code becomes somewhat "hidden" when it resides only in a dependency. Another concern is that a future change in that dependency could break this DS3231 library. It is a controversial viewpoint, perhaps. There is the counter view that we should not re-invent wheels. But then, when we put someone else's wheel onto our car, whom do we expect will fix a flat tire? We should ask ourselves whether it is wise to expect amateur users to figure out which "time.h" library will be accessed when their program is built. For example, on my rather humble workstation, I find more than three dozen different files named "time.h" have been installed at different times in my Arduino15 library directory. By contrast, when the calculation is performed in our library, there is only that single body of source code to examine. And we can fix it if the tire goes flat. Does it conserve memory to rely upon a dependency to perform our calculations? I'm not sure. Perhaps the compiler and linker are smart enough to omit portions of the dependency code that we do not invoke? In any event regarding memory, the trend in Arduinos is toward MPUs having greater amounts of memory compared to the legacy ATmega328P. As for ESP8266 (and ESP32), memory is quite large. Looking forward, we might see that increasing memory capacity effectively reduces the impact of whatever memory our library might consume, reducing the urgency to minimize our code size. |
Hi Dave, thanks for you large comments. First please let me ask a Question? When you buy an new car for example, would you stuck on the performance of the "10 years" old motor? I worked many years on time libraries and I was inspired using standardized libraries instead of write all the time code which other did do already before and tested by the whole world. I am not sure what is now your final statement from you comments above? I have not all above mentioned Boards available. I am open to bring this PR on the bench, but I need input from you. And I need to know a little more about critical code, what you mean by this? For the first impression I am not so sure if you feel well with this PR. Best Regards |
One comment on this:
I also startet with the Arduino IDE lets say, as I noticed then from time to time that the Arduino IDE has nothing to do with a real and professional SW Build chain I switched to PIO which allows to work more with the real build processes. The Arduino IDE, when using multible files are used, copies first all together instead build each first. So coming to the point here, in PIO it is clear what is used. :-) |
I am looking forward to hear from you :-) |
I respond here with respect and sadness. Respect: I do not feel qualified by knowledge or experience to judge whether this PR should be merged. While I sincerely do look forward to studying and learning from the impressive work contributed to this PR, I leave the decision for others to make. Sadness: Two things to say before I go. Caution is very different from bias. It surprised me to see my "feelings" being speculated upon as being somehow relevant to the discussion. When I shared my thoughts with a view to entering a conversation, I did not expect to find my concerns categorically brushed aside on the basis of someone's superior know-how and myself placed on the defensive regarding the validity of my personal thinking process. My participation in the discussion of this PR ends here. |
I think we misunderstand each other in the first Opening of the discussion. I will also treat your thoughts with respect and I will try to explain each step what I did to give you the chance to learn, as you said, by this alternative approach to fill this class with live. Lets give this PR an second chance and put our minds together to challenge this code against you lets say requirements. With respect and best regards |
Dear @IowaDave , one small remark which expresses my will and passion to contribute to this library. With respect |
Dear @awickert, dear @IowaDave,
I provided a refractured version of your DS3231 library.
My goal was to do all calculations based on the available time functions provided by the
time.h
library.The most stuff uses the typical
struct tm
, so I introduced a classinternal struct tm
which holds all date specific data.All stuff based on this idea:
https://openbook.rheinwerk-verlag.de/c_von_a_bis_z/bilder/19_003.gif
And it works :-)
I took your latest master commit and worked on it.
I tested on the ESP8266 and the Arduino Nano, both showed the same results.
I had actually no compiler warnings.
I would appreciate If we could merge it,if not because the changes are to big for you let me know I will then set up a new separate library for the DS3231.
I know in the diff view it looks like all changed, but it is maybee because of the restructure of the comments. In PIO it shows always the wrong syntax highlighting though I placed the comments at the right place then.
Refracture of DateTime Class
time.h
lib.time_t
for timestamps, it is automatically adapted for the different controllersRefracture cpp and header file
Added smart time print function
show_DateTime()
based ofstrftime()
Use like this:
Shows then:
Solve Issues
Remove unused functions