|
| 1 | +# Full `src/` Architecture Audit |
| 2 | + |
| 3 | +Generated: 2026-03-18 |
| 4 | +Updated: 2026-03-18 (applied fixes marked ~~strikethrough~~) |
| 5 | + |
| 6 | +Comprehensive audit of all source files under `src/`, identifying bugs, dead code, |
| 7 | +leftover code, code smells, inconsistencies, and technical debt. |
| 8 | + |
| 9 | +--- |
| 10 | + |
| 11 | +## Bugs (Highest Priority) |
| 12 | + |
| 13 | +| # | File | Line(s) | Description | Status | |
| 14 | +|---|------|---------|-------------|--------| |
| 15 | +| 1 | `pd_esp32/pd_config.h` | 30 | `#pragma "Last supported..."` should be `#error` -- silently ignored by compilers | open | |
| 16 | +| 2 | `pd_esp32/pd_config_idf4.h:31`, `pd_config_idf5.h:35` | -- | `HAVE_ESP32S3_PULSE_COUNTER` set for **ESP32S2** (copy-paste bug) | open | |
| 17 | +| ~~3~~ | ~~`FastAccelStepper.cpp`~~ | ~~885~~ | ~~`d *= steps` uses loop variable instead of `d *= cmd.steps`~~ | **fixed** | |
| 18 | +| 4 | `FastAccelStepper_idf5_esp32_pcnt.cpp` | 81-100 | 5 error paths leak `punit`/`pcnt_chan` before returning `false` | open | |
| 19 | +| ~~5~~ | ~~`FastAccelStepperEngine.cpp`~~ | ~~17, 150~~ | ~~File-scope `fas_stepper[]` shadowed by local `static`~~ | **fixed** | |
| 20 | +| 6 | `FastAccelStepperEngine.cpp` | 123-124 | No bounds check on `_stepper_cnt` increment -- buffer overflow | open | |
| 21 | +| 7 | `StepperISR_idf4_esp32c3_rmt.cpp:61`, `StepperISR_idf4_esp32s3_rmt.cpp:62` | -- | Hardcoded `61/62` in `stop_rmt()` is wrong for C3/S3 where `PART_SIZE=22` (should be `43/44`) | open | |
| 22 | +| ~~8~~ | ~~`StepperISR_esp32xx_rmt.cpp`~~ | ~~116-118~~ | ~~Dead redundant condition: inner `if (steps > PART_SIZE)` always true~~ | **fixed** (changed to `steps < 2*PART_SIZE`) | |
| 23 | +| ~~9~~ | ~~`sam_queue.cpp`~~ | ~~64-65~~ | ~~`timeElapsed = micros() - ...` uses a second `micros()` call instead of saved `t`~~ | **fixed** | |
| 24 | +| 10 | `sam_queue.cpp` | 140-145 | `static` locals in ISR initialized once from mutable pointer -- stale data on remap | open | |
| 25 | +| 11 | `pico_queue.cpp` | 192-196 | Inverted break condition: breaks when FIFO has data, should break when empty | open | |
| 26 | +| 12 | `esp32_queue.cpp` | 154 | `queues_allocated` never incremented in `SUPPORT_SELECT_DRIVER_TYPE + SUPPORT_DYNAMIC_ALLOCATION` path -- `MAX_STEPPER` check is dead code | open | |
| 27 | + |
| 28 | +--- |
| 29 | + |
| 30 | +## Dead Code / Leftover Code |
| 31 | + |
| 32 | +| # | File | Line(s) | Description | |
| 33 | +|---|------|---------|-------------| |
| 34 | +| 14 | `sam_queue.cpp` | 4-5 | `const bool enabled = false;` and `uint32_t junk = 0;` -- never used | |
| 35 | +| 15 | `sam_queue.cpp` | 45-51 | `strobePin()` -- defined but never called | |
| 36 | +| 16 | `sam_queue.cpp` | 23-33, 246-249 | `KEEP_SCORE` debugging block -- permanently disabled (`KEEP_SCORE` never defined) | |
| 37 | +| 17 | `pd_test/test_queue.h` | 20-24, 29-33 | ESP32-specific `SUPPORT_ESP32_RMT` blocks -- can never activate in test platform | |
| 38 | +| 18 | `AVRStepperPins.h` | 58-76 | `FAS_TIMER_MODULE == 3` and `== 5` branches -- values never assigned | |
| 39 | +| 19 | `result_codes.h` | 27-29, 74-76, 116-118, 161-163 | `aqeIsOk()`, `moveIsOk()`, `moveTimedIsOk()`, `delayIsValid()` -- never called | |
| 40 | +| 20 | `result_codes.h` | 78-91, 124-153, 165-176 | 3 `toString()` overloads -- never called | |
| 41 | +| 21 | `Log2RepresentationConst.h` | 9, 25, 29, 33, 37, 49, 71 | 7 `LOG2_CONST_*` constants -- unused in `src/` | |
| 42 | +| 22 | `Log2Representation.h` | 28 | `log2_rsquare(x)` macro -- never used | |
| 43 | +| 23 | `RampControl.h` | 16-18 | `advanceTargetPositionWithinInterruptDisabledScope()` -- never called | |
| 44 | +| 24 | `RampCalculator.cpp` | 31-180 | `calculate_ticks_v1`..`v8` behind `#ifdef TEST_TIMING` -- dead benchmark functions | |
| 45 | +| 25 | `pd_config_idf5.h` / `pd_config_idf6.h` | ~20 lines | Commented-out `SUPPORT_ESP32_MCPWM_PCNT`, `NEED_MCPWM_HEADERS`, etc. | |
| 46 | +| 26 | `pd_config_idf5.h` / `pd_config_idf6.h` | 139-154 | `NEED_MCPWM_HEADERS` / `NEED_PCNT_HEADERS` include blocks -- dead (never compiled) | |
| 47 | +| 27 | `StepperISR_esp32xx_rmt.cpp` | 117 | `steps_to_do = PART_SIZE` -- dead assignment, always overwritten by line 119 | |
| 48 | + |
| 49 | +Note: item #13 (accidental LLM prompt in `pd_pico/pico_pio.cpp:1-10`) was present at audit |
| 50 | +time but may have been removed separately. |
| 51 | + |
| 52 | +--- |
| 53 | + |
| 54 | +## Code Duplication (DRY Violations) |
| 55 | + |
| 56 | +| # | Files | Lines | Description | |
| 57 | +|---|------|-------|-------------| |
| 58 | +| 28 | 3x IDF4 RMT files + idf5 | ~600 lines total | `stop_rmt()`, `tx_intr_handler()`, `init_rmt()`, `forceStop_rmt()`, `isReadyForCommands_rmt()`, `_getPerformedPulses_rmt()`, `startQueue_rmt()` -- nearly identical across 4 files | |
| 59 | +| 29 | `pd_config_idf5.h` vs `pd_config_idf6.h` | entire files | 95%+ duplicated content | |
| 60 | +| 30 | `FastAccelStepperEngine.cpp` | 21-35 vs 37-52 | `init()` function body duplicated | |
| 61 | +| 31 | `FastAccelStepperEngine.cpp` | 108-165 | `stepperConnectToPin()` x4 variants with shared logic | |
| 62 | +| 32 | `FastAccelStepper_idf4_esp32_pcnt.cpp` vs `idf5` | 67-74 vs 111-122 | GPIO matrix setup nearly identical | |
| 63 | +| 33 | `FastAccelStepper.cpp` + `FastAccelStepperEngine.cpp` | 5-8 / 8-11 | `printf`/`puts` redefinition macro duplicated | |
| 64 | +| 34 | `RampControl.cpp` | 446-453 vs 489-497 | Identical pause-ticks-left calculation (only difference: `(uint32_t)` cast) | |
| 65 | +| 35 | `RampControl.cpp` | 141-149 vs 241-249 | Identical "ramp complete" early-return blocks | |
| 66 | +| 36 | `test_pc.h` vs `Log2Representation.cpp` | -- | `PROGMEM` / `pgm_read_byte_near` polyfill duplicated | |
| 67 | +| 37 | `avr_queue.h`, `pico_queue.h`, `sam_queue.h` | -- | `SET_ENABLE_PIN_STATE` macro verbatim duplicated | |
| 68 | +| 38 | `sam_queue.cpp` | 310-335 | 6 identical `attachInterrupt()` calls in switch cases | |
| 69 | + |
| 70 | +--- |
| 71 | + |
| 72 | +## Inconsistencies |
| 73 | + |
| 74 | +### Preprocessor Style |
| 75 | + |
| 76 | +| # | File | Details | |
| 77 | +|---|------|---------| |
| 78 | +| 39 | `esp32_queue.h` (lines 21, 35, 53, 68, 71 vs 76, 229) | `#ifdef SUPPORT_ESP32_RMT` mixed with `#if defined(SUPPORT_ESP32_RMT)` interchangeably | |
| 79 | + |
| 80 | +### Platform Configuration |
| 81 | + |
| 82 | +| # | File | Details | |
| 83 | +|---|------|---------| |
| 84 | +| 40 | `pd_config_idf4.h` vs `idf5/6.h` | `PART_SIZE` formula: IDF4 uses `((RMT_SIZE-1)/4)<<1`=30; IDF5/6 use `RMT_SIZE>>1`=32 | |
| 85 | +| 41 | `StepperISR_idf5_esp32_rmt.cpp:182` vs all IDF4 | `lastChunkContainsSteps` init: IDF5=`false`; all IDF4=`true` | |
| 86 | +| 42 | `pd_avr/pd_config.h:15` vs all others | `MIN_CMD_TICKS` divisor: AVR `/25000` (5x shorter); all others `/5000` | |
| 87 | +| 43 | IDF4 RMT files vs C3/S3 vs IDF5 | TRACE debug output: ESP32=`Serial`; C3/S3=`USBSerial`; IDF5=`printf` | |
| 88 | + |
| 89 | +### Naming Conventions |
| 90 | + |
| 91 | +| # | File | Details | |
| 92 | +|---|------|---------| |
| 93 | +| 44 | `fas_queue`/`fas_arch` vs `fas_ramp`/`log2` | Header guard prefix: `FAS_*` vs no prefix (`RAMP_CONTROL_H`, `LOG2REPRESENTATION_H`) | |
| 94 | +| 45 | `fas_queue/` vs `fas_ramp/` | File naming: `snake_case.cpp` vs `PascalCase.cpp` | |
| 95 | +| 46 | `queue_add_entry.cpp:14` vs elsewhere | `NULL` vs `nullptr` inconsistency | |
| 96 | + |
| 97 | +### Logic Patterns |
| 98 | + |
| 99 | +| # | File | Details | |
| 100 | +|---|------|---------| |
| 101 | +| 47 | `FastAccelStepper.cpp:554, 562` | Bitwise `&` used instead of logical `&&` for `bool` return values | |
| 102 | +| 48 | `StepperISR_idf4_esp32_rmt.cpp:160` vs C3/S3/IDF5 | `connect()` vs `connect_rmt()` called in `init_rmt()` | |
| 103 | +| 49 | `StepperISR_idf4_esp32_rmt.cpp:131-132` vs `idf5:85-86` | Pin setup order: `digitalWrite` then `pinMode` vs `pinMode` then `digitalWrite` | |
| 104 | +| 50 | `pd_config.h:19-21` | `SUPPORT_DYNAMIC_ALLOCATION` missing for IDF6 | |
| 105 | +| 51 | `esp32_queue.h:217-233` | `AFTER_DIR_CHANGE_DELAY_TICKS` missing in non-I2S path | |
| 106 | + |
| 107 | +--- |
| 108 | + |
| 109 | +## Typos |
| 110 | + |
| 111 | +All fixed. |
| 112 | + |
| 113 | +| # | File | Line | Fix | |
| 114 | +|---|------|------|-----| |
| 115 | +| ~~52~~ | ~~`FastAccelStepper.h`~~ | ~~271~~ | ~~"changeing" -> "changing"~~ | **fixed** | |
| 116 | +| ~~53~~ | ~~`FastAccelStepper.h`~~ | ~~501~~ | ~~"obsolote" -> "obsolete"~~ | **fixed** | |
| 117 | +| ~~54~~ | ~~`FastAccelStepper_idf5_esp32_pcnt.cpp`~~ | ~~8~~ | ~~"not save" -> "not safe"~~ | **fixed** | |
| 118 | +| ~~55~~ | ~~`pd_avr/pd_config.h`~~ | ~~36, 44, 53, 60, 64~~ | ~~"derivate" -> "derivative" (5 occurrences)~~ | **fixed** | |
| 119 | +| ~~56~~ | ~~`sam_queue.cpp`~~ | ~~7, 119~~ | ~~"Periphal" -> "Peripheral" (declaration + definition)~~ | **fixed** | |
| 120 | +| ~~57~~ | ~~`pico_pio.h`~~ | ~~1, 2, 19~~ | ~~`PD_PICO_PICO_PIO_H` -> `PD_PICO_PIO_H`~~ | **fixed** | |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## TODOs / Technical Debt |
| 125 | + |
| 126 | +| # | File | Line | Description | |
| 127 | +|---|------|------|-------------| |
| 128 | +| 58 | `queue_get_position.cpp` | 42 | `break; // TODO: ERROR` -- known bad position transitions silently ignored | |
| 129 | +| 59 | `esp32_queue.cpp` | 461 | `break; // TODO: ERROR` -- same pattern in `getCurrentPosition()` | |
| 130 | +| 60 | `pd_sam/pd_config.h` | 27 | `// TO BE CHECKED` -- `SUPPORT_QUEUE_ENTRY_END_POS_U16` unverified on SAM | |
| 131 | +| 61 | `FastAccelStepper.h` | 609 | "Will be renamed in future release" -- `getPeriodInUsAfterCommandsCompleted()` | |
| 132 | +| 62 | `FastAccelStepper_idf5_esp32_pcnt.cpp` | 9-19 | Private struct redeclaration to access `unit_id`/`channel_id` -- fragile across ESP-IDF versions | |
| 133 | + |
| 134 | +--- |
| 135 | + |
| 136 | +## Commented-Out Code (should be removed) |
| 137 | + |
| 138 | +Approximately **80+ lines** of commented-out code across the codebase: |
| 139 | + |
| 140 | +- `StepperISR_idf4_esp32_rmt.cpp:27-28, 148-149, 211-212, 226, 234, 254-255, 257, 271, 273` |
| 141 | +- `StepperISR_idf4_esp32c3_rmt.cpp:47-48, 180-181, 210-212, 288, 336-338` |
| 142 | +- `StepperISR_idf4_esp32s3_rmt.cpp:48-49, 181-182, 211-213, 289, 337-339` |
| 143 | +- `StepperISR_idf5_esp32_rmt.cpp:109-110, 129, 149, 177-180` |
| 144 | +- `StepperISR_idf4_esp32_mcpwm_pcnt.cpp:259, 473-474, 548-553` |
| 145 | +- `StepperISR_esp32xx_rmt.cpp:68-69` |
| 146 | +- `FastAccelStepper.cpp:309-310, 321-322, 335-336, 880-882` |
| 147 | +- `FastAccelStepper_idf4_esp32_pcnt.cpp:65-66` |
| 148 | +- `pico_pio.cpp:75, 79-83, 196-199` |
| 149 | +- `avr_queue.cpp:18-21` (behind `#ifdef DISABLE`) |
| 150 | +- `queue_add_entry.cpp:7-10, 29` |
| 151 | +- `RampControl.cpp:174, 415-417, 463` |
| 152 | +- `RampGenerator.cpp:246` |
| 153 | +- `RampCalculator.cpp:73-76` |
| 154 | +- `pd_pico/pico_queue.cpp:72-77` (duplicate retry call) |
| 155 | +- `pd_config_idf5.h:135, 147-148, 163` |
| 156 | +- `pd_config_idf6.h:135, 147-148, 163` |
| 157 | +- `i2s_fill.cpp:74-79, 87-90, 114-118` |
| 158 | + |
| 159 | +--- |
| 160 | + |
| 161 | +## Design / Encapsulation Issues |
| 162 | + |
| 163 | +| # | File | Line | Description | Status | |
| 164 | +|---|------|------|-------------|--------| |
| 165 | +| ~~63~~ | ~~`FastAccelStepperEngine.h`~~ | ~~145~~ | ~~`_delay_ms` is `public` but named as private (leading underscore)~~ | **fixed** | |
| 166 | +| 64 | `FastAccelStepper.h` | 49 | `#include "fas_ramp/RampGenerator.h"` in public header -- could be forward-declared | open | |
| 167 | +| 65 | `stepper_queue.h` | 6 | `#include "FastAccelStepper.h"` creates heavy dependency from queue layer to API | open | |
| 168 | +| 66 | `FastAccelStepper.cpp` | 739-891 | `moveTimed()` -- ~153 lines, should be split into helpers | open | |
| 169 | +| 67 | `FastAccelStepper.cpp` | 67-207 | `addQueueEntry()` -- ~140 lines, should be split into helpers | open | |
| 170 | +| 68 | `FastAccelStepper_idf5_esp32_pcnt.cpp` | 21-127 | `attachToPulseCounter()` -- ~106 lines | open | |
| 171 | + |
| 172 | +--- |
| 173 | + |
| 174 | +## Summary |
| 175 | + |
| 176 | +| Category | Total | Open | Fixed | |
| 177 | +|----------|-------|------|-------| |
| 178 | +| **Bugs** | 12 | 8 | 4 | |
| 179 | +| **Dead / leftover code** | 14 | 14 | 0 | |
| 180 | +| **Code duplication** | 11 | 11 | 0 | |
| 181 | +| **Inconsistencies** | 13 | 13 | 0 | |
| 182 | +| **Typos** | 6 | 0 | 6 | |
| 183 | +| **TODOs / tech debt** | 5 | 5 | 0 | |
| 184 | +| **Design / encapsulation** | 6 | 5 | 1 | |
| 185 | + |
| 186 | +### Remaining Recommended Fix Priority |
| 187 | + |
| 188 | +1. **Bug #7** -- Hardcoded `61/62` in C3/S3 `stop_rmt()` causes incorrect pause duration |
| 189 | +2. **Bug #2** -- `HAVE_ESP32S3_PULSE_COUNTER` on ESP32S2 may use wrong registers |
| 190 | +3. **Bug #6** -- `_stepper_cnt` overflow risk |
| 191 | +4. **Bug #1** -- `#pragma` should be `#error` |
| 192 | +5. **Bug #4** -- Memory leaks in idf5 pcnt error paths |
| 193 | +6. **Bugs #10, #11, #12** -- Remaining open bugs |
0 commit comments