Add the ChangeBaudrate method + correct some errors in the README's and update them.#13
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new ChangeBaudrate method to enable servo baudrate reconfiguration, includes two test files to demonstrate the functionality, updates documentation to reflect these changes, and corrects several docstring descriptions in the README. The version in setup.py is updated from "IN_PROGRESS" to "0.0.0".
- Implements
ChangeBaudratemethod for servo baudrate configuration - Adds test files demonstrating baudrate changing and position reading
- Updates README and test documentation with corrected descriptions and new method documentation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| st3215/st3215.py | Adds new ChangeBaudrate method following the pattern of ChangeId |
| test/test_11_change_baudrate.py | Test script demonstrating baudrate change functionality |
| test/test_12_read_position.py | Test script for reading servo position with custom baudrate |
| test/README.md | Documents the two new test cases |
| README.md | Adds installation section, corrects method descriptions, and documents ChangeBaudrate |
| setup.py | Updates version from "IN_PROGRESS" to "0.0.0" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **Purpose**: Test StartServo, SetAcceleration, SetSpeed, rotation mode, position mode, and StopServo | ||
| - **⚠️ Important**: Ensure servo has enough physical clearance for movement | ||
|
|
||
| ### Test 11: Change baudrate |
There was a problem hiding this comment.
The test title "Change baudrate" uses lowercase 'b' in baudrate, which is inconsistent with the capitalization pattern used in "Complete Motion Control" (Test 10) and other multi-word titles in the test suite. Consider changing to "Change Baudrate" for consistency.
| ### Test 11: Change baudrate | |
| ### Test 11: Change Baudrate |
| :param sts_id: Actual ID for the servo | ||
| :param new_baudrate: New baudrate for the servo (in bps) | ||
|
|
||
| :return: None when sucedeed otherwise the error message |
There was a problem hiding this comment.
Typo in the return description: "sucedeed" should be spelled "succeeded".
| Change Baudrate of a Servo. | ||
|
|
||
| :param sts_id: Actual ID for the servo | ||
| :param new_baudrate: New baudrate for the servo (in bps) |
There was a problem hiding this comment.
The docstring states the parameter is "New baudrate for the servo (in bps)" but the implementation actually expects an index (0-7) rather than a baudrate value in bps. The docstring should be updated to clarify that new_baudrate is an index value (0-7) corresponding to predefined baudrate settings, not the actual baudrate in bps.
| :param new_baudrate: New baudrate for the servo (in bps) | |
| :param new_baudrate: Index (0-7) selecting a predefined baudrate setting for the servo, not the baudrate value in bps |
| self.LockEprom(sts_id) | ||
| return None | ||
| else: | ||
| return "baudrate is not valid" |
There was a problem hiding this comment.
The error message "baudrate is not valid" is too vague. It should specify the valid range (0-7) to help users understand what values are acceptable, similar to the ChangeId method which returns "new_id is not between 0 and 253".
| return "baudrate is not valid" | |
| return "new_baudrate is not between 0 and 7" |
| servo.portHandler.openPort() | ||
| print("PingServo:", servo.PingServo(1)) | ||
| status = servo.ChangeBaudrate(1, baudrate_nb) | ||
| if status != None: |
There was a problem hiding this comment.
Comparison with None should use 'is not None' instead of '!= None' according to PEP 8 Python style guidelines.
| if status != None: | |
| if status is not None: |
|
|
||
| --- | ||
| ### `ChangeBaudrate(sts_id, new_baudrate)` | ||
| Change the servo baudrate. Return `None` when sucedeed otherwise the error message |
There was a problem hiding this comment.
Typo in the return description: "sucedeed" should be spelled "succeeded".
| from st3215 import ST3215 | ||
|
|
||
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | ||
| baudrate_nb = 0 | ||
| prev_baud_nb = 0 | ||
|
|
||
| servo = ST3215('COM7') | ||
| print("Default baudrate:", servo.portHandler.baudrate) | ||
| print("Close port...") | ||
| servo.portHandler.closePort() | ||
| print("Reopen port with the right baudrate:", baudrate[baudrate_nb]) | ||
| servo.portHandler.baudrate = baudrate[baudrate_nb] | ||
| servo.portHandler.openPort() | ||
| print("Position:", servo.ReadPosition(1)) No newline at end of file |
There was a problem hiding this comment.
This test file does not follow the same structure and patterns as other test files in the test directory. Other test files (e.g., test_01_ping_servo.py) use a main() function, proper error handling with try-except blocks, environment variables for device configuration, and structured output with status indicators. This test should be refactored to match the existing test patterns for consistency.
| from st3215 import ST3215 | |
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | |
| baudrate_nb = 0 | |
| prev_baud_nb = 0 | |
| servo = ST3215('COM7') | |
| print("Default baudrate:", servo.portHandler.baudrate) | |
| print("Close port...") | |
| servo.portHandler.closePort() | |
| print("Reopen port with the right baudrate:", baudrate[baudrate_nb]) | |
| servo.portHandler.baudrate = baudrate[baudrate_nb] | |
| servo.portHandler.openPort() | |
| print("Position:", servo.ReadPosition(1)) | |
| import os | |
| import sys | |
| from st3215 import ST3215 | |
| def main() -> int: | |
| """ | |
| Read and print the position of servo ID 1 using an ST3215 device. | |
| Configuration: | |
| - SERVO_PORT: serial port name (default: "COM7") | |
| - SERVO_BAUD_INDEX: index into the baudrate table (default: "0") | |
| """ | |
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | |
| port = os.getenv("SERVO_PORT", "COM7") | |
| baudrate_nb_str = os.getenv("SERVO_BAUD_INDEX", "0") | |
| try: | |
| try: | |
| baudrate_nb = int(baudrate_nb_str) | |
| except ValueError: | |
| print(f"[WARN] Invalid SERVO_BAUD_INDEX='{baudrate_nb_str}', defaulting to 0") | |
| baudrate_nb = 0 | |
| if baudrate_nb < 0 or baudrate_nb >= len(baudrate): | |
| print(f"[WARN] SERVO_BAUD_INDEX out of range ({baudrate_nb}), defaulting to 0") | |
| baudrate_nb = 0 | |
| print(f"[INFO] Initializing ST3215 on port {port!r}") | |
| servo = ST3215(port) | |
| print(f"[INFO] Default baudrate: {servo.portHandler.baudrate}") | |
| print("[INFO] Closing port...") | |
| servo.portHandler.closePort() | |
| print(f"[INFO] Reopening port with baudrate: {baudrate[baudrate_nb]}") | |
| servo.portHandler.baudrate = baudrate[baudrate_nb] | |
| servo.portHandler.openPort() | |
| position = servo.ReadPosition(1) | |
| print(f"[OK] Position for servo ID 1: {position}") | |
| return 0 | |
| except Exception as exc: | |
| print(f"[ERROR] Failed to read position: {exc}") | |
| return 1 | |
| finally: | |
| # Ensure the port is closed if it was opened successfully. | |
| if "servo" in locals(): | |
| try: | |
| servo.portHandler.closePort() | |
| print("[INFO] Port closed.") | |
| except Exception: | |
| # Best-effort cleanup; ignore errors on close. | |
| pass | |
| if __name__ == "__main__": | |
| sys.exit(main()) |
| - `new_baudrate` (int) | ||
| - **Returns**: `None` or `str` | ||
| - **Example**: | ||
| ```python | ||
| servo.ChangeBaudrate(1, 4) # 115200 |
There was a problem hiding this comment.
The documentation states that new_baudrate is an "int" parameter but doesn't specify that it should be an index value (0-7) rather than an actual baudrate value in bps. This is inconsistent with the method's docstring which states "(in bps)". The documentation should clarify that this is an index value and potentially provide a reference to the baudrate mapping (e.g., 0=1000000, 1=500000, etc.).
| - `new_baudrate` (int) | |
| - **Returns**: `None` or `str` | |
| - **Example**: | |
| ```python | |
| servo.ChangeBaudrate(1, 4) # 115200 | |
| - `new_baudrate` (int): baudrate index in the range 0–7, not the baudrate in bps. | |
| - Index mapping (index → baudrate in bps), for example: | |
| - `0` → `1000000` | |
| - `1` → `500000` | |
| - `2` → `250000` | |
| - `3` → `128000` | |
| - `4` → `115200` | |
| - `5` → `76800` | |
| - `6` → `57600` | |
| - `7` → `38400` | |
| - **Returns**: `None` or `str` | |
| - **Example**: | |
| ```python | |
| servo.ChangeBaudrate(1, 4) # index 4 → 115200 bps |
| from st3215 import ST3215 | ||
|
|
||
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | ||
| baudrate_nb = 2 | ||
| prev_baud_nb = 0 | ||
|
|
||
| servo = ST3215('COM7') | ||
| print("Default baudrate:", servo.portHandler.baudrate) | ||
| print("Close port...") | ||
| servo.portHandler.closePort() | ||
| print("Reopen port with previous baudrate:", baudrate[prev_baud_nb]) | ||
| servo.portHandler.baudrate = baudrate[prev_baud_nb] | ||
| servo.portHandler.openPort() | ||
| print("PingServo:", servo.PingServo(1)) | ||
| status = servo.ChangeBaudrate(1, baudrate_nb) | ||
| if status != None: | ||
| print("Baudrate change failed with status:", status) | ||
| else: | ||
| print("Baudrate change successful") | ||
| print("Close port...") | ||
| servo.portHandler.closePort() | ||
| print("Reopen port with new baudrate:", baudrate[baudrate_nb]) | ||
| servo.portHandler.baudrate = baudrate[baudrate_nb] | ||
| print("Current baudrate:", servo.portHandler.baudrate) | ||
| servo.portHandler.openPort() | ||
| print("PingServo:", servo.PingServo(1)) No newline at end of file |
There was a problem hiding this comment.
This test file does not follow the same structure and patterns as other test files in the test directory. Other test files (e.g., test_01_ping_servo.py) use a main() function, proper error handling with try-except blocks, environment variables for device configuration, and structured output with status indicators. This test should be refactored to match the existing test patterns for consistency.
| from st3215 import ST3215 | |
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | |
| baudrate_nb = 2 | |
| prev_baud_nb = 0 | |
| servo = ST3215('COM7') | |
| print("Default baudrate:", servo.portHandler.baudrate) | |
| print("Close port...") | |
| servo.portHandler.closePort() | |
| print("Reopen port with previous baudrate:", baudrate[prev_baud_nb]) | |
| servo.portHandler.baudrate = baudrate[prev_baud_nb] | |
| servo.portHandler.openPort() | |
| print("PingServo:", servo.PingServo(1)) | |
| status = servo.ChangeBaudrate(1, baudrate_nb) | |
| if status != None: | |
| print("Baudrate change failed with status:", status) | |
| else: | |
| print("Baudrate change successful") | |
| print("Close port...") | |
| servo.portHandler.closePort() | |
| print("Reopen port with new baudrate:", baudrate[baudrate_nb]) | |
| servo.portHandler.baudrate = baudrate[baudrate_nb] | |
| print("Current baudrate:", servo.portHandler.baudrate) | |
| servo.portHandler.openPort() | |
| print("PingServo:", servo.PingServo(1)) | |
| import os | |
| import sys | |
| from st3215 import ST3215 | |
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | |
| def main() -> int: | |
| """ | |
| Test 11: Change servo baudrate and verify communication. | |
| Configuration (via environment variables): | |
| - SERVO_PORT: Serial port to use (default: 'COM7'). | |
| - SERVO_ID: Servo ID to target (default: 1). | |
| - SERVO_PREV_BAUD_INDEX: Index into baudrate list for previous baudrate (default: 0). | |
| - SERVO_NEW_BAUD_INDEX: Index into baudrate list for new baudrate (default: 2). | |
| """ | |
| port = os.getenv("SERVO_PORT", "COM7") | |
| try: | |
| servo_id = int(os.getenv("SERVO_ID", "1")) | |
| except ValueError: | |
| servo_id = 1 | |
| try: | |
| prev_baud_nb = int(os.getenv("SERVO_PREV_BAUD_INDEX", "0")) | |
| except ValueError: | |
| prev_baud_nb = 0 | |
| try: | |
| baudrate_nb = int(os.getenv("SERVO_NEW_BAUD_INDEX", "2")) | |
| except ValueError: | |
| baudrate_nb = 2 | |
| # Clamp indices to valid range | |
| prev_baud_nb = max(0, min(prev_baud_nb, len(baudrate) - 1)) | |
| baudrate_nb = max(0, min(baudrate_nb, len(baudrate) - 1)) | |
| print("[INFO] Starting test_11_change_baudrate") | |
| print(f"[INFO] Using port: {port}, servo ID: {servo_id}") | |
| print(f"[INFO] Previous baud index: {prev_baud_nb}, new baud index: {baudrate_nb}") | |
| servo = None | |
| try: | |
| servo = ST3215(port) | |
| print(f"[INFO] Default baudrate: {servo.portHandler.baudrate}") | |
| print("[INFO] Closing port...") | |
| servo.portHandler.closePort() | |
| print(f"[INFO] Reopening port with previous baudrate: {baudrate[prev_baud_nb]}") | |
| servo.portHandler.baudrate = baudrate[prev_baud_nb] | |
| servo.portHandler.openPort() | |
| ping_status = servo.PingServo(servo_id) | |
| print(f"[INFO] PingServo (previous baud): {ping_status}") | |
| status = servo.ChangeBaudrate(servo_id, baudrate_nb) | |
| if status is not None: | |
| print(f"[FAIL] Baudrate change failed with status: {status}") | |
| return 1 | |
| else: | |
| print("[OK] Baudrate change successful") | |
| print("[INFO] Closing port after baudrate change...") | |
| servo.portHandler.closePort() | |
| print(f"[INFO] Reopening port with new baudrate: {baudrate[baudrate_nb]}") | |
| servo.portHandler.baudrate = baudrate[baudrate_nb] | |
| print(f"[INFO] Current baudrate: {servo.portHandler.baudrate}") | |
| servo.portHandler.openPort() | |
| ping_status_new = servo.PingServo(servo_id) | |
| print(f"[INFO] PingServo (new baud): {ping_status_new}") | |
| print("[OK] test_11_change_baudrate completed successfully") | |
| return 0 | |
| except Exception as exc: | |
| print(f"[ERROR] Exception during test_11_change_baudrate: {exc}") | |
| return 1 | |
| finally: | |
| if servo is not None: | |
| try: | |
| print("[INFO] Ensuring port is closed...") | |
| servo.portHandler.closePort() | |
| except Exception: | |
| # Suppress errors during cleanup | |
| pass | |
| if __name__ == "__main__": | |
| sys.exit(main()) |
|
|
||
| baudrate = [1000000, 500000, 250000, 128000, 115200, 76800, 57600, 38400] | ||
| baudrate_nb = 0 | ||
| prev_baud_nb = 0 |
There was a problem hiding this comment.
The variable 'prev_baud_nb' is defined but never used. It should either be removed or utilized in the test logic if it was intended to track the previous baudrate index.
| prev_baud_nb = 0 |
No description provided.