Celcius / Fahrenheit units selection #411
Conversation
|
Warning Rate limit exceeded@dragm83 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughAdds Fahrenheit/Celsius support and float-based temperatures across firmware and web UI: migrates stored targets to floats, introduces a temperature unit flag, updates controller step logic, exposes unit via API/status, provides C++/JS conversion utilities, and adapts UI components, charts, and settings forms to unit-aware display and input. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Web User
participant Web as Web UI (Settings)
participant API as WebUIPlugin
participant FW as Firmware Settings
participant Store as Preferences
User->>Web: Toggle Temperature Unit (°C/°F)
Web->>API: POST /api/settings { temperatureUnitFahrenheit, targetSteamTemp, targetWaterTemp, ... }
API->>FW: setTemperatureUnit(bool)
API->>FW: setTargetSteamTemp(float), setTargetWaterTemp(float)
FW->>Store: Persist temp_f, tb_f/ts_f/tw_f
Store-->>FW: OK
FW-->>API: Updated settings
API-->>Web: JSON { temperatureUnitFahrenheit, targets... }
Web->>Web: Convert values for display (JS utils)
sequenceDiagram
autonumber
participant Ctrl as Controller
participant FW as Settings
participant UI as Display UI
Ctrl->>FW: isTemperatureUnitFahrenheit()
alt Fahrenheit
Ctrl->>Ctrl: step = 5/9 °C (1°F)
else Celsius
Ctrl->>Ctrl: step = 1 °C
end
Ctrl->>FW: setTarget*Temp(current ± step) // floats
FW-->>UI: Status { temps, temp_f flag }
UI->>UI: Format temps with C++ converter and update gauges
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
web/src/pages/ProfileEdit/ExtendedPhase.jsx (1)
112-121: Duration stored as stringonChange passes e.target.value (string). Parse to number to keep schema consistent.
- onChange={e => onFieldChange('duration', e.target.value)} + onChange={e => onFieldChange('duration', parseInt(e.target.value, 10) || 0)}web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (1)
121-141: Show tenths and align aria with unit; keep input precisionPass precision to display conversion so step=0.1 is visible to users.
- Temperature ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)}) + Temperature ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)}) @@ - value={convertCelsiusToDisplay(data?.temperature, machine.value.status.temperatureUnitFahrenheit)} + value={convertCelsiusToDisplay( + data?.temperature, + machine.value.status.temperatureUnitFahrenheit, + 1 + )} onChange={e => onFieldChange('temperature', e.target.value)} aria-label={`Temperature in degrees ${machine.value.status.temperatureUnitFahrenheit ? 'Fahrenheit' : 'Celsius'}`} min='0' max={machine.value.status.temperatureUnitFahrenheit ? '302' : '150'} step='0.1'web/src/pages/Settings/index.jsx (1)
571-586: Allow negative offsets.Min=0 disallows common negative calibration offsets. Remove min or set a negative bound.
- min='0' - step='any' + step='any'src/display/core/Settings.cpp (3)
45-46: Offset still int; migrate and store as float (add to_f).Offset remains
intwith key "to". This loses decimals and contradicts the PR’s “offset to float” objective.- temperatureOffset = preferences.getInt("to", DEFAULT_TEMPERATURE_OFFSET); + if (preferences.isKey("to_f")) { + temperatureOffset = preferences.getFloat("to_f", static_cast<float>(DEFAULT_TEMPERATURE_OFFSET)); + } else { + // migrate from old int key + temperatureOffset = static_cast<float>(preferences.getInt("to", DEFAULT_TEMPERATURE_OFFSET)); + preferences.putFloat("to_f", temperatureOffset); + preferences.remove("to"); + }Note: Update the member type and getter in Settings.h to
float.
134-137: Make setTemperatureOffset accept float.Aligns with float storage and frontend fractional inputs.
-void Settings::setTemperatureOffset(const int temperature_offset) { +void Settings::setTemperatureOffset(const float temperature_offset) { temperatureOffset = temperature_offset; save(); }Header declaration must be updated accordingly.
442-462: Persist offset as float under to_f.- preferences.putInt("to", temperatureOffset); + preferences.putFloat("to_f", temperatureOffset);
🧹 Nitpick comments (18)
src/display/utils/temperatureConverter.h (2)
32-39: Fix rounding and clarify unused precisionRounding via +0.5f is biased for negatives and precision is ignored. Use lroundf and (optionally) keep precision for future use.
-inline int formatTemperatureForDisplay(float tempCelsius, bool useFahrenheit, int precision = 0) { - if (useFahrenheit) { - float tempF = celsiusToFahrenheit(tempCelsius); - // Add 0.5 for proper rounding to handle cases like 194.999... -> 195 - return static_cast<int>(tempF + 0.5f); - } - return static_cast<int>(tempCelsius + 0.5f); -} +inline int formatTemperatureForDisplay(float tempCelsius, bool useFahrenheit, int /*precision*/ = 0) { + const float v = useFahrenheit ? celsiusToFahrenheit(tempCelsius) : tempCelsius; + return static_cast<int>(std::lround(v)); +}
1-6: Redundant include guardsYou have both #pragma once and a classic guard. Keep either; both is harmless but noisy.
web/src/utils/temperatureConverter.js (1)
29-40: Optional: collapse duplicate “Display” helperscelsiusToFahrenheitDisplay/fahrenheitToCelsiusDisplay duplicate the rounding now handled by formatters. Consider removing to reduce API surface.
src/display/ui/default/DefaultUI.h (1)
95-97: Consider float-aware consumerscurrentTemp/targetTemp are float now, but calculate_angle still takes int set_temp. Ensure callers round intentionally or update that signature to float to avoid truncation surprises.
web/src/pages/ProfileEdit/ExtendedPhase.jsx (2)
14-17: Remove no-op useEffectEmpty effect and import add noise.
-import { useCallback, useEffect } from 'preact/hooks'; +import { useCallback } from 'preact/hooks'; @@ - // Load settings on component mount - useEffect(() => { - }, []); +
29-35: Avoid mutating nested arrayClone targets before write to keep updates predictable.
- const onTargetChange = (index, value) => { - const newPhase = { - ...phase, - }; - newPhase.targets[index] = value; - onChange(newPhase); - }; + const onTargetChange = (index, value) => { + const newTargets = [...(phase.targets || [])]; + newTargets[index] = value; + onChange({ ...phase, targets: newTargets }); + };web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (1)
22-25: Remove no-op useEffectDead effect/import.
-import { useState, useEffect } from 'preact/hooks'; +import { useState } from 'preact/hooks'; @@ - // Load settings on component mount - useEffect(() => { - }, []); +src/display/core/Controller.cpp (1)
404-413: Unit-aware stepping is correct; minor DRYIncrement logic duplicated in raise/lower. Consider a tiny helper for maintainability.
-void Controller::raiseTemp() { - float temp = getTargetTemp(); - float increment = settings.isTemperatureUnitFahrenheit() ? (5.0f / 9.0f) : 1.0f; // 1°F = 1/1.8°C - temp = constrain(temp + increment, MIN_TEMP, MAX_TEMP); - setTargetTemp(temp); -} +static inline float tempStep(const Settings& s) { return s.isTemperatureUnitFahrenheit() ? (5.0f/9.0f) : 1.0f; } +void Controller::raiseTemp() { + float temp = constrain(getTargetTemp() + tempStep(settings), MIN_TEMP, MAX_TEMP); + setTargetTemp(temp); +} @@ -void Controller::lowerTemp() { - float temp = getTargetTemp(); - float increment = settings.isTemperatureUnitFahrenheit() ? (5.0f / 9.0f) : 1.0f; // 1°F = 1/1.8°C - temp = constrain(temp - increment, MIN_TEMP, MAX_TEMP); - setTargetTemp(temp); -} +void Controller::lowerTemp() { + float temp = constrain(getTargetTemp() - tempStep(settings), MIN_TEMP, MAX_TEMP); + setTargetTemp(temp); +}web/src/pages/Autotune/index.jsx (1)
6-9: Include the unit symbol in the warning text.Right now users will see a bare number (e.g., “122”). Append the unit to avoid ambiguity and use the already-imported helper.
Apply this diff:
- Please ensure the boiler temperature is below {formatTemperatureValue(50, machine.value.status.temperatureUnitFahrenheit, 0)} before starting the autotune + Please ensure the boiler temperature is below {formatTemperatureValue(50, machine.value.status.temperatureUnitFahrenheit, 0)} {getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)} before starting the autotuneAlso applies to: 85-86
web/src/pages/ShotHistory/HistoryChart.jsx (1)
101-111: Optional: unify y-axis domain with OverviewChart.If you want consistent visual scaling across views, mirror the °C/°F min/max like OverviewChart or extract shared bounds.
src/display/plugins/WebUIPlugin.cpp (1)
81-81: Expose a boolean unit flag in status (keep legacy for compatibility).Consider also emitting a boolean
temperatureUnitFahrenheitalongsidetempUnitto avoid consumers handling 0/1 ints.doc["led"] = controller->getSystemInfo().capabilities.ledControl; -doc["tempUnit"] = controller->getSettings().isTemperatureUnitFahrenheit() ? 1 : 0; +doc["tempUnit"] = controller->getSettings().isTemperatureUnitFahrenheit() ? 1 : 0; // legacy +doc["temperatureUnitFahrenheit"] = controller->getSettings().isTemperatureUnitFahrenheit();src/display/core/Settings.h (2)
64-64: Clarify internal unit in API doc (Celsius).Add a brief comment stating all stored temps are Celsius; UI converts for display. Helps avoid misuse when Fahrenheit is enabled.
- bool isTemperatureUnitFahrenheit() const { return temperatureUnitFahrenheit; } + // Temperatures are stored in Celsius; this flag only affects display/UI. + bool isTemperatureUnitFahrenheit() const { return temperatureUnitFahrenheit; }
125-125: Boolean setter is fine; consider an enum for extensibility.If more units ever appear, migrate to an enum (Celsius/Fahrenheit).
enum class TemperatureUnit : uint8_t { Celsius = 0, Fahrenheit = 1 }; // API void setTemperatureUnit(TemperatureUnit unit); bool isTemperatureUnitFahrenheit() const { return unit == TemperatureUnit::Fahrenheit; }src/display/ui/default/DefaultUI.cpp (5)
55-55: Prefer fmaxf/std::max over macro max for floats.Reduces macro pitfalls and improves readability.
- const float errorMargin = max(2.0f, targetTemp * 0.02f); + const float errorMargin = fmaxf(2.0f, targetTemp * 0.02f);
354-365: Deduplicate gauge/label temperature logic.Same block repeated per screen; extract a helper to compute gaugeValue and set label once, then call it for each screen.
// Example helper (place static in this file) static inline void setTempDialAndLabel(lv_obj_t* gauge, lv_obj_t* label, float tempC, bool useF) { float gaugeVal = useF ? ((tempC * 9.0f/5.0f + 32.0f) - 32.0f) * 160.0f / 288.0f : tempC; if (gaugeVal < 0) gaugeVal = 0; else if (gaugeVal > 160) gaugeVal = 160; lv_arc_set_value(gauge, gaugeVal); lv_label_set_text_fmt(label, "%d%s", formatTemperatureForDisplay(tempC, useF), getTemperatureUnit(useF)); }Then invoke per screen.
Also applies to: 368-379, 382-393, 396-407, 410-421, 424-435
441-445: Target temperature label formatting duplicated.Extract a small helper to set the formatted target label to keep consistency.
static inline void setTargetTempLabel(lv_obj_t* label, float tempC, bool useF) { lv_label_set_text_fmt(label, "%d%s", formatTemperatureForDisplay(tempC, useF), getTemperatureUnit(useF)); }Also applies to: 451-455, 462-467, 636-641
357-361: Avoid integer-rounded value for gauge math.Use raw float conversion for gauge scaling to prevent jitter from integer rounding.
- float gaugeValue = useFahrenheit ? - (formatTemperatureForDisplay(currentTemp, true) - 32) * 160.0f / (320.0f - 32.0f) : + float gaugeValue = useFahrenheit ? + ((currentTemp * 9.0f/5.0f + 32.0f) - 32.0f) * 160.0f / 288.0f : currentTemp;(Apply similarly to other screens.)
852-870: Clamp percentage and avoid integer conversion in math.Prevents out-of-range pointer positions and removes display-rounding from math path.
- if (useFahrenheit) { - // For Fahrenheit display, we need to scale the target position to match the gauge - // Since gauge shows Fahrenheit values but is scaled to 0-160 range - float targetTempF = formatTemperatureForDisplay(targetTemp, true); - percentage = (targetTempF - 32.0) / (320.0 - 32.0); - } else { - percentage = static_cast<double>(targetTemp) / 160.0; - } + if (useFahrenheit) { + const double targetTempF = targetTemp * 9.0 / 5.0 + 32.0; + percentage = (targetTempF - 32.0) / 288.0; + } else { + percentage = static_cast<double>(targetTemp) / 160.0; + } + if (percentage < 0.0) percentage = 0.0; + if (percentage > 1.0) percentage = 1.0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/display/core/Controller.cpp(2 hunks)src/display/core/Settings.cpp(6 hunks)src/display/core/Settings.h(7 hunks)src/display/plugins/WebUIPlugin.cpp(4 hunks)src/display/ui/default/DefaultUI.cpp(8 hunks)src/display/ui/default/DefaultUI.h(2 hunks)src/display/utils/temperatureConverter.h(1 hunks)web/src/components/OverviewChart.jsx(2 hunks)web/src/pages/Autotune/index.jsx(2 hunks)web/src/pages/Home/ProcessControls.jsx(2 hunks)web/src/pages/ProfileEdit/ExtendedPhase.jsx(2 hunks)web/src/pages/ProfileEdit/ExtendedProfileForm.jsx(3 hunks)web/src/pages/ProfileEdit/StandardProfileForm.jsx(3 hunks)web/src/pages/Settings/index.jsx(8 hunks)web/src/pages/ShotHistory/HistoryChart.jsx(2 hunks)web/src/services/ApiService.js(2 hunks)web/src/utils/temperatureConverter.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
web/src/pages/Home/ProcessControls.jsx (1)
web/src/utils/temperatureConverter.js (2)
formatTemperatureValue(73-85)formatTemperature(49-64)
web/src/pages/Settings/index.jsx (1)
web/src/utils/temperatureConverter.js (3)
convertCelsiusToDisplay(123-134)convertInputToCelsius(102-115)getTemperatureUnit(92-94)
src/display/utils/temperatureConverter.h (3)
web/src/utils/temperatureConverter.js (3)
celsiusToFahrenheit(11-13)fahrenheitToCelsius(20-22)getTemperatureUnit(92-94)web/src/pages/ShotHistory/HistoryChart.jsx (1)
useFahrenheit(11-11)web/src/components/OverviewChart.jsx (1)
useFahrenheit(13-13)
web/src/pages/ShotHistory/HistoryChart.jsx (2)
web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)web/src/utils/temperatureConverter.js (2)
getTemperatureUnit(92-94)formatTemperatureValue(73-85)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (4)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (1)
onFieldChange(26-35)web/src/pages/ProfileEdit/ExtendedPhase.jsx (1)
onFieldChange(18-27)web/src/utils/temperatureConverter.js (3)
convertInputToCelsius(102-115)getTemperatureUnit(92-94)convertCelsiusToDisplay(123-134)web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)
web/src/components/OverviewChart.jsx (2)
web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)web/src/utils/temperatureConverter.js (2)
getTemperatureUnit(92-94)formatTemperatureValue(73-85)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (4)
web/src/pages/ProfileEdit/StandardProfileForm.jsx (2)
onFieldChange(24-33)onFieldChange(192-197)web/src/pages/ProfileEdit/ExtendedPhase.jsx (1)
onFieldChange(18-27)web/src/utils/temperatureConverter.js (3)
convertInputToCelsius(102-115)getTemperatureUnit(92-94)convertCelsiusToDisplay(123-134)web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)
web/src/pages/Autotune/index.jsx (2)
web/src/utils/temperatureConverter.js (1)
formatTemperatureValue(73-85)web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)
web/src/utils/temperatureConverter.js (3)
src/display/utils/temperatureConverter.h (3)
celsiusToFahrenheit(12-14)fahrenheitToCelsius(21-23)getTemperatureUnit(46-48)web/src/pages/ShotHistory/HistoryChart.jsx (1)
useFahrenheit(11-11)web/src/components/OverviewChart.jsx (1)
useFahrenheit(13-13)
web/src/pages/ProfileEdit/ExtendedPhase.jsx (5)
web/src/pages/ProfileEdit/ExtendedProfileForm.jsx (1)
onFieldChange(26-35)web/src/pages/ProfileEdit/StandardProfileForm.jsx (2)
onFieldChange(24-33)onFieldChange(192-197)web/src/utils/temperatureConverter.js (3)
convertInputToCelsius(102-115)getTemperatureUnit(92-94)convertCelsiusToDisplay(123-134)web/src/services/ApiService.js (2)
machine(195-210)machine(195-210)web/src/pages/ProfileList/utils.js (1)
phase(36-48)
src/display/ui/default/DefaultUI.cpp (2)
src/display/utils/temperatureConverter.h (1)
getTemperatureUnit(46-48)src/display/ui/default/DefaultUI.h (1)
currentProfileChoice(109-109)
src/display/core/Settings.h (1)
src/display/core/Settings.cpp (8)
setTargetBrewTemp(119-122)setTargetBrewTemp(119-119)setTargetSteamTemp(124-127)setTargetSteamTemp(124-124)setTargetWaterTemp(129-132)setTargetWaterTemp(129-129)setTemperatureUnit(321-324)setTemperatureUnit(321-321)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: deploy
🔇 Additional comments (22)
web/src/services/ApiService.js (2)
195-204: LGTM: unit flag in initial machine stateDefaulting temperatureUnitFahrenheit to false keeps UI deterministic pre-connection.
168-170: No change needed for temperature unit mapping
The backend’s status event uses thetempUnitkey (see WebUIPlugin.cpp:81), so mappingmessage.tempUnittotemperatureUnitFahrenheitis correct.src/display/ui/default/DefaultUI.h (1)
62-66: Float migration looks goodtempHistory and prevTargetTemp as float align with float temps end-to-end.
web/src/pages/Home/ProcessControls.jsx (1)
185-189: Displayed temp always shows .0With current utils, precision=1 yields trailing .0 (pre-rounded). After fixing utils as suggested, this will show real tenths.
src/display/core/Controller.cpp (1)
352-356: LGTM: float targets for steam/waterPassing floats to settings aligns with the float migration.
web/src/pages/Autotune/index.jsx (1)
2-2: Good use of machine signal for unit-aware UI.Reading
machine.value.status.temperatureUnitFahrenheitin render ties the message to the selected unit.web/src/components/OverviewChart.jsx (2)
13-15: Unit-aware charting looks solid.Datasets, labels, axis bounds, and tick formatting correctly adapt to °C/°F.
Also applies to: 20-27, 29-37, 91-93, 98-99
13-15: Confirm reactivity is correct
_onStatusbuilds and assigns a newmachine.valueobject (with updatedstatus.temperatureUnitFahrenheitand a freshhistoryarray), so toggling the unit flag always triggers a re-render.web/src/pages/ShotHistory/HistoryChart.jsx (1)
3-8: Consistent Fahrenheit/Celsius handling in shot charts.Labels, values, and ticks reflect the selected unit; aligns with OverviewChart.
Also applies to: 11-12, 18-25, 27-35, 104-106
web/src/pages/ProfileEdit/StandardProfileForm.jsx (2)
24-29: Correct temperature persistence via unit-aware conversion.Converting input to Celsius before updating state is the right contract for back-end storage.
111-129: Nice UX/accessibility for temperature input.Dynamic label, aria text, max bounds, and displayed value match the selected unit.
src/display/plugins/WebUIPlugin.cpp (2)
367-370: Parsing target temps as floats: LGTM.Switching to
toFloat()for steam/water targets matches the float migration in Settings.
415-417: Unit toggle handling: LGTM.Boolean parsing and setter usage are correct.
src/display/core/Settings.cpp (3)
10-37: Float migration for brew/steam/water targets: LGTM.The tb/ts/tw → tb_f/ts_f/tw_f migration is sound and idempotent.
76-76: Unit flag persistence: LGTM.Loading/saving
temp_fis consistent.Also applies to: 489-489
119-132: Verified: all setTarget callers use floats*
setTargetSteamTemp and setTargetWaterTemp are invoked with float values in Controller (float temperature) and WebUI (.toFloat()); setTargetBrewTemp has no existing call sites.src/display/core/Settings.h (3)
23-25: Float getters for target temps look good.API aligns with float migration.
154-156: Defaults updated to floats and unit flag added — good.Matches PR intent and keeps sensible Celsius defaults.
Also applies to: 189-189, 198-198
84-86: No float conversion issues in setTarget*Temp call sites
All callers use float parameters (Controller::setTargetSteamTemp/WaterTemp invoked withfloat temperature, WebUIPlugin usestoFloat())—no implicit int casts remain.src/display/ui/default/DefaultUI.cpp (3)
16-16: Including temperatureConverter is correct.Enables unit-aware formatting across screens.
312-314: Controller temps wired as floats — good.
85-90: Verify boiler temperature event publishers
- No
emit("boiler:currentTemperature:change", …)oremit("boiler:targetTemperature:change", …)calls found—locate the event producers.- Confirm they emit a float payload for
"value"(or switch togetDoublein DefaultUI if they’re sending doubles).
| settings->setTargetWaterTemp(request->arg("targetWaterTemp").toInt()); | ||
| settings->setTargetWaterTemp(request->arg("targetWaterTemp").toFloat()); | ||
| if (request->hasArg("temperatureOffset")) | ||
| settings->setTemperatureOffset(request->arg("temperatureOffset").toInt()); |
There was a problem hiding this comment.
Temperature offset truncation and type mismatch (float vs int).
POST uses toInt() and Settings stores/saves offset as int, which drops fractional offsets and conflicts with the PR goal (offsets as floats). Also, GET serializes the offset as a string.
Apply:
- if (request->hasArg("temperatureOffset"))
- settings->setTemperatureOffset(request->arg("temperatureOffset").toInt());
+ if (request->hasArg("temperatureOffset"))
+ settings->setTemperatureOffset(request->arg("temperatureOffset").toFloat());And return a numeric in GET:
- doc["temperatureOffset"] = String(settings.getTemperatureOffset());
+ doc["temperatureOffset"] = settings.getTemperatureOffset();Note: Requires Settings::setTemperatureOffset(float) and persisting as float (see Settings.cpp comments).
Also applies to: 469-471
🤖 Prompt for AI Agents
In src/display/plugins/WebUIPlugin.cpp around lines 371 and also lines 469-471,
the POST handler currently uses toInt() which truncates fractional temperature
offsets and mismatches the intended float storage; change the request parsing to
use toFloat(), call Settings::setTemperatureOffset(float) (update Settings API
and persistence to store a float), and ensure the GET handler serializes the
offset as a numeric JSON value (not a string). Adjust Settings.cpp to
persist/load the offset as a float and update any callers to the new float
signature.
| const Settings &settings = controller->getSettings(); | ||
| bool useFahrenheit = settings.isTemperatureUnitFahrenheit(); | ||
| // Scale gauge value to 0-160 range regardless of display unit | ||
| float gaugeValue = useFahrenheit ? | ||
| (formatTemperatureForDisplay(currentTemp, true) - 32) * 160.0f / (320.0f - 32.0f) : | ||
| currentTemp; | ||
| lv_arc_set_value(uic_MenuScreen_dials_tempGauge, gaugeValue); | ||
| lv_label_set_text_fmt(uic_MenuScreen_dials_tempText, "%d%s", | ||
| formatTemperatureForDisplay(currentTemp, useFahrenheit), | ||
| getTemperatureUnit(useFahrenheit)); | ||
| }, | ||
| ¤tTemp); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure UI updates immediately when unit toggles.
These effects only depend on current/target temp. If unit flips without a temp change, labels/gauges may not refresh unless a periodic rerender hits. Consider adding a dependency tied to the unit flag or emitting a “settings:temperatureUnit:change” that sets rerender.
Also applies to: 368-379, 382-393, 396-407, 410-421, 424-435, 441-445, 451-455, 462-467, 636-641
🏁 Script executed:
#!/bin/bash
# Check for a unit-change event and its handling in UI
rg -nP 'temperatureUnit(Fahrenheit)?|setTemperatureUnit\(|settings:temperatureUnit' -C3 --type=cpp --type=c --type=c++Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for temperature unit usage in UI code
rg -n 'isTemperatureUnitFahrenheit' -C3 --glob '*.cpp' --glob '*.h'
# Search for any Settings change notifications or callbacks
rg -n 'onSettingsChanged|settingsChanged|addObserver|subscribe|notify|emit|unitChanged' -C3 --glob '*.cpp' --glob '*.h'Length of output: 29705
Refresh temperature UI on unit toggle
- In src/display/ui/default/DefaultUI.cpp (Menu/Status/Brew/Grind/SimpleProcess/Profile screens – lines 354–365, 368–379, 382–393, 396–407, 410–421, 424–435, 441–445, 451–455, 462–467, 636–641), each
effect_mgr.use_effectonly watchescurrentTemp. Toggling °C/°F won’t re-run these effects unless the temperature changes. Add the unit flag as a dependency (for example,&settings.isTemperatureUnitFahrenheit()) or emit asettings:temperatureUnit:changeevent to force an immediate rerender on unit change.
🤖 Prompt for AI Agents
In src/display/ui/default/DefaultUI.cpp around lines 354–365 (and the other
listed ranges), each effect only watches ¤tTemp so toggling °C/°F won’t
re-run the effect; add the temperature-unit as a real dependency: create a
persistent bool (e.g., tempUnitFahrenheit) that you update when settings change
(or subscribe to a settings:temperatureUnit:change event) and pass its address
as an additional dependency to effect_mgr.use_effect so the effect is re-run
immediately on unit toggle; alternatively emit a settings:temperatureUnit:change
event to force rerender.
| <label htmlFor={`phase-${index}-temperature`} className='mb-2 block text-sm font-medium'> | ||
| Temperature (0 = Default) | ||
| Temperature (0 = Default) ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)}) | ||
| </label> | ||
| <div className='input-group'> | ||
| <label htmlFor={`phase-${index}-temperature`} className='input w-full'> | ||
| <input | ||
| id={`phase-${index}-target`} | ||
| className='grow' | ||
| type='number' | ||
| value={phase.temperature || 0} | ||
| onChange={e => onFieldChange('temperature', parseFloat(e.target.value))} | ||
| value={convertCelsiusToDisplay(phase.temperature || 0, machine.value.status.temperatureUnitFahrenheit)} | ||
| onChange={e => onFieldChange('temperature', parseFloat(e.target.value) || 0)} | ||
| aria-label='Target temperature' | ||
| min='0' | ||
| step='0.1' | ||
| /> | ||
| <span aria-label='celsius'>°C</span> | ||
| <span aria-label={machine.value.status.temperatureUnitFahrenheit ? 'fahrenheit' : 'celsius'}> | ||
| {getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)} | ||
| </span> | ||
| </label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix label/input association and preserve tenths in temperature field
- htmlFor points to -temperature but input id is -target.
- Keep one decimal in UI by passing precision to convertCelsiusToDisplay (after util change).
- <label htmlFor={`phase-${index}-temperature`} className='mb-2 block text-sm font-medium'>
+ <label htmlFor={`phase-${index}-temperature`} className='mb-2 block text-sm font-medium'>
Temperature (0 = Default) ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)})
</label>
@@
- <input
- id={`phase-${index}-target`}
+ <input
+ id={`phase-${index}-temperature`}
className='grow'
type='number'
- value={convertCelsiusToDisplay(phase.temperature || 0, machine.value.status.temperatureUnitFahrenheit)}
+ value={convertCelsiusToDisplay(
+ phase.temperature || 0,
+ machine.value.status.temperatureUnitFahrenheit,
+ 1
+ )}
onChange={e => onFieldChange('temperature', parseFloat(e.target.value) || 0)}
aria-label='Target temperature'
min='0'
step='0.1'
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <label htmlFor={`phase-${index}-temperature`} className='mb-2 block text-sm font-medium'> | |
| Temperature (0 = Default) | |
| Temperature (0 = Default) ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)}) | |
| </label> | |
| <div className='input-group'> | |
| <label htmlFor={`phase-${index}-temperature`} className='input w-full'> | |
| <input | |
| id={`phase-${index}-target`} | |
| className='grow' | |
| type='number' | |
| value={phase.temperature || 0} | |
| onChange={e => onFieldChange('temperature', parseFloat(e.target.value))} | |
| value={convertCelsiusToDisplay(phase.temperature || 0, machine.value.status.temperatureUnitFahrenheit)} | |
| onChange={e => onFieldChange('temperature', parseFloat(e.target.value) || 0)} | |
| aria-label='Target temperature' | |
| min='0' | |
| step='0.1' | |
| /> | |
| <span aria-label='celsius'>°C</span> | |
| <span aria-label={machine.value.status.temperatureUnitFahrenheit ? 'fahrenheit' : 'celsius'}> | |
| {getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)} | |
| </span> | |
| </label> | |
| <label htmlFor={`phase-${index}-temperature`} className='mb-2 block text-sm font-medium'> | |
| Temperature (0 = Default) ({getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)}) | |
| </label> | |
| <div className='input-group'> | |
| <label htmlFor={`phase-${index}-temperature`} className='input w-full'> | |
| <input | |
| id={`phase-${index}-temperature`} | |
| className='grow' | |
| type='number' | |
| value={convertCelsiusToDisplay( | |
| phase.temperature || 0, | |
| machine.value.status.temperatureUnitFahrenheit, | |
| 1 | |
| )} | |
| onChange={e => onFieldChange('temperature', parseFloat(e.target.value) || 0)} | |
| aria-label='Target temperature' | |
| min='0' | |
| step='0.1' | |
| /> | |
| <span aria-label={machine.value.status.temperatureUnitFahrenheit ? 'fahrenheit' : 'celsius'}> | |
| {getTemperatureUnit(machine.value.status.temperatureUnitFahrenheit)} | |
| </span> | |
| </label> |
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/ExtendedPhase.jsx around lines 127 to 145, the
label's htmlFor currently references `phase-${index}-temperature` while the
input id is `phase-${index}-target` and the temperature display loses tenths;
fix by making the id and htmlFor match (change the input id to
`phase-${index}-temperature` or vice versa so both use the same name), and
preserve one decimal in the UI by passing a precision argument (1) into
convertCelsiusToDisplay when rendering the value (after updating the util to
accept precision), ensuring onChange still uses parseFloat to retain decimals.
| // Load settings on component mount | ||
| useEffect(() => { | ||
| loadSettings(); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
loadSettings is undefined — runtime ReferenceError.
loadSettings() isn’t imported or defined in this file.
Apply one of these quick fixes (pick one):
- // Load settings on component mount
- useEffect(() => {
- loadSettings();
- }, []);Or import/implement loadSettings if intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Load settings on component mount | |
| useEffect(() => { | |
| loadSettings(); | |
| }, []); |
🤖 Prompt for AI Agents
In web/src/pages/ProfileEdit/StandardProfileForm.jsx around lines 19 to 23, the
useEffect calls loadSettings() but loadSettings is not defined or imported,
causing a runtime ReferenceError; fix by either (A) importing the intended
function (add the correct import for loadSettings from its module or file path
and ensure it’s exported there), (B) implementing a local loadSettings function
in this file that performs the settings load (handle async/await and errors), or
(C) remove or replace the useEffect if no load is needed; after
adding/importing, ensure the function is referenced correctly and any necessary
dependencies are added to the useEffect dependency array.
| import { | ||
| convertCelsiusToDisplay, | ||
| convertInputToCelsius, | ||
| getTemperatureUnit | ||
| } from '../../utils/temperatureConverter.js'; |
There was a problem hiding this comment.
Offsets are deltas; current conversion adds/subtracts 32 (wrong).
Using absolute conversions for offsets makes 5°C become 41°F instead of 9°F. Introduce delta-specific converters.
-import {
- convertCelsiusToDisplay,
- convertInputToCelsius,
- getTemperatureUnit
-} from '../../utils/temperatureConverter.js';
+import {
+ convertCelsiusToDisplay,
+ convertInputToCelsius,
+ getTemperatureUnit,
+ convertDeltaCToDisplay,
+ convertDeltaInputToCelsius
+} from '../../utils/temperatureConverter.js';Add these helpers in temperatureConverter.js (outside this file):
export function convertDeltaCToDisplay(deltaC, displayInFahrenheit = false) {
if (deltaC === null || deltaC === undefined || deltaC === '') return 0;
const n = Number(deltaC);
if (Object.is(n, -0)) return 0;
const v = displayInFahrenheit ? n * 9/5 : n;
return Math.round(v * 10) / 10;
}
export function convertDeltaInputToCelsius(inputDelta, inputInFahrenheit = false) {
if (inputDelta === null || inputDelta === undefined || inputDelta === '') return 0;
const n = Number(inputDelta);
if (n === 0) return 0;
return inputInFahrenheit ? n * 5/9 : n;
}🤖 Prompt for AI Agents
In web/src/pages/Settings/index.jsx around lines 15 to 19, offsets are being
converted using absolute temperature converters which incorrectly add/subtract
32; add the two delta-specific helpers to web/src/utils/temperatureConverter.js
(convertDeltaCToDisplay and convertDeltaInputToCelsius) as described in the
review comment, then update this file to import those two functions and replace
any conversion calls used for offsets (delta values) from
convertCelsiusToDisplay/convertInputToCelsius to
convertDeltaCToDisplay/convertDeltaInputToCelsius so offsets are scaled without
the ±32 shift.
| // Explicitly set the temperature unit boolean value | ||
| formDataToSubmit.set('temperatureUnitFahrenheit', formData.temperatureUnitFahrenheit ? 'true' : 'false'); | ||
|
|
||
| // Convert temperature values to Celsius for backend | ||
| formDataToSubmit.set('targetSteamTemp', | ||
| convertInputToCelsius(formData.targetSteamTemp, formData.temperatureUnitFahrenheit)); | ||
| formDataToSubmit.set('targetWaterTemp', | ||
| convertInputToCelsius(formData.targetWaterTemp, formData.temperatureUnitFahrenheit)); | ||
| formDataToSubmit.set('temperatureOffset', | ||
| convertInputToCelsius(formData.temperatureOffset, formData.temperatureUnitFahrenheit)); | ||
|
|
There was a problem hiding this comment.
Submit offset to backend using delta conversion; keep floats.
- formDataToSubmit.set('temperatureOffset',
- convertInputToCelsius(formData.temperatureOffset, formData.temperatureUnitFahrenheit));
+ formDataToSubmit.set(
+ 'temperatureOffset',
+ convertDeltaInputToCelsius(formData.temperatureOffset, formData.temperatureUnitFahrenheit)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Explicitly set the temperature unit boolean value | |
| formDataToSubmit.set('temperatureUnitFahrenheit', formData.temperatureUnitFahrenheit ? 'true' : 'false'); | |
| // Convert temperature values to Celsius for backend | |
| formDataToSubmit.set('targetSteamTemp', | |
| convertInputToCelsius(formData.targetSteamTemp, formData.temperatureUnitFahrenheit)); | |
| formDataToSubmit.set('targetWaterTemp', | |
| convertInputToCelsius(formData.targetWaterTemp, formData.temperatureUnitFahrenheit)); | |
| formDataToSubmit.set('temperatureOffset', | |
| convertInputToCelsius(formData.temperatureOffset, formData.temperatureUnitFahrenheit)); | |
| // Explicitly set the temperature unit boolean value | |
| formDataToSubmit.set( | |
| 'temperatureUnitFahrenheit', | |
| formData.temperatureUnitFahrenheit ? 'true' : 'false' | |
| ); | |
| // Convert temperature values to Celsius for backend | |
| formDataToSubmit.set( | |
| 'targetSteamTemp', | |
| convertInputToCelsius(formData.targetSteamTemp, formData.temperatureUnitFahrenheit) | |
| ); | |
| formDataToSubmit.set( | |
| 'targetWaterTemp', | |
| convertInputToCelsius(formData.targetWaterTemp, formData.temperatureUnitFahrenheit) | |
| ); | |
| formDataToSubmit.set( | |
| 'temperatureOffset', | |
| convertDeltaInputToCelsius(formData.temperatureOffset, formData.temperatureUnitFahrenheit) | |
| ); |
🤖 Prompt for AI Agents
In web/src/pages/Settings/index.jsx around lines 152 to 162, the
temperatureOffset is being converted like an absolute temperature but must be
submitted as a delta (preserving sign and decimal precision). Replace the
convertInputToCelsius call for temperatureOffset with the delta conversion
helper (e.g., convertDeltaToCelsius) so it converts offsets correctly, then set
the FormData value to the resulting numeric value converted to string without
extra rounding (String(deltaCelsius)) to keep floats; keep targetSteamTemp and
targetWaterTemp using convertInputToCelsius as-is.
| // Convert temperature values for display based on user preference | ||
| targetSteamTemp: convertCelsiusToDisplay( | ||
| data.targetSteamTemp, | ||
| data.temperatureUnitFahrenheit | ||
| ), | ||
| targetWaterTemp: convertCelsiusToDisplay( | ||
| data.targetWaterTemp, | ||
| data.temperatureUnitFahrenheit | ||
| ), | ||
| temperatureOffset: convertCelsiusToDisplay( | ||
| data.temperatureOffset, | ||
| data.temperatureUnitFahrenheit | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Apply delta conversion to response mapping for offset.
- temperatureOffset: convertCelsiusToDisplay(
- data.temperatureOffset,
- data.temperatureUnitFahrenheit
- ),
+ temperatureOffset: convertDeltaCToDisplay(
+ data.temperatureOffset,
+ data.temperatureUnitFahrenheit
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Convert temperature values for display based on user preference | |
| targetSteamTemp: convertCelsiusToDisplay( | |
| data.targetSteamTemp, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| targetWaterTemp: convertCelsiusToDisplay( | |
| data.targetWaterTemp, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| temperatureOffset: convertCelsiusToDisplay( | |
| data.temperatureOffset, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| }; | |
| // Convert temperature values for display based on user preference | |
| targetSteamTemp: convertCelsiusToDisplay( | |
| data.targetSteamTemp, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| targetWaterTemp: convertCelsiusToDisplay( | |
| data.targetWaterTemp, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| temperatureOffset: convertDeltaCToDisplay( | |
| data.temperatureOffset, | |
| data.temperatureUnitFahrenheit | |
| ), | |
| }; |
🤖 Prompt for AI Agents
In web/src/pages/Settings/index.jsx around lines 182 to 195, the mapping uses
convertCelsiusToDisplay for temperatureOffset but offsets should be converted as
deltas; change the call to the delta conversion helper (e.g.,
convertCelsiusDeltaToDisplay or the equivalent function) for temperatureOffset
and pass the same unit flag (data.temperatureUnitFahrenheit) so the offset is
converted correctly without applying absolute temperature conversion semantics.
dragm83
force-pushed
the
feature/temp-units
branch
from
3cf6fb2 to
2042e74
Compare
dragm83
force-pushed
the
feature/temp-units
branch
from
2042e74 to
e7be45b
Compare
… "default" regardless of unit.
… to support decimal temperature values.
…l, especially in Fahrenheit mode.
dragm83
force-pushed
the
feature/temp-units
branch
from
e7be45b to
3199a9a
Compare
|
1 participant
Front end
Added a toggle in the settings to select units (C / F).
Modified Web UI and Touch UI. Conversions are done on the front-end
Back end
Changed steam/water/offset temp setpoints to floats
Modified raise/lower controller function to use a dynamic offset based on units
Summary by CodeRabbit
New Features
Improvements
Bug Fixes