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

setAlarm overwrites AlarmPolarity bit #37

Closed INemesisI closed 5 years ago

INemesisI commented 5 years ago

If setAlarmPolarity(1) is called before setting an alarm through setAlarm() the polarity will be forced to zero again. I don't think this is intended, since changing the polarity after setting up the alarm can cause the MFP to have the wrong polarity for the first few us.

Affected line of code: https://github.com/SV-Zanshin/MCP7940/blob/0678a66569f9ea81a9d959c8c6cce5247a6916eb/src/MCP7940.cpp#L592

Fix: also keep the polarity bit. I also force the dow to be only 3 bit long to prevent an accidental overwrite.

    uint8_t wkdayRegister = readByte(MCP7940_ALM0WKDAY + offset);             //                                  //
    wkdayRegister &= ((1 << MCP7940_ALM0IF) | (1 << MCP7940_ALMPOL));         // Keep pol and alm flag bit        //
    wkdayRegister |= alarmType << 4;                                          // Set 3 bits from alarmType        //
    wkdayRegister |= (dt.dayOfTheWeek() & 0x07);                              // Set 3 bits for dow from date     //

Maybe it would also be beneficial to be able to set the polarity right in the setAlarm() function with an additional optional paramter?

bool setAlarm(const uint8_t alarmNumber, const uint8_t alarmType, const DateTime dt, const bool state = true, const uint8_t polarity = 0);
SV-Zanshin commented 5 years ago

I'll look into this in the next couple of days

INemesisI commented 5 years ago

Thank you very much! After sleeping over it I noticed that adding the polarity to the setAlarm function makes no sense, since it would not work when using two alarms. So you can ignore the last part!

SV-Zanshin commented 5 years ago

Made changes and implemented