Add a shared Wheels base class with a drivable gate

[JensOgorek]

Motivation

The three wheels modules (DunkerWheels, ODriveWheels, RoboClawWheels) each inherited directly from Module and duplicated the same scaffolding: the width/linear_speed/angular_speed/enabled properties, the enabled-sync in step() and enable()/disable(). There was no common place to add cross-cutting behaviour — in particular a way to gate driving without disabling the motors.

Implementation

• Shared base. A new abstract Wheels : public Module (wheels.h/wheels.cpp) owns the common properties, the step() enabled-sync and the speed/enable/disable command flow. The concrete drivetrains only implement the motor-specific hooks: do_wheel_speeds, do_enable, do_disable, update_odometry. Behaviour is unchanged for all three (net −85 lines in the refactor commit).
• New drivable property. A gate that external rules can toggle to (dis)allow driving without disabling the motors. While enabled but not drivable, speed()/power() brake the motors to 0 rather than being ignored — the drive loop streams commands continuously, so merely ignoring them would hold the last commanded speed and keep the robot moving. The motors stay enabled and resume as soon as drivable is true again.
• Shadow-aware gate. Shadows mirror method calls but not property writes, so Wheels::write_property forwards the drivable write to its shadow modules. This keeps shadowed setups (e.g. front/back tracks where wheels.shadow(wheels_front)) gated together instead of leaving the shadow driving. (Module::shadow_modules is now protected for this.)

Usage

Our new delta-arm drivetrain already relies on this: a brain-side rule sets wheels.drivable = false while the arm is not parked (both endstops inactive), so the robot can only drive once the arm is raised — and thanks to the shadow forwarding both track modules are gated at once.

Progress

• I chose a meaningful title that completes the sentence: "If applied, this PR will..."
• The implementation is complete.
• It builds (python build.py esp32 --clean).
• Tests with real hardware have been successful (or are not necessary).
• Pytests have been added (or are not necessary).
• Documentation has been added (the drivable property is listed for each wheels module).

[JensOgorek]

Tested today on F21, in combination with RMD-Delta-Arm, and it was working. When the Arm was not at the home position, the robot did not drive

[falkoschindler]

Thanks for taking this on, @JensOgorek — cutting the duplication across the three wheels modules is a worthwhile goal. They'd drifted into copy-pasted scaffolding, and looking for shared structure is a reasonable instinct. A way to gate driving without fully disabling the motors is a welcome capability too.

That said, a few things give me pause, and on balance I'd suggest reworking the change rather than merging it as-is. The two points below are structural — they're my main reason — and the concrete findings after them are the evidence.

Verdict: request changes, ideally by splitting this into two focused PRs (see the proposal at the end).

Blocking concerns

1. The PR does two unrelated things in one (undivided) change

It performs a behavior-preserving refactor (hoisting the shared scaffolding into Wheels) and introduces a new feature (drivable gate + braking + shadow forwarding) — and it does so already in the first commit (394ac92 "Add a shared Wheels base class with a drivable gate"). Mixed like this, the refactor is no longer reviewable: I can't confirm the extraction is faithful to the old per-module behavior, because new behavior is interleaved in the same diff. This is exactly why finding 3 below (a silent property → member behavior change for two of the three drivetrains) is so easy to miss — it's buried in refactor noise.

Please split this into two independent changes: (a) a pure, behavior-preserving refactor introducing Wheels, reviewable line-for-line against the old modules, and (b) a separate feature change that adds drivable.

And a caveat on the refactor itself: I'm not fully convinced a shared base is the right call, and that's worth deciding deliberately rather than by default. The three drivetrains differ in more than cosmetics — DunkerWheels has no power() command and no m_per_tick, RoboClawWheels scales every speed by m_per_tick and reads wrapped-u32 encoder positions, and the odometry derivation differs across all three. Those small content differences may be exactly why a common base didn't exist until now. Tellingly, the power() path couldn't be pulled into the base and had to be duplicated in two subclasses (finding 2) — a hint that the abstraction fits the easy common part but not the parts that actually vary. A separate pure-refactor PR (below) would put this decision in the open: if the behavior-preserving version reads cleanly, the base earns its place; if it's riddled with per-subclass special-casing, that's the answer.

2. drivable's purpose isn't specified, and the "unified" implementation isn't uniform

The description motivates the refactor and shows one usage (delta-arm: only drive when the arm is parked), but it never states the semantic contract of drivable vs. the existing disable. That contract is the entire reason the feature exists: disable switches the motors off; drivable=false is supposed to keep them enabled and hold/brake while blocking driving. Two problems:

• The contract is contradicted by the implementation. speed() brakes (velocity 0, actively holds), power() coasts (torque/duty 0). So in the power case drivable=false does not hold — it behaves like a soft off, collapsing the very distinction from disable that justifies the feature (see finding 1).
• The refactor is sold as unification, but it produces divergent behavior along several axes: speed vs. power braking (finding 1), enabled gated via member vs. property (finding 3), and drivable forwarded to shadows while enabled is not (finding 4). Unifying the code did not unify the behavior — which leaves the semantics more confusing than before, not less.

Please state, in the description and in the docs, what drivable guarantees, how it differs from disable, and make all drive paths honor that one guarantee.

Findings (evidence for the above, most severe first)

1. odrive_wheels.cpp (63) / roboclaw_wheels.cpp (80) — drivable=false coasts a power-driven robot instead of braking it. The speed path brakes via gate_or_brake() → do_wheel_speeds(0,0) = motor->speed(0) (velocity 0, actively holds). The power branch brakes via left_motor->power(0)/right_motor->power(0), which is torque 0 on ODrive (odrive_motor.cpp (150-155)) and duty 0 on RoboClaw (roboclaw_motor.cpp (81)) — coast, no braking force. The docs and wheels.h comment promise drivable=false makes "drive commands force a stop" / "brake". Failure: a robot streaming wheels.power(...) on a slope has drivable revoked → it free-rolls downhill instead of holding, whereas the same revocation under speed() holds. Same knob, two different stop semantics.

