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<bus, false>` 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 <noreply@anthropic.com>

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()

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<bus, ds3231::weekday_source::external>;
+```
+
+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
 

include/ds3231/ds3231.hpp

@@ -3,6 +3,7 @@
 #include <array>
 #include <cstdint>
 #include <expected>
+#include <span>
 
 #include <libavr/i2c.hpp>
 
@@ -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.
+// registers are derived from the date unless the program owns them.
-template <typename Bus, bool SetWeekday = true>
+template <typename Bus, weekday_source Weekday = weekday_source::computed>
 class device {
+	static constexpr bool computes_weekday = Weekday == weekday_source::computed;
+
 	using dev = avr::i2c::device<typename Bus::master, bus_address>;
 
 	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);
+		static_cast<time_of_day &>(now) = decode_time(raw);
-		now.minute = detail::from_bcd(raw[1] & 0x7f);
-		now.hour = detail::hours_from_reg(raw[2]);
 		now.day = detail::from_bcd(raw[4] & 0x3f);
 		now.month = detail::from_bcd(raw[5] & 0x1f);
 		now.year = static_cast<std::uint16_t>(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),
+		return decode_time(raw);
-		                   detail::from_bcd(raw[0] & 0x7f)};
 	}
 
 	[[nodiscard]] static status write_clock(const date_time &now)
 	{
-		std::array raw{detail::to_bcd(now.second),
+		if constexpr (!computes_weekday) {
-		               detail::to_bcd(now.minute),
+			// Two transactions, because the weekday register sits between the
-		               detail::to_bcd(now.hour),
+			// time and the date and this mode may not write it.
-		               SetWeekday ? detail::weekday(now.year, now.month, now.day) : std::uint8_t{1},
+			if (auto s = write_time(now); !s) {
-		               detail::to_bcd(now.day),
+				return s;
-		               detail::to_bcd(now.month),
+			}
-		               detail::to_bcd(static_cast<std::uint8_t>(now.year % 100))};
+			return write_date(now);
-		return dev::write_regs(reg_clock, raw);
+		} 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<std::uint8_t>(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<std::uint8_t>(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<std::uint8_t>(rate);
 		std::array raw{static_cast<std::uint8_t>(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<std::uint8_t>(rate);
 		std::array raw{static_cast<std::uint8_t>(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<date_time> 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<const std::uint8_t> 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<std::uint8_t>((*control & ~bbsqw) | intcn | enable_bit));
+		const auto wired = static_cast<std::uint8_t>((*control & ~bbsqw) | intcn | enable_bit);
+		const auto muted = static_cast<std::uint8_t>(*control & ~enable_bit);
+		return dev::write_reg(reg_control, interrupt == alarm_interrupt::on ? wired : muted);
 	}
 
 	static result<bool> flag_set(std::uint8_t bit)

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)

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 <cstdint>
+
+#include <ds3231/ds3231.hpp>
+
+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<std::uint8_t>(alarm1_rate::once_per_second) == 0b01111);
+static_assert(static_cast<std::uint8_t>(alarm1_rate::seconds_match) == 0b01110);
+static_assert(static_cast<std::uint8_t>(alarm1_rate::minutes_seconds_match) == 0b01100);
+static_assert(static_cast<std::uint8_t>(alarm1_rate::time_match) == 0b01000);
+static_assert(static_cast<std::uint8_t>(alarm1_rate::date_time_match) == 0b00000);
+static_assert(static_cast<std::uint8_t>(alarm1_rate::weekday_time_match) == 0b10000);
+
+using ds3231::alarm2_rate;
+static_assert(static_cast<std::uint8_t>(alarm2_rate::once_per_minute) == 0b0111);
+static_assert(static_cast<std::uint8_t>(alarm2_rate::minutes_match) == 0b0110);
+static_assert(static_cast<std::uint8_t>(alarm2_rate::time_match) == 0b0100);
+static_assert(static_cast<std::uint8_t>(alarm2_rate::date_time_match) == 0b0000);
+static_assert(static_cast<std::uint8_t>(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<std::uint8_t>(alarm2_rate::once_per_minute) ==
+              static_cast<std::uint8_t>(alarm1_rate::once_per_second) >> 1);
+static_assert(static_cast<std::uint8_t>(alarm2_rate::weekday_time_match) ==
+              static_cast<std::uint8_t>(alarm1_rate::weekday_time_match) >> 1);
+
+} // namespace