fix: the quarter-degree rounding was wrong below zero, in three copies
`(quarters + 2) / 4` is round-half-up only for positive values: C truncates a negative quotient toward zero, so -3.00 C read as -2, -1.00 C as 0 and -0.75 C as 0 - nine of fifteen negative quarter-values off by a whole degree, always toward zero. `(quarters + 2) >> 2` is an arithmetic shift, which floors, and is right across the whole range. It is also smaller: the shift skips the bias correction signed division needs. The formula was written three times - thermistor::celsius(), which nothing called, and inline at both live call sites - so the defect had three homes and so would its fix. One `thermistor::whole_degrees()` now, called from both, and the dead entry point is gone (rules 6, 27). test/consteval.cpp is new and is what should have caught it: whole_degrees across zero including both ties and both table limits, the curve's start and saturation points and its monotonicity, and the thermistor table anchored where the Beta equation fixes it - the count at which the divider reads the thermistor's nominal resistance must read the nominal temperature - plus both clamps and the fall across every step. Red-green: four assertions fire against the old division. Beside it: the cubic's three coefficients are named rather than inlined and restated in prose (rule 5), the consteval table builders take explicit 32-bit types (rule 25), the curve's clamp reads the table's own size (rule 36), and the serial override says what expects the rate rather than what the board has always done (rules 12, 13). And bootloader::handle_reset()'s watchdog diversion is gone. Its own comment called it a leftover "kept only because it is free and cannot hurt", and it did not: pureboot peeks WDRF without clearing it and hands back on purpose, so a watchdog reset arrives here with the flag still set and the diversion jumped into the loader with MCUSR already cleared - opening the activation window that policy exists to close. Clearing MCUSR is the whole job and stays. 8206 -> 8168 bytes, byte-identical between generated and reflect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -67,8 +67,8 @@ class sampler {
|
||||
using fan = dev::pwm<avr::pd5, {.frequency = 50_kHz}>;
|
||||
|
||||
// 115200 at 16 MHz lands +2.1 % off, past the receiver-tolerance table the
|
||||
// solver holds rates to - the rate this board has always spoken, so the
|
||||
// override states that it is meant.
|
||||
// solver holds rates to. It is the rate the console on the other end of the
|
||||
// cable expects, so the override states that the miss is meant.
|
||||
using serial_t = dev::uart0<{
|
||||
.baud = 115200_Bd,
|
||||
.rx_buffer = 32,
|
||||
|
||||
@@ -50,19 +50,16 @@ class bootloader {
|
||||
}
|
||||
|
||||
public:
|
||||
// Call first thing in main. consume_reset_cause() reads *and clears* MCUSR, which
|
||||
// matters on its own: a lingering WDRF forces the watchdog back on at its
|
||||
// shortest timeout. The diversion below is a leftover of the legacy route
|
||||
// and is kept only because it is free and cannot hurt - with BOOTRST
|
||||
// programmed the loader has already run before this line, so nothing
|
||||
// normally reaches it.
|
||||
// Call first thing in main. Clearing MCUSR is the whole job: a lingering
|
||||
// WDRF forces the watchdog back on at its shortest timeout, which is a
|
||||
// reset loop rather than a boot. The cause is not routed on - pureboot
|
||||
// peeks WDRF without clearing it and hands back on purpose, so a watchdog
|
||||
// reset arrives here with the flag still set, and diverting into the
|
||||
// loader on it would reopen the very window that policy closes.
|
||||
static void handle_reset()
|
||||
{
|
||||
auto cause = avr::power::consume_reset_cause();
|
||||
avr::power::consume_reset_cause();
|
||||
guard::disable();
|
||||
if (cause.watchdog && present()) {
|
||||
call(reinterpret_cast<jump_fn>(base / 2));
|
||||
}
|
||||
}
|
||||
|
||||
// Hand over for real: no reset, so no WDRF for the loader to refuse.
|
||||
|
||||
@@ -33,7 +33,7 @@ class controller {
|
||||
m_temp_quarters = thermistor::quarters(sample);
|
||||
m_have_data = true;
|
||||
if (m_auto_mode) {
|
||||
m_percent = curve::duty(static_cast<std::int8_t>((m_temp_quarters + 2) / 4));
|
||||
m_percent = curve::duty(thermistor::whole_degrees(m_temp_quarters));
|
||||
}
|
||||
fan::set_duty(avr::percent_t::of(m_percent));
|
||||
}
|
||||
|
||||
@@ -5,20 +5,25 @@
|
||||
|
||||
#include <libavr/flash.hpp>
|
||||
|
||||
// The auto-mode fan curve, tabulated at compile time: a cubic in C
|
||||
// (0.002246*x^3 - 0.09*x^2 + 0.91*x, zero below 20 C) as a flash_table of
|
||||
// duty percent per C.
|
||||
// The auto-mode fan curve, tabulated at compile time as a flash_table of duty
|
||||
// percent per C: a cubic that is held at zero below the temperature the fan
|
||||
// starts at.
|
||||
namespace app::curve {
|
||||
|
||||
inline constexpr double cubic_term = 0.002246;
|
||||
inline constexpr double square_term = -0.09;
|
||||
inline constexpr double linear_term = 0.91;
|
||||
inline constexpr std::int8_t start_celsius = 20;
|
||||
|
||||
namespace detail {
|
||||
|
||||
consteval std::uint8_t duty_entry(int celsius)
|
||||
consteval std::uint8_t duty_entry(std::int32_t celsius)
|
||||
{
|
||||
double x = celsius;
|
||||
if (x < 20) {
|
||||
if (x < start_celsius) {
|
||||
return 0;
|
||||
}
|
||||
double duty = 0.002246 * x * x * x - 0.09 * x * x + 0.91 * x;
|
||||
double duty = cubic_term * x * x * x + square_term * x * x + linear_term * x;
|
||||
if (duty < 0) {
|
||||
duty = 0;
|
||||
}
|
||||
@@ -30,7 +35,7 @@ consteval std::uint8_t duty_entry(int celsius)
|
||||
|
||||
inline constexpr avr::flash_table<[] {
|
||||
std::array<std::uint8_t, 100> out{};
|
||||
for (int t = 0; t < 100; ++t) {
|
||||
for (std::int32_t t = 0; t < static_cast<std::int32_t>(out.size()); ++t) {
|
||||
out[static_cast<std::size_t>(t)] = duty_entry(t);
|
||||
}
|
||||
return out;
|
||||
@@ -39,14 +44,15 @@ inline constexpr avr::flash_table<[] {
|
||||
|
||||
} // namespace detail
|
||||
|
||||
// Duty percent for a temperature (clamped to the 0..99 C table window).
|
||||
// Duty percent for a temperature, clamped to the table's own window.
|
||||
inline std::uint8_t duty(std::int8_t celsius)
|
||||
{
|
||||
constexpr auto highest = static_cast<std::int8_t>(detail::table.size() - 1);
|
||||
if (celsius < 0) {
|
||||
celsius = 0;
|
||||
}
|
||||
if (celsius > 99) {
|
||||
celsius = 99;
|
||||
if (celsius > highest) {
|
||||
celsius = highest;
|
||||
}
|
||||
return detail::table[static_cast<std::uint8_t>(celsius)];
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
#include "controller.hpp"
|
||||
#include "statistics.hpp"
|
||||
#include "terminal.hpp"
|
||||
#include "thermistor.hpp"
|
||||
|
||||
using namespace app;
|
||||
|
||||
@@ -23,7 +24,7 @@ int main()
|
||||
while (true) {
|
||||
controller::poll();
|
||||
if (controller::data_available()) {
|
||||
statistics::record(static_cast<std::int8_t>((controller::temperature_quarters() + 2) / 4));
|
||||
statistics::record(thermistor::whole_degrees(controller::temperature_quarters()));
|
||||
}
|
||||
terminal::poll();
|
||||
}
|
||||
|
||||
@@ -34,7 +34,7 @@ consteval double temperature_of(double adc)
|
||||
// 256 entries over the 10-bit range (steps of 4 counts), quarter- C,
|
||||
// clamped to a sane sensor window; entry 256 mirrors 255 so interpolation
|
||||
// at full scale has a right neighbour.
|
||||
consteval std::int16_t quarters_entry(int index)
|
||||
consteval std::int16_t quarters_entry(std::int32_t index)
|
||||
{
|
||||
double adc = index * 4.0;
|
||||
if (adc < 4) {
|
||||
@@ -55,7 +55,7 @@ consteval std::int16_t quarters_entry(int index)
|
||||
|
||||
inline constexpr avr::flash_table<[] {
|
||||
std::array<std::int16_t, 257> out{};
|
||||
for (int i = 0; i < 257; ++i) {
|
||||
for (std::int32_t i = 0; i < static_cast<std::int32_t>(out.size()); ++i) {
|
||||
out[static_cast<std::size_t>(i)] = quarters_entry(i < 256 ? i : 255);
|
||||
}
|
||||
return out;
|
||||
@@ -74,9 +74,12 @@ inline std::int16_t quarters(std::uint16_t adc)
|
||||
return static_cast<std::int16_t>(a + ((b - a) * frac) / 4);
|
||||
}
|
||||
|
||||
inline std::int8_t celsius(std::uint16_t adc)
|
||||
// Whole degrees from quarter-degrees, rounded to nearest. The shift is the
|
||||
// point: C truncates a negative quotient toward zero, so `(q + 2) / 4` reads
|
||||
// -3.00 C as -2, where an arithmetic shift floors and reads it as -3.
|
||||
inline constexpr std::int8_t whole_degrees(std::int16_t quarters)
|
||||
{
|
||||
return static_cast<std::int8_t>((quarters(adc) + 2) / 4);
|
||||
return static_cast<std::int8_t>((quarters + 2) >> 2);
|
||||
}
|
||||
|
||||
} // namespace app::thermistor
|
||||
|
||||
@@ -1,3 +1,12 @@
|
||||
# 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_include_directories(consteval_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/../src)
|
||||
target_link_libraries(consteval_tests PRIVATE libavr)
|
||||
|
||||
add_test(NAME fantemp.consteval
|
||||
COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} --target consteval_tests)
|
||||
|
||||
# The board has no reset line and no programming header, so the loader-entry
|
||||
# route in the emitted image is the only thing standing between a firmware change
|
||||
# and an unreflashable board.
|
||||
|
||||
84
test/consteval.cpp
Normal file
84
test/consteval.cpp
Normal file
@@ -0,0 +1,84 @@
|
||||
// Compile-only battery, built with the cross compiler so the target's 16-bit
|
||||
// int is exercised. Two of this firmware's three numeric surfaces are decided
|
||||
// entirely at compile time - the fan curve and the thermistor table - and the
|
||||
// third, quarter-degrees to whole ones, is the arithmetic between them.
|
||||
|
||||
#include <cstdint>
|
||||
|
||||
#include "curve.hpp"
|
||||
#include "thermistor.hpp"
|
||||
|
||||
namespace {
|
||||
|
||||
using app::thermistor::whole_degrees;
|
||||
|
||||
// Rounding to nearest, across zero. A negative quotient truncates toward zero
|
||||
// in C, so a `(q + 2) / 4` reads -3.00 C as -2 and every case below the tie
|
||||
// with it; the arithmetic shift floors, which is what these hold it to.
|
||||
static_assert(whole_degrees(0) == 0);
|
||||
static_assert(whole_degrees(1) == 0); // +0.25
|
||||
static_assert(whole_degrees(2) == 1); // +0.50, the tie
|
||||
static_assert(whole_degrees(3) == 1); // +0.75
|
||||
static_assert(whole_degrees(4) == 1); // +1.00
|
||||
static_assert(whole_degrees(-1) == 0); // -0.25
|
||||
static_assert(whole_degrees(-2) == 0); // -0.50, the tie, toward zero
|
||||
static_assert(whole_degrees(-3) == -1);
|
||||
static_assert(whole_degrees(-4) == -1);
|
||||
static_assert(whole_degrees(-12) == -3);
|
||||
static_assert(whole_degrees(-160) == -40); // the table's own floor
|
||||
static_assert(whole_degrees(500) == 125); // and its ceiling
|
||||
|
||||
// The curve is held at zero below the temperature the fan starts at, and
|
||||
// saturates inside the table's window rather than at its edge.
|
||||
using app::curve::detail::duty_entry;
|
||||
static_assert(duty_entry(app::curve::start_celsius - 1) == 0);
|
||||
static_assert(duty_entry(app::curve::start_celsius) == 0);
|
||||
static_assert(duty_entry(30) == 7);
|
||||
static_assert(duty_entry(40) == 36);
|
||||
static_assert(duty_entry(49) == 93);
|
||||
static_assert(duty_entry(50) == 100);
|
||||
static_assert(duty_entry(99) == 100);
|
||||
|
||||
// Monotone across the whole table: a warmer reading never asks for less air.
|
||||
consteval bool curve_rises()
|
||||
{
|
||||
for (std::int32_t t = 1; t < 100; ++t) {
|
||||
if (duty_entry(t) < duty_entry(t - 1)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
static_assert(curve_rises());
|
||||
|
||||
// The thermistor table, anchored where the Beta equation fixes it rather than
|
||||
// against numbers this file computed the same way: the divider reads the
|
||||
// thermistor's nominal resistance at
|
||||
// adc = full_scale * nominal / (series + nominal), and the equation's own
|
||||
// definition puts that count at the nominal temperature.
|
||||
using app::thermistor::detail::quarters_entry;
|
||||
|
||||
inline constexpr std::int32_t nominal_count = static_cast<std::int32_t>(
|
||||
app::thermistor::adc_full_scale * app::thermistor::nominal_resistance /
|
||||
(app::thermistor::series_resistor + app::thermistor::nominal_resistance));
|
||||
static_assert(quarters_entry(nominal_count / 4) >= 100); // 25.00 C, in quarters
|
||||
static_assert(quarters_entry(nominal_count / 4 + 1) < 100); // and the step below it
|
||||
|
||||
// Both clamps, at the ends the divider cannot leave.
|
||||
static_assert(quarters_entry(0) == 125 * 4);
|
||||
static_assert(quarters_entry(255) == -40 * 4);
|
||||
|
||||
// An NTC on this divider falls with the count: more counts is more resistance
|
||||
// is a colder sensor, over every step of the table.
|
||||
consteval bool thermistor_falls()
|
||||
{
|
||||
for (std::int32_t i = 1; i < 256; ++i) {
|
||||
if (quarters_entry(i) > quarters_entry(i - 1)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
static_assert(thermistor_falls());
|
||||
|
||||
} // namespace
|
||||
Reference in New Issue
Block a user