Skip to content
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

Closed
wants to merge 20 commits into from
Closed

Refracture DateTime Class #95

wants to merge 20 commits into from

Conversation

hasenradball
Copy link
Contributor

@hasenradball hasenradball commented Oct 23, 2023

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 class internal 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

  • refracture of DateTime class with respect to use standardized time function from time.h lib.
  • usage of type time_t for timestamps, it is automatically adapted for the different controllers
  • always had in mind -> don't invent the wheel twice.

Refracture cpp and header file

  • comments and description of functions and declarations are placed before the definition/declaration => to show the correct information when moving over the function (syntax highlighting).

Added smart time print function

  • add function show_DateTime() based of strftime()
    Use like this:
  DateTime mytime {RTC.now()};
  char buffer[25];
  mytime.show_DateTime(buffer, sizeof(buffer), "%c");
  Serial.println(buffer);

Shows then:
grafik

Solve Issues

Remove unused functions

  • remove all functions which are actually not used anymore => easier maintainment.

@IowaDave
Copy link
Collaborator

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.

@IowaDave
Copy link
Collaborator

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.

@hasenradball
Copy link
Contributor Author

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?
Or would you also want to use the new performance and the benefit of the new emissions and other stuff?

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?
Go for it, take it on the bench and look what it brings?
Or lets keep the old engine?

I have not all above mentioned Boards available.
If this is the requirement for the PR getting merged, the please say it directly.
I have the ESP8266, ESP32, Arduino NANO, NANO IOT.

I am open to bring this PR on the bench, but I need input from you.
What are the requirement in performance?

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
Frank

@hasenradball
Copy link
Contributor Author

One comment on this:

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.

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.
Arduino is not so clear how it works behind the scenes, and if it is so you should measure the performance stuff also with other than Arduino IDE only.

:-)

@hasenradball
Copy link
Contributor Author

I am looking forward to hear from you :-)

@IowaDave
Copy link
Collaborator

@awickert , @hasenradball

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.

@hasenradball
Copy link
Contributor Author

@IowaDave

I think we misunderstand each other in the first Opening of the discussion.
Please lets give another try.
I am willing to improve this library because there is create stuff done in the past.

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
Frank

@hasenradball
Copy link
Contributor Author

Dear @IowaDave ,

one small remark which expresses my will and passion to contribute to this library.
See links:
C++ Core Guidelines Mainpage
The C++ Standard Library
Use Libraries wherever possible

With respect
and best regards
Frank

@hasenradball hasenradball deleted the refracture_DateTime_class branch October 26, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants