Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Python scripts for controlling a desk lifter via Raspberry Pi GPIO pins, including test scripts, calibration utilities, and height positioning functionality.
Key Changes:
- Added GPIO control scripts for desk up/down movement
- Implemented calibration-based height positioning logic
- Documented calibration measurements and movement rates
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| scripts/test_down.py | Test script for verifying DOWN button control via GPIO pin 27 |
| scripts/reset_to_lowest.py | Utility to reset desk to lowest position using 56-second calibrated duration |
| scripts/move_to_height.py | Main positioning script that resets to lowest then moves to target height based on calibration |
| scripts/lifter_calibration.txt | Documents measured calibration data (height range and movement rates) |
| Highest height: 54.5 inches | ||
|
|
||
| Down movement rate: 0.55 inches/second (measured from 54.5" to 49" in 10s) | ||
| Up movement rate: 0.55 inches/second (measured from 23.7" to 29.1" in 10s) |
There was a problem hiding this comment.
Inconsistent calibration data documentation: Line 7 states "Up movement rate: 0.55 inches/second" but move_to_height.py uses UP_RATE = 0.54. Additionally, line 6 states "Down movement rate: 0.55 inches/second" but this constant is not defined in move_to_height.py where it's used (line 34 hardcodes 56.0 seconds). Ensure calibration values are consistent across all files.
| Up movement rate: 0.55 inches/second (measured from 23.7" to 29.1" in 10s) | |
| Up movement rate: 0.54 inches/second (measured from 23.7" to 29.1" in 10s) |
scripts/move_to_height.py
Outdated
| if __name__ == "__main__": | ||
| try: | ||
| target = float(input(f"Enter target height in inches ({LOWEST_HEIGHT}-{HIGHEST_HEIGHT}): ")) | ||
| move_to_height(target) | ||
| finally: | ||
| release_up() | ||
| release_down() | ||
| GPIO.cleanup() |
There was a problem hiding this comment.
Script violates custom guideline against if __name__ == "__main__" patterns. According to the project's coding guidelines, this should be avoided unless explicitly requested. The code should be left as a top-level script instead.
scripts/move_to_height.py
Outdated
| # Calibration data | ||
| LOWEST_HEIGHT = 23.7 # inches | ||
| HIGHEST_HEIGHT = 54.5 # inches | ||
| UP_RATE = 0.54 # inches per second (from calibration) |
There was a problem hiding this comment.
Inconsistent calibration data: The calibration file shows Up movement rate: 0.55 inches/second but this script uses UP_RATE = 0.54. This mismatch will cause incorrect positioning calculations.
| UP_RATE = 0.54 # inches per second (from calibration) | |
| UP_RATE = 0.55 # inches per second (from calibration) |
scripts/move_to_height.py
Outdated
|
|
||
| def move_to_height(target_height): | ||
| if not (LOWEST_HEIGHT <= target_height <= HIGHEST_HEIGHT): | ||
| raise ValueError(f"Target height {target_height}'' is out of range.") |
There was a problem hiding this comment.
The error message incorrectly displays the target height using double-quote notation ({target_height}'') instead of inch notation ({target_height}"). The double single-quote is confusing and non-standard.
| raise ValueError(f"Target height {target_height}'' is out of range.") | |
| raise ValueError(f'Target height {target_height}" is out of range.') |
scripts/move_to_height.py
Outdated
| raise ValueError(f"Target height {target_height}'' is out of range.") | ||
| # Step 1: Reset to lowest | ||
| print(f"Resetting to lowest position ({LOWEST_HEIGHT})...") | ||
| release_down() | ||
| input("Press Enter to move all the way DOWN (about 56 seconds from max height)...") | ||
| press_down() | ||
| time.sleep(56.0) | ||
| release_down() | ||
| print("At lowest position.") | ||
| # Step 2: Move up for calculated time | ||
| delta = target_height - LOWEST_HEIGHT | ||
| up_time = delta / UP_RATE | ||
| print(f"Moving UP for {up_time:.1f} seconds to reach {target_height}''...") | ||
| release_up() | ||
| input("Press Enter to move UP...") | ||
| press_up() | ||
| time.sleep(up_time) | ||
| release_up() | ||
| print(f"Arrived at {target_height}'' (approximate).") | ||
|
|
||
| if __name__ == "__main__": | ||
| try: | ||
| target = float(input(f"Enter target height in inches ({LOWEST_HEIGHT}-{HIGHEST_HEIGHT}): ")) |
There was a problem hiding this comment.
Inconsistent inch notation throughout the function: Lines 28, 30, 40, 46, and 50 use different notations for inches (double single-quotes '', no notation, or inconsistent formatting). Standardize to use the proper inch symbol (") throughout for clarity and consistency.
scripts/move_to_height.py
Outdated
| # Step 1: Reset to lowest | ||
| print(f"Resetting to lowest position ({LOWEST_HEIGHT})...") | ||
| release_down() | ||
| input("Press Enter to move all the way DOWN (about 56 seconds from max height)...") |
There was a problem hiding this comment.
Misleading prompt message: The message says "about 56 seconds from max height" but this function is called during reset which should start from the current position (unknown), not necessarily from max height. This could confuse users if the desk isn't at max height.
| input("Press Enter to move all the way DOWN (about 56 seconds from max height)...") | |
| input("Press Enter to move all the way DOWN (this will take up to 56 seconds)...") |
scripts/move_to_height.py
Outdated
| release_down() | ||
| input("Press Enter to move all the way DOWN (about 56 seconds from max height)...") | ||
| press_down() | ||
| time.sleep(56.0) |
There was a problem hiding this comment.
Hardcoded magic number: The 56.0 second duration is duplicated from reset_to_lowest.py without explanation. Consider extracting this as a named constant (e.g., MAX_TO_MIN_TIME = 56.0) with a comment explaining its derivation, or better yet, calculate it from (HIGHEST_HEIGHT - LOWEST_HEIGHT) / DOWN_RATE where DOWN_RATE = 0.55.
…working OK. Still assumes developer permissions.
…for desk lifter control
…ctor GPIO pin management
|
See requirements below:
Related to last point, including a snippet from context7 chat below: |
- Updated duty_cycle.py to enhance movement validation against duty cycle limits. - Introduced generate_movements.py for generating movement configurations based on current duty cycle status. - Modified lifter_state.json to reflect updated usage periods and total up time. - Enhanced main.py to support new commands for deploying immediate and custom movements. - Created movement_configs.json to store generated movement configurations. - Updated prefect_flows.py to simplify flow definitions and integrate custom movements execution. - Added duty cycle monitoring flow for scheduled checks and recommendations based on capacity.
No description provided.