feat: Added RoboticArm #255
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds a new RoboticArm class in pslab.external.motor to coordinate up to four existing Servo instances, including input validation and a run_schedule method for time-based servo angle sequences. Class diagram for the new RoboticArm class and its relationship to ServoclassDiagram
class Servo {
angle
_get_duty_cycle(angle)
}
class RoboticArm {
MAX_SERVOS
servos
__init__(servos)
run_schedule(timeline)
}
RoboticArm --> "*" Servo : uses up to 4
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rahul31124 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pslab/external/motor.py:77` </location>
<code_context>
return angle / (self._frequency**-1 * MICROSECONDS)
+
+
+class RoboticArm:
+ MAX_SERVOS = 4
+
</code_context>
<issue_to_address>
Consider validating timeline dimensions in run_schedule.
Currently, run_schedule may raise an IndexError or ignore extra values if a sublist's length doesn't match self.servos. Adding a check for sublist length would improve robustness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
class RoboticArm:
MAX_SERVOS = 4
=======
class RoboticArm:
MAX_SERVOS = 4
def run_schedule(self, timeline: List[List[Union[int, float]]]):
"""
Run a schedule of servo positions.
Args:
timeline: A list of lists, where each sublist contains positions for each servo at a time step.
Raises:
ValueError: If any sublist does not match the number of servos.
"""
for idx, positions in enumerate(timeline):
if len(positions) != len(self.servos):
raise ValueError(
f"Timeline entry at index {idx} has {len(positions)} positions, "
f"but {len(self.servos)} servos are configured."
)
# Existing logic for running the schedule goes here
# for positions in timeline:
# self.set_positions(positions)
# time.sleep(self.step_delay)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `pslab/external/motor.py:92` </location>
<code_context>
+ for j in range(len(self.servos)):
+ angle = timeline[i][j]
+ self.servos[j].angle = angle
+ time.sleep(1)
</code_context>
<issue_to_address>
Consider making the sleep duration configurable.
Allow users to set the sleep interval via a parameter or class attribute for greater flexibility.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pslab/external/motor.py
Outdated
Comment on lines
77
to
79
| class RoboticArm: | ||
| MAX_SERVOS = 4 | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Consider validating timeline dimensions in run_schedule.
Currently, run_schedule may raise an IndexError or ignore extra values if a sublist's length doesn't match self.servos. Adding a check for sublist length would improve robustness.
Suggested change
| class RoboticArm: | |
| MAX_SERVOS = 4 | |
| class RoboticArm: | |
| MAX_SERVOS = 4 | |
| def run_schedule(self, timeline: List[List[Union[int, float]]]): | |
| """ | |
| Run a schedule of servo positions. | |
| Args: | |
| timeline: A list of lists, where each sublist contains positions for each servo at a time step. | |
| Raises: | |
| ValueError: If any sublist does not match the number of servos. | |
| """ | |
| for idx, positions in enumerate(timeline): | |
| if len(positions) != len(self.servos): | |
| raise ValueError( | |
| f"Timeline entry at index {idx} has {len(positions)} positions, " | |
| f"but {len(self.servos)} servos are configured." | |
| ) | |
| # Existing logic for running the schedule goes here | |
| # for positions in timeline: | |
| # self.set_positions(positions) | |
| # time.sleep(self.step_delay) |
pslab/external/motor.py
Outdated
| for j in range(len(self.servos)): | ||
| angle = timeline[i][j] | ||
| self.servos[j].angle = angle | ||
| time.sleep(1) |
There was a problem hiding this comment.
suggestion: Consider making the sleep duration configurable.
Allow users to set the sleep interval via a parameter or class attribute for greater flexibility.
pslab/external/motor.py
Outdated
Comment on lines
88
to
92
| for i in range(len(timeline)): | ||
| for j in range(len(self.servos)): | ||
| angle = timeline[i][j] | ||
| self.servos[j].angle = angle | ||
| time.sleep(1) |
There was a problem hiding this comment.
issue (code-quality): Replace index in for loop with direct reference (for-index-replacement)
|
Hello @bessman, could you please review this PR and suggest any changes that might be needed? |
bessman
requested changes
Jul 11, 2025
pslab/external/motor.py
Outdated
Comment on lines
91
to
94
| for i in range(len(timeline)): | ||
| for j in range(len(self.servos)): | ||
| angle = timeline[i][j] | ||
| self.servos[j].angle = angle |
There was a problem hiding this comment.
Use for-each style loops instead of range(len(..)):
Suggested change
| for i in range(len(timeline)): | |
| for j in range(len(self.servos)): | |
| angle = timeline[i][j] | |
| self.servos[j].angle = angle | |
| for tl in timeline: | |
| for i, s in enumerate(self.servos): | |
| angle = tl[i] | |
| s.angle = angle |
Also add a check that all timelines are the same length:
tl_len = len(timeline[0])
if not all(len(tl) == tl_len for tl in timeline):
raise ValueError
pslab/external/motor.py
Outdated
| for j in range(len(self.servos)): | ||
| angle = timeline[i][j] | ||
| self.servos[j].angle = angle | ||
| time.sleep(1) |
There was a problem hiding this comment.
Is one second fine-grained enough? Seems like this should be smaller or configurable.
There was a problem hiding this comment.
Currently, on the mobile app, is set to 1 second. However, I agree that making it configurable would offer more flexibility. I will update the method to include a parameter, defaulting to 1 second
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #252 #253
Changes
Added
RoboticArmclass topslab.external.motorThis class enables controlling RoboticArm
Supports control of up to 4 servos via
ServoclassInternally uses existing
Servoobjects, validating servo count and managing individual angle settingsImplemented
run_schedule()to move RoboticArm according to a timelineEach servo position is updated per second based on a time-dependent list of angle
Summary by Sourcery
Add RoboticArm class to motor module enabling multi-servo control and timed movement scheduling
New Features: