SerialProcessor.tsx #78
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis pull request introduces a new React component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SerialProcessor
participant SerialPort
User->>SerialProcessor: Click "Connect"
SerialProcessor->>SerialPort: connectSerial()
SerialPort-->>SerialProcessor: Connection response
User->>SerialProcessor: Click "Start Recording"
SerialProcessor->>SerialPort: send START command
SerialPort-->>SerialProcessor: Stream data
Note right of SerialProcessor: Process incoming packets
User->>SerialProcessor: Click "Stop Recording"
SerialProcessor->>SerialPort: send STOP command
User->>SerialProcessor: Click "Save to CSV"
SerialProcessor->>User: Trigger CSV file download
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/components/SerialProcessor.tsx (3)
5-10: Add JSDoc documentation for the Sample interface.Consider adding documentation to explain the purpose and units of each field, especially for timing-related fields like
TimeDeltaandtimestamp.+/** + * Represents a single sample of data from the serial port. + * @interface Sample + * @property {number} Counter - Sequential counter for the sample + * @property {number} Channel1 - Normalized channel value (0-1) + * @property {number} [TimeDelta] - Time difference from previous sample in milliseconds + * @property {number} timestamp - Raw timestamp from the device + */ interface Sample { Counter: number; Channel1: number; TimeDelta?: number; timestamp: number; }
111-133: Improve CSV export robustness.Consider the following improvements:
- Add error handling for file operations
- Use more descriptive filename format
- Clean up URL object after use
const saveToCSV = () => { + let url: string | null = null; try { const csvContent = [ // CSV Header ['Counter', 'Channel1', 'TimeDelta'].join(','), // CSV Data ...data.map(sample => [ sample.Counter, sample.Channel1.toFixed(6), sample.TimeDelta?.toFixed(3) ].join(',') ) ].join('\n'); const blob = new Blob([csvContent], { type: 'text/csv;charset=utf-8;' }); - const url = URL.createObjectURL(blob); + url = URL.createObjectURL(blob); const link = document.createElement('a'); link.setAttribute('href', url); - link.setAttribute('download', `ChordsWeb${new Date().toISOString().slice(0,-5).replace(/[:-]/g, '')}.csv`); + const timestamp = new Date().toISOString() + .replace(/[:.]/g, '-') + .replace('T', '_') + .slice(0, -1); + link.setAttribute('download', `chords_web_recording_${timestamp}.csv`); document.body.appendChild(link); link.click(); document.body.removeChild(link); + } catch (error) { + console.error('Error saving CSV:', error); + // TODO: Add user-friendly error notification + } finally { + if (url) { + URL.revokeObjectURL(url); + } } };
135-167: Improve component accessibility.Add ARIA labels and roles to improve accessibility:
- <div className="p-4"> + <div className="p-4" role="region" aria-label="Serial Data Recording"> <div className="flex gap-4 mb-4"> <Button onClick={connectSerial} disabled={isConnected} className="bg-primary text-primary-foreground hover:bg-primary/90" + aria-label={isConnected ? "Already connected to serial port" : "Connect to serial port"} > Connect Serial </Button> <Button onClick={isRecording ? stopRecording : startRecording} disabled={!isConnected} className={isRecording ? "bg-destructive text-destructive-foreground hover:bg-destructive/90" : "bg-primary text-primary-foreground hover:bg-primary/90"} + aria-label={isRecording ? "Stop recording data" : "Start recording data"} > {isRecording ? 'Stop Recording' : 'Start Recording'} </Button> <Button onClick={saveToCSV} disabled={data.length === 0} className="bg-secondary text-secondary-foreground hover:bg-secondary/90" + aria-label={data.length === 0 ? "No data to save" : "Save data as CSV"} > Save CSV </Button> </div> - <div className="text-sm text-muted-foreground"> + <div className="text-sm text-muted-foreground" role="status" aria-live="polite"> {isConnected ? 'Connected to serial port' : 'Not connected'} <br /> Samples recorded: {data.length} </div> </div>
Comment on lines
+12
to
+18
| const SerialProcessor = () => { | ||
| const [port, setPort] = useState<SerialPort | null>(null); | ||
| const [isConnected, setIsConnected] = useState(false); | ||
| const [isRecording, setIsRecording] = useState(false); | ||
| const [data, setData] = useState<Sample[]>([]); | ||
| const [lastTimestamp, setLastTimestamp] = useState<number | null>(null); | ||
|
|
There was a problem hiding this comment.
Add type definition for SerialPort.
The SerialPort type is used but not imported or defined. Consider importing it from the Web Serial API types.
+/// <reference types="w3c-web-serial" />
import React, { useState, useEffect, useCallback } from 'react';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const SerialProcessor = () => { | |
| const [port, setPort] = useState<SerialPort | null>(null); | |
| const [isConnected, setIsConnected] = useState(false); | |
| const [isRecording, setIsRecording] = useState(false); | |
| const [data, setData] = useState<Sample[]>([]); | |
| const [lastTimestamp, setLastTimestamp] = useState<number | null>(null); | |
| /// <reference types="w3c-web-serial" /> | |
| import React, { useState, useEffect, useCallback } from 'react'; | |
| const SerialProcessor = () => { | |
| const [port, setPort] = useState<SerialPort | null>(null); | |
| const [isConnected, setIsConnected] = useState(false); | |
| const [isRecording, setIsRecording] = useState(false); | |
| const [data, setData] = useState<Sample[]>([]); | |
| const [lastTimestamp, setLastTimestamp] = useState<number | null>(null); | |
| // ... rest of the component logic | |
| }; |
Comment on lines
+19
to
+28
| const connectSerial = async () => { | ||
| try { | ||
| const selectedPort = await navigator.serial.requestPort(); | ||
| await selectedPort.open({ baudRate: 230400 }); | ||
| setPort(selectedPort); | ||
| setIsConnected(true); | ||
| } catch (error) { | ||
| console.error('Error connecting to serial:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add cleanup.
- Add user-friendly error messages
- Implement cleanup on component unmount
const connectSerial = async () => {
try {
const selectedPort = await navigator.serial.requestPort();
await selectedPort.open({ baudRate: 230400 });
setPort(selectedPort);
setIsConnected(true);
} catch (error) {
- console.error('Error connecting to serial:', error);
+ const message = error instanceof Error ? error.message : 'Unknown error';
+ console.error('Error connecting to serial:', message);
+ // TODO: Add user-friendly error notification
}
};
+useEffect(() => {
+ return () => {
+ if (port) {
+ port.close().catch(console.error);
+ }
+ };
+}, [port]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const connectSerial = async () => { | |
| try { | |
| const selectedPort = await navigator.serial.requestPort(); | |
| await selectedPort.open({ baudRate: 230400 }); | |
| setPort(selectedPort); | |
| setIsConnected(true); | |
| } catch (error) { | |
| console.error('Error connecting to serial:', error); | |
| } | |
| }; | |
| const connectSerial = async () => { | |
| try { | |
| const selectedPort = await navigator.serial.requestPort(); | |
| await selectedPort.open({ baudRate: 230400 }); | |
| setPort(selectedPort); | |
| setIsConnected(true); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : 'Unknown error'; | |
| console.error('Error connecting to serial:', message); | |
| // TODO: Add user-friendly error notification | |
| } | |
| }; | |
| useEffect(() => { | |
| return () => { | |
| if (port) { | |
| port.close().catch(console.error); | |
| } | |
| }; | |
| }, [port]); |
Comment on lines
+30
to
+61
| const extractTimestamp = (packet: Uint8Array): number => { | ||
| const timeIndex = 3 + (6 * 2); // HEADER_LEN + (NUM_CHANNELS * 2) | ||
| return (packet[timeIndex] << 24) | | ||
| (packet[timeIndex + 1] << 16) | | ||
| (packet[timeIndex + 2] << 8) | | ||
| packet[timeIndex + 3]; | ||
| }; | ||
|
|
||
| const processPacket = (packet: Uint8Array): Sample | null => { | ||
| if (packet.length < 20) return null; // Minimum packet size check | ||
|
|
||
| // Check sync bytes | ||
| if (packet[0] !== 0xC7 || packet[1] !== 0x7C) return null; | ||
|
|
||
| const timestamp = extractTimestamp(packet); | ||
| const counter = packet[2]; | ||
|
|
||
| // Process Channel1 (assuming 14-bit ADC) | ||
| const channel1High = packet[3]; | ||
| const channel1Low = packet[4]; | ||
| const channel1Value = ((channel1High << 8) | channel1Low) / 16384.0; // Normalize to 0-1 | ||
|
|
||
| const timeDelta = lastTimestamp ? (timestamp - lastTimestamp) / 1000 : 0; | ||
| setLastTimestamp(timestamp); | ||
|
|
||
| return { | ||
| Counter: counter, | ||
| Channel1: channel1Value, | ||
| TimeDelta: timeDelta, | ||
| timestamp: timestamp | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve packet processing robustness.
- Define constants for packet structure
- Add validation for channel value range
- Consider using DataView for safer binary parsing
+const PACKET_CONSTANTS = {
+ SYNC_BYTE1: 0xC7,
+ SYNC_BYTE2: 0x7C,
+ MIN_PACKET_SIZE: 20,
+ HEADER_LEN: 3,
+ CHANNELS: 6,
+ TIMESTAMP_SIZE: 4
+} as const;
const extractTimestamp = (packet: Uint8Array): number => {
- const timeIndex = 3 + (6 * 2);
+ const timeIndex = PACKET_CONSTANTS.HEADER_LEN + (PACKET_CONSTANTS.CHANNELS * 2);
return (packet[timeIndex] << 24) |
(packet[timeIndex + 1] << 16) |
(packet[timeIndex + 2] << 8) |
packet[timeIndex + 3];
};
const processPacket = (packet: Uint8Array): Sample | null => {
- if (packet.length < 20) return null;
+ if (packet.length < PACKET_CONSTANTS.MIN_PACKET_SIZE) return null;
// Check sync bytes
- if (packet[0] !== 0xC7 || packet[1] !== 0x7C) return null;
+ if (packet[0] !== PACKET_CONSTANTS.SYNC_BYTE1 ||
+ packet[1] !== PACKET_CONSTANTS.SYNC_BYTE2) return null;
const timestamp = extractTimestamp(packet);
const counter = packet[2];
- // Process Channel1 (assuming 14-bit ADC)
- const channel1High = packet[3];
- const channel1Low = packet[4];
- const channel1Value = ((channel1High << 8) | channel1Low) / 16384.0;
+ // Use DataView for safer binary parsing
+ const view = new DataView(packet.buffer);
+ const channel1Raw = view.getUint16(3, false); // big-endian
+ const channel1Value = channel1Raw / 16384.0;
+
+ // Validate channel value
+ if (channel1Value < 0 || channel1Value > 1) {
+ console.warn(`Invalid channel value: ${channel1Value}`);
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const extractTimestamp = (packet: Uint8Array): number => { | |
| const timeIndex = 3 + (6 * 2); // HEADER_LEN + (NUM_CHANNELS * 2) | |
| return (packet[timeIndex] << 24) | | |
| (packet[timeIndex + 1] << 16) | | |
| (packet[timeIndex + 2] << 8) | | |
| packet[timeIndex + 3]; | |
| }; | |
| const processPacket = (packet: Uint8Array): Sample | null => { | |
| if (packet.length < 20) return null; // Minimum packet size check | |
| // Check sync bytes | |
| if (packet[0] !== 0xC7 || packet[1] !== 0x7C) return null; | |
| const timestamp = extractTimestamp(packet); | |
| const counter = packet[2]; | |
| // Process Channel1 (assuming 14-bit ADC) | |
| const channel1High = packet[3]; | |
| const channel1Low = packet[4]; | |
| const channel1Value = ((channel1High << 8) | channel1Low) / 16384.0; // Normalize to 0-1 | |
| const timeDelta = lastTimestamp ? (timestamp - lastTimestamp) / 1000 : 0; | |
| setLastTimestamp(timestamp); | |
| return { | |
| Counter: counter, | |
| Channel1: channel1Value, | |
| TimeDelta: timeDelta, | |
| timestamp: timestamp | |
| }; | |
| }; | |
| const PACKET_CONSTANTS = { | |
| SYNC_BYTE1: 0xC7, | |
| SYNC_BYTE2: 0x7C, | |
| MIN_PACKET_SIZE: 20, | |
| HEADER_LEN: 3, | |
| CHANNELS: 6, | |
| TIMESTAMP_SIZE: 4 | |
| } as const; | |
| const extractTimestamp = (packet: Uint8Array): number => { | |
| const timeIndex = PACKET_CONSTANTS.HEADER_LEN + (PACKET_CONSTANTS.CHANNELS * 2); | |
| return (packet[timeIndex] << 24) | | |
| (packet[timeIndex + 1] << 16) | | |
| (packet[timeIndex + 2] << 8) | | |
| packet[timeIndex + 3]; | |
| }; | |
| const processPacket = (packet: Uint8Array): Sample | null => { | |
| if (packet.length < PACKET_CONSTANTS.MIN_PACKET_SIZE) return null; | |
| // Check sync bytes | |
| if (packet[0] !== PACKET_CONSTANTS.SYNC_BYTE1 || | |
| packet[1] !== PACKET_CONSTANTS.SYNC_BYTE2) return null; | |
| const timestamp = extractTimestamp(packet); | |
| const counter = packet[2]; | |
| // Use DataView for safer binary parsing | |
| const view = new DataView(packet.buffer); | |
| const channel1Raw = view.getUint16(3, false); // big-endian | |
| const channel1Value = channel1Raw / 16384.0; | |
| // Validate channel value | |
| if (channel1Value < 0 || channel1Value > 1) { | |
| console.warn(`Invalid channel value: ${channel1Value}`); | |
| return null; | |
| } | |
| const timeDelta = lastTimestamp ? (timestamp - lastTimestamp) / 1000 : 0; | |
| setLastTimestamp(timestamp); | |
| return { | |
| Counter: counter, | |
| Channel1: channel1Value, | |
| TimeDelta: timeDelta, | |
| timestamp: timestamp | |
| }; | |
| }; |
Comment on lines
+63
to
+109
| const startRecording = async () => { | ||
| if (!port) return; | ||
|
|
||
| setIsRecording(true); | ||
| setData([]); | ||
| setLastTimestamp(null); | ||
|
|
||
| try { | ||
| // Send START command | ||
| const encoder = new TextEncoder(); | ||
| const writer = port.writable.getWriter(); | ||
| await writer.write(encoder.encode('START\n')); | ||
| writer.releaseLock(); | ||
|
|
||
| // Start reading | ||
| const reader = port.readable.getReader(); | ||
|
|
||
| while (isRecording) { | ||
| const { value, done } = await reader.read(); | ||
| if (done) break; | ||
|
|
||
| const sample = processPacket(value); | ||
| if (sample) { | ||
| setData(prev => [...prev, sample]); | ||
| } | ||
| } | ||
|
|
||
| reader.releaseLock(); | ||
| } catch (error) { | ||
| console.error('Error during recording:', error); | ||
| } | ||
| }; | ||
|
|
||
| const stopRecording = async () => { | ||
| if (!port) return; | ||
|
|
||
| setIsRecording(false); | ||
|
|
||
| try { | ||
| const encoder = new TextEncoder(); | ||
| const writer = port.writable.getWriter(); | ||
| await writer.write(encoder.encode('STOP\n')); | ||
| writer.releaseLock(); | ||
| } catch (error) { | ||
| console.error('Error stopping recording:', error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix potential infinite loop and improve resource handling.
The current implementation has several issues:
- The while loop might not break if state updates are missed
- Resources aren't properly released on errors
const startRecording = async () => {
if (!port) return;
setIsRecording(true);
setData([]);
setLastTimestamp(null);
+ let reader: ReadableStreamDefaultReader<Uint8Array> | null = null;
+ let writer: WritableStreamDefaultWriter<Uint8Array> | null = null;
try {
// Send START command
const encoder = new TextEncoder();
- const writer = port.writable.getWriter();
+ writer = port.writable.getWriter();
await writer.write(encoder.encode('START\n'));
- writer.releaseLock();
+ await writer.close();
+ writer = null;
// Start reading
- const reader = port.readable.getReader();
+ reader = port.readable.getReader();
- while (isRecording) {
+ // Use AbortController for safe cancellation
+ const abortController = new AbortController();
+ const signal = abortController.signal;
+
+ const recordingPromise = async () => {
+ while (!signal.aborted) {
const { value, done } = await reader.read();
if (done) break;
const sample = processPacket(value);
if (sample) {
setData(prev => [...prev, sample]);
}
- }
+ }
+ };
- reader.releaseLock();
+ await recordingPromise();
} catch (error) {
console.error('Error during recording:', error);
+ setIsRecording(false);
} finally {
+ if (writer) {
+ try {
+ await writer.close();
+ } catch (e) {
+ console.error('Error closing writer:', e);
+ }
+ writer.releaseLock();
+ }
+ if (reader) {
+ try {
+ await reader.cancel();
+ } catch (e) {
+ console.error('Error cancelling reader:', e);
+ }
+ reader.releaseLock();
+ }
}
};Committable suggestion skipped: line range outside the PR's diff.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add another column for time taken
Summary by CodeRabbit