Improve BLE Stability and Battery Notifications with New Firmware Support#115
Improve BLE Stability and Battery Notifications with New Firmware Support#115lorforlinux merged 4 commits intoupsidedownlabs:mainfrom
Conversation
Amanmahe
had a problem deploying
to
github-pages
GitHub Actions
Failure
📝 WalkthroughWalkthroughThis PR refactors the NPG Lite page to introduce a device configuration abstraction that dynamically determines channel counts, sample lengths, and battery support. It adds battery monitoring with UI indicators, improves BLE connection/disconnection flows with proper cleanup, and makes data processing logic dynamic based on detected device capabilities. Changes
Sequence DiagramsequenceDiagram
participant App as App Component
participant BLE as BLE Device
participant Config as Device Config
participant Battery as Battery Service
participant Filters as Filter Pipeline
participant UI as UI Renderer
App->>BLE: Connect to device
BLE-->>App: Device connected
App->>BLE: Read device.name
BLE-->>App: Return device.name
App->>Config: Detect & apply configuration<br/>(maxChannels, sampleLength, hasBattery)
Config-->>App: Config initialized
alt hasBattery == true
App->>Battery: Initialize battery notifications
Battery-->>App: Battery level updates
App->>UI: Render battery status & icon
else hasBattery == false
App->>UI: Render status without battery
end
App->>Filters: Reinitialize filters<br/>(notch, EXG, pointone)
Filters-->>App: Filters ready
App->>UI: Render UI with dynamic channels
UI-->>App: Ready for data processing
App->>App: Start data processing<br/>per deviceConfig
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/npg-lite/page.tsx (2)
126-126:⚠️ Potential issue | 🔴 CriticalBug:
activeBufferIndexis aletvariable reset to 0 on every render.Same issue as
prevSampleCounter/channelData— this loses the current buffer index across re-renders, causing recording data to always write to buffer 0.🐛 Proposed fix
- let activeBufferIndex = 0; + const activeBufferIndexRef = useRef<number>(0);Update usages in
processSample(line 508, 512) anddisconnect(line 730) accordingly.
832-835:⚠️ Potential issue | 🔴 CriticalSyntax error:
elseblock uses arrow function syntax instead of a proper conditional.
} else (error: any) => {is invalid JavaScript/TypeScript. This will either fail to compile or be silently ignored as an expression statement. It should be anelse ifblock.🐛 Proposed fix
- } else (error: any) => { - console.error("Worker error:", error); - toast.error(`Error during file download: ${error.message}`); - } + } else if (error) { + console.error("Worker error:", error); + toast.error(`Error during file download: ${error.message}`); + }
🤖 Fix all issues with AI agents
In `@src/app/npg-lite/page.tsx`:
- Around line 1146-1176: Change the exact-equality checks and transient interval
logic in the useEffect that depends on batteryLevel: replace `batteryLevel ===
20`, `=== 70`, `=== 99` with range/threshold checks (e.g., <=20 && >10, <=70 &&
>20, >=99) or use a threshold-set so notifications fire the first time a
threshold is crossed; use a ref (e.g., shownThresholdsRef or intervalRef) to
track which thresholds have been shown and to hold the intervalId so you only
call window.setInterval for `batteryLevel <= 10` once (create interval only if
intervalRef.current is undefined) and clear it when batteryLevel rises above 10,
and ensure cleanup clears intervalRef.current and marks thresholds appropriately
(use useRef for previousBatteryLevel or shownThresholdsRef to avoid resetting
the 1-minute timer on every reading).
- Around line 529-531: The callback's dependency array incorrectly includes ref
.current values (e.g., canvasElementCountRef.current and, in updatePlots,
dataPointCountRef.current and zoomRef.current) which React won't track; fix by
removing the .current entries from the dependency arrays and either (A)
introduce and use corresponding state variables (e.g., canvasElementCountState /
dataPointCountState / zoomState) in the deps so the callback re-creates when
those values change, or (B) keep the refs and have the callback read
canvasElementCountRef.current, dataPointCountRef.current and zoomRef.current at
runtime (and not include them in deps) so the callback always sees the latest
values; update the useCallback where canvasElementCountRef is referenced and the
updatePlots function to follow one of these two approaches.
- Line 729: The linter flags that the forEach callback on recordingBuffers
returns a value because it uses the assignment expression; change the callback
to a statement block so it does not return anything — e.g. replace
recordingBuffers.forEach(buffer => buffer.length = 0) with
recordingBuffers.forEach(buffer => { buffer.length = 0 }); alternatively use a
for...of loop over recordingBuffers to set buffer.length = 0; update the code
around the recordingBuffers usage accordingly.
- Around line 1178-1186: getBatteryColor currently falls through to green for
level ≤ 10 because there's no branch for that range; update the function
(getBatteryColor) to explicitly handle level <= 10 (e.g., return "text-red-500"
or a stronger danger class) before the other checks, or reorder/simplify the
conditions so ranges are exhaustive (<=10 => red, <=20 => red/orange as
intended, <=70 => orange, >70 => green).
- Around line 368-369: The variables prevSampleCounter and channelData are
declared with let at component scope and get reset on each render; change them
to refs by creating prevSampleCounterRef = useRef<number | null>(null) and
channelDataRef = useRef<number[]>([]), then replace all accesses:
prevSampleCounter → prevSampleCounterRef.current and channelData →
channelDataRef.current across the file (including in processSample,
handleNotification, and disconnect) and update the reset logic to assign to
.current instead of reassigning the outer variables.
- Around line 656-661: The code creates and immediately clears a new interval
(no-op) inside page.tsx; remove the dead block that does
setInterval(()=>{})/clearInterval(...) and instead clear the real
interval/timeout IDs by storing them when created (e.g., introduce
checkIntervalRef or sampleTimeoutRef) and call
clearInterval(checkIntervalRef.current) / clearTimeout(sampleTimeoutRef.current)
where appropriate (use samplesReceivedRef.current only for counting, not for
clearing timers). Ensure any created intervals use a saved id (checkIntervalRef)
so they can be cleared correctly.
- Around line 735-736: Replace the hardcoded size 6 when resetting
sweepPositions and currentSweepPos in the disconnect cleanup with a dynamic
channel count: use the active deviceConfig.maxChannels if available, otherwise
fall back to defaultConfig.maxChannels (e.g., new
Array(deviceConfig?.maxChannels ?? defaultConfig.maxChannels).fill(0)); this
keeps it consistent with createCanvasElements and the config reset logic.
🧹 Nitpick comments (3)
src/app/npg-lite/page.tsx (3)
396-453: Duplicate filter initialization —updateDeviceConfigurationanduseEffectondeviceConfigboth reinitialize filters.Lines 430-444 reinitialize all filters, and then
setDeviceConfig(newConfig)at line 427 triggers theuseEffectat lines 377-393 which does the same thing again. Either remove the manual initialization here or remove the useEffect — having both is redundant and wasteful.
597-613: Battery characteristic event listener is never removed during disconnect.The
characteristicvaluechangedlistener added on line 603 for the battery characteristic is not removed in thedisconnectfunction. While the GATT disconnect may implicitly stop notifications, explicitly removing the listener ensures no stale references or memory leaks if the device object persists.🔧 Proposed fix: add cleanup in the disconnect try block (around line 684)
+ // Stop battery notifications + try { + if (batteryCharacteristicRef.current) { + await batteryCharacteristicRef.current.stopNotifications(); + batteryCharacteristicRef.current = null; + } + } catch (error) { + console.log("Error stopping battery notifications:", error); + } + // Send stop command if control characteristic exists
1832-1897: Battery popover is always rendered regardless of connection state or battery support.The battery popover button is always visible (even when disconnected or when the device doesn't support battery). The popover handles disconnected/old-firmware states inside, but having a permanently visible battery icon when no device is connected may confuse users. Consider conditionally rendering this section or at minimum disabling the button when not connected.
|
@Amanmahe Thanks for your contribution, I am merging this to test with newest firmware. |
2 participants
@lorforlinux, Please review and merge it.
Summary by CodeRabbit
Release Notes