adafruit / Adafruit_CircuitPython_Register

Python data descriptor classes to represent hardware registers on I2C devices.
MIT License
47 stars 21 forks source link

BCDAlarmTimeRegister does not support minutely on DS3231 #50

Open caternuson opened 1 year ago

caternuson commented 1 year ago

More discussion here: https://forums.adafruit.com/viewtopic.php?t=201104

The registers for Alarm 2 of the DS3231 do not have seconds: alarm_reg

However, "minutely" (once per minute) triggering is supported: alarm

b-blake commented 1 year ago

A short summary: alarm2 has minutes but they are not supported. alarm2 does not have seconds and justly are not supported. Changing line 172 of "i2c_bcd_alarm.py" to frequency < 1 fixes the issue. Status will need to be updated too.

tekktrik commented 1 year ago

Reading the forum, it looks like that line is (nearly) what the fix should be. Am I misunderstanding the issue?

caternuson commented 1 year ago

I think there is more to be investigated.

It seems like there is a base assumption that supporting "minutely" requires seconds to be supported by the RTC: https://github.com/adafruit/Adafruit_CircuitPython_PCF8523/issues/2#issuecomment-336771467

So that logic is correct based on that. But I do not know what drives that requirement.

If the DS3231's alarm2 is an odd ball, then maybe the answer is the DS3231 library can not use BCDAlarmTimeRegister?

b-blake commented 1 year ago

I am currently using minutely without problems from alarm2 on the DS3231 using the fix I have suggested. The part I can't find/fix is where in the other libraries "None" is reported when there is a status request of alarm2 on the DS3231 when minutely has been selected.

The PCF8523 mentioned above is a different chip which should have a different sub-function in the adafruit_register.py library. Limitations of the PCF8523 should not block functionality of the DS3231.

b-blake commented 1 year ago

I just looked at the data sheets on the PCF8523 and the DS3231. The internal registers do not match. e.g. The PCF8523 uses the first three registers for control where the DS3231 uses them for real time seconds, minutes, hours. The PCF8523 has only one alarm so how can it be a reason to limit the second alarm of the DS3231?

b-blake commented 1 year ago

I downloaded fresh all of the library from here and ran it. It still says minutley is not supported. Line 172 of i2c_bcd_alarm.py needs to read: if frequency < 1 and not self.has_seconds: not if frequency <= 1 and not self.has_seconds:

b-blake commented 1 year ago

I found where the minutely vs None issue resides: It is lines 108 - 130. I am not sure what the change should be to follow the github rules, etc.

b-blake commented 1 year ago

This code addresses the issue of 'None' being displayed when 'minutely' is selected for alarm2. ~ Line 108 old Line 172 still needs to be changed to frequency < 1 and not self.has_seconds: The added lines have the ########### marking them.


        frequency = None
        i = 1
        seconds = 0
        if self.has_seconds:
            if (self.buffer[1] & 0x80) != 0: # 0x80 no Seconds, else seconds
                frequency = "secondly" # = 0x80 NO seconds
            else:
                frequency = "minutely" # = 0x80 HAS seconds
                seconds = _bcd2bin(self.buffer[i] & 0x7F) # mask off 0x80
            i = 2
        else: ###############
            frequency = "minutely" ###############
            seconds = _bcd2bin(self.buffer[i] & 0x7F) ###############
        minute = 0