Fix confirmed logic bugs found while auditing the library sources (13 fixes, one per commit)

[stijncarelsbergh]

Fix confirmed logic bugs found while auditing the library sources

This is the code-change PR that goes with the documentation/comment clean-up (#570).
Every commit here is a separate, self-contained bug fix so that individual commits can be
dropped, reverted or discussed independently - there is no need for a new PR per disagreement.

All of these were found by reading the sources against the documented behaviour and modelling
the arithmetic by hand; none of them are style/refactor changes. Two platform builds were run
(see "How this was checked") but there was no hardware in the loop, so please treat the
behavioural claims as reasoning + build evidence, not as measured results.

The fixes

Memory safety

1. Commander::add() wrote past its fixed arrays (commander-bounds)
call_list[20], call_ids[20], call_label[20] with an unchecked call_count++: the 21st
add() writes a function pointer, a char and a pointer past the end of the object. Now guarded
with sizeof so the arrays can grow without touching the check.

Current sense (this is the "auto alignment" feature that fixes bad wiring)

2. alignBLDCDriver() swapped a variable with itself (cs-swap)
_swap(c_a.b, c_a.b) is a no-op, so after relabelling the pins the polarity check
(_sign(c_a.a) < 0) looked at the sample of the other channel and inverted the gain of the
phase it had just repaired - positive feedback in foc_current/dc_current mode. Same defect in
the A-(B)NC branch, and the A-(C)NC branch swapped the wrong pair (c_a.b, c_a.c while the
pins swapped were A and C).

3. alignStepperDriver() had the same problem (cs-stepper)
The pins/offsets/gains move but the samples do not, so if (c.a < 0) tested a channel that reads
~0 and the polarity decision was effectively random for the wiring the routine exists to correct.

4. Three magnitude comparisons in the hybrid alignment were ineffective (cs-hybrid-mag)
(fabs(c.a) - fabs(c.c)) > 0.1f passes silently when the second channel reads higher (missing
fabs); the B-C branch compared c.a with c.c (copy-paste) while its message refers to phase B;
the phase-B section had the same missing fabs().

Sensors

5. HallSensor started from uninitialised state (hall-init)
The constructor never initialised hall_state, electric_sector, electric_rotations,
total_interrupts, pulse_diff, pulse_timestamp, direction, old_direction,
use_interrupt - indeterminate for heap/stack instances, and wrong even for globals: init()
call updateState() before any state was adopted, and when the rotor sits in sector 4 or 5 at
power-up the sector difference (4-5) is read as an electrical underflow, so the sensor starts with
electric_rotations = -1 (2 of the 6 possible power-up positions). init() now adopts the
measured hall state directly instead.

6. MagneticSensorSPI shared one data mask between all instances (spi-mask)
const static word data_mask = 0xFFFF >> (16 - bit_resolution); is initialised once, so a second
sensor with a different resolution silently gets the first sensor's mask (and therefore wrong
angles). Removing static costs one shift per read.

7. MagneticSensorAnalog never subtracted min_raw_count (analog-min)
cpr = max_raw_count - min_raw_count was computed and min_raw_count stored, but the angle was
raw_count / cpr * 2*PI. With the 14..1020 range used in the docs/examples that is a constant
offset of 14/10062PI ~ 5 degrees, while the span is still exactly 2*PI - which is why it looks
almost right.

8. MagneticSensorI2C threw away the left-aligned LSB bits (i2c-lsb)
For a left-aligned sensor the remaining bits sit in the upper part of the low byte, so the mask
must be shifted too. The constructor used the right-aligned mask (0x3F for 6 remaining bits) and
then shifted the result right by 8 - lsb_used, keeping the status/parity bits and dropping the
real data. The preset configurations in the header (MT6701_I2C with lsb_mask 0xFC,
lsb_shift 2) show the intended convention.

9. MagneticSensorPWM read timeout was too short for the documented frequencies (pwm-timeout)
The 5-argument constructor documents the AS5600 PWM modes (115/230/460/920 Hz) and computes the
raw counts from them, but left timeout_us at 1200 us. One period at 115 Hz is ~8.7 ms and the
high pulse can be ~8.4 ms, so pulseIn() times out, returns 0, the angle sticks at
min_raw_count - a silently dead sensor for an officially supported configuration (only 920 Hz
fitted into the old timeout). The timeout is now 1.2 periods.

10. HallSensor::getVelocity() read direction outside the critical section (hall-velocity)
direction is written by the ISR and was read after interrupts(); a sector change in between
mixes the old pulse timing with the new direction. Now sampled together with pulse_diff.

Control

11. Non-centred trapezoid commutation produced no output for Uq < 0 (trap-negative)
center = ... : Uq makes all three phase voltages <= 0 for negative Uq; setPwm() clamps them
to 0, so the motor gets nothing in one direction:

Uq	map	map*Uq + Uq	after [0,V] clamp
+5	{0,+1,-1}	{5,10,0}	{5,10,0} works
-5	{0,+1,-1}	{-5,-10,0}	{0,0,0} no output
-5	min-clamp	{5,0,10}	{5,0,10} works

The trapezoid branches now use the same "shift up by the minimum" idiom as the sine/SVPWM
branches. Every sector map contains a -1, so for Uq > 0 the result is provably identical to
before (map*Uq + Uq), and for Uq < 0 it produces the correct waveform.

12. angleOpenloop() stored an unsigned speed (focmotor-velocity)
It moves in both directions but always wrote a positive shaft_velocity, while
velocityOpenloop() stores the signed value. Monitoring and the back-EMF term of
estimated_current torque control therefore saw a positive speed while moving backwards.

13. _atan2() lost its FLT_MIN guard (atan2-guard)
The comment says "inject FLT_MIN in denominator to avoid division by zero" but the term is missing
(it was dropped from the ODrive original), so _atan2(0,0) is 0/0 -> NaN. Restored; only the
degenerate (0,0) case changes.

How this was checked

• All library sources compile with the real Arduino toolchains for
  arduino:avr:mega (39708 bytes flash / 1383 bytes RAM) and esp32:esp32:esp32
  (360796 bytes / 24700 bytes) using the example
  examples/motor_commands_serial_examples/magnetic_sensor/full_control_serial.
  The Arduino build compiles every file under src/, so all ten touched files were compiled on
  both platforms.
• Structural check: the number of (/), [/] and {/} in every modified file is
  unchanged relative to master (10 files, 0 unbalanced).
• No size/diff noise: git diff --stat master is 10 files, +71/-20.

What is deliberately not in this PR

These were found in the same audit but need a maintainer decision or are not simple bug fixes;
I have opened a separate issue for them: _micros() on AVR (/32 vs /64 - needs an
oscilloscope/hardware check), the move() downsampling off-by-one, the EFR32 dead-time clamp and
its SILABBS macro, the characteriseMotor() correction-factor inconsistency, the
trapezoid -> sine phase re-enable (needs a cheap way to detect a modulation change in the PWM
hot path), the performance items and the motor-class/Commander refactoring.

Suggested review path

git log --oneline gives 13 commits, one per bug. Reviewing cs-swap, hall-init and
trap-negative in detail is probably the most valuable; the rest are one to five lines each.
Commits that change observable behaviour (analog offset, trapezoid Uq < 0, PWM timeout, the
HallSensor start-up angle) are worth a quick hardware sanity check before release.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.