Conversation
Updated defaults.py, encoded_motor.py, and resetbot.py to only initialize and use the fourth motor if Pin.board.MOTOR_4_IN_1 is available. This improves compatibility with hardware configurations that do not include a fourth motor.
syncing with main
Import sys.implementation and make defaults platform-aware: instantiate motor_four conditionally, remove duplicate instantiation, and choose a NanoXRP-specific drivetrain with explicit wheel dimensions. Replace sys.implementation usage in EncodedMotor with a direct import. Extend Encoder to accept a flip_dir flag, normalize pin ordering at init, and invert returned counts when flip_dir is true to allow reversing encoder direction.
Adjust defaults and encoder behavior for NanoXRP hardware. In defaults.py the drivetrain is constructed directly for NanoXRP with left_motor, right_motor, imu and corrected wheel_diam (3.46) and wheel_track (7.8) instead of the invalid annotated call. In encoder.py the module now imports sys.implementation and selects a NanoXRP-specific gear_ratio (68) while keeping counts_per_motor_shaft_revolution at 12; other platforms retain the original gear ratio. These changes adapt drivetrain and encoder resolution to the NanoXRP platform.
bradamiller
left a comment
bradamiller
left a comment
There was a problem hiding this comment.
PR Review: NanoXRP
I've reviewed this PR for compatibility issues with the existing XRP library. There are several concerns that should be addressed before merging.
Critical Issues
1. Reflectance __init__ will crash on existing XRP boards
In reflectance.py, the new middlePin parameter defaults to "LINE_M". Since get_default_reflectance() calls cls() with no arguments, ADC(Pin("LINE_M")) will raise an exception on any board that doesn't define that pin. This will break from XRPLib.defaults import * on every existing XRP board.
This needs a hasattr(Pin.board, "LINE_M") guard, similar to what was done for motor 4.
2. Encoder pin comparison using str() is fragile and likely incorrect
In encoder.py, the original code used min(encAPin, encBPin) / max(...) to ensure pin ordering. The PR replaces this with str(basePin) > str(nextPin), comparing the string representation of machine.Pin objects. The format of str(Pin(...)) is implementation-defined (e.g., Pin(GPIO16, mode=IN)), and lexicographic comparison doesn't reliably determine which GPIO number is higher — for example "Pin(GPIO9...)" > "Pin(GPIO16...)" because '9' > '1'. Since the PIO state machine requires in_base to be the lower-numbered pin, this could silently break encoders.
3. Motor type selection default is inverted
In encoded_motor.py, the current logic defaults to SinglePWMMotor and opts in to DualPWMMotor for RP2350. The PR flips this so DualPWMMotor is now the default for everything except "Beta" boards. This changes behavior for all board variants and means any future boards will silently get DualPWMMotor. If NanoXRP needs SinglePWMMotor, it won't get it either since its _machine string won't contain "Beta".
Minor Issues
4. get_middle() docstring says "left" — copy-paste error in the docstring.
5. No fallback for motor_four in defaults.py — on NanoXRP, motor_four is never defined, so from XRPLib.defaults import motor_four will raise NameError. Should have an else: motor_four = None branch.
6. flip_dir on Encoder duplicates existing mechanism — EncodedMotor.get_position() already handles direction flipping via self._motor.flip_dir. A second flip_dir on the Encoder creates two independent flip mechanisms that could conflict.
7. Reliance on implementation._machine string — The underscore-prefixed _machine attribute is used in multiple files for platform detection. If the firmware string changes slightly (e.g., "Nano XRP" or "nanoXRP"), the checks silently fall through to wrong defaults with no error.
2 participants
No description provided.