Feat/sensor integration - #16
Conversation
… T10A driver, sensor manager with workers, configuration support, and new metrics API endpoints.
|
Looks good for what we have to go off of, will have to try once we are able to actually connect to the sensor |
There was a problem hiding this comment.
Pull request overview
This PR adds a sensor integration pipeline to enable collection and storage of environmental metrics (e.g., illuminance) from Konica Minolta T-10A light meters. The implementation includes database tables for sensor metadata and readings, a pluggable sensor client interface, a T-10A serial driver, a background worker manager, and REST API endpoints for querying sensor data.
Key Changes:
- Database schema extension with sensor metadata and time-series readings tables
- Background worker threads for continuous sensor polling at configurable intervals
- Three new API endpoints:
/sensors,/metrics/latest, and/metrics/history
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| web/package-lock.json | Removed peer: true flag from React dependency (unrelated cosmetic change) |
| svc/requirements.txt | Added pyserial==3.5 dependency for serial communication |
| svc/pyproject.toml | Added pyserial>=3.5 to project dependencies |
| svc/tests/test_t10a_serial.py | Manual integration test script for T-10A serial communication (not pytest-compatible) |
| svc/data/sensors_config.json | Configuration file defining T-10A devices, COM ports, polling intervals, and sensor heads |
| svc/app/state.py | Added sensor database tables and CRUD operations (register_sensor, insert_sensor_reading, fetch_readings, etc.) |
| svc/app/sensors/interface.py | Defined SensorClient protocol and SensorReading dataclass for pluggable sensor drivers |
| svc/app/sensors/t10a_client.py | Implemented T-10A serial driver with frame building, command sending, and measurement parsing |
| svc/app/sensors/manager.py | Sensor lifecycle manager that starts worker threads for polling configured sensors |
| svc/app/routes.py | Added three sensor API endpoints for listing sensors and retrieving metric data |
| svc/app/models.py | Added SensorInfo and SensorReadingResponse Pydantic models |
| svc/main.py | Integrated sensor worker startup/shutdown with FastAPI lifecycle events |
Files not reviewed (1)
- web/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - 9600 baud, 7 data bits, even parity, 1 stop bit. | ||
| - ASCII framed commands with STX/ETX + BCC. | ||
| - Command '54' → PC mode. | ||
| - Command '10' → measurement data output (long frame). | ||
| """ | ||
|
|
There was a problem hiding this comment.
The _build_frame method at lines 60-83 builds empty frame data (pc_mode_frame = b"" and meas_frame = b""), which means actual serial communication will fail. The frame building logic needs to be properly implemented according to the T-10A Communication Specifications before this can work in production. This appears to be a stub implementation.
| client = T10AClient(device_id=device_id, port=port, heads=heads_cfg) | ||
| clients_with_interval.append((client, interval_s)) | ||
|
|
There was a problem hiding this comment.
If T10AClient initialization fails (e.g., COM port unavailable or serial communication error), the exception is not caught and will propagate, potentially preventing the entire application from starting. Consider wrapping the client creation in a try-except block and logging the error so other sensors can still initialize successfully.
| client = T10AClient(device_id=device_id, port=port, heads=heads_cfg) | |
| clients_with_interval.append((client, interval_s)) | |
| try: | |
| client = T10AClient(device_id=device_id, port=port, heads=heads_cfg) | |
| clients_with_interval.append((client, interval_s)) | |
| except Exception as e: | |
| logger.error(f"Failed to initialize T10AClient for device_id={device_id} on port={port}: {e}", exc_info=True) |
| import time | ||
| import serial | ||
|
|
||
| PORT = "COM3" | ||
|
|
||
|
|
||
| def main(): | ||
| ser = serial.Serial( | ||
| port=PORT, | ||
| baudrate=9600, | ||
| bytesize=serial.SEVENBITS, | ||
| parity=serial.PARITY_EVEN, | ||
| stopbits=serial.STOPBITS_ONE, | ||
| timeout=1.0, | ||
| ) | ||
|
|
||
| print(f"Opened {PORT}") | ||
|
|
||
|
|
||
| def send_frame(raw: bytes) -> str: | ||
| ser.write(raw) | ||
| ser.flush() | ||
| line = ser.readline().decode("ascii", errors="replace").strip() | ||
| print("REPLY:", repr(line)) | ||
| return line | ||
|
|
||
| # 1) Build PC mode frame (command 54, head 0) | ||
| pc_mode_frame = b"" | ||
| send_frame(pc_mode_frame) | ||
| time.sleep(0.5) | ||
|
|
||
| # 2) Build measurement frame (command 10, head 0) | ||
| meas_frame = b"" | ||
| send_frame(meas_frame) | ||
|
|
||
| ser.close() | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
This test file doesn't follow pytest conventions and won't be automatically discovered by pytest. The function should be named with a test_ prefix to be recognized as a test (e.g., def test_t10a_serial_communication()), and it should include assertions. Alternatively, if this is meant to be a manual integration test or standalone script, it should be moved out of the tests/ directory to avoid confusion.
| self.ser = serial.Serial( | ||
| port=self.port, | ||
| baudrate=9600, | ||
| bytesize=serial.SEVENBITS, | ||
| parity=serial.PARITY_EVEN, | ||
| stopbits=serial.STOPBITS_ONE, | ||
| timeout=timeout_s, | ||
| ) |
There was a problem hiding this comment.
The serial port connection is not thread-safe. Since poll() is called from a worker thread in manager.py, concurrent calls to self.ser.write() and self.ser.readline() could interleave, leading to corrupted communication. Consider adding a lock (threading.Lock()) around serial I/O operations in _send_command() to ensure thread-safe access.
| @@ -0,0 +1,161 @@ | |||
| # app/sensors/t10a_client.py | |||
There was a problem hiding this comment.
File contains a UTF-8 BOM (Byte Order Mark) character at the beginning. This is unnecessary for UTF-8 files and can cause issues with some tools. Remove the BOM character.
| # app/sensors/t10a_client.py | |
| # app/sensors/t10a_client.py |
| id: str | ||
| kind: str | ||
| label: str | ||
| location: Optional[str] = None | ||
| config: Dict = Field(default_factory=dict) | ||
|
|
||
|
|
||
| class SensorReadingResponse(BaseModel): | ||
| sensor_id: str | ||
| metric: str | ||
| value: float | ||
| ts: float |
There was a problem hiding this comment.
The new sensor models lack field descriptions. For consistency with the rest of the codebase (which uses Field(description=...) for all model fields), add descriptions to the fields. For example:
class SensorInfo(BaseModel):
id: str = Field(description="Unique sensor identifier, e.g. 'KM1-00'")
kind: str = Field(description="Sensor type, e.g. 't10a', 'jeti'")
label: str = Field(description="Human-readable sensor label")
location: Optional[str] = Field(None, description="Physical location of the sensor")
config: Dict = Field(default_factory=dict, description="Sensor-specific configuration")| id: str | |
| kind: str | |
| label: str | |
| location: Optional[str] = None | |
| config: Dict = Field(default_factory=dict) | |
| class SensorReadingResponse(BaseModel): | |
| sensor_id: str | |
| metric: str | |
| value: float | |
| ts: float | |
| id: str = Field(description="Unique sensor identifier, e.g. 'KM1-00'") | |
| kind: str = Field(description="Sensor type, e.g. 't10a', 'jeti'") | |
| label: str = Field(description="Human-readable sensor label") | |
| location: Optional[str] = Field(default=None, description="Physical location of the sensor") | |
| config: Dict = Field(default_factory=dict, description="Sensor-specific configuration") | |
| class SensorReadingResponse(BaseModel): | |
| sensor_id: str = Field(description="Unique sensor identifier for this reading") | |
| metric: str = Field(description="Type of metric measured (e.g., 'illuminance', 'temperature')") | |
| value: float = Field(description="Measured value for the metric") | |
| ts: float = Field(description="Unix timestamp when the reading was taken") |
| clients_with_interval: list[tuple[SensorClient, float]] = [] | ||
|
|
||
| # --- T-10A ------------------------------------------------------------- | ||
| for dev_cfg in cfg.get("t10a", [])[:4]: # enforce 1-4 devices |
There was a problem hiding this comment.
The hardcoded slice [:4] limiting devices to 4 is somewhat arbitrary and inconsistent with the docstring that mentions "up to 4 devices". Consider either removing this limit or making it configurable, or at minimum logging a warning when more than 4 devices are configured but only 4 are being used.
|
|
||
|
|
There was a problem hiding this comment.
The sensor_readings table lacks indexes on commonly queried columns. Given the queries in fetch_latest_readings() and fetch_readings(), consider adding indexes on (sensor_id, metric, ts) and (ts) to improve query performance as the table grows with continuous sensor data:
CREATE INDEX IF NOT EXISTS idx_readings_sensor_metric_ts
ON sensor_readings(sensor_id, metric, ts);
CREATE INDEX IF NOT EXISTS idx_readings_ts
ON sensor_readings(ts);| # Add indexes to improve query performance for common queries | |
| conn.execute( | |
| "CREATE INDEX IF NOT EXISTS idx_readings_sensor_metric_ts ON sensor_readings(sensor_id, metric, ts);" | |
| ) | |
| conn.execute( | |
| "CREATE INDEX IF NOT EXISTS idx_readings_ts ON sensor_readings(ts);" | |
| ) |
| def get_metric_history( | ||
| sensor_id: str = Query(..., description="Sensor ID, e.g. KM1-00"), | ||
| metric: str = Query(..., description="Metric name, e.g. 'lux'"), | ||
| ts_from: float = Query(..., description="Start timestamp (unix seconds)"), | ||
| ts_to: float = Query(..., description="End timestamp (unix seconds)"), |
There was a problem hiding this comment.
The /metrics/history endpoint lacks input validation for the time range. Consider validating that:
ts_from <= ts_toto prevent invalid ranges- The time range is reasonable (e.g., not more than 1 year) to prevent excessive queries
- Both timestamps are not in the future
Example:
if ts_from > ts_to:
raise HTTPException(status_code=400, detail="ts_from must be <= ts_to")
if ts_to - ts_from > 31536000: # 1 year
raise HTTPException(status_code=400, detail="Time range cannot exceed 1 year")| @app.on_event("startup") | ||
| async def _start_sensors() -> None: | ||
| start_sensor_workers() | ||
|
|
||
|
|
||
| @app.on_event("shutdown") | ||
| async def _stop_sensors() -> None: | ||
| stop_sensor_workers() |
There was a problem hiding this comment.
The @app.on_event decorator is deprecated in FastAPI 0.115.5. Use lifespan context managers instead with @asynccontextmanager and pass it to the FastAPI app initialization. See: https://fastapi.tiangolo.com/advanced/events/
Example:
from contextlib import asynccontextmanager
@asynccontextmanager
async def lifespan(app: FastAPI):
# Startup
start_sensor_workers()
yield
# Shutdown
stop_sensor_workers()
app = FastAPI(lifespan=lifespan)
…results to the backend. Accessible at /metrics/latest
- Add [JetiSpectravalFileWatcher] to ingest sensor data from files. - Implement dynamic directory watching: automatically detects and switches to the newest capture file. - Refactor [JetiSpectravalSimClient] to act as a pure data generator (sim mode). - Update [manager.py] to coordinate Watcher and Sim clients. - Config updated to use directory-based output paths for automatic file rotation support. Frontend: - Add [LiveGraph] component using for real-time Lux visualization. - Integrate graph into . - Optimize polling intervals for responsiveness.
…of python for now as it seems it may be moore user friendly as we can run it sandboxed in the browser. Will reevaluate and decide if running python server side is a better option. Allows for saving routines, setting intervals, and access to currently available sensor data
…update mappings - Migrated the Routine Engine from frontend JavaScript execution to Python backend subprocesses for robust, asynchronous execution. - Implemented SQLite persistence for active and saved routines, ensuring they cleanly survive complete server restarts. - Refactored the UI to use backend APIs instead of browser local storage for state and logs. - Fixed a critical database schema bug that caused exponentially recursive runaway subprocess loops on restart. - Added routine actor tagging to backend commands for clearer audit logging. - Integrated React Router to support multi-page navigation. - Built a comprehensive Routine Builder Documentation page with syntax guides, API wrapper references, and example scripts. - Updated the window mapping configuration with the correct Halio UUIDs.
… button. Vary sim data slightly
Summary
Added sensor pipeline to the backend, including sensor database tables, a simple SensorClient interface, the Konica Minolta T-10A driver, a sensor manager, and new API endpoints for listing sensors and retrieving latest or past metric data.
Type
Testing
/sensorslists registered sensor heads./metrics/latestreturns current values./metrics/historyreturns time-ranged samples suitable for graphing.Risk and rollout
Checklist