search

Zanduino / MCP7940

Arduino Library to access the MCP7940M, MCP7940N and MCP7940x Real-Time chips
GNU General Public License v3.0
35 stars 22 forks source link

Bug fixes - alarm setting #16

Closed wvmarle closed 6 years ago

wvmarle commented 6 years ago

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.

SV-Zanshin commented 6 years ago

Hello wvmarle,

Thanks for putting all that work into finding and fixing library errors! I think part of the issue with all the differences between the main branch and the merge is that I modified the main branch after the fork that you used to make the corrections. Thus if I pull your changes into the main branch I’d lose the changes that I made. Since this is a standard type of issue with source control systems I’m sure that git has some sort of mechanism to handle this. But I’m no git guru so it may take a bit to get that done but your fixed will be integrated even if I have to type them in myself.

Thanks,

-Arnd.

From: wvmarle notifications@github.com Sent: 05 July 2018 06:14 To: SV-Zanshin/MCP7940 MCP7940@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [SV-Zanshin/MCP7940] Bug fixes - alarm setting (#16)

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.

You can view, comment on, or merge this pull request online at:

https://github.com/SV-Zanshin/MCP7940/pull/16

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SV-Zanshin/MCP7940/pull/16 , or mute the thread https://github.com/notifications/unsubscribe-auth/AWfRxIESS1Mk1SwgT7MZ8kpaq4vmLmBoks5uDaCXgaJpZM4VDQmu . https://github.com/notifications/beacon/AWfRxC7iehe2GVWbdfkQuGkVoRkIDFJAks5uDaCXgaJpZM4VDQmu.gif

SV-Zanshin commented 6 years ago

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.

wvmarle commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.