From f9bc2cd8e29b8173b593f53bfb917e8dc2bcf05c Mon Sep 17 00:00:00 2001 From: BlackMark Date: Fri, 23 Oct 2026 21:55:02 +0200 Subject: [PATCH] refactor: one alarm encoder, named register bits, and the example 14 B smaller Both alarms' registers come from one encoder that derives DY/DT and the mask bits from the rate, and the battery checks the bytes each Table 2 row writes instead of restating the enum values. Register addresses and bits are named, the year register is the year less 2000 rather than a division, and an alarm day reads back with one decode for both forms. Over the whole API the image is 10 B smaller on both parts; a rate only known at run time costs set_alarm1 6 B more. Co-Authored-By: Claude Opus 5.5 --- README.md | 15 +-- include/ds3231/ds3231.hpp | 208 ++++++++++++++++++++------------------ test/CMakeLists.txt | 8 +- test/consteval.cpp | 59 ++++++----- 4 files changed, 156 insertions(+), 134 deletions(-) diff --git a/README.md b/README.md index d3f2684..4f38141 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ auto now = rtc::read_clock(); // result auto armed = rtc::set_alarm1({}, ds3231::alarm1_rate::once_per_second); ``` -`example/main.cpp` is the full tour. libavr rides as the `libavr/` submodule, +`example/main.cpp` is a complete program. libavr rides as the `libavr/` submodule, pinned to the commit this driver builds against; `LIBAVR_ROOT` (cache or environment) overrides it for development against a working tree: @@ -26,9 +26,10 @@ cmake --build --preset attiny85-generated Presets cover attiny85/atmega328p in both libavr modes (generated and reflect); `ctest` runs the consteval battery over the BCD, the hours -register's two formats, the weekday and the alarm rate encodings, and holds -the example to its stated size — **1480 B** on the mega328P and **1270 B** -on the tiny85, byte-identical across the two modes. The legacy +register's two formats, the weekday, the alarm registers each rate writes and +the control and status write-backs, and holds the example to its stated size — +**1468 B** on the mega328P and **1256 B** on the tiny85, byte-identical across +the two modes. The legacy yazoalfa-based driver lives on the `master` branch. The weekday register is derived from the date by default. A program that @@ -91,6 +92,6 @@ One reference describes one part, hence `--project`. Release is what the gate compares, the presets defining no debug build; Debug carries the `-Og -gdwarf-4` pair libavr's own debug preset uses. -Differences from legacy: weekday-rate alarms now actually set the DY bit -(the old `setAlarmHelper` always cleared it), reads/writes are single -coherent bus transactions, and errors are reported instead of ignored. +Differences from legacy: weekday-rate alarms set the DY bit (the legacy +driver always cleared it), each register block is read or written in one +coherent bus transaction, and errors are reported instead of ignored. diff --git a/include/ds3231/ds3231.hpp b/include/ds3231/ds3231.hpp index 86966a4..dc907f3 100644 --- a/include/ds3231/ds3231.hpp +++ b/include/ds3231/ds3231.hpp @@ -1,9 +1,11 @@ #pragma once #include +#include #include #include #include +#include #include @@ -65,6 +67,19 @@ enum class alarm2_rate : std::uint8_t { namespace detail { +// The register map, datasheet 19-5170 Figure 1. +inline constexpr std::uint8_t reg_seconds = 0x00, reg_day = 0x03, reg_date = 0x04; +inline constexpr std::uint8_t reg_alarm1 = 0x07, reg_alarm2 = 0x0b; +inline constexpr std::uint8_t reg_control = 0x0e, reg_status = 0x0f, reg_temp = 0x11; + +inline constexpr std::uint8_t alarm_mask = 0x80, day_of_week = 0x40; +inline constexpr std::uint8_t twelve_hour = 0x40, pm = 0x20; +inline constexpr std::uint8_t century = 0x80; +inline constexpr std::uint8_t a1ie = 0x01, a2ie = 0x02, intcn = 0x04; +inline constexpr std::uint8_t a1f = 0x01, a2f = 0x02, osf = 0x80; + +inline constexpr std::uint16_t first_year = 2000; + constexpr std::uint8_t to_bcd(std::uint8_t value) { return static_cast(((value / 10) << 4) | (value % 10)); @@ -75,19 +90,18 @@ constexpr std::uint8_t from_bcd(std::uint8_t value) return static_cast((value >> 4) * 10 + (value & 0x0f)); } -// Hours register: bit 6 selects 12-hour mode, bit 5 is then PM. Writes -// always use 24-hour form; reads accept either. +// Writes always use the 24-hour form; reads accept either. constexpr std::uint8_t hours_from_reg(std::uint8_t reg) { - if (reg & 0x40) { - auto hour = from_bcd(reg & 0x1f); - bool pm = reg & 0x20; + if (reg & twelve_hour) { + const auto hour = from_bcd(reg & ~(alarm_mask | twelve_hour | pm)); + const bool after_noon = reg & pm; if (hour == 12) { - return pm ? 12 : 0; + return after_noon ? 12 : 0; } - return static_cast(pm ? hour + 12 : hour); + return static_cast(after_noon ? hour + 12 : hour); } - return from_bcd(reg & 0x3f); + return from_bcd(reg & ~(alarm_mask | twelve_hour)); } // Sakamoto's method; the device counts weekdays 1..7 with a free epoch, @@ -101,9 +115,32 @@ constexpr std::uint8_t weekday(std::uint16_t year, std::uint8_t month, std::uint return static_cast((year + year / 4 - year / 100 + year / 400 + offsets[month - 1] + day) % 7 + 1); } -// Control and status bits, datasheet 19-5170 Figure 1. -inline constexpr std::uint8_t a1ie = 0x01, a2ie = 0x02, intcn = 0x04; -inline constexpr std::uint8_t a1f = 0x01, a2f = 0x02, osf = 0x80; +constexpr time_of_day time_from_registers(std::span raw) +{ + return {hours_from_reg(raw[2]), from_bcd(raw[1] & ~alarm_mask), from_bcd(raw[0] & ~alarm_mask)}; +} + +// Rate bit n is the mask bit of the alarm's n-th register and the bit above +// them is DY/DT (Table 2). Alarm 2 has no seconds register, so its registers +// are alarm 1's last three. +inline constexpr std::size_t alarm1_registers = reg_alarm2 - reg_alarm1; +inline constexpr std::size_t alarm2_registers = reg_control - reg_alarm2; + +template +constexpr std::array alarm_registers(const date_time &at, std::uint8_t rate) +{ + const bool weekday_mode = (rate >> Count) & 1; + const std::array fields{ + to_bcd(at.second), + to_bcd(at.minute), + to_bcd(at.hour), + weekday_mode ? static_cast(day_of_week | at.day) : to_bcd(at.day), + }; + return [&](std::index_sequence) { + return std::array{ + static_cast(fields[alarm1_registers - Count + I] | (((rate >> I) & 1) ? alarm_mask : 0))...}; + }(std::make_index_sequence{}); +} constexpr std::uint8_t control_with(std::uint8_t control, std::uint8_t enable_bit, alarm_interrupt interrupt) { @@ -125,10 +162,9 @@ constexpr std::uint8_t status_clearing(std::uint8_t status, std::uint8_t flag) // address pins (datasheet 19-5170, "I2C interface"). inline constexpr std::uint8_t bus_address = 0x68; -// Maxim DS3231 RTC on any libavr i2c bus (register map: datasheet -// 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 +// Maxim DS3231 RTC on any libavr i2c bus: the master role is taken from the +// bus's own resolution, so a declared `dev::i2c<...>` is handed over whole. +// Every read is one bus transaction; errors surface as std::expected. Weekday // registers are derived from the date unless the program owns them. template class device { @@ -136,25 +172,18 @@ class device { using dev = avr::i2c::device; - static constexpr std::uint8_t reg_clock = 0x00; - static constexpr std::uint8_t reg_alarm1 = 0x07; - static constexpr std::uint8_t reg_alarm2 = 0x0b; - static constexpr std::uint8_t reg_control = 0x0e; - static constexpr std::uint8_t reg_status = 0x0f; - static constexpr std::uint8_t reg_temp = 0x11; - public: [[nodiscard]] static result read_clock() { std::array raw; - if (auto s = dev::read_regs(reg_clock, raw); !s) { + if (auto s = dev::read_regs(detail::reg_seconds, raw); !s) { return std::unexpected(s.error()); } date_time now; - 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])); + static_cast(now) = detail::time_from_registers(raw); + now.day = detail::from_bcd(raw[4]); + now.month = detail::from_bcd(raw[5] & ~detail::century); + now.year = static_cast(detail::first_year + detail::from_bcd(raw[6])); return now; } @@ -170,50 +199,50 @@ class device { [[nodiscard]] static result read_time() { std::array raw; - if (auto s = dev::read_regs(reg_clock, raw); !s) { + if (auto s = dev::read_regs(detail::reg_seconds, raw); !s) { return std::unexpected(s.error()); } - return decode_time(raw); + return detail::time_from_registers(raw); } [[nodiscard]] static status write_clock(const date_time &now) { - 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 { + if constexpr (computes_weekday) { 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); + detail::to_bcd(static_cast(now.year - detail::first_year))}; + return dev::write_regs(detail::reg_seconds, raw); + } else { + // 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); } } [[nodiscard]] static status write_date(const date &value) { + const auto year = detail::to_bcd(static_cast(value.year - detail::first_year)); 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); + detail::to_bcd(value.month), year}; + return dev::write_regs(detail::reg_day, raw); } else { - std::array raw{detail::to_bcd(value.day), detail::to_bcd(value.month), - detail::to_bcd(static_cast(value.year % 100))}; - return dev::write_regs(reg_clock + 4, raw); + std::array raw{detail::to_bcd(value.day), detail::to_bcd(value.month), year}; + return dev::write_regs(detail::reg_date, raw); } } [[nodiscard]] static status write_time(const time_of_day &value) { std::array raw{detail::to_bcd(value.second), detail::to_bcd(value.minute), detail::to_bcd(value.hour)}; - return dev::write_regs(reg_clock, raw); + return dev::write_regs(detail::reg_seconds, raw); } // Alarm times use .day as the date of month, or as weekday 1..7 with @@ -221,57 +250,25 @@ class device { [[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)), - static_cast(detail::to_bcd(at.minute) | (((m >> 1) & 1) << 7)), - static_cast(detail::to_bcd(at.hour) | (((m >> 2) & 1) << 7)), - static_cast(day_date(at.day, rate == alarm1_rate::weekday_time_match) | - (((m >> 3) & 1) << 7))}; - if (auto s = dev::write_regs(reg_alarm1, raw); !s) { - return s; - } - return set_alarm_interrupt(detail::a1ie, interrupt); + return set_alarm(detail::reg_alarm1, at, std::to_underlying(rate), detail::a1ie, + interrupt); } [[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)), - static_cast(detail::to_bcd(at.hour) | (((m >> 1) & 1) << 7)), - static_cast(day_date(at.day, rate == alarm2_rate::weekday_time_match) | - (((m >> 2) & 1) << 7))}; - if (auto s = dev::write_regs(reg_alarm2, raw); !s) { - return s; - } - return set_alarm_interrupt(detail::a2ie, interrupt); + return set_alarm(detail::reg_alarm2, at, std::to_underlying(rate), detail::a2ie, + interrupt); } [[nodiscard]] static result read_alarm1() { - std::array raw; - if (auto s = dev::read_regs(reg_alarm1, raw); !s) { - return std::unexpected(s.error()); - } - date_time at{}; - at.second = detail::from_bcd(raw[0] & 0x7f); - at.minute = detail::from_bcd(raw[1] & 0x7f); - at.hour = detail::hours_from_reg(raw[2] & 0x7f); - at.day = (raw[3] & 0x40) ? (raw[3] & 0x0f) : detail::from_bcd(raw[3] & 0x3f); - return at; + return read_alarm(detail::reg_alarm1); } [[nodiscard]] static result read_alarm2() { - std::array raw; - if (auto s = dev::read_regs(reg_alarm2, raw); !s) { - return std::unexpected(s.error()); - } - date_time at{}; - at.minute = detail::from_bcd(raw[0] & 0x7f); - at.hour = detail::hours_from_reg(raw[1] & 0x7f); - at.day = (raw[2] & 0x40) ? (raw[2] & 0x0f) : detail::from_bcd(raw[2] & 0x3f); - return at; + return read_alarm(detail::reg_alarm2); } [[nodiscard]] static result alarm1_fired() @@ -310,7 +307,7 @@ class device { [[nodiscard]] static result temperature_quarters() { std::array raw; - if (auto s = dev::read_regs(reg_temp, raw); !s) { + if (auto s = dev::read_regs(detail::reg_temp, raw); !s) { return std::unexpected(s.error()); } return static_cast((static_cast(static_cast(raw[0])) << 2) | @@ -324,50 +321,65 @@ class device { { return set_alarm_interrupt(detail::a1ie, alarm_interrupt::on); } + [[nodiscard]] static status mute_alarm1() { return set_alarm_interrupt(detail::a1ie, alarm_interrupt::off); } + [[nodiscard]] static status listen_alarm2() { return set_alarm_interrupt(detail::a2ie, alarm_interrupt::on); } + [[nodiscard]] static status mute_alarm2() { return set_alarm_interrupt(detail::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) + template + static status set_alarm(std::uint8_t reg, const date_time &at, std::uint8_t rate, std::uint8_t enable_bit, + alarm_interrupt interrupt) { - return {detail::hours_from_reg(clock[2]), detail::from_bcd(clock[1] & 0x7f), detail::from_bcd(clock[0] & 0x7f)}; + if (auto s = dev::write_regs(reg, detail::alarm_registers(at, rate)); !s) { + return s; + } + return set_alarm_interrupt(enable_bit, interrupt); } - static constexpr std::uint8_t day_date(std::uint8_t day, bool weekday_mode) + template + static result read_alarm(std::uint8_t reg) { - if (weekday_mode) { - return static_cast(0x40 | (day & 0x0f)); + std::array raw; + if (auto s = dev::read_regs(reg, raw); !s) { + return std::unexpected(s.error()); } - return detail::to_bcd(day) & std::uint8_t{0x3f}; + constexpr auto first = detail::alarm1_registers - Count; + date_time at{}; + if constexpr (first == 0) { + at.second = detail::from_bcd(raw[0] & ~detail::alarm_mask); + } + at.minute = detail::from_bcd(raw[1 - first] & ~detail::alarm_mask); + at.hour = detail::hours_from_reg(raw[2 - first]); + at.day = detail::from_bcd(raw[3 - first] & ~(detail::alarm_mask | detail::day_of_week)); + return at; } // INTCN routes the alarms to INT/SQW, 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); + auto control = dev::read_reg(detail::reg_control); if (!control) { return std::unexpected(control.error()); } - return dev::write_reg(reg_control, detail::control_with(*control, enable_bit, interrupt)); + return dev::write_reg(detail::reg_control, detail::control_with(*control, enable_bit, interrupt)); } static result flag_set(std::uint8_t bit) { - auto flags = dev::read_reg(reg_status); + auto flags = dev::read_reg(detail::reg_status); if (!flags) { return std::unexpected(flags.error()); } @@ -376,11 +388,11 @@ class device { static status clear_flag(std::uint8_t bit) { - auto flags = dev::read_reg(reg_status); + auto flags = dev::read_reg(detail::reg_status); if (!flags) { return std::unexpected(flags.error()); } - return dev::write_reg(reg_status, detail::status_clearing(*flags, bit)); + return dev::write_reg(detail::reg_status, detail::status_clearing(*flags, bit)); } }; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b8f11c2..d6bd91a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -3,9 +3,9 @@ libavr_format_test() # The example's size is stated in the README, per chip, and a library advance # that moves it silently is what this catches. if(LIBAVR_MCU STREQUAL "atmega328p") - libavr_size_claim_test(clock 1480) + libavr_size_claim_test(clock 1468) elseif(LIBAVR_MCU STREQUAL "attiny85") - libavr_size_claim_test(clock 1270) + libavr_size_claim_test(clock 1256) endif() # The README says the two modes emit the same image, and only a tree with @@ -21,7 +21,5 @@ add_test(NAME ds3231.consteval COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target consteval_tests) # A check this host cannot run reads as Skipped rather than Failed: the marker -# a stubbed launcher prints, over every test registered here. Nothing in this -# suite is host-gated today, and this is what keeps that a property of the -# suite rather than of whoever adds the next check. +# a stubbed launcher prints, over every test registered here. libavr_skip_unverified() diff --git a/test/consteval.cpp b/test/consteval.cpp index dc581e0..1765fe5 100644 --- a/test/consteval.cpp +++ b/test/consteval.cpp @@ -1,9 +1,10 @@ // 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. +// formats, Sakamoto's weekday, the alarm registers each rate writes, and the +// control and status values written back. +#include #include #include @@ -53,31 +54,41 @@ 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. +// The alarm registers each rate writes (19-5170 Table 2): bit 7 of the n-th +// register is AxM(n+1), and DY/DT is bit 6 of the day register. Rows are read +// off the table, so a slip in the rate values or in their distribution over +// the registers arms the wrong alarm here first. 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); +using ds3231::detail::alarm_registers; +constexpr ds3231::date_time at{{2026, 1, 15}, {12, 30, 45}}; +constexpr ds3231::date_time on_weekday{{2026, 1, 3}, {7, 5, 0}}; -// 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); +template +constexpr auto alarm1(const ds3231::date_time &value, Rate rate) +{ + return alarm_registers(value, static_cast(rate)); +} + +template +constexpr auto alarm2(const ds3231::date_time &value, Rate rate) +{ + return alarm_registers(value, static_cast(rate)); +} + +static_assert(alarm1(at, alarm1_rate::once_per_second) == std::to_array({0xc5, 0xb0, 0x92, 0x95})); +static_assert(alarm1(at, alarm1_rate::seconds_match) == std::to_array({0x45, 0xb0, 0x92, 0x95})); +static_assert(alarm1(at, alarm1_rate::minutes_seconds_match) == std::to_array({0x45, 0x30, 0x92, 0x95})); +static_assert(alarm1(at, alarm1_rate::time_match) == std::to_array({0x45, 0x30, 0x12, 0x95})); +static_assert(alarm1(at, alarm1_rate::date_time_match) == std::to_array({0x45, 0x30, 0x12, 0x15})); +static_assert(alarm1(on_weekday, alarm1_rate::weekday_time_match) == + std::to_array({0x00, 0x05, 0x07, 0x43})); + +static_assert(alarm2(at, alarm2_rate::once_per_minute) == std::to_array({0xb0, 0x92, 0x95})); +static_assert(alarm2(at, alarm2_rate::minutes_match) == std::to_array({0x30, 0x92, 0x95})); +static_assert(alarm2(at, alarm2_rate::time_match) == std::to_array({0x30, 0x12, 0x95})); +static_assert(alarm2(at, alarm2_rate::date_time_match) == std::to_array({0x30, 0x12, 0x15})); +static_assert(alarm2(on_weekday, alarm2_rate::weekday_time_match) == std::to_array({0x05, 0x07, 0x43})); // A1F and A2F ignore a written 1 and OSF holds until written 0 (19-5170, // Status Register): clearing one alarm must write the other as 1, or an alarm