-
Notifications
You must be signed in to change notification settings - Fork 114
Description
A recent PR introduced several problematic changes to the Dynamixel driver.
Issues
1. Security and System Stability Concerns
The driver now aggressively attempts to gain port access by:
- Killing other processes using
fuser -kwithout user consent - Modifying system permissions using
sudo chmod 666 - Running privileged commands automatically
Why this is problematic:
- Security risk: Automatically running
sudocommands - System stability: Killing processes that might be legitimately using the port
- User consent: No warning or confirmation before making system changes
- Privilege escalation: Requires sudo access for basic driver functionality
2. Poor Architecture and Code Duplication
The retry/fallback mechanism is poorly implemented:
- Wrong abstraction level: Retry logic belongs in application code, not the driver.
- Code duplication: Instead of using the existing
FakeDynamixelDriver, the code adds_is_fakeflags and duplicate fake behavior inDynamixelDriver - Hard to extend: The mixed real/fake logic makes the driver harder to maintain and extend
Suggested Solution
-
Regarding Issue 1: This functionality should be removed entirely. Instead, we should catch the
SerialExceptions that originate from the DynamixelSDK and provide meaningful error messages that help users understand why initialization failed (e.g., "Port not found", "Permission denied", "Port already in use"). -
Regarding Issue 2: Move the retry/fallback mechanism to
launch_utils.py, where the driver is instantiated. The_is_fakeflags and duplicate fake logic inDynamixelDrivershould be removed. Instead,launch_utils.pycan implement a clean factory pattern that falls back to the existingFakeDynamixelDriverwhen hardware initialization fails.
I will create a PR with the suggested changes.