Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds two new files to support motor configuration management and firmware file serving:
- A comprehensive motor configuration module (
motor_config.py) with dataclasses for managing motor settings - A simple file server (
fwServer.py) for serving firmware files over HTTP
Changes:
- Introduced structured motor configuration using dataclasses with support for TMC driver settings, homing, motion parameters, and limits
- Added conversion methods to interface with existing ImSwitch configuration format
- Created a FastAPI-based file server for browsing and downloading files
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| uc2rest/motor_config.py | Comprehensive dataclass-based motor configuration system with serialization/deserialization support and ImSwitch compatibility |
| binaries/latest/fwServer.py | Simple FastAPI file server with directory browsing and file download capabilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs_path = safe_path(req_path) | ||
|
|
||
| if not fs_path.exists(): | ||
| raise HTTPException(status_code=404) |
There was a problem hiding this comment.
The error message for HTTP 404 (Not Found) is empty. Provide a descriptive error message to help users understand what resource was not found, such as "Resource not found".
| raise HTTPException(status_code=404) | |
| raise HTTPException(status_code=404, detail=f"Resource not found: {req_path}") |
| html = "<ul>" + "".join( | ||
| f"<li><a href='{e['name']}'>{e['name']}</a></li>" | ||
| for e in entries | ||
| ) + "</ul>" |
There was a problem hiding this comment.
The HTML generation for directory listing does not escape the entry names, which could lead to Cross-Site Scripting (XSS) vulnerabilities if filenames contain HTML or JavaScript. Use proper HTML escaping for the name values in both the href and display text.
| # index.html suppresses listing | ||
| index = fs_path / "index.html" | ||
| if index.exists(): | ||
| return HTMLResponse(index.read_text()) |
There was a problem hiding this comment.
Reading the entire index.html file into memory using read_text() without error handling could cause issues with very large files or encoding errors. Consider adding error handling or using FileResponse instead for consistency with how other files are served.
| return HTMLResponse(index.read_text()) | |
| return FileResponse(index, media_type="text/html") |
| def from_dict(cls, data: dict) -> 'MotionSettings': | ||
| if data is None: | ||
| return cls() | ||
| # Handle None values from JSON | ||
| if data.get('min_pos') is None: | ||
| data['min_pos'] = float('-inf') | ||
| if data.get('max_pos') is None: | ||
| data['max_pos'] = float('inf') | ||
| return cls(**{k: v for k, v in data.items() if k in cls.__dataclass_fields__}) |
There was a problem hiding this comment.
The from_dict method mutates the input data dictionary when handling None values for min_pos and max_pos. This can lead to unexpected side effects if the caller reuses the dictionary. Consider creating a copy of the data dictionary before modifying it, or use a different approach that doesn't mutate the input.
| return cls.from_dict(json.loads(json_str)) | ||
|
|
||
| @classmethod | ||
| def from_imswitch_config(cls, manager_properties: dict, stage_offsets: dict = None) -> 'MotorSystemConfig': |
There was a problem hiding this comment.
The stage_offsets parameter in the method signature is documented but never used in the implementation. Either remove this unused parameter or implement its intended functionality.
| @@ -0,0 +1,79 @@ | |||
| # dummy_caddy_like_server.py | |||
There was a problem hiding this comment.
The comment refers to this file as "dummy_caddy_like_server.py" but the actual filename is "fwServer.py". This inconsistency can cause confusion. Update the comment to match the actual filename.
| # dummy_caddy_like_server.py | |
| # fwServer.py |
| def safe_path(req_path: str) -> Path: | ||
| p = (ROOT / req_path).resolve() | ||
| if not p.is_relative_to(ROOT): | ||
| raise HTTPException(status_code=403) |
There was a problem hiding this comment.
The error message for HTTP 403 (Forbidden) in the safe_path function is empty. Provide a descriptive error message to help users understand why their request was denied, such as "Access denied: path is outside allowed directory".
| raise HTTPException(status_code=403) | |
| raise HTTPException( | |
| status_code=403, | |
| detail="Access denied: path is outside allowed directory", | |
| ) |
1 participant
No description provided.