search

adafruit / Adafruit_CircuitPython_DS3231

Adafruit CircuitPython drivers for the DS3231 realtime clock.
MIT License
21 stars 19 forks source link

RS registers shouldn't be assumed #15

Closed gritnix closed 4 years ago

gritnix commented 5 years ago

This code has the following comment:

Try and verify this is the RTC we expect by checking the rate select

    # control bits which are 1 on reset and shouldn't ever be changed.

My project uses the 1Hz square wave output from the interrupt pin so these registers ARE changed to 00. I'm not sure why the authors believe these registers will never be changed. Anyone wishing to use the square wave output from the interrupt pin might change them. The code, as written, cannot "find" my DS3231 because of this, although it is there and completely readable. Not sure if there's a better way. I'm just going to remove that bit in my local copy.

caternuson commented 5 years ago

It is checking for the power on reset values. Can you provide more details on your use case. How are you setting to 00 and then later trying to create a new instance?

gritnix commented 5 years ago

Sure. I guess I do have a weird scenario. Those registers can easily be written just using I2C commands so that the square wave is set to one of the 4 available frequencies desired. I'm using the 1 Hz square wave, so I set the registers to 00. I'm using an Adafruit RTC wing that has a battery backup, so that setting will be in there for a LONG time. Even if the wing or feather are unplugged, it'll still read 00 from that register. I get the idea, that other chips also have a 0x68 address and you're simply trying to verify that this really is a 3231. It's probably fine for most scenarios, and maybe just a little different comment in the code would do it. Something along the line of those registers controlling the square wave output and you're assuming they're 11, comment out this check if you're sure no other I2C device in your project has an address of 0x68.

caternuson commented 5 years ago

Thanks, that makes sense. I don't think your case is weird. Most people will probably use the RTC with a battery, so it will persist any custom settings to that register.

The intent is to do a simple "is it there?" check. Not only that it's the expected sensor, but even if it's just there. Wiring issues are more common than different sensor/same address problems. Guess we should sharpen the pencil a little and see if there's another way of doing this for the DS3231.

gritnix commented 5 years ago

An i2c.scan() will give back a 104 (0x68) as part of the list. That's probably the easiest way unless there's a reason to avoid that during that phase.

caternuson commented 5 years ago

Right, but is the thing at 0x68 a DS3231 or something else? Ideally, the check does both.

gritnix commented 5 years ago

Right. If the reason for the check is to get an idea of it really being a 3231, then the registers are good. If it's just to check wiring and that something with that address is there, then the scan is really all you need.

With a battery in the board, and the wing even comes like that, the control register is something that you really just can't be sure of. Here's a thought, but it'll mean a 1 second delay. Read the seconds, do a time.sleep(1) and read them again and see if it's offset by 1, or a 59 would of course become a 0. If that actually happens, it's gotta be a clock. :) Could you do a param in the constructor, verify=true or something so if the user knows it's all good, they can opt out of the 1 second delay.

killer4king commented 5 years ago

I'm also running in this kind of issue... I have bunch of feathers M0 RF868 and a variaty of sensors My preference is to make one code that I can upload to them all and they report back the sensors that are connected and their measurements ... so I was indeed assuming that when we start up the sensor, we get something back there is indeed something on this address and what type of sensor... not the case with teh DS3231 for the moment.. looking forward if it can be implemented...

(I do have such a lib for the BME280/BMP280 by Tyler Glenn for instance)

tannewt commented 5 years ago

@caternuson Is correct. The register check is to try and validate we're actually talking to a DS3231. I'd recommend editing the library for your use @gritnix. If there is a place we should document this better I'm open to it. I don't think removing it is good though because most people will leave that register as is.

caternuson commented 4 years ago

@gritnix Took forever to get back to this. But if you can/want, checkout changes made in #20 . It incorporates your suggestion to check for changes in seconds.

caternuson commented 4 years ago

@gritnix Please try the 2.2.1 version of the library when it becomes available. https://github.com/adafruit/Adafruit_CircuitPython_DS3231/releases/tag/2.2.1

Ended up just removing the verification check for now.

caternuson commented 4 years ago

Closing. Can reopen if still an issue.