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

Bug fixes - alarm setting #16

Merged
merged 7 commits into from
Jul 5, 2018
Merged

Bug fixes - alarm setting #16

merged 7 commits into from
Jul 5, 2018

Conversation

wvmarle
Copy link
Contributor

@wvmarle wvmarle commented Jul 5, 2018

Found issues with setting the alarms (see also http://forum.arduino.cc/index.php?topic=556111), fixed it.

Git does believe some of your later additions are deleted by me, I don't really understand git so you'll have to be careful when merging.

Lots of enhancements on layout for readability as well.

Various bugfixes and lots of formatting changes to enhance readability.
Added a "polarity" parameter to setAlarm for the user to be able to set the
polarity as desired.
@SV-Zanshin
Copy link
Collaborator

SV-Zanshin commented Jul 5, 2018 via email

@SV-Zanshin
Copy link
Collaborator

It looks like the best way forward is to merge your modifications and enhancements and then I will add my changes which seem to predate your fork but weren't included. One is adding multiple I2C speed capability to the library and the other is a bug fix related to the base day-of-week.

@SV-Zanshin SV-Zanshin merged commit f57440e into Zanduino:master Jul 5, 2018
@wvmarle
Copy link
Contributor Author

wvmarle commented Jul 5, 2018

Thanks. I'm going to update my branch asap.

Another possible enhancement (but it's a bit of tedious work) would be to improve the bit handling by using the bit names as defined in the datasheet, and Arduino's built-in bitSet(), bitClear(), bitWrite() and bitRead() functions. Again for general readability and maintainability.

So far I like this RTC. Much cheaper than the DS2321, pretty decent power characteristics, large voltage range and it has NVRAM.

You may have noticed I did add an extra parameter to the setAlarm function. Not sure if that's the best solution (it breaks any existing sketches!). Maybe better to handle the ALMPOL in a separate function?

@SV-Zanshin
Copy link
Collaborator

wvmarle - I see that you are making further enhancements and changes to your forked copy. Please remember to do a "pull" from the master so that the changes that I've incorporated to the master branch are loaded into your copy, then you can put in a pull request so your changes can be integrated back into the master. If you don't do that, then the copies would become out of sync.

@wvmarle
Copy link
Contributor Author

wvmarle commented Jul 6, 2018

Yes... done that already, got the I2C things.
Another big pull is coming soon for you. Lots of enhancements - reduced my compiled size by some 100 bytes and using just part of the available.
Still testing a few things to make sure everything works.
After that it's time to implement the NVRAM part, and go through the data sheet to make sure complete functionality is there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants