From 2e9c929a19d8bd8b22ccd148a379f1c0c48c7960 Mon Sep 17 00:00:00 2001 From: BlackMark Date: Wed, 12 Aug 2026 15:40:52 +0200 Subject: [PATCH] fix: an alarm's interrupt could be turned on and never off, and nothing tested any of it `set_alarm1/2`'s `enable_interrupt = false` did nothing at all - it skipped the control-register write rather than clearing A1IE/A2IE - and there was no other way to clear them, so an alarm armed once with its interrupt on drove INT/SQW for good. It is `alarm_interrupt::off` now and it clears the bit, with `listen_alarm1/2()` and `mute_alarm1/2()` beside it for a caller that arms once and changes its mind later. The bare bool went with it (rule 37): `set_alarm1(at, rate, false)` said nothing at the point of use. `bool SetWeekday` was the same shape one level up - `device` names nothing - and its two writers disagreed about what it meant: `write_date` skipped the weekday register, `write_clock` stamped a placeholder 1 into it, so a program that turned the maintenance off still had the register overwritten and no way to set it. `weekday_source::external` now means the register is the program's, in both writers. test/consteval.cpp is the battery this driver never had, over the arithmetic that has no bus in it: the BCD round trip across every representable value and its nibble layout, the hours register in both formats including all four noon/midnight cases, Sakamoto's weekday against six calendar dates (leap day, the day after it, and 2100's missing leap), and both alarms' rate encodings transcribed against 19-5170 Table 2 - including that alarm 2's masks sit one place below alarm 1's, which is the property that makes one distribution loop wrong for the other. Red-green: two assertions fire on a flipped DY bit. Beside it, one decode rather than two: `read_clock` and `read_time` spelled the seconds/minutes/hours triplet out separately (rule 6). Every image byte-identical on both chips in both modes. Co-Authored-By: Claude Opus 5 --- CMakeLists.txt | 2 + README.md | 21 +++++++- include/ds3231/ds3231.hpp | 100 +++++++++++++++++++++++++++++--------- test/CMakeLists.txt | 7 +++ test/consteval.cpp | 82 +++++++++++++++++++++++++++++++ 5 files changed, 189 insertions(+), 23 deletions(-) create mode 100644 test/CMakeLists.txt create mode 100644 test/consteval.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 697b98d..227b970 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -22,4 +22,6 @@ target_link_libraries(ds3231 INTERFACE libavr) if(PROJECT_IS_TOP_LEVEL) add_subdirectory(example) + enable_testing() + add_subdirectory(test) endif() diff --git a/README.md b/README.md index 98c322b..f753f10 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,26 @@ cmake --build --preset attiny85-generated ``` Presets cover attiny85/atmega328p in both libavr modes (generated and -reflect). The legacy yazoalfa-based driver lives on the `master` branch. +reflect); `ctest` runs the consteval battery over the BCD, the hours +register's two formats, the weekday and the alarm rate encodings. The legacy +yazoalfa-based driver lives on the `master` branch. + +The weekday register is derived from the date by default. A program that +assigns its own meaning to the device's 1..7 takes ownership of it, and no +write here touches it: + +```cpp +using rtc = ds3231::device; +``` + +An alarm arms its INT/SQW output unless told otherwise, and can be muted and +re-wired without re-arming: + +```cpp +auto quiet = rtc::set_alarm2(at, ds3231::alarm2_rate::minutes_match, + ds3231::alarm_interrupt::off); +auto again = rtc::listen_alarm2(); +``` ## Atmel Studio diff --git a/include/ds3231/ds3231.hpp b/include/ds3231/ds3231.hpp index 4f8a269..76d677d 100644 --- a/include/ds3231/ds3231.hpp +++ b/include/ds3231/ds3231.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -42,6 +43,18 @@ enum class alarm1_rate : std::uint8_t { weekday_time_match = 0b10000, }; +// Who owns the weekday register. The device counts 1..7 with a free epoch and +// maintains nothing itself, so either this driver derives it from the date on +// every write, or the register belongs to the program and no write here +// touches it - which is what a caller that assigns its own meaning to those +// seven values needs. +enum class weekday_source : std::uint8_t { computed, external }; + +// Whether arming an alarm also wires its INT/SQW output. `off` is not +// silence about the bit - it clears A1IE/A2IE, so an alarm armed with the +// interrupt on and re-armed with it off stops driving the pin. +enum class alarm_interrupt : std::uint8_t { off, on }; + enum class alarm2_rate : std::uint8_t { once_per_minute = 0b0111, minutes_match = 0b0110, @@ -98,9 +111,11 @@ inline constexpr std::uint8_t bus_address = 0x68; // 19-5170 Table 1): the master role is taken from the bus's own // resolution, so a declared `dev::i2c<...>` is handed over whole. Every // call is one bus transaction; errors surface as std::expected. Weekday -// registers maintain themselves from the date when SetWeekday is on. -template +// registers are derived from the date unless the program owns them. +template class device { + static constexpr bool computes_weekday = Weekday == weekday_source::computed; + using dev = avr::i2c::device; static constexpr std::uint8_t reg_clock = 0x00; @@ -121,9 +136,7 @@ class device { return std::unexpected(s.error()); } date_time now; - now.second = detail::from_bcd(raw[0] & 0x7f); - now.minute = detail::from_bcd(raw[1] & 0x7f); - now.hour = detail::hours_from_reg(raw[2]); + static_cast(now) = decode_time(raw); now.day = detail::from_bcd(raw[4] & 0x3f); now.month = detail::from_bcd(raw[5] & 0x1f); now.year = static_cast(2000 + detail::from_bcd(raw[6])); @@ -145,25 +158,33 @@ class device { if (auto s = dev::read_regs(reg_clock, raw); !s) { return std::unexpected(s.error()); } - return time_of_day{detail::hours_from_reg(raw[2]), detail::from_bcd(raw[1] & 0x7f), - detail::from_bcd(raw[0] & 0x7f)}; + return decode_time(raw); } [[nodiscard]] static status write_clock(const date_time &now) { - std::array raw{detail::to_bcd(now.second), - detail::to_bcd(now.minute), - detail::to_bcd(now.hour), - SetWeekday ? detail::weekday(now.year, now.month, now.day) : std::uint8_t{1}, - detail::to_bcd(now.day), - detail::to_bcd(now.month), - detail::to_bcd(static_cast(now.year % 100))}; - return dev::write_regs(reg_clock, raw); + if constexpr (!computes_weekday) { + // Two transactions, because the weekday register sits between the + // time and the date and this mode may not write it. + if (auto s = write_time(now); !s) { + return s; + } + return write_date(now); + } else { + std::array raw{detail::to_bcd(now.second), + detail::to_bcd(now.minute), + detail::to_bcd(now.hour), + detail::weekday(now.year, now.month, now.day), + detail::to_bcd(now.day), + detail::to_bcd(now.month), + detail::to_bcd(static_cast(now.year % 100))}; + return dev::write_regs(reg_clock, raw); + } } [[nodiscard]] static status write_date(const date &value) { - if constexpr (SetWeekday) { + if constexpr (computes_weekday) { std::array raw{detail::weekday(value.year, value.month, value.day), detail::to_bcd(value.day), detail::to_bcd(value.month), detail::to_bcd(static_cast(value.year % 100))}; return dev::write_regs(reg_clock + 3, raw); @@ -182,7 +203,8 @@ class device { // Alarm times use .day as the date of month, or as weekday 1..7 with // the weekday_time_match rates. - [[nodiscard]] static status set_alarm1(const date_time &at, alarm1_rate rate, bool enable_interrupt = true) + [[nodiscard]] static status set_alarm1(const date_time &at, alarm1_rate rate, + alarm_interrupt interrupt = alarm_interrupt::on) { auto m = static_cast(rate); std::array raw{static_cast(detail::to_bcd(at.second) | ((m & 1) << 7)), @@ -193,10 +215,11 @@ class device { if (auto s = dev::write_regs(reg_alarm1, raw); !s) { return s; } - return enable_interrupt ? enable_alarm_interrupt(a1ie) : status{}; + return set_alarm_interrupt(a1ie, interrupt); } - [[nodiscard]] static status set_alarm2(const date_time &at, alarm2_rate rate, bool enable_interrupt = true) + [[nodiscard]] static status set_alarm2(const date_time &at, alarm2_rate rate, + alarm_interrupt interrupt = alarm_interrupt::on) { auto m = static_cast(rate); std::array raw{static_cast(detail::to_bcd(at.minute) | ((m & 1) << 7)), @@ -206,7 +229,7 @@ class device { if (auto s = dev::write_regs(reg_alarm2, raw); !s) { return s; } - return enable_interrupt ? enable_alarm_interrupt(a2ie) : status{}; + return set_alarm_interrupt(a2ie, interrupt); } [[nodiscard]] static result read_alarm1() @@ -279,7 +302,35 @@ class device { (raw[1] >> 6)); } + // The alarms' INT/SQW wiring on its own, for a caller that arms once and + // mutes later. Alarm 1 and alarm 2 share INTCN, so muting one leaves the + // other's route intact. + [[nodiscard]] static status listen_alarm1() + { + return set_alarm_interrupt(a1ie, alarm_interrupt::on); + } + [[nodiscard]] static status mute_alarm1() + { + return set_alarm_interrupt(a1ie, alarm_interrupt::off); + } + [[nodiscard]] static status listen_alarm2() + { + return set_alarm_interrupt(a2ie, alarm_interrupt::on); + } + [[nodiscard]] static status mute_alarm2() + { + return set_alarm_interrupt(a2ie, alarm_interrupt::off); + } + private: + // Seconds, minutes and hours decode the same way wherever they are read + // from - the high bit of the first two is reserved, and the hours register + // carries its own 12/24-hour flag. + static constexpr time_of_day decode_time(std::span clock) + { + return {detail::hours_from_reg(clock[2]), detail::from_bcd(clock[1] & 0x7f), detail::from_bcd(clock[0] & 0x7f)}; + } + static constexpr std::uint8_t day_date(std::uint8_t day, bool weekday_mode) { if (weekday_mode) { @@ -288,13 +339,18 @@ class device { return detail::to_bcd(day) & std::uint8_t{0x3f}; } - static status enable_alarm_interrupt(std::uint8_t enable_bit) + // INTCN routes the alarms to INT/SQW and BBSQW is the square wave that + // would otherwise drive it, so arming an alarm's interrupt takes the pin + // and muting it gives up only that alarm's claim on it. + static status set_alarm_interrupt(std::uint8_t enable_bit, alarm_interrupt interrupt) { auto control = dev::read_reg(reg_control); if (!control) { return std::unexpected(control.error()); } - return dev::write_reg(reg_control, static_cast((*control & ~bbsqw) | intcn | enable_bit)); + const auto wired = static_cast((*control & ~bbsqw) | intcn | enable_bit); + const auto muted = static_cast(*control & ~enable_bit); + return dev::write_reg(reg_control, interrupt == alarm_interrupt::on ? wired : muted); } static result flag_set(std::uint8_t bit) diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt new file mode 100644 index 0000000..0e0d797 --- /dev/null +++ b/test/CMakeLists.txt @@ -0,0 +1,7 @@ +# The battery is a compile: a static_assert that fails is the failure. It is a +# target rather than only a ctest so a plain build catches a regression too. +add_library(consteval_tests OBJECT consteval.cpp) +target_link_libraries(consteval_tests PRIVATE ds3231) + +add_test(NAME ds3231.consteval + COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target consteval_tests) diff --git a/test/consteval.cpp b/test/consteval.cpp new file mode 100644 index 0000000..0541b51 --- /dev/null +++ b/test/consteval.cpp @@ -0,0 +1,82 @@ +// Compile-only battery, built with the cross compiler so the target's 16-bit +// int is exercised. Everything in this driver that is arithmetic rather than a +// bus transaction lives here: the BCD both ways, the hours register's two +// formats, Sakamoto's weekday, and the alarm rate encodings as the datasheet +// tabulates them. + +#include + +#include + +namespace { + +using ds3231::detail::from_bcd; +using ds3231::detail::hours_from_reg; +using ds3231::detail::to_bcd; + +// BCD is a round trip over every value the registers hold, and the encoding is +// the datasheet's: tens in the high nibble, units in the low one. +consteval bool bcd_round_trips() +{ + for (std::uint8_t value = 0; value < 100; ++value) { + const std::uint8_t packed = to_bcd(value); + if (from_bcd(packed) != value) { + return false; + } + if ((packed >> 4) != value / 10 || (packed & 0x0f) != value % 10) { + return false; + } + } + return true; +} +static_assert(bcd_round_trips()); + +// The hours register (19-5170 Table 1): bit 6 selects 12-hour mode and bit 5 +// is then PM. Both noons and both midnights are the cases a naive decode gets +// wrong - 12 AM is hour 0 and 12 PM is hour 12, neither of which is 12 + 12. +static_assert(hours_from_reg(to_bcd(0)) == 0); // 24-hour midnight +static_assert(hours_from_reg(to_bcd(13)) == 13); // 24-hour afternoon +static_assert(hours_from_reg(to_bcd(23)) == 23); +static_assert(hours_from_reg(0x40 | to_bcd(12)) == 0); // 12 AM +static_assert(hours_from_reg(0x40 | 0x20 | to_bcd(12)) == 12); // 12 PM +static_assert(hours_from_reg(0x40 | to_bcd(1)) == 1); // 1 AM +static_assert(hours_from_reg(0x40 | 0x20 | to_bcd(1)) == 13); // 1 PM +static_assert(hours_from_reg(0x40 | 0x20 | to_bcd(11)) == 23); // 11 PM + +// Sakamoto's method, against dates a calendar can be checked against rather +// than against this implementation run twice. Sunday is 1. +using ds3231::detail::weekday; +static_assert(weekday(2000, 1, 1) == 7); // Saturday +static_assert(weekday(2024, 1, 1) == 2); // Monday +static_assert(weekday(2024, 2, 29) == 5); // Thursday, the leap day +static_assert(weekday(2024, 3, 1) == 6); // Friday, the day after it +static_assert(weekday(2100, 3, 1) == 2); // Monday - 2100 is not a leap year +static_assert(weekday(2026, 8, 12) == 4); // Wednesday + +// The alarm rate encodings (19-5170 Table 2), transcribed: the low bits are +// A1M4..A1M1 in register order and the high one is DY/DT. A rate is the mask +// pattern its row names, and a transcription slip here arms the wrong alarm. +using ds3231::alarm1_rate; +static_assert(static_cast(alarm1_rate::once_per_second) == 0b01111); +static_assert(static_cast(alarm1_rate::seconds_match) == 0b01110); +static_assert(static_cast(alarm1_rate::minutes_seconds_match) == 0b01100); +static_assert(static_cast(alarm1_rate::time_match) == 0b01000); +static_assert(static_cast(alarm1_rate::date_time_match) == 0b00000); +static_assert(static_cast(alarm1_rate::weekday_time_match) == 0b10000); + +using ds3231::alarm2_rate; +static_assert(static_cast(alarm2_rate::once_per_minute) == 0b0111); +static_assert(static_cast(alarm2_rate::minutes_match) == 0b0110); +static_assert(static_cast(alarm2_rate::time_match) == 0b0100); +static_assert(static_cast(alarm2_rate::date_time_match) == 0b0000); +static_assert(static_cast(alarm2_rate::weekday_time_match) == 0b1000); + +// Alarm 2 has no seconds register, so its mask bits are one place lower than +// alarm 1's throughout - the property that makes one distribution loop wrong +// for the other. +static_assert(static_cast(alarm2_rate::once_per_minute) == + static_cast(alarm1_rate::once_per_second) >> 1); +static_assert(static_cast(alarm2_rate::weekday_time_match) == + static_cast(alarm1_rate::weekday_time_match) >> 1); + +} // namespace