2. odrive_wheels.cpp (58-66) / roboclaw_wheels.cpp (75-83) — the power-command gate is special-cased in each subclass instead of routed through the shared gate. Both subclasses inline if (!enabled) return; if (!is_drivable()) { power(0,0); return; } — a hand-rolled copy of gate_or_brake() minus the brake — while wheels.h (34) documents gate_or_brake() as "Speed gate shared by all drive commands". The abstraction is leaky: power() never calls it. Cost: the gate lives in three places; the coast-vs-brake fix from finding 1 (and any future gate condition) must be applied in both files and is easy to miss in one. A base do_wheel_powers(l,r) hook plus a shared power path would unify both.

3. odrive_wheels.cpp (60) / roboclaw_wheels.cpp (77) — the enabled gate for ODrive/RoboClaw switched from the enabled property to the enabled member, which only syncs once per step(). Pre-PR these checked properties.at("enabled")->boolean_value (immediate); the PR now checks this->enabled, reconciled with the property only inside Wheels::step(). Failure: a same-cycle wheels.enabled = false; wheels.speed(1,0) (or .power(...)) is honored as if still enabled — the motors drive for ~one cycle until the next step() runs disable(). Under the old property check the command was blocked immediately. Narrow, one-cycle-transient behavior change, specific to ODrive and RoboClaw (Dunker already keyed off the member) — and precisely the kind of thing the entangled refactor hides.

4. wheels.cpp (74-82) — only drivable is forwarded to shadows; enabled is not, so the two "stop" knobs treat shadowed tracks differently. wheels.drivable = false brakes both master and shadow, but wheels.enabled = false (a property write) is not forwarded — the master disables in step() while the shadow keeps executing the mirrored speed commands from call_with_shadows and keeps driving. If deliberate, it needs a one-line comment on why enabled is excluded; otherwise a shadow that keeps moving after the master is disabled is a safety-relevant surprise.

5. wheels.cpp (76-79) — drivable forwarding re-enters write_property per shadow, unlike the one-level call_with_shadows. Method mirroring calls module->call (one level, module.cpp (99-104)), but this override calls the shadow's virtual write_property, which for a Wheels shadow re-enters this override and forwards again. Failure: a chain a.shadow(b); b.shadow(c) propagates drivable transitively to c even though speed() only reaches b (so c brakes but was never driven); a mutual a.shadow(b); b.shadow(a) (only self-shadowing is rejected in shadow()) recurses a→b→a→… unbounded → stack overflow. Unusual configs, but a one-level forward (or a cycle guard) removes the hazard.

Not flagged

Shadow modules are guaranteed to have drivable (the shadow method enforces identical typeid and all wheels get it from get_wheels_defaults()), so the forwarding can't throw on a missing key; the update_odometry() → enabled-sync → Module::step() ordering matches the pre-PR per-module step() exactly; and the RoboClaw m_per_tick split math is preserved correctly. The identical do_enable/do_disable bodies across the three subclasses can't be hoisted as-is (heterogeneous motor pointer types), so that duplication is expected.

Suggested path forward

Rather than iterating on this combined PR, I'd propose closing it in favor of two independent changes:

1. A standalone, behavior-preserving refactor — extract the shared scaffolding into Wheels (shared properties, the step() enabled-sync, the speed/enable/disable flow), with no drivable, no braking change and no shadow forwarding. Its review is also where we decide whether a shared base actually earns its place: if the behavior-preserving version reads cleanly, it lands; if it only works with per-subclass special-casing (see the caveat under blocker 1), we're better off leaving the three modules as they are.
2. The drivable feature — as its own change, with the contract spelled out (what it guarantees, how it differs from disable) and every drive path — speed and power, all three drivetrains, master and shadow — honoring that one guarantee consistently. This doesn't depend on (1): it can land on the shared base if that's accepted, or on the three existing modules if it isn't.

Either way, each piece becomes reviewable on its own and the behavior questions above get a clear home. Happy to help scope either one.

[JensOgorek]

Thanks for the thorough review, @falkoschindler — the structural points landed. We're following your proposal and splitting this into two focused, independently reviewable PRs:

• Extract a shared Wheels base class from the three wheels modules #225 — the pure, behavior-preserving refactor. Extracts the shared Wheels base, reviewable line-for-line against the old modules. One deliberate reconciliation (declared there): the shared speed gate reads the immediate enabled property, so Dunker now matches what ODrive/RoboClaw already did.
• Add a locked safety interlock to the Wheels base class #226 — the drivable feature, stacked on Extract a shared Wheels base class from the three wheels modules #225, with the contract spelled out: drivable is a handbrake (motors stay enabled, drive commands brake to a hold), distinct from enabled/disable which switch the motors off.

All five of your concrete findings are addressed in the split:

1. power now routes through the shared gate_or_brake(), so a power-driven robot brakes to a hold when drivable is revoked instead of coasting.
2. The gate is no longer hand-rolled per subclass — speed and power share one gate_or_brake().
3. The enabled gate reads the immediate enabled property again (no more one-cycle member lag).
4. write_property forwards both drivable and enabled to shadows, so a shadow no longer keeps driving after its master is gated or disabled.
5. Shadow forwarding is one level deep (via Module::write_property), matching call_with_shadows and removing the recursion/stack-overflow hazard on shadow chains or mutual shadows.

Both build clean for esp32 and were exercised on hardware.

This PR stays open for now — several robots currently depend on this branch. We'll close it in favor of #225/#226 once none of them depend on it anymore.