add new Rest api wrapper - #13965
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new REST API wrapper for interacting with RealSense devices and WebRTC streaming by introducing various endpoints and supporting code.
- Adds core security functions and placeholder implementations for token creation and password hashing.
- Introduces several API endpoints for devices, sensors, streams, point cloud, options, and WebRTC communications.
- Provides dependency injection for service managers and updates documentation to include the new REST API wrapper.
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| app/core/security.py | Introduces token creation and password verification placeholders |
| app/core/errors.py | Adds custom error handlers for various FastAPI exceptions |
| app/core/config.py | Provides settings retrieval with an lru_cache decorator |
| app/core/auth.py | Implements basic authentication logic with a fake user database |
| app/api/router.py | Configures API routing to include various endpoint modules |
| app/api/endpoints/* | Implements endpoints for webrtc, streams, sensors, point cloud, options, and devices |
| app/api/dependencies.py | Supplies singleton dependencies for RealSense and WebRTC managers |
| wrappers/rest-api/README.md & wrappers/readme.md | Updates documentation to document the new REST API capabilities |
Files not reviewed (1)
- wrappers/rest-api/.gitignore: Language not supported
Comments suppressed due to low confidence (1)
wrappers/rest-api/app/api/endpoints/point_cloud.py:32
- Consider renaming 'get_stream_status' to 'get_point_cloud_status' for consistency with the underlying service call.
async def get_stream_status(
| fake_users_db = { | ||
| "admin": { | ||
| "username": "admin", | ||
| "hashed_password": "$2b$12$EixZaYVK1fsbw1ZfbX3OXePaWxn96p36WQoeG6Lruj3vjPGga31lW", # "password" |
There was a problem hiding this comment.
just placeholders for future use in case password authentication will be required in the rest api
| from typing import Any, Optional | ||
|
|
||
| # from jose import jwt | ||
| #from passlib.context import CryptContext |
There was a problem hiding this comment.
This 2 commented lines are needed?
There was a problem hiding this comment.
just placeholders for future use in case password authentication will be required in the rest api
|
|
||
| from app.core.config import get_settings | ||
|
|
||
| #pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") |
There was a problem hiding this comment.
Same for all other commented out lines in this file
There was a problem hiding this comment.
same answer :)
I can remove but then it will be little bit more dificult to find it in the future.
| from time import time | ||
| from .pyrealsense_mock import context, create_mock_device | ||
|
|
||
| # # Global variables for frame generation |
There was a problem hiding this comment.
What is all of this commented out code?
There was a problem hiding this comment.
testing code, will remove.
| physical_port = None | ||
|
|
||
| try: | ||
| usb_type = dev.get_info(rs.camera_info.usb_type_descriptor) |
There was a problem hiding this comment.
I suggest to wrap each get info with (dev.supports()) condition,
Here for example it will fail for MIPI devices as they do not support USB_type
I am not sure if it will throw
There was a problem hiding this comment.
np. will check it.
| #!/bin/bash | ||
|
|
||
| # Start the FastAPI server | ||
| uvicorn main:combined_app --host 0.0.0.0 --port 8000 No newline at end of file |
There was a problem hiding this comment.
There's an option to do --reload for a fast reload, maybe adding it will make working on the wrapper easier?
There was a problem hiding this comment.
the reload is a flag for debugging purposes. not needed for regular usage.
| This fixture will automatically be used in all tests. | ||
| """ | ||
| # Create mock instances | ||
| rs_manager = RealSenseManager() |
There was a problem hiding this comment.
Shouldn't we initialize sio and do RealSenseManager(sio) ?
There was a problem hiding this comment.
this is a mock code
|
We also need to add License and Copyright on some of the files |
There was a problem hiding this comment.
if device_id not in self.devices:
self.refresh_devices()
if device_id not in self.devices:
raise RealSenseError(
status_code=404, detail=f"Device {device_id} not found"
)
I see this code (or similar variations) are repeated multiple times in the code, let's use get_device or a new helper function
There was a problem hiding this comment.
Good point, I agree it can be an improvement, but let's keep it simple for the first step, and do it in the future code changes.
|
@ArthurRaizIntel can we progress here? |
8999ce6 to
ebba8d5
Compare
Tracked on [LRS-1272]