feat(encoder): CRTP magnetic_encoder base, espp::Timer update path, AS5600 12-bit fix

[finger563]

Summary

Refactors the as5600 and mt6701 magnetic encoders, which were ~700 lines of verbatim-duplicated logic differing only in the per-device sensor read.

• New CRTP base — components/magnetic_encoder. espp::MagneticEncoderBase<Derived, UseHighResTimer, CountsPerRevolution, MinDiff, RegAddr, UseAddress> holds the shared update loop, all get_* accessors, start/stop/initialize, and the periodic driver. It refreshes state via static_cast<Derived*>(this)->read(ec) — zero-cost static dispatch, no vtable, so no overhead even at 1–2 kHz. As5600 and Mt6701<I> derive from it and supply only read().
• Removed the "override the update task" APIs — the (Config, espp::Task::Config) constructor and the initialize(..., espp::Task::Config, ...) overload are gone from both encoders. Users can no longer swap the update task out from under the encoder.
• Task → Timer for the non-HRT path. The old fallback used espp::Task with a manual cv.wait_until loop that drifts (fixed-delay). It now uses espp::Timer, which schedules against an absolute wake-up time (start + k*period) — periodic and in phase, which matters for stable velocity / accumulator state. The choice stays a compile-time Kconfig switch, renamed *_USE_TIMER → *_USE_HIGH_RESOLUTION_TIMER (y = HighResolutionTimer, n = espp::Timer).
• AS5600 12-bit correctness fix. The AS5600 is 12-bit (4096 counts/rev) but was read as 14-bit (16384) with the low 2 bits dropped — reporting ~1/16 of the true angle. read() now packs the full 12-bit value and COUNTS_PER_REVOLUTION is 4096.
• Accumulator int → int64_t. atomic<int> overflowed after ~131072 rev (~44 min @ 3000 RPM). get_accumulator() now returns int64_t.
• MT6701 SSI CRC is now computed and warned on (rate-limited) rather than read-and-ignored; removed the unused MagneticFieldStatus struct.

⚠️ Behavior changes to be aware of

• AS5600 angle scale changes 16× (now correct per the datasheet). A full mechanical turn previously read ~22.5°; it now reads 360°. Wants hardware confirmation.
• get_accumulator() return type int → int64_t (minor source-compat; no in-tree consumers use it).
• Kconfig symbol renamed *_USE_TIMER → *_USE_HIGH_RESOLUTION_TIMER (a mild sdkconfig break; existing y configs still select the HighResolutionTimer).

Note on the MT6701 CRC

The CRC-6 check is observability-only (warn on mismatch, the sample is still used). The CRC table/algorithm hasn't been verified against hardware yet — rejecting samples with a subtly wrong implementation would brick the encoder. Once verified on-device, it can be promoted to set ec and skip the bad sample. Watch the logs for CRC mismatch warnings during clean reads.

Consumer impact

BldcMotor (and the MotorGo boards) call only update / get_radians / get_rpm / get_mechanical_radians / needs_zero_search on the encoder — all unchanged signatures. Nothing in-tree calls get_accumulator, so the int64_t widening breaks no callers. The two encoder examples build unchanged (they used only the plain Config constructor).

Verification

Built an esp32s3 project instantiating As5600, Mt6701<I2C>, and Mt6701<SSI> exercising the full public API + the fmt formatters. Compiles cleanly in both Kconfig branches (HighResolutionTimer default and USE_HIGH_RESOLUTION_TIMER=n → espp::Timer). static_asserts lock in int64_t get_accumulator() and the 4096 / 16384 resolutions.

Not yet hardware-tested — holding for on-device validation (AS5600 angle scale + MT6701 CRC) before merge.

🤖 Generated with Claude Code

[github-actions]

✅Static analysis result - no issues found! ✅

[Copilot]

Pull request overview

Refactors magnetic encoders around a shared CRTP base while correcting AS5600 scaling and improving MT6701 diagnostics.

Changes:

