Usermod Temperature: use full 12-bit precision #4916
Conversation
WalkthroughAdds a configurable Dallas/OneWire resolution (9–12 bit), persists it in cfg.json, exposes it in the UI, and changes Temperature.cpp to read sensor scratchpad as a 16-bit value with masking by resolution for temperature calculation; DS18S20 handling remains special-cased. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (2)
usermods/Temperature/Temperature.cpp (2)
27-31: Fix swapped family-code comments for 0x22 and 0x28.0x22 is DS1822 and 0x28 is DS18B20. Comments are reversed in both the read switch and the discovery switch. Functionality is fine, but this can confuse future readers.
Apply:
@@ - case 0x22: // DS18B20 - case 0x28: // DS1822 + case 0x22: // DS1822 + case 0x28: // DS18B20 @@ - case 0x22: // DS18B20 - case 0x28: // DS1822 + case 0x22: // DS1822 + case 0x28: // DS18B20Also applies to: 71-76
23-26: Clarify DS18S20 note; optional: enable 12-bit interpolation using COUNT_REMAIN.
- Wording nit: “9-bit precision - 1-bit fraction part” is accurate but slightly cryptic. Suggest clarifying LSB size.
- Optional enhancement: For DS18S20 (0x10), you can derive effective 12-bit resolution via COUNT_REMAIN (data[6]) per Maxim’s app note.
Minimal comment tweak:
- case 0x10: // DS18S20 has 9-bit precision - 1-bit fraction part + case 0x10: // DS18S20: 9-bit resolution (LSB = 0.5°C)Optional interpolation (keeps your unified scaling path):
- result = (data[1] << 8) | data[0]; - retVal = float(result) * 0.5f; + result = (data[1] << 8) | data[0]; // 9-bit native + result <<= 3; // scale to 12-bit grid + if (data[7] == 0x10) { // COUNT_PER_C present per DS18S20 + result = (result & 0xFFF0) + 12 - data[6]; // apply COUNT_REMAIN + } + retVal = float(result) * 0.0625f; // align with DS18B20-family scaling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/Temperature/Temperature.cpp(1 hunks)
🔇 Additional comments (1)
usermods/Temperature/Temperature.cpp (1)
31-33: 12-bit direct scaling is correct and simpler.Combining the 16-bit two’s-complement raw value and multiplying by 0.0625f correctly yields °C across DS18B20/1822/1825/28EA00, including negatives. Nice cleanup.
|
coderabbitai is actually correct about the swapped family codes of the DS1822 and DS18B20. That did confuse me a bit. So I fixed it now. Also as coderabbitai said you could increase resolution of the DS18S20 by checking the COUNT_REMAIN, but I don't know if this is necessary. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/Temperature/Temperature.cpp (1)
31-33: Nit: replace magic numbers with named constexprsSmall readability win and avoids repeating literals.
Apply this diff locally:
- retVal = float(result) * 0.0625f; // 2^(-4) + retVal = float(result) * kDS18X20Scale; // 2^(-4)And here too:
- retVal = float(result) * 0.5f; + retVal = float(result) * kDS18S20Scale;Add these near the top of the file (outside any function):
static constexpr float kDS18S20Scale = 0.5f; // 0.5°C LSB static constexpr float kDS18X20Scale = 0.0625f; // 1/16°C LSB
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/Temperature/Temperature.cpp(2 hunks)
🔇 Additional comments (3)
usermods/Temperature/Temperature.cpp (3)
23-26: DS18S20 path unchanged and correct — LGTMCombining bytes to int16_t and scaling by 0.5°C matches the DS18S20 9‑bit format; no functional issues spotted.
27-34: Use full 12‑bit precision with proper sign handling — LGTMDirect 16‑bit combine plus 1/16°C scaling is correct for DS18B20/DS1822/DS1825/DS28EA00 and simplifies the logic while preserving negatives.
72-74: Fix comment labels for family codes — LGTM0x22 → DS1822 and 0x28 → DS18B20 are correct; matches the read logic above.
|
The 0.5°C (9 bit resolution) was intentional. |
|
@blazoncek I added the dropdown
And what intention is that? I cannot think of an application where the increased resolution (from 0.5 °C to 0.0625°C) would break something. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
usermods/Temperature/UsermodTemperature.h (1)
51-51: Consider using a boolean for the resolution flag.
highResolutionis a binary toggle. Usingboolreduces ambiguity and keeps JSON values the same (0/1). If you keepint8_t, clamp to {0,1} when reading config (see cpp comment).usermods/Temperature/Temperature.cpp (1)
312-313: Clamp config value to 0/1.Defensive read avoids accidental non-binary values.
Apply this diff:
- highResolution = top[FPSTR(_highResolution)] | highResolution; + highResolution = (top[FPSTR(_highResolution)] | highResolution) ? 1 : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/Temperature/Temperature.cpp(6 hunks)usermods/Temperature/UsermodTemperature.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/Temperature/Temperature.cpp (2)
usermods/BME68X_v2/BME68X_v2.cpp (4)
appendConfigData(758-782)appendConfigData(758-758)F(323-323)F(352-352)usermods/sht/sht.cpp (2)
appendConfigData(244-261)appendConfigData(244-244)
🔇 Additional comments (7)
usermods/Temperature/UsermodTemperature.h (1)
60-60: String key addition looks good.The
_highResolutionkey aligns with usage in Temperature.cpp.usermods/Temperature/Temperature.cpp (6)
23-26: Comment update is correct.DS18S20 0.5°C step (1 fractional bit) is accurate; logic unchanged.
76-78: Family code labels corrected.0x22→DS1822 and 0x28→DS18B20 mapping is now correct.
284-285: Config persistence OK.New
high-resolutionfield is saved alongside existing fields.
333-334: Return condition aligned with “newest parameter” pattern.Good choice to key readiness off
_highResolutionfor migration.
358-369: String table extension looks consistent.New
_highResolutionkey defined in PROGMEM alongside existing keys.
341-345: No functional change needed — addDD/addO exist; small spacing nit.addDD/addO are defined in wled00/data/settings_um.htm (addDD + addO, with var addDropdown = addDD) and addOption/addDropdown exist elsewhere, so the dropdown will render.
- Nit: usermods/Temperature/Temperature.cpp (around lines 341–344) — make label spacing consistent: change "0.0625°C (12-bit) resolution" → "0.0625 °C (12-bit) resolution".
- Optional: convert to addDropdown/addOption for stylistic consistency (not required).
Likely an incorrect or invalid review comment.
It might not break anything, but please explain the use case for why you need more than 0.5C accuracy |
I already linked the related issue here: emsesp/EMS-ESP32#2620. |
imagine a relay application that relies on the hysteresis of the 0.5°C for example. |
How would that be a problem? If you have a dallas sensor that floats between the values 21.475 and 21.5 °C, then the current implementation would float between 21.0 and 21.5 °C. If somebody has implemented a hysteresis against that the hysteresis would need to 1.0 °C or higher. That would not break with higher resolution. |
That's really insignificant. Be mindful that these sensors tend to be rather inaccurate. Especially the ones from dubious sources like Aliexpress. |
OK, I'll implement it like this.
That will not be necessary as IEEE float can represent negative powers of two (2^-n) acurractly. |
|
Once variable resolution is implemented I have no objections merging this. |
|
@blazoncek I implemented it. I took some inspiration from your commit. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
usermods/Temperature/Temperature.cpp (2)
310-310: Add bounds checking for resolution valueThe resolution value read from config should be validated to ensure it's within the valid range (0-3).
Add validation:
- resolution = top[FPSTR(_resolution)] | resolution; + resolution = constrain(top[FPSTR(_resolution)] | resolution, 0, 3);
31-35: Fix incorrect bit shift for resolution maskingThe current bit-shift implementation
0xFFFF << (3 - (resolution & 3))produces incorrect masks. Whenresolution=3(12-bit), this evaluates to0xFFFF << 0 = 0xFFFF, which doesn't mask any bits as intended. The correct formula should be0xFFFF << (3 - resolution)without the& 3operation.Apply this fix:
- result &= 0xFFFF << (3 - (resolution & 3)); + result &= 0xFFFF << (3 - resolution);The masks should be:
resolution=0(9-bit):0xFFF8(clears 3 LSBs)resolution=1(10-bit):0xFFFC(clears 2 LSBs)resolution=2(11-bit):0xFFFE(clears 1 LSB)resolution=3(12-bit):0xFFFF(no masking)
🧹 Nitpick comments (3)
usermods/Temperature/Temperature.cpp (3)
33-34: Clarify the rounding behavior in the commentThe comment mentions "rounding towards negative infinity" which could be misleading. Clearing LSBs performs truncation toward zero for positive values and toward negative infinity for negative values (due to two's complement representation).
Consider updating the comment for clarity:
- // Clear LSBs to match desired precision (9/10/11-bit) rounding towards negative infinity + // Clear LSBs to match desired precision (9/10/11-bit) - truncates toward zero for positive, toward negative infinity for negative
23-30: Consider adding resolution validation for DS18S20The DS18S20 (case 0x10) has fixed 9-bit resolution and doesn't support configurable resolution. While the UI note mentions this limitation, the code doesn't enforce it.
Consider logging a warning if a user tries to set higher resolution for DS18S20:
case 0x10: // DS18S20 has 9-bit precision - 1-bit fraction part if (resolution > 0) { DEBUG_PRINTLN(F("Warning: DS18S20 doesn't support resolution > 9-bit, ignoring setting")); } result = (data[1] << 8) | data[0]; retVal = float(result) * 0.5f; break;
341-344: Add missing space in resolution options for consistencyThe 10-bit, 11-bit, and 12-bit options are missing a space before "°C" while the 9-bit option has it.
- oappend(F("addO(dd,'0.25°C (10-bit)',1);")); - oappend(F("addO(dd,'0.125°C (11-bit)',2);")); - oappend(F("addO(dd,'0.0625°C (12-bit)',3);")); + oappend(F("addO(dd,'0.25 °C (10-bit)',1);")); + oappend(F("addO(dd,'0.125 °C (11-bit)',2);")); + oappend(F("addO(dd,'0.0625 °C (12-bit)',3);"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/Temperature/Temperature.cpp(6 hunks)usermods/Temperature/UsermodTemperature.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/Temperature/UsermodTemperature.h
🧰 Additional context used
🧬 Code graph analysis (1)
usermods/Temperature/Temperature.cpp (3)
usermods/BME68X_v2/BME68X_v2.cpp (4)
appendConfigData(758-782)appendConfigData(758-758)F(323-323)F(352-352)usermods/sht/sht.cpp (2)
appendConfigData(244-261)appendConfigData(244-244)usermods/Internal_Temperature_v2/Internal_Temperature_v2.cpp (1)
oappend(148-156)
🔇 Additional comments (2)
usermods/Temperature/Temperature.cpp (2)
282-282: Good implementation of configurable resolution featureThe implementation correctly:
- Persists the resolution setting in
cfg.json- Provides a user-friendly dropdown UI with clear options
- Maintains backward compatibility by defaulting to 9-bit (resolution=0)
- Follows the existing usermod patterns for configuration management
Also applies to: 331-331, 366-366
339-346: Verify dropdown UI generation & addDD/addO usageRepo search shows addDD/addO used only in usermods/Temperature/Temperature.cpp (lines 339–346). Confirm these helpers are defined in the WLED UI core and accept the (id,label,value) calls used here; if they aren't, replace with the framework's dropdown API or add a compatibility wrapper.
The popular DS18B20 temperature sensor offers 12-bit precision, but currently the Temperature Usermod only uses 9-bits of precision and does not use the 3 least significant bits.
This PR changes implementation so the full 12-bit precision when calculating the floating point Celsius value. Also I was able to remove all if statements similar to how the DS18S20 is already implemented.
I also checked the datasheets of the DS18B20, DS1822, DS1825 and DS28EA00. They all use the same format and all have the factory default of 12-bit resolution. So this should not break anything.
Summary by CodeRabbit
New Features
Bug Fixes
Notes