Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds hardware testing functionality for controlling an LG-07 lift via a Raspberry Pi using GPIO pins connected to relay modules that simulate button presses on the FLTCON remote control.
Key changes:
- Low-level GPIO driver for relay-based lift control
- Interactive hardware test script with automated and manual test modes
- Command-line application for basic lift operations
- Setup scripts and documentation for Raspberry Pi deployment
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/progressive_automations_python/pi/lg07_lift/lift_driver.py |
Core GPIO driver implementing relay control for UP/DOWN movements with thread locking and emergency stop functionality |
src/progressive_automations_python/pi/lg07_lift/test_hardware.py |
Interactive hardware test script providing both automated test sequences and manual command mode |
src/progressive_automations_python/pi/lg07_lift/app.py |
Command-line interface for lift control with signal handling for graceful shutdown |
src/progressive_automations_python/pi/lg07_lift/config.yaml |
Configuration file defining GPIO pins, timing, and safety settings (currently not used by code) |
src/progressive_automations_python/pi/lg07_lift/README.md |
Comprehensive documentation covering hardware setup, software installation, testing procedures, and troubleshooting |
setup-pi.sh |
Bash setup script automating Raspberry Pi environment configuration and package installation |
requirements-pi.txt |
Raspberry Pi-specific Python dependencies (RPi.GPIO) |
find_pi.ps1 |
PowerShell utility script for discovering Raspberry Pi devices on local network |
CHANGELOG.md |
Updated with new features and changes in this PR |
| from progressive_automations_python.pi.lg07_lift.lift_driver import ( | ||
| press_up, press_down, nudge, emergency_stop, cleanup | ||
| ) |
There was a problem hiding this comment.
Missing __init__.py file for the lg07_lift package. Python needs this file to recognize progressive_automations_python.pi.lg07_lift as a package, which is required for the imports to work correctly.
Create an empty __init__.py file at src/progressive_automations_python/pi/lg07_lift/__init__.py.
| if _initialized: | ||
| return | ||
|
|
||
| GPIO.setmode(GPIO.BCM) # use BCM numbering (GPIO17 = pin 11) | ||
| GPIO.setup(PIN_UP, GPIO.OUT, initial=GPIO.HIGH) | ||
| GPIO.setup(PIN_DOWN, GPIO.OUT, initial=GPIO.HIGH) | ||
|
|
||
| _initialized = True |
There was a problem hiding this comment.
The _init_gpio() function is not thread-safe, which could lead to race conditions if multiple threads call press_up() or press_down() simultaneously. The check if _initialized: and the subsequent GPIO initialization could execute concurrently in different threads before _initialized is set to True.
Wrap the entire function body in a lock:
def _init_gpio():
"""Initialize GPIO pins once."""
global _initialized
with _lock:
if _initialized:
return
GPIO.setmode(GPIO.BCM)
GPIO.setup(PIN_UP, GPIO.OUT, initial=GPIO.HIGH)
GPIO.setup(PIN_DOWN, GPIO.OUT, initial=GPIO.HIGH)
_initialized = True| if _initialized: | |
| return | |
| GPIO.setmode(GPIO.BCM) # use BCM numbering (GPIO17 = pin 11) | |
| GPIO.setup(PIN_UP, GPIO.OUT, initial=GPIO.HIGH) | |
| GPIO.setup(PIN_DOWN, GPIO.OUT, initial=GPIO.HIGH) | |
| _initialized = True | |
| with _lock: | |
| if _initialized: | |
| return | |
| GPIO.setmode(GPIO.BCM) # use BCM numbering (GPIO17 = pin 11) | |
| GPIO.setup(PIN_UP, GPIO.OUT, initial=GPIO.HIGH) | |
| GPIO.setup(PIN_DOWN, GPIO.OUT, initial=GPIO.HIGH) | |
| _initialized = True |
| # Add the package to path | ||
| sys.path.insert(0, os.path.dirname(__file__)) |
There was a problem hiding this comment.
This sys.path manipulation is problematic because os.path.dirname(__file__) points to the lg07_lift directory, not a package root. This won't help with imports and violates the custom coding guideline to avoid defensive path manipulation.
Remove this sys.path modification and rely on proper package installation instead.
| if args.test: | ||
| # Run the hardware test | ||
| print("🧪 Running hardware test...") | ||
| from test_hardware import test_sequence |
There was a problem hiding this comment.
This relative import will fail because test_hardware is not in the Python path when app.py is run as a module. The import should use an absolute import path:
from progressive_automations_python.pi.lg07_lift.test_hardware import test_sequence| from test_hardware import test_sequence | |
| from progressive_automations_python.pi.lg07_lift.test_hardware import test_sequence |
| from progressive_automations_python.pi.lg07_lift.lift_driver import ( | ||
| press_up, press_down, nudge, emergency_stop, cleanup | ||
| ) |
There was a problem hiding this comment.
Missing __init__.py file for the pi package. Python needs this file to recognize progressive_automations_python.pi as a package, which is required for the imports in app.py and test_hardware.py to work correctly.
Create an empty __init__.py file at src/progressive_automations_python/pi/__init__.py.
|
Daniel said OK to close in favor of #3 |
No description provided.