• Adds shared timer-driven encoder logic with 64-bit accumulation.
• Migrates AS5600 and MT6701 to the new base.
• Adds MT6701 CRC observability and updates documentation/configuration.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
doc/en/encoder/mt6701.rst	Documents timer-based updates.
doc/en/encoder/magnetic_encoder.rst	Adds base-class documentation.
doc/en/encoder/index.rst	Links the new documentation.
doc/en/encoder/as5600.rst	Documents timer-based updates.
doc/Doxyfile	Includes the new header.
components/mt6701/Kconfig	Renames timer configuration.
components/mt6701/include/mt6701.hpp	Adopts CRTP base and adds CRC checking.
components/mt6701/idf_component.yml	Adds the new dependency.
components/mt6701/CMakeLists.txt	Updates build dependencies.
components/magnetic_encoder/include/magnetic_encoder_base.hpp	Implements shared encoder behavior.
components/magnetic_encoder/idf_component.yml	Defines the registry component.
components/magnetic_encoder/CMakeLists.txt	Registers the component.
components/as5600/Kconfig	Renames timer configuration.
components/as5600/include/as5600.hpp	Adopts CRTP base and fixes 12-bit reads.
components/as5600/idf_component.yml	Adds the new dependency.
components/as5600/CMakeLists.txt	Updates build dependencies.

Suppressed comments (2)

components/magnetic_encoder/include/magnetic_encoder_base.hpp:220

• This limit is symmetric, but the comparison only warns for positive rotation. A negative velocity at the measurement limit is never reported; compare the magnitude instead.

      if (raw_velocity >= max_velocity) {

components/magnetic_encoder/include/magnetic_encoder_base.hpp:329

• When initialized with run_task = false, prev_time_us_ remains zero, so the first documented manual update() computes velocity over time since boot rather than time since initialization. Seed the timestamp alongside the accumulator; start() can still refresh it for timer mode.

    accumulator_ = count_.load();

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[finger563]

Addressed the review in d855c4b:

Correctness (magnetic_encoder_base.hpp)

• Failed read advancing prev_time_us_ — update() now samples now_us/prev_count before the read and commits prev_time_us_ only after read() succeeds, so a failed read no longer shortens the next sample's dt (which had inflated velocity / the aliasing check).
• First manual update() after run_task = false — init() now seeds prev_time_us_ right after the initial read, so the first manual update() measures dt from init, not from boot.
• One-sided velocity-limit warning — the "nearing measurement limit" check now compares std::abs(raw_velocity), so it fires for both rotation directions.

Registry / docs

• Added components/magnetic_encoder/README.md, and listed the component in upload_components.yml ahead of as5600/mt6701 (with a note) so it publishes first and the espp/magnetic_encoder >=1.0 dependency resolves.
• Updated the as5600/mt6701 component READMEs to describe the timer (not a task); clarified in the example READMEs that the polling Task is the example's own (the encoder maintains its state via an internal timer).

One item intentionally not done: the request for a standalone example project + CI build-matrix entry for magnetic_encoder. This is a header-only CRTP base that isn't instantiated directly — it mirrors base_peripheral, which likewise has no example/ and no build.yml entry. It's compile-validated transitively through the as5600/mt6701 example builds and validated as a registry package by the upload_components dry-run on this PR. Happy to add an example if you'd prefer it not follow the base_peripheral precedent.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

components/as5600/include/as5600.hpp:21

• The class-level comment still states it "starts its own measurement task", but the implementation now uses an internal timer (HighResolutionTimer/Timer) rather than a task. Update this wording to avoid misleading API users.

/**
 * @brief Class for position and velocity measurement using a AS5600 magnetic
 *        encoder. This class starts its own measurement task at the specified

components/mt6701/include/mt6701.hpp:27

• The class-level comment still states it "starts its own measurement task", but the implementation now uses an internal timer (HighResolutionTimer/Timer) rather than a task. Update this wording to keep the API documentation accurate.

/**
 * @brief Class for position and velocity measurement using a MT6701 magnetic
 *        encoder. This class starts its own measurement task at the specified

components/as5600/include/as5600.hpp:74

• The field name run_task now controls whether the internal timer runs on startup, which is inconsistent and can confuse users (especially since task-based override APIs were removed). Consider introducing a clearer name (e.g., run_timer / auto_start) while keeping run_task as a backward-compatible alias/deprecated field, or update public docs to explicitly call out that run_task refers to the timer.

    bool run_task{true};  ///< Whether to run the timer on startup. If false, you must call update()