I'll take a look at the code. Let me start by reading the skill for handling uploaded files, then examine the code.
Read the file-reading skill to know how to handle this upload
Read the file-reading skill to know how to handle this upload
An .ino file is Arduino source code — it's plain text. Let me check size and read it.
Ran a command, viewed a file
Ran a command, viewed a file
I have a clear picture now. Let me write up the analysis.
What this sketch is trying to do
It's a
propeller (or fan/rotor) balancer running on an ESP32 with an ADXL355 accelerometer and a tachometer pulse input. The intent:
- The prop spins. A tach pulse marks the start of each revolution (a once-per-rev signal — magnet + hall sensor or optical pickup).
- The ADXL355 streams acceleration samples at ~4 kHz via SPI, signaling each sample with DRDY.
- While RPM is within tolerance of a target, each sample is binned into one of 360 slots based on its angular position within the current revolution (delta / period × 360). The intent is to average x/y acceleration per angle slot over many revolutions, which integrates out random noise and leaves the once-per-rev imbalance signature.
- The slot with the highest vector magnitude √(x² + y²) is the angle of the heavy spot. Magnitude is converted from g to IPS (inches per second of vibration velocity) and pushed over BLE to a phone, along with RPM.
- Target RPM, tolerance, and a buffer-clear command are writable BLE characteristics.
The architecture is reasonable: ISRs do almost nothing, the loop() services accelerometer samples, and a separate FreeRTOS task pinned to core 0 handles BLE notifications every 500 ms.
Bugs
These range from "will not work at all" to "subtle and intermittent."
1. =+ instead of += — this is the show-stopper. Lines 293–295:
=+ is not a compound-assignment operator — it parses as = followed by a unary +, so each line
overwrites the slot with the latest value. NumberOfSamples becomes 1 every time, x and y become the most recent sample. No averaging happens; the whole binning scheme is defeated. This must be +=. This single typo is probably why the device doesn't appear to work the way the author expects.
2. MAXRPM is named backwards. The constant is 23000 µs and is used as a
minimum period check (if (d > MAXRPM)). 23,000 µs corresponds to ~2,600 RPM, so values
above MAXRPM mean the rotor is going
slower than ~2,600 RPM. The comment "2,600 max expected RPM" contradicts the variable name and contradicts the comment one line below it ("less than 2,600 RPM"). The check is actually doing
debounce / glitch rejection — discarding tach edges that arrive impossibly close together. Rename to MIN_TACH_PERIOD_US or TACH_DEBOUNCE_US and fix the comments.
More importantly, this debounce caps the
measurable RPM at ~2,600. If the user ever sets TargetRPM above 2,600, the tach ISR will reject every edge and period_us / rpm will never update. For a prop balancer that's probably fine; for general use it's a hidden limit.
3. Race conditions on multi-byte shared variables. period_us, last_tach_time_us, and sample_time_us are 64-bit and written in ISRs but read in non-ISR code without protection. ESP32 is 32-bit, so a 64-bit read is two loads and an interrupt can fire between them, giving a torn value. rpm is 32-bit so it's atomic on read, but period_us = d followed by rpm = 60000000 / period_us in the ISR means the loop can read rpm derived from the
new period while still seeing the
old last_tach_time_us (or vice versa). The delta = sample_time_us - last_tach_time_us calculation in loop() is the most exposed — if last_tach_time_us updates mid-calculation, delta can be wildly wrong. Also none of these are declared volatile except sample_time_us (and bDataReady isn't either, though it should be).
Fix: declare them all volatile, and read them inside a portENTER_CRITICAL/portEXIT_CRITICAL (or noInterrupts()/interrupts()) block, copying to local variables before use.
4. delta is signed 32-bit, period_us is unsigned 64-bit. Line 288:
This subtracts two uint64_t values and assigns to int32_t, truncating. As long as the actual delta fits in ~35 minutes (2³¹ µs) it works, but the intent is clearly "microseconds within one revolution," so use int64_t or compute the unsigned difference and check the bounds explicitly. Then on line 289 (delta < period_us) mixes signed and unsigned — the compiler will promote delta to unsigned, so a negative delta (which the previous line delta > -1 is trying to guard against) becomes a huge positive number and the guard fires backwards. The delta > -1 test does almost nothing useful here.
5. ClearBuffer() from a BLE callback races with loop() and UpdatePhone(). It zeroes the buffer non-atomically while the data thread is +=-ing into it and the phone thread is reading it. Worst case is a transient garbage angle reading, which probably isn't catastrophic for a balancer, but worth knowing.
6. pCharRPM->setValue(rpm) passes uint32_t, but the comparison on the line above uses String(rpm). setValue(uint32_t) and String(rpm) produce different byte representations on the BLE side (raw 4 bytes vs. ASCII digits). The phone app sees binary; the comparison logic compares against ASCII. The "did this change" check will
always think it changed, so notifications fire every 500 ms regardless. Same pattern for iMax and max. Either send strings everywhere (setValue(String(rpm).c_str())) or compare on the underlying numeric value.
7. Integer overflow in the angle-bin index. Line 290: (MAX_SAMPLES * delta) / period_us. MAX_SAMPLES is 360, delta can be ~23,000 at the slow end, product is ~8.3 million — fits in int32 easily. But if someone bumps MAX_SAMPLES or operates at lower RPM and the period is, say, 100,000 µs, 360 * 100000 = 36 million, still fine; at 1,000,000 µs (60 RPM) it's 360 million, still fine for int32 but getting close. Promote one operand to 64-bit to be safe: (int64_t)MAX_SAMPLES * delta / period_us.
8. uint TargetRPM — uint isn't standard C++. It works in the Arduino-ESP32 toolchain because <sys/types.h> is dragged in, but it's non-portable. Use uint32_t.
9. accel.selfTest() runs after startMeasurement() would normally be expected, but it's actually run before — and the result is ignored. It's just printed. If self-test fails, the sketch carries on regardless. Probably fine for hobby use; flag it and consider halting on failure.
10. while (true) {} on init failure starves the watchdog. On ESP32 the task watchdog will reset the chip. Use delay(1000) inside the loop or call esp_task_wdt_reset().
11. Sample struct has default initializers but is allocated as a global array — those initializers don't all reliably run for global aggregates in every toolchain version. Belt and suspenders: call ClearBuffer() once in setup().
12. The IPS conversion factor 3690 is a magic number. From the formula IPS = 3690 × g / RPM, this corresponds to g_to_ips = 386.4 × 60 / (2π) ≈ 3690, where 386.4 in/s² is 1g and the rest converts angular frequency. That's correct — but it assumes the measured g
is peak vibration amplitude. After per-slot averaging, what's in the slot is roughly the once-per-rev component's value at that angle, which is the right quantity, so this is OK once the += bug is fixed.
Suggestions for improvement
Beyond the bug fixes:
- Use a proper accumulator. Right now you're storing a running sum of g and dividing by N at display time, which is fine but loses precision at large N. A two-pass running mean is more numerically stable, though for ≤4 kHz × a few seconds of data it doesn't matter.
- Consider DC removal. Gravity contributes a constant ~1g on whichever axis is vertical; you don't want it in the imbalance estimate. The ADXL355 has a built-in HPF (the comment notes it's currently off). Turning it on (cutoff ~ODR/247) removes gravity and slow drift while leaving the once-per-rev signal intact. This is a real win for accuracy.
- Reset the buffer when RPM leaves tolerance, not just on user command. Otherwise stale samples from a prior speed contaminate the average.
- Phase reference matters. The current scheme labels slot 0 as "tach edge moment." That's only meaningful if the user knows the angular offset between the tach pickup and the prop's reference mark. Document this, or expose a calibration offset over BLE.
- Send all six characteristics with consistent encoding. Either everything as float bytes (and document the layout) or everything as ASCII strings. Mixing them makes the phone app's life unnecessarily hard.
- The String(...) != comparisons in UpdatePhone allocate on every iteration. Not a correctness issue, but ESP32 heap fragmentation from String is a classic. Cache the last sent values as numerics and compare those.
- Consider a queue from the sample ISR/loop to the BLE task rather than scanning the whole 360-slot buffer every 500 ms; it's only ~5 KB so the scan is cheap, but a queue would let you decouple update rate from buffer size.
- attachInterrupt(... RISING) on DRDY is correct for the ADXL355, but make sure the pin actually idles low. Some breakouts have DRDY active-low; check the datasheet for your board variant.
The single most important fix is
=+ → += on lines 293–295. That alone is the difference between "the balancer works" and "the balancer reports the angle of whatever the last sample happened to be."