8000
Skip to content

[rotary_encoder] account for min value when resetting - #18197

Merged
ssieb merged 5 commits into
devfrom
ssieb-patch-1
Aug 15, 2026
Merged

[rotary_encoder] account for min value when resetting#18197
ssieb merged 5 commits into
devfrom
ssieb-patch-1

Conversation

@ssieb
@ssieb ssieb commented Aug 9, 2026
Copy link
Copy Markdown
Member

What does this implement/fix?

If the min value is greater than 0, then the reset pin sets an invalid value of 0.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New developer-facing feature (adds functionality for component developers; no end-user configuration change)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — policy
  • Developer breaking change (an API change that could break external components) — policy
  • Undocumented C++ API change (removal or change of undocumented public methods that lambda users may depend on) — policy
  • Code quality improvements to existing code or addition of tests
  • Other

Related issue or feature (if applicable):

Pull request in esphome.io with documentation (if applicable):

  • esphome/esphome.io#<esphome.io PR number goes here>

Pull request in developers.esphome.io with developer documentation (if applicable):

  • esphome/developers.esphome.io#<developers.esphome.io PR number goes here>

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040/RP2350
  • BK72xx
  • RTL87xx
  • LN882x
  • nRF52840

Example entry for config.yaml:

# Example config.yaml

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@esphome
esphome Bot commented Aug 9, 2026
Copy link
Copy Markdown
Contributor

To use the changes from this PR as an external component, add the following to your ESPHome configuration YAML file:

external_components:
  - source: github://pr#18197
    components: [rotary_encoder]
    refresh: 1h

(Added by the PR bot)

ssieb
ssieb commented Aug 9, 2026
View reviewed changes
Comment thread esphome/components/rotary_encoder/rotary_encoder.cpp Outdated
@codecov
codecov Bot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.68%. Comparing base (c7d6b4a) to head (da7ae7a).
⚠️ Report is 147 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #18197      +/-   ##
==========================================
+ Coverage   87.28%   88.68%   +1.39%     
==========================================
  Files          64       68       +4     
  Lines       14697    15363     +666     
  Branches     2217     2327     +110     
==========================================
+ Hits        12829    13624     +795     
+ Misses       1558     1428     -130     
- Partials      310      311       +1

see 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ssieb
ssieb commented Aug 9, 2026
View reviewed changes
Comment thread esphome/components/rotary_encoder/rotary_encoder.cpp Outdated
@github-actions
github-actions Bot commented Aug 9, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Memory Impact Analysis

Components: rotary_encoder
Platform: esp8266-ard

Metric Target Branch This PR Change
RAM 29,700 bytes 29,700 bytes ➡️ +0 bytes (0.00%)
Flash 272,099 bytes 272,115 bytes 📈 🔸 +16 bytes (+0.01%)
📊 Component Memory Breakdown
Component Target Flash PR Flash Change
[esphome]rotary_encoder 1,473 bytes 1,481 bytes 📈 🔸 +8 bytes (+0.54%)
🔍 Symbol-Level Changes (click to expand)

Changed Symbols

Symbol Target Size PR Size Change
esphome::rotary_encoder::RotaryEncoderSensor::loop() 340 bytes 348 bytes 📈 +8 bytes (+2.35%)

Note: This analysis measures static RAM and Flash usage only (compile-time allocation).
Dynamic memory (heap) cannot be measured automatically.
⚠️ You must test this PR on a real device to measure free heap and ensure no runtime memory issues.

This analysis runs automatically when components change. Memory usage is measured from a representative test configuration.

ssieb
ssieb commented Aug 9, 2026
View reviewed changes
Comment thread esphome/components/rotary_encoder/rotary_encoder.cpp Outdated
@esphbot
esphbot commented Aug 15, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

esphbot
esphbot approved these changes Aug 15, 2026
View reviewed changes
@esphbot esphbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

ssieb
ssieb commented Aug 15, 2026
View reviewed changes
Comment thread esphome/components/rotary_encoder/rotary_encoder.cpp Outdated
@ssieb ssieb added this to the 2026.8.0 milestone Aug 15, 2026
@esphbot
esphbot commented Aug 15, 2026
Copy link
Copy Markdown
Contributor

PR Review — [rotary_encoder] account for min value when resetting

Merge-ready. The one suggestion from the prior review has been adopted — reset now clamps against both ends of the range.

What's solid:

  • The diagnosis is exact: the ISR only ever moves the counter within [min_value, max_value] (guards at rotary_encoder.cpp:95 and :102), so writing a bare 0 on reset was the only path that could park the counter outside its own range. Fixing it at the single write site keeps the change to one expression.
  • Switching from std::max to std::clamp<int32_t> also covers the mirror-image case (an all-negative range such as min: -100 / max: -10), and now matches what setup() already does at rotary_encoder.cpp:143. Reset path and init path finally agree.
  • The explicit <int32_t> template argument is required, not decorative — int32_t is long on ESP8266, so a bare std::clamp(0, ...) would fail template deduction there.

Verified against the codebase:

  • std::clamp's precondition (lo <= hi) always holds: validate_min_max_value in sensor.py:47 rejects min >= max when both are set, and the defaults (INT32_MIN/INT32_MAX, rotary_encoder.h:32-33) leave the other bound unreachable when only one is configured.

  • <algorithm> reaches the TU via esphome/core/helpers.h; no new include needed. helpers.h:74 does using std::clamp;, so the qualified std::clamp here and the unqualified clamp at line 143 are literally the same function.

  • No behavior change for existing configs: any range spanning zero still yields 0, exactly as before. Cost is +8 bytes flash in loop(), 0 RAM.

  • No findings. PR description matches the diff, no scope creep.

✅ Resolved since last review (1)

Previously-flagged issues verified fixed
  • esphome/components/rotary_encoder/rotary_encoder.cpp:223 Reset still ignores max_value — use clamp() like setup() does

Checklist

  • Fix matches the described bug and the linked issue
  • Boundary values handled at both ends of the range
  • clamp() precondition (min <= max) guaranteed by config validation
  • No behavior change for default/existing configurations
  • Required headers available ( via helpers.h)
  • Compiles on ESP8266 where int32_t is long
  • No scope creep relative to PR description

Automated review by Kōan (Claude) HEAD=da7ae7a 1 min 2s

esphbot
esphbot approved these changes Aug 15, 2026
View reviewed changes
@esphbot esphbot left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

No blocking issues found — ready to merge.

@ssieb
ssieb merged commit 5a000cf into dev Aug 15, 2026
42 checks passed
@ssieb
ssieb deleted the ssieb-patch-1 branch August 15, 2026 18:16
@jesserockz jesserockz mentioned this pull request Aug 16, 2026
Merged
pull Bot pushed a commit to mcx/esphome that referenced this pull request Aug 16, 2026
@ssieb @jesserockz
[rotary_encoder] account for min and max value when resetting (esphom…
46a5665 
…e#18197)
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bdraco bdraco bdraco approved these changes
+1 more reviewer
@esphbot esphbot esphbot approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix cherry-picked component: rotary_encoder small-pr PR < 30 lines

Projects

None yet

Milestone

2026.8.0

Development

Successfully merging this pull request may close these issues.

Rotary Encoder Sensor - reset and min_value

4 participants

@ssieb @esphbot @bdraco @jesserockz
0