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

Finished first draft revision of README #59

Merged
merged 16 commits into from
Aug 8, 2022
Merged

Finished first draft revision of README #59

merged 16 commits into from
Aug 8, 2022

Conversation

IowaDave
Copy link
Collaborator

@IowaDave IowaDave commented Aug 6, 2022

I think I've got here a complete draft to revise README. This draft supersedes one previously submitted on 8/2/22 as a PR and then withdrawn.

More work remains to do. I am not finished. I'm submitting this one as an interim work revising the previous content of README. Future work is intended to add new content rather than to revise previous content. And that will come later after I attend to some other things.

Please look at my discussion of RTC::now(). Invoking the function by its fully qualified name is what has worked for me. There is an example program, now(), that invokes it as a method of the DS3231 object. I just don't see the method being defined within the DS3231 object header. That's why I come at it through the fully qualified function name. Do I understand this bit correctly? What am I missing? Teach me, Obe Wuan.

Thanks, David

@IowaDave
Copy link
Collaborator Author

IowaDave commented Aug 6, 2022

Note to reviewer/approver: Please consider using the "Squash and merge" alternative for merging this PR. The thing contains a bunch of trivial commits in my fork along the way to understanding how Markdown handles certain internal links. The NorthernWidget repo has no need for that history.

Thanks!

David

@jnuernberg jnuernberg self-requested a review August 6, 2022 06:05
@IowaDave
Copy link
Collaborator Author

IowaDave commented Aug 6, 2022

I pushed an edit to the name of the now() function. The correct fully qualified name is RTClib::now().

Please consider correcting the usage in the Example program named "now". The code in there invokes a "now()" member function of the DS3231 object, which returns a "no member function" error during compilation.

IowaDave and others added 4 commits August 6, 2022 03:34
Also:

- delete redundant discussion of Wire.h, 
- generalize the discussion of the unsigned integer value returned by DateTime::unixtime().  All we need to say in this README is that it's a large number of seconds.
@IowaDave
Copy link
Collaborator Author

IowaDave commented Aug 6, 2022

Hey guys, my PR is getting really large now that I started creating additional documentation files.

My concern is that it is becoming too large with too many different parts. I don't know how to do separate PRs for separate files, so I think I'll just stop work now and await further developments.

After this PR has been dealt with by the Powers That Be, one way or another, then I could see my way clear to resume work on more documentation.

Meanwhile, it's been a pleasure working with you. Thanks!

@awickert awickert self-requested a review August 8, 2022 07:09
Copy link
Member

@awickert awickert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation is a huge improvement over the nearly nonexistent docs we had before. I have not gone through and tested it exhaustively (sorry; no time!) but I don't see any reason to hold up its being published. If we notice issues later on, we can always fix them :)

Loads of thanks!

@awickert
Copy link
Member

awickert commented Aug 8, 2022

I could "squash and merge", but this will remove @IowaDave's credit for these updates. Since this is cited sometimes in the academic literature, having edit counts helps keep the author list correct and current.

@awickert awickert merged commit 7dc8619 into NorthernWidget:master Aug 8, 2022
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