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.

[askuric]

Thanks for this.

Once again a lot of changes....
I appreciate the effort (and tokens :D) but its really hard to review this in this form.

First of all we only accept the PRs against the dev branch. And some of the fixes have already been merged by other people.
I would prefer this to be 13 PRs than 1 big one.
If you can separate this into smaller PRs.

If not I'm going to do my best to check it but it will take time.

[stijncarelsbergh]

Split into one PR per fix, all against dev, as you asked:

fix	PR
current-sense A/B sample swap (_swap(c_a.b, c_a.b) was a no-op)	#573
current-sense stepper auto-alignment sample swap	#574
current-sense A-C / B-C magnitude comparisons	#575
Commander::add() overflowing its fixed arrays	#576
HallSensor uninitialised state + fake rotation at start-up	#577
MagneticSensorSPI sharing one static data mask	#578
MagneticSensorI2C left-aligned LSB mask/shift	#579
trapezoid non-centred modulation producing nothing for Uq < 0	#580
angleOpenloop() storing an unsigned velocity	#581
MagneticSensorPWM pulseIn() timeout vs documented frequencies	#582
_atan2(0,0) losing its FLT_MIN guard	#583
HallSensor::getVelocity() reading direction outside the critical section	#584

Dropped because dev already has it: MagneticSensorAnalog subtracting min_raw_count
(the cpr off-by-one came with it), thanks to whoever landed that. Also dropped the phase-B
hunk of #575, since dev rewrote that section (it now uses the same max-ratio detection as
phase A, so the missing-fabs comparison is gone).

Each branch contains exactly that one fix on top of dev, so they can be reviewed, amended or
dropped independently. Closing this PR as superseded - reopen it if you would rather keep it as
a tracker.

One note on the checks, in case it saves you time: the STM32 jobs were failing on this PR
because it was based on master, not because of the fixes. master does not build against the
stm32duino core 3.x that the workflow installs:

src/drivers/hardware_specific/stm32/stm32_mcu.cpp:126:20: error: cannot convert
'TIM_HandleTypeDef*' to 'TIM_TypeDef*'

while the same sketch (Bluepill_examples/encoder/bluepill_position_control, FQBN
STMicroelectronics:stm32:GenF1:pnum=BLUEPILL_F103C8) compiles cleanly on dev, which is where
the STM32 v3 compatibility work is. The new PRs are based on dev; their runs are currently
action_required, i.e. waiting for approval because they come from a fork.