feat: Added RoboticArm #255
Conversation
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
| 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.
| 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
| 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? |
| for tl in timeline: | ||
| for i, s in enumerate(self.servos): | ||
| s.angle = tl[i] | ||
| time.sleep(time_step) |
There was a problem hiding this comment.
Looking at this again, I think it is still not quite right. As is, this will apply every timeline to every servo in sequence, instead of the intended one timeline per servo in parallel. It should be something like
for step in zip(*timeline):
for s, angle in zip(self.servos, step):
s.angle = angleExplanation of these steps (aka. crash course in Python iterators):
- asterisk (*) is the unpack operator. It unpacks the timeline list to its component lists.
ziprepacks the timeline lists into one combined parallel iterator, which yields n angles at a time, i.e. one timestep.- We again
zipthe n servos and the n angles into a parallel iterator, which yields one servo and one angle at a time. - We set each angle on its corresponding servo.
This should apply each step in timeline 1 to servo 1, each step in timeline 2 to servo 2, etc. in parallel.
There was a problem hiding this comment.
Thanks @bessman, for the detailed explanation!
I just wanted to clarify that in Mobile app, the timeline is structured per timestep, like this:
[
[10, 20, 30, 40], # timestep 1: servo1, servo2, servo3, servo4
[15, 25, 35, 45], # timestep 2
[20, 30, 40, 50], # timestep 3
]
So each inner list already represents one point in time, containing the angle values for all servos in order
There was a problem hiding this comment.
Thank you for the clarification! Then, the code is correct as-is.
Please document the timeline parameter in the run_schedule docstring, noting that it's one sublist per timestep.
| tl_len = len(timeline[0]) | ||
| if not all(len(tl) == tl_len for tl in timeline): | ||
| raise ValueError("All timeline entries must have the same length") |
There was a problem hiding this comment.
Also, check that the number of timelines (len(timeline)) is equal to the number of servos (len(self.servos)).
|
@bessman, I've added the changes |
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: