From 4633c20979ef628a28b3dd1266903c4196ffb55b Mon Sep 17 00:00:00 2001 From: Nehal Patel Date: Sun, 6 Sep 2026 18:39:29 +0000 Subject: [PATCH] refactor(arm): configure peripherals in their constructors GpioPin and I2CBus now bring their hardware up as they are constructed. configured_/initialized_ and the eight kInvalidState guards they gated are gone, NucleoF767ZiBoard's member list is a description of the board rather than a set of promises Init() has to keep, and Init() is left with InitSysTick(), which needs the NVIC and so genuinely cannot run earlier. #37 assumed a constructor could not touch registers, because the board is a namespace-scope object built before the clock tree is up. That does not hold. Reset_Handler copies .data, zeroes .bss and calls SystemInit *before* __libc_init_array; SystemInit only enables the FPU and sets VTOR; no code here configures clocks at all, since the F767 runs on HSI at 16 MHz out of reset, which every timing constant already assumes; and RCC is live from reset, with each driver enabling its own peripheral clock first. Two comments stated that ordering backwards and are corrected here. That makes the fixes #37 proposed unnecessary. A factory with a private constructor is the right answer *given* the no-registers rule, but once the rule goes it guards a state that cannot occur -- and it forces std::optional members, which model a hardware absence that cannot happen on a fixed board. A friend declaration constrains who constructs, not who configures. A readiness gate is vacuous when declaring the member is the bring-up. Three invariants hold this up, documented on NucleoF767ZiBoard: bring-up must not fail, must not call mcu::Delay (SysTick is not up yet), and this board must stay the only object with a dynamic initializer. mcu::Usart violates the first and stays two-phase, which suits it -- only the application knows its UartConfig, and Init() validates it and can fail. Bus speed becomes a constructor argument with a derived TIMINGR table, deliberately undefaulted so a board must name the rate its wiring can carry. Both values are checked against the I2C-bus rev.03 limits rather than copied from ST's spreadsheet; the previous comment credited a table in AN4235 that does not exist, and that document covers F0/F3 rather than F7. arm_cm7 gains its first test. Its public headers name no vendor type, so test_peripheral_contract.cpp compiles on the host whichever backend is selected and asserts that a pin cannot be built without a direction, nor a bus without a speed. It proves the types, not the register writes. HostPin takes its direction at construction to match. HostBoard still builds its peripherals in Init(), which is a real difference and not an oversight: an emulated peripheral depends on a socket that can fail, a real one only on registers that are always there. board::Board and both applications are untouched. Flash drops 100-152 bytes per image and .bss by 4; .init_array still holds exactly crtbegin's entry plus the board. Not yet verified on hardware: blinky, the button, an I2C round trip and uart_echo on /dev/ttyACM0 all still need a physical boot. kFast400kHz is likewise unexercised -- nothing selects it today. Closes #37 Closes #38 Co-Authored-By: Claude Opus 5 (1M context) --- docs/PROJECT_PLAN.md | 43 ++++++++++- src/libs/board/host/host_board.cpp | 23 +++--- .../board/stm32f767zi_nucleo/nucleo_board.cpp | 23 ++---- .../board/stm32f767zi_nucleo/nucleo_board.hpp | 38 ++++++++-- src/libs/mcu/CMakeLists.txt | 16 ++++ src/libs/mcu/arm_cm7/cortex_m7.hpp | 6 +- src/libs/mcu/arm_cm7/gpio_pin.cpp | 29 +++---- src/libs/mcu/arm_cm7/gpio_pin.hpp | 30 +++++--- src/libs/mcu/arm_cm7/i2c.cpp | 76 ++++++++++++------- src/libs/mcu/arm_cm7/i2c.hpp | 26 +++---- .../mcu/arm_cm7/test_peripheral_contract.cpp | 63 +++++++++++++++ src/libs/mcu/arm_cm7/usart.hpp | 12 ++- src/libs/mcu/host/host_pin.hpp | 15 +++- src/libs/mcu/pin.hpp | 5 ++ 14 files changed, 293 insertions(+), 112 deletions(-) create mode 100644 src/libs/mcu/arm_cm7/test_peripheral_contract.cpp diff --git a/docs/PROJECT_PLAN.md b/docs/PROJECT_PLAN.md index 6b45d74..3215638 100644 --- a/docs/PROJECT_PLAN.md +++ b/docs/PROJECT_PLAN.md @@ -92,9 +92,9 @@ is the open part. hardware board exists: optional accessors (`std::expected`) vs. capability mix-ins vs. compile-time board traits. Deferred from Milestone 2 deliberately — with - one board there was nothing to design against. Entangled with #37: an - accessor that can report "this peripheral did not come up" is what the - factory-based fixes there need. + one board there was nothing to design against. No longer entangled with + #37: peripherals now configure themselves at construction, so there is no + "did not come up" state for an accessor to report. - [ ] Additional example application exercising more complex behavior - [ ] Cross-board validation: blinky runs on both boards unmodified @@ -129,6 +129,43 @@ is the open part. ## Decision Log +### 2026-09-06: Peripheral configuration moved into the constructor (#37, #38) + +- `mcu::GpioPin` and `mcu::I2CBus` now configure the hardware as they are + constructed. `configured_`/`initialized_` and the eight `kInvalidState` + guards they gated are gone, and `NucleoF767ZiBoard`'s member list is now a + description of the board rather than a set of promises `Init()` has to keep. + `Init()` is left with `InitSysTick()`, which needs the NVIC and so genuinely + cannot run before `main()`. +- #37 assumed a constructor could not touch registers, because the board is a + namespace-scope object. That turned out not to hold. `Reset_Handler` copies + `.data`, zeroes `.bss` and calls `SystemInit` *before* `__libc_init_array`; + `SystemInit` only enables the FPU and sets VTOR; no code here configures + clocks at all (the F767 runs on HSI at 16 MHz out of reset, which every + timing constant already assumes); and RCC is live from reset, with each + driver enabling its own peripheral clock first. Two comments in the tree + stated that ordering backwards and were corrected. +- Rejected the alternatives #37 listed. A factory with a private constructor + (options 1/2) was the right answer *given* the no-registers-in-constructors + rule, but once that rule proved unnecessary it was machinery guarding a state + that no longer exists — and it forced `std::optional` members, which model a + hardware absence that cannot happen on a fixed board. Option 3's friend + declaration enforces who constructs, not who configures. Option 4's readiness + gate became vacuous: declaring the member *is* the bring-up. +- Three invariants now hold this up, documented on `NucleoF767ZiBoard`: + bring-up must not fail, must not call `mcu::Delay` (SysTick is not up yet), + and this board must stay the only object with a dynamic initializer. + `mcu::Usart` violates the first and stays two-phase, which also suits it: + only the application knows its `UartConfig`. +- I2C bus speed became a constructor argument with a derived `TIMINGR` table + (#38), deliberately undefaulted so a board has to name the rate its wiring + can carry. +- `arm_cm7` gained its first test. Its public headers name no vendor type, so + `test_peripheral_contract.cpp` compiles on the host whichever backend is + selected and asserts the contract — that a pin cannot be constructed without + a direction, and a bus without a speed. It proves the types, not the register + writes; those still need the board in hand. + ### 2026-09-04: First hardware backend (arm_cm7 + F767ZI Nucleo) - Chose CMSIS device headers over the Cube HAL. The 30k lines of register diff --git a/src/libs/board/host/host_board.cpp b/src/libs/board/host/host_board.cpp index bb4ada6..49eecaa 100644 --- a/src/libs/board/host/host_board.cpp +++ b/src/libs/board/host/host_board.cpp @@ -22,9 +22,12 @@ auto HostBoard::Init() -> std::expected { } zmq_transport_ = std::move(transport_result.value()); - user_led_1_ = std::make_unique("LED 1", *zmq_transport_); - user_led_2_ = std::make_unique("LED 2", *zmq_transport_); - user_button_1_ = std::make_unique("Button 1", *zmq_transport_); + user_led_1_ = std::make_unique("LED 1", *zmq_transport_, + mcu::PinDirection::kOutput); + user_led_2_ = std::make_unique("LED 2", *zmq_transport_, + mcu::PinDirection::kOutput); + user_button_1_ = std::make_unique("Button 1", *zmq_transport_, + mcu::PinDirection::kInput); uart_1_ = std::make_unique("UART 1", *zmq_transport_); i2c_1_ = std::make_unique("I2C 1", *zmq_transport_); @@ -33,13 +36,13 @@ auto HostBoard::Init() -> std::expected { std::ref(*uart_1_), std::ref(*i2c_1_), }; - return user_led_1_->Configure(mcu::PinDirection::kOutput) - .and_then([this]() { - return user_led_2_->Configure(mcu::PinDirection::kOutput); - }) - .and_then([this]() { - return user_button_1_->Configure(mcu::PinDirection::kInput); - }); + // No Configure() chain: the pins were given their direction above. What is + // left here is what genuinely cannot happen at construction -- the transport + // has to connect first, and that can fail, which a constructor could not + // report. That is why this board builds its peripherals in Init() while + // NucleoF767ZiBoard holds them as members: an emulated peripheral depends on + // a socket, a real one only on registers that are always there. + return {}; } auto HostBoard::UserLed1() -> mcu::OutputPin& { return *user_led_1_; } auto HostBoard::UserLed2() -> mcu::OutputPin& { return *user_led_2_; } diff --git a/src/libs/board/stm32f767zi_nucleo/nucleo_board.cpp b/src/libs/board/stm32f767zi_nucleo/nucleo_board.cpp index a6f6119..6fc672d 100644 --- a/src/libs/board/stm32f767zi_nucleo/nucleo_board.cpp +++ b/src/libs/board/stm32f767zi_nucleo/nucleo_board.cpp @@ -11,22 +11,15 @@ namespace board { auto NucleoF767ZiBoard::Init() -> std::expected { - // The core is already configured: SystemInit ran from Reset_Handler, before - // .data was copied. What is left is everything that needs a working C++ - // runtime -- starting with the tick that mcu::Delay is built on. + // The core is already configured: SystemInit ran from Reset_Handler, and the + // pins and the I2C bus configured themselves as this object was constructed. + // What is left is everything that cannot happen before main() -- which is + // just the tick that mcu::Delay is built on, since it needs the NVIC. + // + // The USART is absent on purpose: mcu::Uart::Init is the application's to + // call, because only the application knows the UartConfig it wants. mcu::InitSysTick(); - - // The button is externally pulled down on this board (UM1974), so it needs - // no internal pull: it reads low at rest and high while pressed. - return user_led_1_.Configure(mcu::PinDirection::kOutput) - .and_then( - [this] { return user_led_2_.Configure(mcu::PinDirection::kOutput); }) - .and_then([this] { - return user_button_1_.Configure(mcu::PinDirection::kInput); - }) - // I2C has no Init() in the portable interface -- unlike Uart, which the - // application configures itself -- so the board brings the bus up here. - .and_then([this] { return i2c_1_.Init(); }); + return {}; } auto NucleoF767ZiBoard::UserLed1() -> mcu::OutputPin& { return user_led_1_; } diff --git a/src/libs/board/stm32f767zi_nucleo/nucleo_board.hpp b/src/libs/board/stm32f767zi_nucleo/nucleo_board.hpp index 0800cf6..81bec27 100644 --- a/src/libs/board/stm32f767zi_nucleo/nucleo_board.hpp +++ b/src/libs/board/stm32f767zi_nucleo/nucleo_board.hpp @@ -21,8 +21,23 @@ namespace board { /// namespace-scope object in main.cpp, so its constructor runs from /// __libc_init_array before main(). /// -/// Constructing a GpioPin touches no registers, which is what makes that safe -/// -- the pins record where they are, and Init() is what configures them. +/// Constructing a peripheral configures it, which is what makes this list a +/// description of the hardware rather than a set of promises Init() has to +/// keep. Reset_Handler copies .data, zeroes .bss and calls SystemInit before +/// __libc_init_array, and RCC is live out of reset, so a peripheral constructor +/// can bring its own clock up. Three invariants hold that up, and a new +/// peripheral has to satisfy all three before it can be a member here: +/// +/// 1. Bring-up must not fail. A constructor cannot report an error. If a +/// peripheral has to poll a status bit that can time out, it needs a +/// separate call -- see mcu::Usart, which stays two-phase for this reason +/// as well as because only the application knows its UartConfig. +/// 2. Bring-up must not call mcu::Delay. InitSysTick() runs from Init(), +/// after main(), so a constructor that waited on the tick would hang. +/// 3. This board must stay the only object with a dynamic initializer. +/// Construction order within it is declaration order and well defined; +/// order across translation units is not. `.init_array` holding one entry +/// besides crtbegin's is what that looks like in the map file. class NucleoF767ZiBoard final : public Board { public: [[nodiscard]] auto Init() -> std::expected override; @@ -34,10 +49,16 @@ class NucleoF767ZiBoard final : public Board { [[nodiscard]] auto Uart1() -> mcu::Uart& override; private: - mcu::GpioPin user_led_1_{pin_map::kUserLed1.port, pin_map::kUserLed1.pin}; - mcu::GpioPin user_led_2_{pin_map::kUserLed2.port, pin_map::kUserLed2.pin}; + mcu::GpioPin user_led_1_{pin_map::kUserLed1.port, pin_map::kUserLed1.pin, + mcu::PinDirection::kOutput}; + mcu::GpioPin user_led_2_{pin_map::kUserLed2.port, pin_map::kUserLed2.pin, + mcu::PinDirection::kOutput}; + + // The button is externally pulled down on this board (UM1974), so it needs + // no internal pull: it reads low at rest and high while pressed. mcu::GpioPin user_button_1_{pin_map::kUserButton1.port, - pin_map::kUserButton1.pin}; + pin_map::kUserButton1.pin, + mcu::PinDirection::kInput}; mcu::Usart uart_1_{mcu::UsartId::kUsart3, { @@ -55,7 +76,12 @@ class NucleoF767ZiBoard final : public Board { .sda_port = pin_map::kI2C1Sda.port, .sda_pin = pin_map::kI2C1Sda.pin, .alternate_function = pin_map::kI2C1AlternateFunction, - }}; + }, + // The driver brings the bus up on the pins' internal + // pull-ups, which it notes are weak (~40k) and good for + // 100 kHz over short wiring. Going faster is a decision + // for whoever adds external resistors. + mcu::I2CSpeed::kStandard100kHz}; }; } // namespace board diff --git a/src/libs/mcu/CMakeLists.txt b/src/libs/mcu/CMakeLists.txt index a8f3de8..c47e577 100644 --- a/src/libs/mcu/CMakeLists.txt +++ b/src/libs/mcu/CMakeLists.txt @@ -14,3 +14,19 @@ if(NOT IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/${EMBEDDED_CPP_MCU}") "Available: ${available}. See CLAUDE.md.") endif() add_subdirectory(${EMBEDDED_CPP_MCU}) + +# The hardware backend has no tests of its own: cross builds run no CTest and +# have no googletest. But the arm_cm7 public headers deliberately name no vendor +# type, so the contract they encode compiles -- and can be asserted -- on the +# host, whichever backend is selected. add_host_unit_test comes from the host +# backend's CMakeLists, and CMake functions are global once defined. +# +# include(GoogleTest) again here rather than relying on the host backend's: it +# sets the discovery script path as a directory-scoped variable, so without it +# gtest_discover_tests silently registers a _NOT_BUILT placeholder instead of +# the tests. +if(BUILD_TESTING AND EMBEDDED_CPP_MCU STREQUAL "host") + include(GoogleTest) + add_host_unit_test(test_peripheral_contract + arm_cm7/test_peripheral_contract.cpp mcu) +endif() diff --git a/src/libs/mcu/arm_cm7/cortex_m7.hpp b/src/libs/mcu/arm_cm7/cortex_m7.hpp index bcea078..c0fdd5c 100644 --- a/src/libs/mcu/arm_cm7/cortex_m7.hpp +++ b/src/libs/mcu/arm_cm7/cortex_m7.hpp @@ -4,8 +4,10 @@ namespace mcu { /// @brief Bring the core to a state where compiled C++ can run correctly. /// -/// Called from Reset_Handler (as SystemInit) before .data is usable and before -/// static constructors run, so it must touch nothing but core registers. +/// Called from Reset_Handler (as SystemInit) after .data is copied and .bss +/// zeroed, but before static constructors run. It touches nothing but core +/// registers: peripheral clocks are each driver's own business, and this +/// project never leaves the reset clock configuration (HSI, 16 MHz) at all. /// /// Enables the FPU and points the vector table at flash. The FPU matters even /// for code with no floating point in sight: the toolchain compiles with diff --git a/src/libs/mcu/arm_cm7/gpio_pin.cpp b/src/libs/mcu/arm_cm7/gpio_pin.cpp index 36b1b7c..ddc9f94 100644 --- a/src/libs/mcu/arm_cm7/gpio_pin.cpp +++ b/src/libs/mcu/arm_cm7/gpio_pin.cpp @@ -13,8 +13,12 @@ namespace mcu { -auto GpioPin::Configure(PinDirection direction) - -> std::expected { +GpioPin::GpioPin(GpioPort port, std::uint32_t pin, PinDirection direction) + : port_(port), pin_(pin), direction_(direction) { + ApplyDirection(direction); +} + +auto GpioPin::ApplyDirection(PinDirection direction) -> void { // ConfigurePin enables the port clock first, which matters: before it is // running every register here reads as zero and ignores writes, silently. ConfigurePin(port_, pin_, @@ -26,14 +30,15 @@ auto GpioPin::Configure(PinDirection direction) }); direction_ = direction; - configured_ = true; +} + +auto GpioPin::Configure(PinDirection direction) + -> std::expected { + ApplyDirection(direction); return {}; } auto GpioPin::Get() -> std::expected { - if (!configured_) { - return std::unexpected(common::Error::kInvalidState); - } // IDR, not ODR, for both directions: it reports what the pad is actually at, // so a shorted or externally driven output reads as what it really is rather // than as what it was told to be. @@ -42,9 +47,6 @@ auto GpioPin::Get() -> std::expected { } auto GpioPin::SetHigh() -> std::expected { - if (!configured_) { - return std::unexpected(common::Error::kInvalidState); - } if (direction_ != PinDirection::kOutput) { return std::unexpected(common::Error::kInvalidOperation); } @@ -56,9 +58,6 @@ auto GpioPin::SetHigh() -> std::expected { } auto GpioPin::SetLow() -> std::expected { - if (!configured_) { - return std::unexpected(common::Error::kInvalidState); - } if (direction_ != PinDirection::kOutput) { return std::unexpected(common::Error::kInvalidOperation); } @@ -67,9 +66,6 @@ auto GpioPin::SetLow() -> std::expected { } auto GpioPin::Toggle() -> std::expected { - if (!configured_) { - return std::unexpected(common::Error::kInvalidState); - } if (direction_ != PinDirection::kOutput) { return std::unexpected(common::Error::kInvalidOperation); } @@ -86,9 +82,6 @@ auto GpioPin::Toggle() -> std::expected { auto GpioPin::SetInterruptHandler(std::function handler, PinTransition transition) -> std::expected { - if (!configured_) { - return std::unexpected(common::Error::kInvalidState); - } if (direction_ != PinDirection::kInput) { return std::unexpected(common::Error::kInvalidOperation); } diff --git a/src/libs/mcu/arm_cm7/gpio_pin.hpp b/src/libs/mcu/arm_cm7/gpio_pin.hpp index d518c5e..cc79259 100644 --- a/src/libs/mcu/arm_cm7/gpio_pin.hpp +++ b/src/libs/mcu/arm_cm7/gpio_pin.hpp @@ -12,17 +12,23 @@ namespace mcu { /// @brief One STM32 GPIO pin. /// -/// Construction only records where the pin is; it touches no registers, so a -/// board can hold pins as members and have them constructed before the clock -/// tree is up. Configure() is what makes the pin real, and every operation -/// before it returns kInvalidState rather than writing into a dead register -/// block. +/// Constructing a pin configures it: the constructor enables the port clock and +/// programs the pin, so there is no window in which an unconfigured GpioPin +/// exists and no operation has to ask whether there is one. /// -/// The split is deliberate; that nothing enforces the second half of it is -/// not. Every peripheral in this backend has the same shape. See issue #37. +/// That is safe even though a board holds its pins as namespace-scope members. +/// Reset_Handler copies .data, zeroes .bss and calls SystemInit before +/// __libc_init_array, RCC is live out of reset, and ConfigurePin enables its +/// own port clock before touching anything else. See the invariants on +/// board::NucleoF767ZiBoard before adding a peripheral that needs more. +/// +/// The direction can still be changed at run time -- that is what makes this a +/// BidirectionalPin -- so operations that require a particular direction still +/// check for it. That check is about what the pin is right now, not about +/// whether anyone remembered to set it up. class GpioPin final : public BidirectionalPin { public: - GpioPin(GpioPort port, std::uint32_t pin) : port_(port), pin_(pin) {} + GpioPin(GpioPort port, std::uint32_t pin, PinDirection direction); [[nodiscard]] auto Configure(PinDirection direction) -> std::expected override; @@ -37,12 +43,16 @@ class GpioPin final : public BidirectionalPin { -> std::expected override; private: + /// Program the pin for a direction. Shared by the constructor and Configure, + /// which is why it returns void rather than the expected Configure owes its + /// caller: there is nothing here that can fail. + auto ApplyDirection(PinDirection direction) -> void; + [[nodiscard]] auto Mask() const -> std::uint32_t { return 1U << pin_; } GpioPort port_; std::uint32_t pin_; - PinDirection direction_ = PinDirection::kInput; - bool configured_ = false; + PinDirection direction_; }; } // namespace mcu diff --git a/src/libs/mcu/arm_cm7/i2c.cpp b/src/libs/mcu/arm_cm7/i2c.cpp index 480e6de..23e092a 100644 --- a/src/libs/mcu/arm_cm7/i2c.cpp +++ b/src/libs/mcu/arm_cm7/i2c.cpp @@ -14,22 +14,52 @@ namespace mcu { namespace { -/// Timing for 100 kHz standard mode from a 16 MHz I2CCLK (the reset -/// configuration: HSI, no PLL, APB1 prescaler 1). The F7's I2C is the v2 -/// peripheral: timing is these five fields, not the old CCR divisor, and the -/// value is normally taken from ST's tool rather than derived. Matches -/// AN4235's table for 16 MHz; the arithmetic, so the number is checkable: +/// TIMINGR for each supported speed, from a 16 MHz I2CCLK -- the reset +/// configuration (HSI, no PLL, APB1 prescaler 1), which is the only one this +/// project runs at. **Raising the core clock invalidates every value here.** /// -/// PRESC = 3 -> t_PRESC = (3+1) x 62.5 ns = 250 ns -/// SCLL = 0x13 -> (19+1) x 250 ns = 5.00 us low period -/// SCLH = 0x0F -> (15+1) x 250 ns = 4.00 us high period -/// SDADEL = 0x2 -> 2 x 250 ns = 500 ns data hold -/// SCLDEL = 0x4 -> (4+1) x 250 ns = 1.25 us data setup +/// The F7's I2C is the v2 peripheral: timing is five fields, not the old CCR +/// divisor. TIMINGR packs them as PRESC[31:28], SCLDEL[23:20], SDADEL[19:16], +/// SCLH[15:8], SCLL[7:0], and each field counts one *more* than it holds -- +/// RM0410's I2C timings section, and AN4235 Table 3 for the same layout on the +/// F0/F3 parts that got this peripheral first. /// -/// 5.00 + 4.00 us plus rise and fall gives a ~10 us period: 100 kHz. -/// Raising the core clock invalidates this constant. Issue #38 covers turning -/// this into a table the board selects from, rather than one hardcoded rate. -constexpr std::uint32_t kTiming100kHzAt16MHz = 0x3042'0F13U; +/// Each value below is derived rather than taken from ST's spreadsheet tool, so +/// the arithmetic is spelled out and every one is checked against the I2C-bus +/// specification rev.03 limits (AN4235 Table 2): t_LOW >= 4.7 us standard / +/// 1.3 us fast, t_HIGH >= 4.0 / 0.6 us, t_SU;DAT >= 250 / 100 ns, t_HD;DAT > 0. +[[nodiscard]] auto Timing(I2CSpeed speed) -> std::uint32_t { + switch (speed) { + case I2CSpeed::kStandard100kHz: + /// 0x3042'0F13 -- t_LOW 5.00 us and t_HIGH 4.00 us clear the standard + /// mode minima of 4.7 and 4.0 us. + /// + /// PRESC = 3 -> t_PRESC = (3+1) x 62.5 ns = 250 ns + /// SCLL = 0x13 -> (19+1) x 250 ns = 5.00 us low period + /// SCLH = 0x0F -> (15+1) x 250 ns = 4.00 us high period + /// SDADEL = 0x2 -> 2 x 250 ns = 500 ns data hold + /// SCLDEL = 0x4 -> (4+1) x 250 ns = 1.25 us data setup + /// + /// 5.00 + 4.00 us plus rise and fall gives a ~10 us period: 100 kHz. + return 0x3042'0F13U; + case I2CSpeed::kFast400kHz: + /// 0x0032'1115 -- t_LOW 1.375 us and t_HIGH 1.125 us clear the fast mode + /// minima of 1.3 and 0.6 us without leaning on any particular rise time, + /// so this errs slow rather than out of spec. **Not yet exercised on + /// hardware:** nothing in the tree selects this speed today. + /// + /// PRESC = 0 -> t_PRESC = (0+1) x 62.5 ns = 62.5 ns + /// SCLL = 0x15 -> (21+1) x 62.5 ns = 1.375 us low period + /// SCLH = 0x11 -> (17+1) x 62.5 ns = 1.125 us high period + /// SDADEL = 0x2 -> 2 x 62.5 ns = 125 ns data hold + /// SCLDEL = 0x3 -> (3+1) x 62.5 ns = 250 ns data setup + /// + /// 1.375 + 1.125 us is a 2.5 us period: 400 kHz. Note that 400 kHz needs + /// stronger pull-ups than the internal ones this driver enables. + return 0x0032'1115U; + } + return 0x3042'0F13U; +} /// A transfer that makes no progress must fail rather than spin forever: a /// bus held low by a stuck device never sets any completion flag. @@ -117,11 +147,8 @@ auto FinishTransfer(I2C_TypeDef* registers) } // namespace -auto I2CBus::Init() -> std::expected { - if (initialized_) { - return std::unexpected(common::Error::kInvalidState); - } - +I2CBus::I2CBus(I2CId bus_id, const I2CPins& pins, I2CSpeed speed) + : id_(bus_id), pins_(pins) { EnablePeripheralClock(id_); // Open drain, because I2C signals are wire-AND: a push-pull driver would @@ -142,18 +169,12 @@ auto I2CBus::Init() -> std::expected { // TIMINGR is writable only while the peripheral is disabled. registers->CR1 &= ~I2C_CR1_PE; - registers->TIMINGR = kTiming100kHzAt16MHz; + registers->TIMINGR = Timing(speed); registers->CR1 |= I2C_CR1_PE; - - initialized_ = true; - return {}; } auto I2CBus::SendData(std::uint16_t address, std::span data) -> std::expected { - if (!initialized_) { - return std::unexpected(common::Error::kInvalidState); - } if (data.empty()) { return {}; } @@ -176,9 +197,6 @@ auto I2CBus::SendData(std::uint16_t address, std::span data) auto I2CBus::ReceiveData(std::uint16_t address, std::span buffer) -> std::expected { - if (!initialized_) { - return std::unexpected(common::Error::kInvalidState); - } if (buffer.empty()) { return 0U; } diff --git a/src/libs/mcu/arm_cm7/i2c.hpp b/src/libs/mcu/arm_cm7/i2c.hpp index f3d928e..9011902 100644 --- a/src/libs/mcu/arm_cm7/i2c.hpp +++ b/src/libs/mcu/arm_cm7/i2c.hpp @@ -16,6 +16,14 @@ namespace mcu { /// it. `kI2C1` is a distinct token and safe. enum class I2CId : std::uint8_t { kI2C1, kI2C2, kI2C3, kI2C4 }; +/// @brief How fast the bus runs. +/// +/// A property of the bus, not of a link: every device on a shared bus has to +/// agree, and the achievable rate depends on pull-up strength, trace length and +/// the slowest device present. All of that is board knowledge, which is why the +/// board names it and mcu::I2CController does not carry it. +enum class I2CSpeed : std::uint8_t { kStandard100kHz = 1, kFast400kHz }; + /// @brief Where an I2C instance's two pins are, and their alternate function. struct I2CPins { GpioPort scl_port; @@ -34,18 +42,11 @@ struct I2CPins { /// writes and reads as two separate transactions. class I2CBus final : public I2CController { public: - I2CBus(I2CId id, const I2CPins& pins) : id_(id), pins_(pins) {} - - /// @brief Enable the peripheral and configure its pins. Called by the board; - /// SendData and ReceiveData report kInvalidState until it has run. - /// - /// That the board must call this is a convention, not something the type - /// enforces -- and unlike Uart, mcu::I2CController has no Init(), so nothing - /// in board::Board's shape hints that the call is required. See issue #37. - /// - /// The bus runs at 100 kHz; making the speed a board-supplied parameter is - /// issue #38. - [[nodiscard]] auto Init() -> std::expected; + /// Constructing the bus brings it up: this enables the peripheral clock, + /// configures the pins and programs TIMINGR, so an I2CBus that exists is one + /// that works. The board supplies the speed because the wiring, not the + /// application, is what determines the rate the bus can carry. + I2CBus(I2CId bus_id, const I2CPins& pins, I2CSpeed speed); [[nodiscard]] auto SendData(std::uint16_t address, std::span data) @@ -58,7 +59,6 @@ class I2CBus final : public I2CController { private: I2CId id_; I2CPins pins_; - bool initialized_ = false; }; } // namespace mcu diff --git a/src/libs/mcu/arm_cm7/test_peripheral_contract.cpp b/src/libs/mcu/arm_cm7/test_peripheral_contract.cpp new file mode 100644 index 0000000..45e4244 --- /dev/null +++ b/src/libs/mcu/arm_cm7/test_peripheral_contract.cpp @@ -0,0 +1,63 @@ +/// @file +/// The arm_cm7 peripherals' bring-up contract, checked at compile time. +/// +/// This is the only coverage the hardware backend has, and it is possible only +/// because its public headers name no vendor type: they include CMSIS nowhere, +/// so they compile on the host even though the backend itself does not. Nothing +/// here odr-uses a driver, so the binary links without arm_cm7 or CMSIS. +/// +/// What it can prove is that the types make an unconfigured peripheral +/// unrepresentable. What it cannot prove is that the constructors write the +/// right bits -- that still needs the board in hand. + +#include + +#include +#include + +#include "libs/mcu/arm_cm7/gpio_pin.hpp" +#include "libs/mcu/arm_cm7/gpio_port.hpp" +#include "libs/mcu/arm_cm7/i2c.hpp" +#include "libs/mcu/pin.hpp" + +namespace { + +// A pin cannot be made without saying which direction it is, and making one is +// what configures it. So an unconfigured GpioPin cannot be named. That is the +// whole of issue #37, stated as something the compiler checks. +static_assert(!std::is_default_constructible_v); +static_assert( + !std::is_constructible_v); +static_assert(std::is_constructible_v); + +// It stays a BidirectionalPin: the direction is still switchable at run time, +// which is why the direction guards in gpio_pin.cpp are correct rather than +// leftovers. +static_assert(std::is_base_of_v); +static_assert(std::is_base_of_v); +static_assert(std::is_base_of_v); + +// Same contract for the bus, plus: no I2CBus without a speed. The board has to +// say how fast its wiring can go, because a default would let that stay +// invisible. +static_assert(!std::is_default_constructible_v); +static_assert(!std::is_constructible_v); +static_assert(std::is_constructible_v); + +// The peripherals are still usable through the portable interfaces the board +// hands out, which is what keeps applications unaware of any of this. +static_assert(std::is_base_of_v); + +TEST(PeripheralContract, UnconfiguredPeripheralsAreUnrepresentable) { + // The static_asserts above are the real test -- this binary failing to + // compile is the failure mode. Restating one at run time gives CTest + // something to report, so a silently dropped test target is visible. + EXPECT_FALSE( + (std::is_constructible_v)); + EXPECT_FALSE( + (std::is_constructible_v)); +} + +} // namespace diff --git a/src/libs/mcu/arm_cm7/usart.hpp b/src/libs/mcu/arm_cm7/usart.hpp index c38e859..b75a51a 100644 --- a/src/libs/mcu/arm_cm7/usart.hpp +++ b/src/libs/mcu/arm_cm7/usart.hpp @@ -33,9 +33,15 @@ struct UsartPins { /// @brief Blocking USART, with an optional receive interrupt. /// -/// Like every peripheral here, this is constructed inert and made real by a -/// separate call -- Init(), which the application makes. Nothing enforces -/// that; see issue #37. +/// Alone among the peripherals here, this is constructed inert and made real by +/// a separate call. Two things keep it that way. Only the application knows the +/// UartConfig it wants, so the board cannot supply one -- and Init() validates +/// that config and can fail, which a constructor has no way to report. GpioPin +/// and I2CBus have neither problem, so they configure themselves at +/// construction and cannot be reached before they are live. +/// +/// Send, Receive and SetRxHandler therefore still report kInvalidState until +/// Init() has run. /// /// Send and Receive poll the status register; there is no transmit buffering, /// so Send returns once the last byte has left the shift register and the line diff --git a/src/libs/mcu/host/host_pin.hpp b/src/libs/mcu/host/host_pin.hpp index 01e7a71..c0b70f4 100644 --- a/src/libs/mcu/host/host_pin.hpp +++ b/src/libs/mcu/host/host_pin.hpp @@ -9,10 +9,19 @@ namespace mcu { +/// @brief One emulated pin, mirrored to the Python host emulator. +/// +/// The direction is a constructor argument, mirroring the hardware backends: +/// there is no window in which a pin exists but has no direction. Configure() +/// stays -- BidirectionalPin requires it and a pin's direction really is +/// switchable at run time -- but it is no longer a step anyone can forget. +/// +/// Unlike mcu::GpioPin this touches no hardware at construction; the emulator +/// learns a pin's direction from the messages that follow. class HostPin final : public BidirectionalPin, public Receiver { public: - explicit HostPin(std::string name, Transport& transport) - : name_{std::move(name)}, transport_{transport} {} + HostPin(std::string name, Transport& transport, PinDirection direction) + : name_{std::move(name)}, transport_{transport}, direction_{direction} {} ~HostPin() override = default; HostPin(const HostPin&) = delete; HostPin(HostPin&&) = delete; @@ -39,7 +48,7 @@ class HostPin final : public BidirectionalPin, public Receiver { const std::string name_; Transport& transport_; - PinDirection direction_{PinDirection::kOutput}; + PinDirection direction_; PinState state_{PinState::kHighZ}; PinTransition transition_{PinTransition::kBoth}; std::function handler_; diff --git a/src/libs/mcu/pin.hpp b/src/libs/mcu/pin.hpp index f67ee38..8bb4c68 100644 --- a/src/libs/mcu/pin.hpp +++ b/src/libs/mcu/pin.hpp @@ -32,6 +32,11 @@ class OutputPin : public virtual InputPin { [[nodiscard]] virtual auto Toggle() -> std::expected = 0; }; +/// @brief A pin whose direction is a runtime property. +/// +/// Configure() changes the direction of a pin that is already live; it is not a +/// setup step a backend can leave undone. Backends give a pin its initial +/// direction at construction, so an unconfigured pin cannot be obtained. class BidirectionalPin : public virtual InputPin, public virtual OutputPin { public: ~BidirectionalPin() override = default;