Ingore CRC on dataFlash Reads #4619
Conversation
|
Warning Rate limit exceeded@ctzsnooze has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughResolve runtime MSP object; send a 7‑byte DATAFLASH_READ request; parse 7‑byte response header; handle raw or Huffman‑compressed payloads with fallback; always deliver result via onDataCallback(address, payloadView|null); remove Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI/Caller
participant Onboard as OnboardLogging
participant Helper as MSPHelper
participant MSP as runtime mspObj
participant FS as FileSystem
UI->>Helper: dataflashRead(address, blockSize, onChunkRead)
Helper->>Helper: resolve mspObj (this.msp || global MSP)
alt mspObj missing
Helper->>UI: onChunkRead(address, null)
else mspObj present
Helper->>MSP: send_message(MSP_DATAFLASH_READ, 7-byte request)
MSP-->>Helper: response (ArrayBuffer)
Helper->>Helper: parse 7-byte header (chunkAddress, dataSize, compType)
alt compType = 0 (raw)
Helper->>Helper: payloadView = DataView(response, 7, dataSize)
else compType = 1 (compressed)
Helper->>Helper: read compressedCount & bytes
Helper->>Helper: decompressed = Huffman.decode(...) or fallback
Helper->>Helper: payloadView = DataView(decompressed.buffer) or raw fallback
end
Helper->>Onboard: onChunkRead(chunkAddress, payloadView, bytesCompressed)
Onboard->>FS: writeChunk(blob) %% async write without awaiting
Onboard->>Onboard: advance nextAddress (or EOF on zero-length, skip on null)
Onboard->>Helper: dataflashRead(nextAddress, blockSize, onChunkRead) %% loop until done
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🔇 Additional comments (3)
src/js/msp/MSPHelper.js (3)
2449-2451: Incorrect bitwise operations for address extraction.The address and blockSize are being combined with
0xFFmasks, but the original values should be extracted with bitwise AND operations, not combined. The current implementation will always produce0xFFvalues.- let outData = [address & 0xFF, (address >> 8) & 0xFF, (address >> 16) & 0xFF, (address >> 24) & 0xFF]; - - outData = outData.concat([blockSize & 0xFF, (blockSize >> 8) & 0xFF]); + let outData = [ + address & 0xFF, + (address >> 8) & 0xFF, + (address >> 16) & 0xFF, + (address >> 24) & 0xFF, + blockSize & 0xFF, + (blockSize >> 8) & 0xFF + ];Likely an incorrect or invalid review comment.
2488-2488: Confirm dataflashRead callback handles CRC errors and optional arityMSPHelper.dataflashRead enables callback-on-error (the trailing
}, true);) and invokes onDataCallback as either onDataCallback(address, DataView[, dataSize]) on success or onDataCallback(address, null) on CRC/address/size errors. Call sites (e.g. src/js/tabs/onboard_logging.js → onChunkRead) must guard against a null data arg and the optional third parameter before accessing DataView (avoid dereferencing byteLength/etc). If callers don’t handle null/variable arity, add an explicit guard to retry/abort/report error.
2456-2456: No action required: MSP$1 is the bundler-generated alias for the defaultMSPimport
The top of this file importsMSP(import MSP from "../msp";), and during transpilation the bundler renames it toMSP$1. There is no separateMSP$1module or missinglastCommandproperty.Likely an incorrect or invalid review comment.
src/js/msp/MSPHelper.js
Outdated
| MspHelper.prototype.dataflashRead = function(address, blockSize, onDataCallback) { | ||
| let outData = [address & 0xFF, (address >> 8) & 0xFF, (address >> 16) & 0xFF, (address >> 24) & 0xFF]; | ||
|
|
||
| outData = outData.concat([blockSize & 0xff, (blockSize >> 8) & 0xff]); | ||
| outData = outData.concat([blockSize & 0xFF, (blockSize >> 8) & 0xFF]); | ||
|
|
||
| // Allow compression | ||
| outData = outData.concat([1]); | ||
|
|
||
| MSP.send_message( | ||
| MSPCodes.MSP_DATAFLASH_READ, | ||
| outData, | ||
| false, | ||
| function (response) { | ||
| if (!response.crcError) { | ||
| const chunkAddress = response.data.readU32(); | ||
|
|
||
| const headerSize = 7; | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| // Verify that the address of the memory returned matches what the caller asked for and there was not a CRC error | ||
| if (chunkAddress == address) { | ||
| /* Strip that address off the front of the reply and deliver it separately so the caller doesn't have to | ||
| * figure out the reply format: | ||
| */ | ||
| if (dataCompressionType == 0) { | ||
| onDataCallback( | ||
| address, | ||
| new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize), | ||
| ); | ||
| } else if (dataCompressionType == 1) { | ||
| // Read compressed char count to avoid decoding stray bit sequences as bytes | ||
| const compressedCharCount = response.data.readU16(); | ||
|
|
||
| // Compressed format uses 2 additional bytes as a pseudo-header to denote the number of uncompressed bytes | ||
| const compressedArray = new Uint8Array( | ||
| response.data.buffer, | ||
| response.data.byteOffset + headerSize + 2, | ||
| dataSize - 2, | ||
| ); | ||
| const decompressedArray = huffmanDecodeBuf( | ||
| compressedArray, | ||
| compressedCharCount, | ||
| defaultHuffmanTree, | ||
| defaultHuffmanLenIndex, | ||
| ); | ||
|
|
||
| onDataCallback(address, new DataView(decompressedArray.buffer), dataSize); | ||
| } | ||
| } else { | ||
| // Report address error | ||
| console.log(`Expected address ${address} but received ${chunkAddress} - retrying`); | ||
| onDataCallback(address, null); // returning null to the callback forces a retry | ||
| MSP$1.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { | ||
| if (!response.crcError) { | ||
| const chunkAddress = response.data.readU32(); | ||
|
|
||
| const headerSize = 7; | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| if (chunkAddress == address) { | ||
| if (dataCompressionType == 0) { | ||
| onDataCallback(address, new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize)); | ||
| } else if (dataCompressionType == 1) { | ||
| const compressedCharCount = response.data.readU16(); | ||
| const compressedArray = new Uint8Array(response.data.buffer, response.data.byteOffset + headerSize + 2, dataSize - 2); | ||
| const decompressedArray = huffmanDecodeBuf(compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex); | ||
|
|
||
| onDataCallback(address, new DataView(decompressedArray.buffer), dataSize); | ||
| } | ||
| } else { | ||
| // Report crc error | ||
| console.log(`Expected address ${address} but received ${chunkAddress} - retrying`); | ||
| onDataCallback(address, null); | ||
| } | ||
| } else { | ||
| // --- PATCHED BLOCK: skip CRC only for dataflash read --- | ||
| if (MSP$1.lastCommand === MSPCodes.MSP_DATAFLASH_READ) { | ||
| console.log(`CRC error for address ${address} - ignored (dataflash read)`); | ||
| onDataCallback(address, new DataView(response.data.buffer)); | ||
| } else { | ||
| console.log(`CRC error for address ${address} - retrying`); | ||
| onDataCallback(address, null); // returning null to the callback forces a retry | ||
| onDataCallback(address, null); | ||
| } | ||
| }, | ||
| true, | ||
| ); | ||
| } | ||
| }, true); | ||
| }; |
There was a problem hiding this comment.
❓ Verification inconclusive
Consider the implications of ignoring CRC errors in DataFlash reads.
While this change allows "Save Flash to File" to potentially work with corrupted data, it bypasses data integrity checks. This could result in saving corrupted log files without any indication to the user that the data might be unreliable.
Consider adding a user-visible warning when CRC errors are ignored. You could:
- Track when CRC errors are ignored and display a warning in the UI
- Mark saved files that had CRC errors (e.g., append "_crc_errors" to filename)
- Log the number of CRC errors encountered for user awareness
- if (MSP$1.lastCommand === MSPCodes.MSP_DATAFLASH_READ) {
- console.log(`CRC error for address ${address} - ignored (dataflash read)`);
- onDataCallback(address, new DataView(response.data.buffer));
+ if (MSP$1.lastCommand === MSPCodes.MSP_DATAFLASH_READ) {
+ console.warn(`CRC error for address ${address} - ignored (dataflash read)`);
+ // Consider tracking CRC errors for user notification
+ // FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1;
+ onDataCallback(address, new DataView(response.data.buffer), true /* hasCrcError */);Don't silently ignore CRC errors for DATAFLASH reads — surface and track them.
Ignoring CRC here allows corrupted flash to be saved with no user-visible indication.
- File: src/js/msp/MSPHelper.js (lines 2448–2489) — do not swallow CRC failures. Replace the console.log with console.warn, increment/record a CRC error counter (e.g., FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount||0)+1), and pass a CRC flag to the callback so callers can handle it: onDataCallback(address, new DataView(response.data.buffer), true /* hasCrcError */).
- UI/UX: when exporting/saving flash, show a warning if any CRC errors occurred and mark the output (e.g., append "_crc_errors" or require explicit user confirmation to save corrupted data).
- If ignoring CRC is required for recovery, make it explicit/opt-in and document/log the decision.
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2448–2489, stop silently swallowing CRC
errors for DATAFLASH reads: replace the console.log that ignores CRC with
console.warn, increment a persistent counter (e.g., FC = FC || {}; FC.DATAFLASH
= FC.DATAFLASH || {}; FC.DATAFLASH.crcErrorCount =
(FC.DATAFLASH.crcErrorCount||0)+1), and invoke the callback with a CRC flag so
callers can react (onDataCallback(address, new DataView(response.data.buffer),
true)). Make this “ignore CRC” behavior explicit and opt-in via a config flag
(e.g., MSP$1.ignoreDataflashCrc default false) so when it’s true you still
return the data but with the CRC flag set; otherwise return
onDataCallback(address, null) as before. Also ensure upstream callers are
updated to accept the third boolean parameter and UI export/save checks
FC.DATAFLASH.crcErrorCount to surface a warning or require confirmation.
|
This is from an error log during 'save to file' MANY OF THE unsupported characters appear in normal log files that are taken from the flash by the mass storage mode |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/msp/MSPHelper.js (1)
2464-2491: Fix: guard header reads on CRC/short frames; keep payload-only shape; gate “ignore CRC” behind a flag.Reading the header before checking
response.crcErroror length will throw on zero‑lengthDataView. Also, returning the whole buffer on CRC error breaks the payload‑only contract. Gate the bypass, add bounds checks, and keep payload‑only output.- msp.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { - const headerSize = 7; - const chunkAddress = response.data.readU32(); - const dataSize = response.data.readU16(); - const dataCompressionType = response.data.readU8(); - - if (chunkAddress == address) { - if (dataCompressionType == 0) { - onDataCallback(address, new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize)); - } else if (dataCompressionType == 1) { - const compressedCharCount = response.data.readU16(); - const compressedArray = new Uint8Array(response.data.buffer, response.data.byteOffset + headerSize + 2, dataSize - 2); - const decompressedArray = huffmanDecodeBuf(compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex); - - onDataCallback(address, new DataView(decompressedArray.buffer), dataSize); - } - } else { - console.log(`Expected address ${address} but received ${chunkAddress} - retrying`); - onDataCallback(address, null); - } - - // --- PATCHED BLOCK: always deliver data even if CRC failed --- - if (response.crcError) { - console.log(`CRC error for address ${address} - ignored (dataflash read)`); - onDataCallback(address, new DataView(response.data.buffer)); - } - }, true); + msp.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + const headerSize = 7; + + // Handle CRC errors explicitly; optionally bypass via flag + if (response.crcError) { + console.warn(`CRC error for address ${address} (len=${response.length})`); + FC.DATAFLASH = FC.DATAFLASH || {}; + FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1; + if (!FC.DATAFLASH.ignoreCrc) { + onDataCallback(address, null); + return; + } + } + + // Basic length guard + if (response.length < headerSize) { + console.warn(`DATAFLASH_READ short frame at address ${address} (len=${response.length})`); + onDataCallback(address, null); + return; + } + + const chunkAddress = response.data.readU32(); + const dataSize = response.data.readU16(); + const dataCompressionType = response.data.readU8(); + + if (chunkAddress !== address) { + console.log(`Expected address ${address} but received ${chunkAddress} - retrying`); + onDataCallback(address, null); + return; + } + + if (dataCompressionType === 0) { + // bounds check + if (response.length < headerSize + dataSize) { + console.warn(`DATAFLASH_READ payload truncated at ${address} (need ${headerSize + dataSize}, have ${response.length})`); + onDataCallback(address, null); + return; + } + onDataCallback( + address, + new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize) + ); + } else if (dataCompressionType === 1) { + if (response.length < headerSize + 2) { + console.warn(`DATAFLASH_READ compressed header truncated at ${address}`); + onDataCallback(address, null); + return; + } + const compressedCharCount = response.data.readU16(); + if (response.length < headerSize + 2 + (dataSize - 2)) { + console.warn(`DATAFLASH_READ compressed payload truncated at ${address}`); + onDataCallback(address, null); + return; + } + const compressedArray = new Uint8Array( + response.data.buffer, + response.data.byteOffset + headerSize + 2, + dataSize - 2 + ); + const decompressedArray = huffmanDecodeBuf( + compressedArray, + compressedCharCount, + defaultHuffmanTree, + defaultHuffmanLenIndex + ); + // Keep payload-only contract; preserve existing 3rd arg (dataSize) for compatibility + onDataCallback(address, new DataView(decompressedArray.buffer), dataSize); + } else { + console.warn(`Unknown compression type ${dataCompressionType} at address ${address}`); + onDataCallback(address, null); + } + }, true);
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2457-2463: Minor: dynamic MSP resolution is redundant.
MSPis already imported at Line 9. Thetypeof MSP !== 'undefined' ? MSP : nullcheck is unnecessary; callMSP.send_messagedirectly.- // Use the correct MSP object - const msp = typeof MSP !== 'undefined' ? MSP : null; - if (!msp) { - console.error('MSP object not found, cannot read dataflash.'); - onDataCallback(address, null); - return; - } - - msp.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + MSP.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (3)
src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🔇 Additional comments (1)
src/js/msp/MSPHelper.js (1)
2481-2483: No action required—caller retries on null
TheonChunkReadcallback in src/js/tabs/onboard_logging.js re-queues failed reads (calls mspHelper.dataflashRead(...) on null/mismatch), so the “retrying” log correctly reflects downstream retry logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/js/msp/MSPHelper.js (3)
2483-2491: Don’t silently ignore CRC: gate behind a flag and warn user on export.Make “ignore CRC on dataflash reads” opt‑in (e.g., CONFIGURATOR.ignoreDataflashCrc) and surface a warning if any CRCs were ignored when saving. Keep counters (crcErrorCount, addrMismatchCount) to annotate filenames or require confirmation.
2483-2491: Use chunkAddress and unify callback contract; surface CRC/mismatch and track counts.
- Pass chunkAddress, not the requested address. This avoids misassembly when the FC returns a different address.
- Always pass a 3rd arg bytesCompressed for callers (compressed: dataSize-2; uncompressed: dataSize).
- Warn (not log) on CRC/mismatch and increment FC.DATAFLASH counters for UI surfacing later.
- // Always deliver data, even if CRC fails - onDataCallback(address, payloadView); - if (response.crcError) { - console.log(`CRC error for block at ${address} ignored; data delivered`); - } else if (chunkAddress !== address) { - console.log(`Expected address ${address} but received ${chunkAddress}`); - } else { - console.log(`Block at ${address} received (${dataSize} bytes)`); - } + const bytesCompressed = dataCompressionType === 1 ? Math.max(0, dataSize - 2) : dataSize; + if (payloadView) { + onDataCallback(chunkAddress, payloadView, bytesCompressed); + } else { + onDataCallback(address, null, 0); + } + + // Telemetry + diagnostics + FC.DATAFLASH = FC.DATAFLASH || {}; + if (response.crcError) { + FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1; + console.warn(`MSP_DATAFLASH_READ: CRC error ignored at addr=${chunkAddress}; delivered=${!!payloadView}`); + } else if (chunkAddress !== address) { + FC.DATAFLASH.addrMismatchCount = (FC.DATAFLASH.addrMismatchCount || 0) + 1; + console.warn(`MSP_DATAFLASH_READ: address mismatch expected=${address} got=${chunkAddress}`); + } else { + console.log( + `MSP_DATAFLASH_READ: block ${chunkAddress} received (${bytesCompressed} byte${bytesCompressed===1?'':'s'}${dataCompressionType===1?' compressed':''})` + ); + }
2467-2472: Guard against CRC-error empty payload; current code can throw when reading header.With doCallbackOnError=true, MSP may invoke the callback with crcError=true and an empty DataView; readU32/readU16 will crash. Bail out early if payload is shorter than header.
- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { - const headerSize = 7; + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + const headerSize = 7; + if (response.data.byteLength < headerSize) { + console.warn(`MSP_DATAFLASH_READ: insufficient payload (len=${response.data.byteLength}) at addr=${address}; skipping.`); + onDataCallback(address, null); + return; + } const chunkAddress = response.data.readU32(); const dataSize = response.data.readU16(); const dataCompressionType = response.data.readU8();
🧹 Nitpick comments (2)
src/js/msp/MSPHelper.js (2)
2449-2457: Fix ESLint “comma-dangle” in outData literal.Add a trailing comma to satisfy the repo’s linting rule for multiline arrays.
- 1 // allow compression + 1, // allow compression
2463-2464: Keep callback shape consistent even on MSP missing path.If you standardize the callback to include a 3rd arg (bytesCompressed), pass 0 here to avoid mixed shapes.
- onDataCallback(address, null); + onDataCallback(address, null, 0);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2456-2456: Missing trailing comma.
(comma-dangle)
src/js/msp/MSPHelper.js
Outdated
| mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { | ||
| const headerSize = 7; | ||
| const chunkAddress = response.data.readU32(); | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| let payloadView; | ||
| if (dataCompressionType === 0) { | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| } else if (dataCompressionType === 1) { | ||
| const compressedCharCount = response.data.readU16(); | ||
| const compressedArray = new Uint8Array(response.data.buffer, response.data.byteOffset + headerSize + 2, dataSize - 2); | ||
| const decompressedArray = huffmanDecodeBuf(compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex); | ||
| payloadView = new DataView(decompressedArray.buffer); | ||
| } | ||
|
|
||
| // Always deliver data, even if CRC fails | ||
| onDataCallback(address, payloadView); | ||
| if (response.crcError) { | ||
| console.log(`CRC error for block at ${address} ignored; data delivered`); | ||
| } else if (chunkAddress !== address) { | ||
| console.log(`Expected address ${address} but received ${chunkAddress}`); | ||
| } else { | ||
| console.log(`Block at ${address} received (${dataSize} bytes)`); | ||
| } | ||
| }, true); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify downstream callers expect (address, dataView, bytesCompressed).
Some consumers (e.g., onboard_logging) expect a 3rd param; the current code sometimes sent only two args. Ensure all call sites match the unified contract.
🏁 Script executed:
#!/bin/bash
# Verify dataflashRead() call sites and callback arity usage across the repo
set -euo pipefail
echo "== dataflashRead call sites =="
rg -nP -C3 '\bdataflashRead\s*\(' --type=js
echo
echo "== callbacks that handle chunk reads =="
rg -nP -C3 '\bonChunkRead\b|\bonDataCallback\s*=\s*function|\bonDataCallback\s*\(' --type=js
echo
echo "== check for shouldBypassCrc usage (transport-level CRC bypass) =="
rg -nP -C2 'shouldBypassCrc' --type=jsLength of output: 6214
Pass bytesCompressed to onDataCallback (fix API mismatch)
onChunkRead in src/js/tabs/onboard_logging.js (line 492) expects (chunkAddress, chunkDataView, bytesCompressed); MSPHelper currently calls onDataCallback(address, payloadView) at src/js/msp/MSPHelper.js:2484 — change it to onDataCallback(address, payloadView, bytesCompressed) (use 0 when uncompressed; for Huffman/compressed blocks pass compressedArray.byteLength or dataSize - 2). Call sites: src/js/tabs/onboard_logging.js lines 516, 530, 538.
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2467 to 2492 the onDataCallback is
invoked with only (address, payloadView) but the onChunkRead API expects
(chunkAddress, chunkDataView, bytesCompressed); update the call to pass a third
bytesCompressed parameter: use 0 when dataCompressionType === 0 (uncompressed),
and for compressed blocks pass the compressed byte length
(compressedArray.byteLength or dataSize - 2) when dataCompressionType === 1;
ensure the bytesCompressed value is computed before calling onDataCallback and
replace the single invocation accordingly so downstream callers (e.g.
src/js/tabs/onboard_logging.js) receive the correct third argument.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/js/msp/MSPHelper.js (3)
2489-2491: Unify onDataCallback signature to include bytesCompressed (callers expect 3 args).Downstream handlers (e.g., onboard_logging) consume a 3rd parameter. Always pass (address, payloadView, bytesCompressed). See consolidated patch above.
Run to confirm call sites and arity:
#!/bin/bash set -euo pipefail echo "== dataflashRead call sites ==" rg -nP -C3 '\bdataflashRead\s*\(' --type=js src || true echo echo "== handlers expecting a 3rd arg (bytesCompressed/onChunkRead) ==" rg -nP -C3 '\bonChunkRead\b|\bonDataCallback\s*\(' --type=js src || true
2492-2496: Surface CRC as warnings and track occurrences; avoid silent corruption.Use console.warn and increment a counter instead of console.log so the UI can warn users when exporting.
See consolidated patch above (adds FC.DATAFLASH.crcErrorCount and warn logs).
2466-2497: Harden DATAFLASH_READ parsing, surface CRC, and align callback API (avoid crashes on bad frames).Current code can throw on short/CRC‑corrupted frames (unbounded reads; negative lengths; decompression without guards) and the callback shape is inconsistent with callers expecting a 3rd arg (bytesCompressed). Also, CRC is silently tolerated with console.log only.
Apply this consolidated patch:
- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { - let payloadView = null; - - if (response && response.data) { - const headerSize = 7; - const chunkAddress = response.data.readU32(); - const dataSize = response.data.readU16(); - const dataCompressionType = response.data.readU8(); - - if (chunkAddress === address) { - if (dataCompressionType === 0) { - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); - } else if (dataCompressionType === 1) { - const compressedCharCount = response.data.readU16(); - const compressedArray = new Uint8Array(response.data.buffer, response.data.byteOffset + headerSize + 2, dataSize - 2); - const decompressedArray = huffmanDecodeBuf(compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex); - payloadView = new DataView(decompressedArray.buffer); - } - } else { - console.log(`Expected address ${address} but received ${chunkAddress}`); - } - } - - // Deliver payloadView if defined, otherwise pass null - onDataCallback(address, payloadView); - - if (!response || response.crcError) { - console.log(`CRC error or missing data at address ${address} - delivering whatever we got`); - } else if (payloadView) { - console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); - } - }, true); + const ignoreCrc = (this.ignoreDataflashCrc ?? true); + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, (response) => { + let payloadView = null; + let bytesCompressed = 0; + try { + const dv = response && response.data; + if (dv) { + const headerSize = 7; + if (dv.byteLength < headerSize) { + console.warn(`MSP_DATAFLASH_READ: short header at addr=${address} (len=${dv.byteLength})`); + } else { + const chunkAddress = dv.readU32(); + const dataSize = dv.readU16(); + const dataCompressionType = dv.readU8(); + + if (chunkAddress !== address) { + console.warn(`MSP_DATAFLASH_READ: expected addr=${address} but got ${chunkAddress}`); + } else if (dataSize > (dv.byteLength - headerSize)) { + console.warn(`MSP_DATAFLASH_READ: truncated payload at addr=${address} (dataSize=${dataSize}, available=${dv.byteLength - headerSize})`); + } else if (dataCompressionType === 0) { + payloadView = new DataView(dv.buffer, dv.byteOffset + headerSize, dataSize); + bytesCompressed = 0; + } else if (dataCompressionType === 1) { + if (dataSize < 2 || dv.byteLength < headerSize + 2) { + console.warn(`MSP_DATAFLASH_READ: invalid compressed header at addr=${address} (dataSize=${dataSize})`); + } else { + const compressedCharCount = dv.readU16(); + const compressedLen = Math.max(0, Math.min(dataSize - 2, dv.byteLength - headerSize - 2)); + const compressedArray = new Uint8Array(dv.buffer, dv.byteOffset + headerSize + 2, compressedLen); + try { + const decompressedArray = huffmanDecodeBuf( + compressedArray, + compressedCharCount, + defaultHuffmanTree, + defaultHuffmanLenIndex + ); + payloadView = new DataView(decompressedArray.buffer); + bytesCompressed = compressedArray.byteLength; + } catch (e) { + console.warn(`MSP_DATAFLASH_READ: decompression failed at addr=${address}`, e); + } + } + } else { + console.warn(`MSP_DATAFLASH_READ: unknown compression type=${dataCompressionType} at addr=${address}`); + } + } + } + } catch (e) { + console.error(`MSP_DATAFLASH_READ: exception while parsing addr=${address}`, e); + } + + // Always notify; callers expect (address, payloadView, bytesCompressed) + onDataCallback(address, payloadView, bytesCompressed); + + if (!response || response.crcError) { + FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1; + console.warn(`MSP_DATAFLASH_READ: CRC error or missing data at addr=${address} (crcError=${response?.crcError === true})`); + } else if (payloadView) { + console.log(`MSP_DATAFLASH_READ: block at ${address} received (${payloadView.byteLength} bytes)`); + } + }, ignoreCrc);Notes:
- Bounds checks prevent negative/overrun lengths and TDZ‑style throws.
- try/catch around Huffman handles corrupted frames when CRC is ignored.
- console.warn for CRC paths and mismatches; increments FC.DATAFLASH.crcErrorCount for UI surfacing later.
- Keeps current “ignore CRC” behavior but gates it via this.ignoreDataflashCrc.
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2449-2457: Fix ESLint “missing trailing comma” in multiline array.Add a trailing comma after the last element to satisfy comma-dangle.
- 1 // allow compression + 1, // allow compression
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2456-2456: Missing trailing comma.
(comma-dangle)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/js/msp/MSPHelper.js (1)
2485-2494: Don’t silently ignore CRC — track and surface it (opt-in).Keep the experiment, but count CRC occurrences and expose them so the user knows the export may be corrupted. Make this behind a feature flag (e.g., FC.DATAFLASH.ignoreCrc).
If you prefer a minimal add-on, after the onDataCallback line, increment a counter:
+ if (response?.crcError) { + FC.DATAFLASH = FC.DATAFLASH || {}; + FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1; + }And have the save/export UI warn when crcErrorCount > 0.
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2449-2456: Clamp blockSize to protocol’s 16-bit field.Avoid overflow/FC-side rejection by clamping to 0..0xffff before encoding. Included in the unified patch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (3)
src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 Biome (2.1.2)
src/js/msp/MSPHelper.js
[error] 2506-2506: Expected a statement but instead found '}, true)'.
Expected a statement here.
(parse)
🪛 GitHub Actions: Preview Deployment
src/js/msp/MSPHelper.js
[error] 2506-2506: Build failed during 'vite build' (via yarn build): [vite-plugin-pwa:build] MSPHelper.js (2506:4): Expression expected
| const headerSize = 7; | ||
| const chunkAddress = response.data.readU32(); | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| try { | ||
| if (dataCompressionType === 0) { | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| } else if (dataCompressionType === 1) { | ||
| const compressedCharCount = response.data.readU16(); | ||
| const compressedArray = new Uint8Array(response.data.buffer, response.data.byteOffset + headerSize + 2, dataSize - 2); | ||
| const decompressedArray = huffmanDecodeBuf(compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex); | ||
| payloadView = new DataView(decompressedArray.buffer); | ||
| } |
There was a problem hiding this comment.
Bounds-check dataSize and handle unknown compression types.
Prevent negative lengths and OOB views; add an explicit else for unknown types. The unified patch clamps lengths and logs unknown types.
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2470 to 2483, the code assumes dataSize
and compression type are valid which can produce negative lengths or
out-of-bounds DataViews and misses handling unknown compression types; fix by
validating and clamping dataSize to a non-negative integer and ensuring
headerSize and any extra bytes (e.g. compressedCharCount) fit inside
response.data.byteLength before creating a DataView, check that compressedArray
length is non-negative and matches expected bounds, and add an explicit else
branch that logs/throws for unknown dataCompressionType and aborts processing
instead of continuing.
src/js/msp/MSPHelper.js
Outdated
| // always deliver the data, even if addresses mismatch or CRC fails | ||
| onDataCallback(chunkAddress, payloadView); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Unify callback arity with consumers.
Downstream expects (address, payloadView, bytesCompressed). Pass bytesCompressed consistently; include a 4th boolean hasCrcError for optional UI surfacing.
Run to confirm caller expectations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dataflash read consumers =="
rg -nP -C2 '\bonDataCallback\s*\(|\bonChunkRead\b' --type=js
echo
echo "== onboard logging handlers (bytesCompressed usage) =="
rg -nP -C2 'onChunkRead\s*\(|bytesCompressed' src/js/tabs/onboard_logging.js || trueLength of output: 3765
Unify dataflash callback arity to (address, payloadView, bytesCompressed, hasCrcError)
- Always invoke onDataCallback/onChunkRead with four args: (address, payloadView, bytesCompressed|null, hasCrcError:boolean).
- bytesCompressed: numeric when known or null; hasCrcError: !!response.crcError so UI can surface CRC failures.
Relevant spots to change: src/js/msp/MSPHelper.js (calls around ~2462, ~2490, ~2499) and consumer src/js/tabs/onboard_logging.js (onChunkRead at ~492 and dataflashRead calls at ~516, ~530, ~538).
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2462, 2490 and 2499, the dataflash
callbacks currently call onDataCallback/onChunkRead with only (address,
payloadView); update those invocations so they always pass four arguments:
(chunkAddress, payloadView, bytesCompressedOrNull, !!response.crcError) where
bytesCompressedOrNull is the numeric bytesCompressed when available or null
otherwise, and hasCrcError is the boolean coerced from response.crcError; also
update the consumer in src/js/tabs/onboard_logging.js (around lines ~492, ~516,
~530, ~538) to accept the fourth parameter and forward it through dataflashRead
calls so the UI can surface CRC failures.
ctzsnooze
force-pushed
the
IgnoreCRCforDataFLashReadss
branch
from
d3a405d to
4678895
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/js/msp/MSPHelper.js (2)
2475-2497: Add bounds checking for buffer operationsThe code doesn't validate that the buffer has enough data before reading, which could cause exceptions if the response is malformed.
Add validation before reading from the buffer:
if (response && response.data) { const headerSize = 7; + // Validate minimum buffer size + if (response.data.byteLength < headerSize) { + console.error(`Invalid response: buffer too small (${response.data.byteLength} < ${headerSize})`); + onDataCallback(address, null, true); + return; + } + const chunkAddress = response.data.readU32(); const dataSize = response.data.readU16(); const dataCompressionType = response.data.readU8(); + + // Validate data size doesn't exceed buffer + const availableData = response.data.byteLength - headerSize; + if (dataSize > availableData) { + console.warn(`Data size ${dataSize} exceeds available buffer ${availableData}`); + dataSize = availableData; + } if (chunkAddress === address) { try { if (dataCompressionType === 0) { payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); } else if (dataCompressionType === 1) { + if (availableData < 2) { + throw new Error('Insufficient data for compressed payload'); + } const compressedCharCount = response.data.readU16(); const compressedArray = new Uint8Array( response.data.buffer, response.data.byteOffset + headerSize + 2, - dataSize - 2 + Math.min(dataSize - 2, availableData - 2) );
2494-2497: Fallback to raw data delivery masks decompression failuresDelivering raw compressed data when decompression fails will likely result in garbage data being saved. Consider marking this as a critical error.
} catch (e) { - console.warn('Decompression or read failed, delivering raw data anyway'); - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); + console.error('Decompression failed:', e); + // Don't deliver raw compressed data as decompressed - it will be garbage + payloadView = null; + FC.DATAFLASH.decompressionErrorCount = (FC.DATAFLASH.decompressionErrorCount || 0) + 1; }
🧹 Nitpick comments (3)
src/js/msp/MSPHelper.js (3)
2456-2457: Fix trailing comma style inconsistencyThe code has inconsistent trailing commas in the array literal, which violates the project's linting rules.
(blockSize >> 8) & 0xFF, - 1 // allow compression + 1, // allow compression ];
2499-2500: Address mismatch should be treated as an error conditionWhen the received chunk address doesn't match the requested address, the code logs but still delivers null data. This should be treated as a more serious error condition with proper error tracking.
} else { - console.log(`Expected address ${address} but received ${chunkAddress}`); + console.error(`Address mismatch: expected ${address} but received ${chunkAddress}`); + // Track address mismatches as potential corruption indicators + FC.DATAFLASH.addressMismatchCount = (FC.DATAFLASH.addressMismatchCount || 0) + 1; }
2503-2510: Consider making CRC error bypass configurableThis is currently a hard-coded workaround. Consider making it a user-configurable option so users can choose between strict CRC checking and best-effort recovery.
Add a configuration option to control CRC bypass behavior:
// Add to FC configuration or as a module-level option FC.DATAFLASH.ignoreCrcErrors = false; // Default to strict checking // Then in the code: if (!response || response.crcError) { if (FC.DATAFLASH.ignoreCrcErrors) { console.warn(`CRC error at address ${address} - ignoring as configured`); // Continue with data delivery } else { console.error(`CRC error at address ${address} - data rejected`); onDataCallback(address, null, true); return; } }Would you like me to help implement a UI toggle for this option in the onboard logging tab?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2456-2456: Missing trailing comma.
(comma-dangle)
[error] 2484-2485: Missing trailing comma.
(comma-dangle)
[error] 2490-2491: Missing trailing comma.
(comma-dangle)
🔇 Additional comments (2)
src/js/msp/MSPHelper.js (2)
2448-2512: Critical: CRC bypass without user awareness — track errors and propagate them to callersRepo search for callers returned no results (the provided rg produced no output); do not change the onDataCallback signature until all call sites are located and updated. Either implement global tracking (FC.DATAFLASH.crcErrorCount / crcErrorAddresses) and keep onDataCallback(address, payloadView), or pass hasCrcError as a third arg and update every caller in the repo (run: rg -n "\bdataflashRead\s*(" to list call sites). File: src/js/msp/MSPHelper.js (dataflashRead).
2511-2511: Pass doCallbackOnError parameter correctlyThe trailing
trueshould be passed as the 5th argument (after the callback) to enable callback-on-CRC errors. I couldn't locate thesend_messagesignature in the repo — verifysend_message's parameter order and either move the boolean to the 5th parameter or confirm the current ordering is correct.Location: src/js/msp/MSPHelper.js:2511
ctzsnooze
force-pushed
the
IgnoreCRCforDataFLashReadss
branch
from
308b621 to
b901fa1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/js/tabs/onboard_logging.js (2)
542-542: Fix indentation to match project standards.The
startTimedeclaration and the entireonChunkReadfunction have incorrect indentation according to the ESLint configuration.Apply this formatting fix:
-const startTime = new Date().getTime(); + const startTime = new Date().getTime();Also ensure the
onChunkReadfunction is properly indented to match the surrounding code context (20 spaces base indentation).
492-540: Consider adding safeguards for infinite loops.The forward-progress strategy may still result in infinite loops if
nextAddressdoesn't advance properly or ifmaxBytesis miscalculated. Consider adding a maximum retry counter or timeout mechanism.Add a retry counter to prevent infinite loops:
let retryCount = 0; const MAX_RETRIES = 1000; // Adjust based on expected file sizes function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { if (++retryCount > MAX_RETRIES) { console.error(`Maximum retries (${MAX_RETRIES}) exceeded. Terminating download.`); mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); FileSystem.closeFile(openedFile); return; } // ... existing logic }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (1)
src/js/msp/MSPHelper.js (1)
chunkAddress(2471-2471)
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 24 spaces but found 4.
(indent)
[error] 494-494: Expected indentation of 28 spaces but found 8.
(indent)
[error] 495-495: Expected indentation of 28 spaces but found 8.
(indent)
[error] 497-497: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 32 spaces but found 12.
(indent)
[error] 499-499: Expected indentation of 28 spaces but found 8.
(indent)
[error] 500-500: Expected indentation of 32 spaces but found 12.
(indent)
[error] 501-501: Expected indentation of 28 spaces but found 8.
(indent)
[error] 503-503: Expected indentation of 28 spaces but found 8.
(indent)
[error] 505-505: Expected indentation of 28 spaces but found 8.
(indent)
[error] 506-506: Expected indentation of 28 spaces but found 8.
(indent)
[error] 507-507: Expected indentation of 32 spaces but found 12.
(indent)
[error] 508-508: Expected indentation of 36 spaces but found 16.
(indent)
[error] 509-509: Expected indentation of 36 spaces but found 16.
(indent)
[error] 510-510: Expected indentation of 32 spaces but found 12.
(indent)
[error] 511-511: Expected indentation of 36 spaces but found 16.
(indent)
[error] 512-512: Expected indentation of 32 spaces but found 12.
(indent)
[error] 513-513: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 24 spaces but found 4.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 522-522: Expected indentation of 28 spaces but found 8.
(indent)
[error] 524-524: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 32 spaces but found 12.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 32 spaces but found 12.
(indent)
[error] 529-529: Expected indentation of 28 spaces but found 8.
(indent)
[error] 530-530: Expected indentation of 32 spaces but found 12.
(indent)
[error] 531-531: Expected indentation of 28 spaces but found 8.
(indent)
[error] 533-533: Expected indentation of 28 spaces but found 8.
(indent)
[error] 534-534: Expected indentation of 32 spaces but found 12.
(indent)
[error] 535-535: Expected indentation of 32 spaces but found 12.
(indent)
[error] 536-536: Expected indentation of 28 spaces but found 8.
(indent)
[error] 537-537: Expected indentation of 32 spaces but found 12.
(indent)
[error] 538-538: Expected indentation of 28 spaces but found 8.
(indent)
[error] 539-539: Expected indentation of 24 spaces but found 4.
(indent)
[error] 540-540: Expected indentation of 20 spaces but found 0.
(indent)
[error] 542-542: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
492-540: Verify MSPHelper.js supports the new callback-based approach
onChunkRead (src/js/tabs/onboard_logging.js:492–540) expects (chunkAddress, chunkDataView, bytesCompressed) from mspHelper.dataflashRead — confirm MSPHelper invokes the callback with those three arguments and that all callers/exports are updated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/js/tabs/onboard_logging.js (2)
496-505: Good: write-before-advance race fixedAwaiting FileSystem.writeChunck before incrementing nextAddress resolves the earlier double-write/advance ordering issue.
Also applies to: 500-505
492-528: Unreachable CRC branch and unused chunkAddress — implement explicit mismatch handling and simplify flowThe else-branch assumes “CRC/address mismatch” but is only reachable when chunkDataView is falsy. The nested check “if (chunkDataView && chunkDataView.byteLength > 0)” inside this else is therefore dead code. Also, chunkAddress is never used, so you cannot actually detect address mismatches.
Refactor to:
- Detect mismatch via chunkAddress !== nextAddress.
- Write-and-advance when bytes are present (logging if mismatch).
- Skip by blockSize only when no data arrived.
Apply this diff within onChunkRead:
-async function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { +async function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { try { - // Handle received data if any - if (chunkDataView && chunkDataView.byteLength > 0) { - // Write the chunk first - const blob = new Blob([chunkDataView]); - await FileSystem.writeChunck(openedFile, blob); - - // Only advance nextAddress after write completes - nextAddress += chunkDataView.byteLength; - - // Update progress bar - $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); - - // Track compressed bytes if valid - if (isNaN(bytesCompressed) || isNaN(totalBytesCompressed)) { - totalBytesCompressed = null; - } else { - totalBytesCompressed += bytesCompressed; - } - } else if (chunkDataView && chunkDataView.byteLength === 0) { - // Zero-length chunk → treat as EOF - nextAddress = maxBytes; // ensure we stop - } else { - // CRC or address mismatch - console.warn(`Skipping block at address ${nextAddress} due to CRC or mismatch`); - - // If some data arrived, write it anyway - if (chunkDataView && chunkDataView.byteLength > 0) { - const blob = new Blob([chunkDataView]); - await FileSystem.writeChunck(openedFile, blob); - nextAddress += chunkDataView.byteLength; - } else { - // Skip the block size to keep address in sync - nextAddress += self.blockSize; - } - } + const hasBytes = !!(chunkDataView && chunkDataView.byteLength > 0); + const mismatch = typeof chunkAddress === "number" && chunkAddress !== nextAddress; + + if (hasBytes) { + if (mismatch) { + console.warn(`CRC/address mismatch: expected ${nextAddress}, got ${chunkAddress}; writing ${chunkDataView.byteLength} bytes anyway`); + } + const blob = new Blob([chunkDataView]); + await FileSystem.writeChunck(openedFile, blob); + nextAddress += chunkDataView.byteLength; + $(".dataflash-saving progress").attr("value", Math.min(100, (nextAddress / maxBytes) * 100)); + if (Number.isFinite(bytesCompressed)) { + totalBytesCompressed = Number.isFinite(totalBytesCompressed) + ? totalBytesCompressed + bytesCompressed + : bytesCompressed; + } else { + totalBytesCompressed = Number.NaN; + } + } else if (chunkDataView && chunkDataView.byteLength === 0) { + // Zero-length chunk → treat as EOF + nextAddress = maxBytes; + } else { + // No data with mismatch → skip ahead by block size + console.warn(`CRC/address mismatch at ${nextAddress}; no data received. Skipping ${self.blockSize} bytes.`); + nextAddress += self.blockSize; + }
🧹 Nitpick comments (2)
src/js/tabs/onboard_logging.js (2)
547-552: Avoid potential TDZ on startTime — declare before using in onChunkReadIf mspHelper.dataflashRead ever invokes the callback synchronously, referencing startTime inside onChunkRead before its initialization would throw. Move startTime above onChunkRead for safety.
Apply this diff around the declarations:
-async function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { +const startTime = Date.now(); +async function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { @@ -} - -const startTime = new Date().getTime(); +}Please confirm dataflashRead is strictly async (never calling back synchronously). If unsure, the above change is required.
506-511: Compression stats can log Infinity due to null + isNaN coercion — use Number.isFinite and avoid divide-by-zeroSetting totalBytesCompressed = null and then checking with global isNaN can pass the check and produce a division by zero. Switch to Number.isFinite and ensure the denominator > 0 at log time.
Apply these diffs:
- // Track compressed bytes if valid - if (isNaN(bytesCompressed) || isNaN(totalBytesCompressed)) { - totalBytesCompressed = null; - } else { - totalBytesCompressed += bytesCompressed; - } + // Track compressed bytes if valid; otherwise mark as NaN + if (Number.isFinite(bytesCompressed)) { + totalBytesCompressed = Number.isFinite(totalBytesCompressed) + ? totalBytesCompressed + bytesCompressed + : bytesCompressed; + } else { + totalBytesCompressed = Number.NaN; + }- if (!isNaN(totalBytesCompressed)) { + if (Number.isFinite(totalBytesCompressed) && totalBytesCompressed > 0) { console.log( "Compressed into", totalBytesCompressed, "bytes with mean compression factor of", - totalBytes / totalBytesCompressed, + (totalBytes / totalBytesCompressed).toFixed(2), ); }Also applies to: 448-455
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 24 spaces but found 4.
(indent)
[error] 494-494: Expected indentation of 28 spaces but found 8.
(indent)
[error] 495-495: Expected indentation of 28 spaces but found 8.
(indent)
[error] 496-496: Expected indentation of 32 spaces but found 12.
(indent)
[error] 497-497: Expected indentation of 32 spaces but found 12.
(indent)
[error] 498-498: Expected indentation of 32 spaces but found 12.
(indent)
[error] 500-500: Expected indentation of 32 spaces but found 12.
(indent)
[error] 501-501: Expected indentation of 32 spaces but found 12.
(indent)
[error] 503-503: Expected indentation of 32 spaces but found 12.
(indent)
[error] 504-504: Expected indentation of 32 spaces but found 12.
(indent)
[error] 506-506: Expected indentation of 32 spaces but found 12.
(indent)
[error] 507-507: Expected indentation of 32 spaces but found 12.
(indent)
[error] 508-508: Expected indentation of 36 spaces but found 16.
(indent)
[error] 509-509: Expected indentation of 32 spaces but found 12.
(indent)
[error] 510-510: Expected indentation of 36 spaces but found 16.
(indent)
[error] 511-511: Expected indentation of 32 spaces but found 12.
(indent)
[error] 512-512: Expected indentation of 28 spaces but found 8.
(indent)
[error] 513-513: Expected indentation of 32 spaces but found 12.
(indent)
[error] 514-514: Expected indentation of 32 spaces but found 12.
(indent)
[error] 515-515: Expected indentation of 28 spaces but found 8.
(indent)
[error] 516-516: Expected indentation of 32 spaces but found 12.
(indent)
[error] 517-517: Expected indentation of 32 spaces but found 12.
(indent)
[error] 519-519: Expected indentation of 32 spaces but found 12.
(indent)
[error] 520-520: Expected indentation of 32 spaces but found 12.
(indent)
[error] 521-521: Expected indentation of 36 spaces but found 16.
(indent)
[error] 522-522: Expected indentation of 36 spaces but found 16.
(indent)
[error] 523-523: Expected indentation of 36 spaces but found 16.
(indent)
[error] 524-524: Expected indentation of 32 spaces but found 12.
(indent)
[error] 525-525: Expected indentation of 36 spaces but found 16.
(indent)
[error] 526-526: Expected indentation of 36 spaces but found 16.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 28 spaces but found 8.
(indent)
[error] 530-530: Expected indentation of 28 spaces but found 8.
(indent)
[error] 531-531: Expected indentation of 28 spaces but found 8.
(indent)
[error] 532-532: Expected indentation of 32 spaces but found 12.
(indent)
[error] 533-533: Expected indentation of 32 spaces but found 12.
(indent)
[error] 534-534: Expected indentation of 32 spaces but found 12.
(indent)
[error] 535-535: Expected indentation of 28 spaces but found 8.
(indent)
[error] 537-537: Expected indentation of 28 spaces but found 8.
(indent)
[error] 538-538: Expected indentation of 28 spaces but found 8.
(indent)
[error] 540-540: Expected indentation of 24 spaces but found 4.
(indent)
[error] 541-541: Expected indentation of 28 spaces but found 8.
(indent)
[error] 542-542: Expected indentation of 28 spaces but found 8.
(indent)
[error] 543-543: Expected indentation of 28 spaces but found 8.
(indent)
[error] 544-544: Expected indentation of 24 spaces but found 4.
(indent)
[error] 545-545: Expected indentation of 20 spaces but found 0.
(indent)
[error] 547-547: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
492-545: Indentation errors auto-fixed — no action required.
Ran eslint --fix; the onChunkRead indentation in src/js/tabs/onboard_logging.js was auto-fixed and the linter now reports no errors for that file (only unrelated warnings remain).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/js/tabs/onboard_logging.js (5)
520-533: Unreachable salvage logic in CRC/mismatch branch; simplify to deterministic skipIn the else-branch you only enter when chunkDataView === null, so the inner “if (chunkDataView && …)” can never run. This is confusing and suggests a missed case. If CRC/mismatch is signaled by null, just advance by blockSize.
- // If some data arrived, write it anyway - if (chunkDataView && chunkDataView.byteLength > 0) { - const blob = new Blob([chunkDataView]); - FileSystem.writeChunck(openedFile, blob); - nextAddress += chunkDataView.byteLength; - } else { - // No data → just skip block size - nextAddress += self.blockSize; - } + // No data returned → skip one block + nextAddress += self.blockSize;
534-539: Reduce duplication: factor finalize/continue into a helper to keep both branches consistentThe finalize/continue logic is duplicated here and above; extract a tiny helper to avoid future drift.
- if (nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); - } + const continueOrFinalize = () => { + if (nextAddress >= maxBytes || saveCancelled) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + }; + continueOrFinalize();
492-492: Optional: short‑circuit on cancel before doing any workEarly-exit keeps behavior predictable if the user cancelled between requests.
function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + if (saveCancelled) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + return; + }
543-543: Move startTime above onChunkRead for clarity (avoid TDZ worries for readers)It’s safe as-is, but initializing startTime before declaring onChunkRead improves readability.
-const startTime = new Date().getTime(); +const startTime = new Date().getTime(); // move this just before onChunkRead()
492-541: Indentation deviates from ESLint (indent) across this blockStatic analysis flags many indent errors here. Please run the formatter (or fix indentation) to keep diffs clean.
Example: npm run lint -- --fix
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 24 spaces but found 4.
(indent)
[error] 494-494: Expected indentation of 28 spaces but found 8.
(indent)
[error] 495-495: Expected indentation of 28 spaces but found 8.
(indent)
[error] 496-496: Expected indentation of 32 spaces but found 12.
(indent)
[error] 497-497: Expected indentation of 32 spaces but found 12.
(indent)
[error] 498-498: Expected indentation of 36 spaces but found 16.
(indent)
[error] 500-500: Expected indentation of 36 spaces but found 16.
(indent)
[error] 501-501: Expected indentation of 40 spaces but found 20.
(indent)
[error] 502-502: Expected indentation of 36 spaces but found 16.
(indent)
[error] 503-503: Expected indentation of 40 spaces but found 20.
(indent)
[error] 504-504: Expected indentation of 36 spaces but found 16.
(indent)
[error] 506-506: Expected indentation of 36 spaces but found 16.
(indent)
[error] 508-508: Expected indentation of 36 spaces but found 16.
(indent)
[error] 509-509: Expected indentation of 40 spaces but found 20.
(indent)
[error] 510-510: Expected indentation of 40 spaces but found 20.
(indent)
[error] 511-511: Expected indentation of 36 spaces but found 16.
(indent)
[error] 512-512: Expected indentation of 40 spaces but found 20.
(indent)
[error] 513-513: Expected indentation of 36 spaces but found 16.
(indent)
[error] 514-514: Expected indentation of 32 spaces but found 12.
(indent)
[error] 515-515: Expected indentation of 28 spaces but found 8.
(indent)
[error] 516-516: Expected indentation of 32 spaces but found 12.
(indent)
[error] 517-517: Expected indentation of 32 spaces but found 12.
(indent)
[error] 518-518: Expected indentation of 32 spaces but found 12.
(indent)
[error] 519-519: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 522-522: Expected indentation of 28 spaces but found 8.
(indent)
[error] 524-524: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 32 spaces but found 12.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 32 spaces but found 12.
(indent)
[error] 529-529: Expected indentation of 28 spaces but found 8.
(indent)
[error] 530-530: Expected indentation of 32 spaces but found 12.
(indent)
[error] 531-531: Expected indentation of 32 spaces but found 12.
(indent)
[error] 532-532: Expected indentation of 28 spaces but found 8.
(indent)
[error] 534-534: Expected indentation of 28 spaces but found 8.
(indent)
[error] 535-535: Expected indentation of 32 spaces but found 12.
(indent)
[error] 536-536: Expected indentation of 32 spaces but found 12.
(indent)
[error] 537-537: Expected indentation of 28 spaces but found 8.
(indent)
[error] 538-538: Expected indentation of 32 spaces but found 12.
(indent)
[error] 539-539: Expected indentation of 28 spaces but found 8.
(indent)
[error] 540-540: Expected indentation of 24 spaces but found 4.
(indent)
[error] 541-541: Expected indentation of 20 spaces but found 0.
(indent)
[error] 543-543: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (2)
src/js/tabs/onboard_logging.js (2)
516-519: EOF handling is clear and correctZero-length block → finalize and close is a sensible termination path.
500-504: Accounting logic is fine; totals nulling is a reasonable sentinelCarrying totals until an unknown/NaN arrives, then switching to null sentinel is acceptable.
If desired, verify that UI code tolerates null for totalBytesCompressed in any downstream display.
|
CODERABBITAI I think this now works better than any of the intermediary versions. It hasn't hung in the last 8 tries on a quad wqhich always hung. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/js/tabs/onboard_logging.js (2)
499-518: Add .catch to FileSystem.writeChunck to avoid unhandled rejections.A failed write would stall the flow. The refactor above includes .catch for both writes. If you prefer a minimal change, wrap both calls with .catch that logs and continues.
Also applies to: 549-552
494-559: Zero-byte chunk wrongly treated as EOF; unify mismatch/empty handling, add error handling, and honor cancel.A zero-length payload can occur for reasons other than EOF; finalizing immediately risks truncation. Also, writes lack .catch, progress isn’t updated on skip paths, and cancel isn’t checked in the CRC/mismatch flow. The branch that tries to write in the “mismatch” path is effectively dead because the
elsecurrently only triggers forchunkDataView === null.Apply this refactor to make forward progress reliable and consistent:
-function onChunkRead(chunkAddress, chunkDataView, bytesCompressed, attempt = 1) { - if (chunkDataView !== null) { - // Write whatever we received, even if block is partially bad - if (chunkDataView.byteLength > 0) { - const blob = new Blob([chunkDataView]); - FileSystem.writeChunck(openedFile, blob).then(() => { - nextAddress += chunkDataView.byteLength; - - if (!isNaN(bytesCompressed) && totalBytesCompressed !== null) { - totalBytesCompressed += bytesCompressed; - } else { - totalBytesCompressed = null; - } - - $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); - - if (saveCancelled || nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, (addr, dataView, bytes) => - onChunkRead(addr, dataView, bytes) - ); - } - }); - } else { - // Zero-byte block → end-of-file - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } - } else { - // Block failed CRC or mismatch - if (attempt < MAX_RETRIES) { - console.warn(`Retrying block at ${nextAddress} (attempt ${attempt})`); - mspHelper.dataflashRead(nextAddress, self.blockSize, (addr, dataView, bytes) => - onChunkRead(addr, dataView, bytes, attempt + 1) - ); - return; - } - - console.error(`Skipping block at address ${nextAddress} after ${MAX_RETRIES} retries`); - - const proceedNext = () => { - if (nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, (addr, dataView, bytes) => - onChunkRead(addr, dataView, bytes) - ); - } - }; - - if (chunkDataView && chunkDataView.byteLength > 0) { - const blob = new Blob([chunkDataView]); - FileSystem.writeChunck(openedFile, blob).then(() => { - nextAddress += chunkDataView.byteLength; - proceedNext(); - }); - } else { - // No data → just skip block size - nextAddress += self.blockSize; - proceedNext(); - } - } -} +function onChunkRead(chunkAddress, chunkDataView, bytesCompressed, attempt = 1) { + const hasBytes = !!(chunkDataView && chunkDataView.byteLength > 0); + + const continueOrFinalize = () => { + $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead( + nextAddress, + self.blockSize, + (addr, dataView, bytes) => onChunkRead(addr, dataView, bytes), + ); + } + }; + + if (hasBytes) { + // Write whatever we received, even if partially corrupted + const blob = new Blob([chunkDataView]); + const afterWrite = () => { + nextAddress += chunkDataView.byteLength; + if (!isNaN(bytesCompressed) && totalBytesCompressed !== null) { + totalBytesCompressed += bytesCompressed; + } else { + totalBytesCompressed = null; + } + continueOrFinalize(); + }; + FileSystem.writeChunck(openedFile, blob) + .then(afterWrite) + .catch((err) => { + console.error("File write failed:", err); + afterWrite(); // keep forward progress + }); + return; + } + + // No bytes or null: treat as CRC/mismatch/empty block (not EOF) + if (attempt < MAX_RETRIES) { + console.warn(`Retrying block at ${chunkAddress} (attempt ${attempt})`); + mspHelper.dataflashRead( + nextAddress, + self.blockSize, + (addr, dataView, bytes) => onChunkRead(addr, dataView, bytes, attempt + 1), + ); + return; + } + + console.error(`Skipping block at address ${chunkAddress} after ${MAX_RETRIES} retries`); + nextAddress += self.blockSize; // skip when nothing usable arrived + continueOrFinalize(); +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 494-494: Expected indentation of 20 spaces but found 0.
(indent)
[error] 495-495: Expected indentation of 24 spaces but found 4.
(indent)
[error] 496-496: Expected indentation of 28 spaces but found 8.
(indent)
[error] 497-497: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 32 spaces but found 12.
(indent)
[error] 499-499: Expected indentation of 32 spaces but found 12.
(indent)
[error] 500-500: Expected indentation of 36 spaces but found 16.
(indent)
[error] 502-502: Expected indentation of 36 spaces but found 16.
(indent)
[error] 503-503: Expected indentation of 40 spaces but found 20.
(indent)
[error] 504-504: Expected indentation of 36 spaces but found 16.
(indent)
[error] 505-505: Expected indentation of 40 spaces but found 20.
(indent)
[error] 506-506: Expected indentation of 36 spaces but found 16.
(indent)
[error] 508-508: Expected indentation of 36 spaces but found 16.
(indent)
[error] 510-510: Expected indentation of 36 spaces but found 16.
(indent)
[error] 511-511: Expected indentation of 40 spaces but found 20.
(indent)
[error] 512-512: Expected indentation of 40 spaces but found 20.
(indent)
[error] 513-513: Expected indentation of 36 spaces but found 16.
(indent)
[error] 514-514: Expected indentation of 40 spaces but found 20.
(indent)
[error] 515-515: Expected indentation of 44 spaces but found 24.
(indent)
[error] 515-516: Missing trailing comma.
(comma-dangle)
[error] 516-516: Expected indentation of 40 spaces but found 20.
(indent)
[error] 517-517: Expected indentation of 36 spaces but found 16.
(indent)
[error] 518-518: Expected indentation of 32 spaces but found 12.
(indent)
[error] 519-519: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 32 spaces but found 12.
(indent)
[error] 521-521: Expected indentation of 32 spaces but found 12.
(indent)
[error] 522-522: Expected indentation of 32 spaces but found 12.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 524-524: Expected indentation of 24 spaces but found 4.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 28 spaces but found 8.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 32 spaces but found 12.
(indent)
[error] 529-529: Expected indentation of 36 spaces but found 16.
(indent)
[error] 529-530: Missing trailing comma.
(comma-dangle)
[error] 530-530: Expected indentation of 32 spaces but found 12.
(indent)
[error] 531-531: Expected indentation of 32 spaces but found 12.
(indent)
[error] 532-532: Expected indentation of 28 spaces but found 8.
(indent)
[error] 534-534: Expected indentation of 28 spaces but found 8.
(indent)
[error] 536-536: Expected indentation of 28 spaces but found 8.
(indent)
[error] 537-537: Expected indentation of 32 spaces but found 12.
(indent)
[error] 538-538: Expected indentation of 36 spaces but found 16.
(indent)
[error] 539-539: Expected indentation of 36 spaces but found 16.
(indent)
[error] 540-540: Expected indentation of 32 spaces but found 12.
(indent)
[error] 541-541: Expected indentation of 36 spaces but found 16.
(indent)
[error] 542-542: Expected indentation of 40 spaces but found 20.
(indent)
[error] 542-543: Missing trailing comma.
(comma-dangle)
[error] 543-543: Expected indentation of 36 spaces but found 16.
(indent)
[error] 544-544: Expected indentation of 32 spaces but found 12.
(indent)
[error] 545-545: Expected indentation of 28 spaces but found 8.
(indent)
[error] 547-547: Expected indentation of 28 spaces but found 8.
(indent)
[error] 548-548: Expected indentation of 32 spaces but found 12.
(indent)
[error] 549-549: Expected indentation of 32 spaces but found 12.
(indent)
[error] 550-550: Expected indentation of 36 spaces but found 16.
(indent)
[error] 551-551: Expected indentation of 36 spaces but found 16.
(indent)
[error] 552-552: Expected indentation of 32 spaces but found 12.
(indent)
[error] 553-553: Expected indentation of 28 spaces but found 8.
(indent)
[error] 554-554: Expected indentation of 32 spaces but found 12.
(indent)
[error] 555-555: Expected indentation of 32 spaces but found 12.
(indent)
[error] 556-556: Expected indentation of 32 spaces but found 12.
(indent)
[error] 557-557: Expected indentation of 28 spaces but found 8.
(indent)
[error] 558-558: Expected indentation of 24 spaces but found 4.
(indent)
[error] 559-559: Expected indentation of 20 spaces but found 0.
(indent)
[error] 561-561: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
492-559: No indent/comma-dangle errors remain in this block (autofix run).npm run lint -- --fix produced 0 errors and only 42 warnings (unused vars); no further fixes needed in src/js/tabs/onboard_logging.js (lines 492–559).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/tabs/onboard_logging.js (1)
544-549: Restore access to closure variables.The
onChunkReadfunction is now defined outside the closure but still needs access tosaveCancelled,maxBytes,nextAddress,totalBytesCompressed,openedFile, andstartTime.Move the function definition back inside the
flash_save_beginfunction after these variables are declared:-const startTime = new Date().getTime(); - // Fetch the initial block - FileSystem.openFile(fileWriter).then((file) => { - openedFile = file; - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); - }); + const startTime = new Date().getTime(); + + function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + // Implementation here with access to closure variables + } + + // Fetch the initial block + FileSystem.openFile(fileWriter).then((file) => { + openedFile = file; + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + });
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
492-549: Fix indentation issues throughout the new code.The entire new block of code has incorrect indentation that doesn't match the surrounding code style.
The code should be indented to match the existing indentation level (20 spaces for the base level within the function). Use your editor's auto-format feature or manually adjust the indentation to match the surrounding code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 494-494: Expected indentation of 20 spaces but found 0.
(indent)
[error] 495-495: Expected indentation of 24 spaces but found 4.
(indent)
[error] 496-496: Expected indentation of 28 spaces but found 8.
(indent)
[error] 497-497: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 24 spaces but found 4.
(indent)
[error] 500-500: Expected indentation of 24 spaces but found 4.
(indent)
[error] 501-501: Expected indentation of 24 spaces but found 4.
(indent)
[error] 502-502: Expected indentation of 28 spaces but found 8.
(indent)
[error] 504-504: Expected indentation of 28 spaces but found 8.
(indent)
[error] 505-505: Expected indentation of 28 spaces but found 8.
(indent)
[error] 506-506: Expected indentation of 32 spaces but found 12.
(indent)
[error] 507-507: Expected indentation of 32 spaces but found 12.
(indent)
[error] 508-508: Expected indentation of 28 spaces but found 8.
(indent)
[error] 509-509: Expected indentation of 28 spaces but found 8.
(indent)
[error] 511-511: Expected indentation of 28 spaces but found 8.
(indent)
[error] 512-512: Expected indentation of 28 spaces but found 8.
(indent)
[error] 514-514: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 32 spaces but found 12.
(indent)
[error] 516-516: Expected indentation of 32 spaces but found 12.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 519-519: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 522-522: Expected indentation of 24 spaces but found 4.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 524-524: Expected indentation of 24 spaces but found 4.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 24 spaces but found 4.
(indent)
[error] 527-527: Expected indentation of 20 spaces but found 0.
(indent)
[error] 529-529: Expected indentation of 20 spaces but found 0.
(indent)
[error] 530-530: Expected indentation of 24 spaces but found 4.
(indent)
[error] 531-531: Expected indentation of 28 spaces but found 8.
(indent)
[error] 532-532: Expected indentation of 28 spaces but found 8.
(indent)
[error] 533-533: Expected indentation of 24 spaces but found 4.
(indent)
[error] 534-534: Expected indentation of 28 spaces but found 8.
(indent)
[error] 535-535: Expected indentation of 28 spaces but found 8.
(indent)
[error] 536-536: Expected indentation of 24 spaces but found 4.
(indent)
[error] 537-537: Expected indentation of 20 spaces but found 0.
(indent)
[error] 539-539: Expected indentation of 20 spaces but found 0.
(indent)
[error] 540-540: Expected indentation of 24 spaces but found 4.
(indent)
[error] 541-541: Expected indentation of 24 spaces but found 4.
(indent)
[error] 542-542: Expected indentation of 20 spaces but found 0.
(indent)
[error] 544-544: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
529-537: Remove unused async functions that break the implementation.The
requestBlockfunction references an undefinedStobject and isn't called from anywhere since the mainonChunkReadis no longer async.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/tabs/onboard_logging.js (1)
492-555: Critical: onChunkRead function breaks file saving, has undefined references, and doesn't integrate with the file download flow.The refactored
onChunkReadfunction completely breaks the "Save Flash to File" functionality. Major issues:
- No file writing: The function calls
St.process_data(blockData)(Line 533) but never writes to the opened file- Undefined
Stparameter: The function accepts anStparameter that isn't provided when called from Line 561- Lost progress tracking: No
nextAddressadvancement or progress updates- Broken continuation: Doesn't request the next block after processing
- No completion handling: Never closes the file or shows completion
- Incorrect
checkCRCimplementation: Always returns false since DataView objects don't have acrcValidpropertyThe function needs to be completely restructured. Based on the PR's objective to ignore CRC errors and continue saving despite them, here's the correct implementation that maintains file saving while tolerating CRC errors:
-const activeRetries = new Set(); // Tracks blocks currently retrying -const MAX_RETRIES = 3; // Maximum number of retries per block -const READ_TIMEOUT = 5000; // 5 seconds per block read - -// Main function to process a chunk -async function onChunkRead(St, blockAddr, blockData) { - if (!blockData || blockData.length === 0) { - console.warn(`Empty block at ${blockAddr}, skipping`); - return; - } - - let retries = 0; - - // Retry loop for CRC failures - while (!checkCRC(blockData) && retries < MAX_RETRIES) { - console.warn(`CRC failed at ${blockAddr}, retrying (${retries + 1}/${MAX_RETRIES})`); - - if (activeRetries.has(blockAddr)) { - console.warn(`Already retrying block at ${blockAddr}, skipping`); - return; - } - activeRetries.add(blockAddr); - - const retryBlock = await requestBlock(St, blockAddr); - activeRetries.delete(blockAddr); - - if (!retryBlock) { - console.error(`Retry failed for block at ${blockAddr}`); - break; - } - - blockData = retryBlock; - retries++; - } - - if (!checkCRC(blockData)) { - console.error(`Skipping block at ${blockAddr} after ${retries} retries`); - return; - } - - try { - St.process_data(blockData); - } catch (err) { - console.error(`Processing error at ${blockAddr}:`, err); - } -} - -// Request a block with timeout -async function requestBlock(St, addr) { - try { - return await Promise.race([ - St.dataflashRead(addr), - new Promise((_, reject) => setTimeout(() => reject(new Error("Read timeout")), READ_TIMEOUT)) - ]); - } catch (e) { - console.error(`Request failed for block at ${addr}:`, e); - return null; - } -} - -// CRC check (replace with actual CRC logic) -function checkCRC(block) { - return block?.crcValid === true; -} + function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + // Write whatever data we received, ignoring CRC errors (per PR objective) + if (chunkDataView && chunkDataView.byteLength > 0) { + const blob = new Blob([chunkDataView]); + FileSystem.writeChunck(openedFile, blob).then(() => { + nextAddress += chunkDataView.byteLength; + + if (!isNaN(bytesCompressed)) { + totalBytesCompressed += bytesCompressed; + } + + $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + }).catch((err) => { + console.error("Write failed, continuing anyway:", err); + // Continue forward progress even on write errors + nextAddress += chunkDataView.byteLength; + continueOrFinish(); + }); + } else { + // Skip blocks with no data (could be CRC error or address mismatch) + console.warn(`No data at address ${nextAddress}, skipping block`); + nextAddress += self.blockSize; + continueOrFinish(); + } + + function continueOrFinish() { + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 20 spaces but found 0.
(indent)
[error] 494-494: Expected indentation of 20 spaces but found 0.
(indent)
[error] 496-496: Expected indentation of 20 spaces but found 0.
(indent)
[error] 497-497: Expected indentation of 20 spaces but found 0.
(indent)
[error] 498-498: Expected indentation of 24 spaces but found 4.
(indent)
[error] 499-499: Expected indentation of 28 spaces but found 8.
(indent)
[error] 500-500: Expected indentation of 28 spaces but found 8.
(indent)
[error] 501-501: Expected indentation of 24 spaces but found 4.
(indent)
[error] 503-503: Expected indentation of 24 spaces but found 4.
(indent)
[error] 505-505: Expected indentation of 24 spaces but found 4.
(indent)
[error] 506-506: Expected indentation of 24 spaces but found 4.
(indent)
[error] 507-507: Expected indentation of 28 spaces but found 8.
(indent)
[error] 509-509: Expected indentation of 28 spaces but found 8.
(indent)
[error] 510-510: Expected indentation of 32 spaces but found 12.
(indent)
[error] 511-511: Expected indentation of 32 spaces but found 12.
(indent)
[error] 512-512: Expected indentation of 28 spaces but found 8.
(indent)
[error] 513-513: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 28 spaces but found 8.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 28 spaces but found 8.
(indent)
[error] 519-519: Expected indentation of 32 spaces but found 12.
(indent)
[error] 520-520: Expected indentation of 32 spaces but found 12.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 524-524: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 24 spaces but found 4.
(indent)
[error] 527-527: Expected indentation of 24 spaces but found 4.
(indent)
[error] 528-528: Expected indentation of 28 spaces but found 8.
(indent)
[error] 529-529: Expected indentation of 28 spaces but found 8.
(indent)
[error] 530-530: Expected indentation of 24 spaces but found 4.
(indent)
[error] 532-532: Expected indentation of 24 spaces but found 4.
(indent)
[error] 533-533: Expected indentation of 28 spaces but found 8.
(indent)
[error] 534-534: Expected indentation of 24 spaces but found 4.
(indent)
[error] 535-535: Expected indentation of 28 spaces but found 8.
(indent)
[error] 536-536: Expected indentation of 24 spaces but found 4.
(indent)
[error] 537-537: Expected indentation of 20 spaces but found 0.
(indent)
[error] 539-539: Expected indentation of 20 spaces but found 0.
(indent)
[error] 540-540: Expected indentation of 20 spaces but found 0.
(indent)
[error] 541-541: Expected indentation of 24 spaces but found 4.
(indent)
[error] 542-542: Expected indentation of 28 spaces but found 8.
(indent)
[error] 543-543: Expected indentation of 32 spaces but found 12.
(indent)
[error] 544-544: Expected indentation of 32 spaces but found 12.
(indent)
[error] 544-545: Missing trailing comma.
(comma-dangle)
[error] 545-545: Expected indentation of 28 spaces but found 8.
(indent)
[error] 546-546: Expected indentation of 24 spaces but found 4.
(indent)
[error] 547-547: Expected indentation of 28 spaces but found 8.
(indent)
[error] 548-548: Expected indentation of 28 spaces but found 8.
(indent)
[error] 549-549: Expected indentation of 24 spaces but found 4.
(indent)
[error] 550-550: Expected indentation of 20 spaces but found 0.
(indent)
[error] 552-552: Expected indentation of 20 spaces but found 0.
(indent)
[error] 553-553: Expected indentation of 20 spaces but found 0.
(indent)
[error] 554-554: Expected indentation of 24 spaces but found 4.
(indent)
[error] 555-555: Expected indentation of 20 spaces but found 0.
(indent)
[error] 557-557: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
557-557: Minor: Use Date.now() consistentlyGood modernization using
Date.now()instead ofnew Date().getTime().
src/js/tabs/onboard_logging.js
Outdated
| const activeRetries = new Set(); // Tracks blocks currently retrying | ||
| const MAX_RETRIES = 3; // Maximum number of retries per block | ||
| const READ_TIMEOUT = 5000; // 5 seconds per block read | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect indentation throughout the function
The entire function block has incorrect indentation that violates the project's style guide.
Apply proper indentation (20 spaces for the function body as per ESLint configuration):
-const activeRetries = new Set(); // Tracks blocks currently retrying
+ const activeRetries = new Set(); // Tracks blocks currently retryingCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 20 spaces but found 0.
(indent)
[error] 494-494: Expected indentation of 20 spaces but found 0.
(indent)
🤖 Prompt for AI Agents
In src/js/tabs/onboard_logging.js around lines 492 to 495, the block declaring
activeRetries, MAX_RETRIES and READ_TIMEOUT is mis-indented; reformat the
surrounding function so every line in the function body is indented with 20
spaces as required by the ESLint config, ensuring nested blocks keep relative
indentation, update any preceding or trailing lines that break the 20-space
rule, and run the linter to confirm the indentation style is satisfied.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/js/tabs/onboard_logging.js (2)
506-508: Remove the placeholder CRC check that will always fail.The
checkCRCfunction checks forblock.crcValidwhich doesn't exist on DataView objects returned by MSPHelper. Since the PR's objective is to ignore CRC errors (not validate them), this function should be removed entirely.
492-562: Critical: The refactoredonChunkReadbreaks the core "Save Flash to File" functionality.The new implementation completely removes file writing capability, breaking the PR's stated objective of making "Save Flash to File" more reliable. Key issues:
- No file writing: Calls
St.process_data(blockData)but never writes to the opened file- Undefined reference:
Stis not defined anywhere in the codebase- Lost context: Variables like
openedFile,saveCancelled,maxBytes,nextAddress, andtotalBytesCompressedare not accessible- No progress tracking: Doesn't advance
nextAddressor request the next block- No completion handling: Doesn't check for completion or close the file
The function needs to maintain the original file-saving workflow while incorporating the CRC-ignore logic. Apply this fix:
-const activeRetries = new Set(); // Tracks blocks currently retrying -const MAX_RETRIES = 3; // Maximum number of retries per block - -// Safely get length of any block type -function getBlockLength(block) { - if (!block) return 0; - if (block instanceof DataView) return block.byteLength; - if (ArrayBuffer.isView(block)) return block.byteLength; - if (block instanceof ArrayBuffer) return block.byteLength; - if (Array.isArray(block)) return block.length; - return 0; -} - -// CRC check placeholder -function checkCRC(block) { - return block && block.crcValid === true; // adjust to real CRC logic -} - -// Request a block -async function requestBlock(St, addr) { - try { - const result = await St.dataflashRead(addr); - return result; - } catch (e) { - console.error(`Request failed for block at ${addr}:`, e); - return null; - } -} - -// Main chunk reader -async function onChunkRead(St, blockAddr, blockData) { - const len = getBlockLength(blockData); - if (!blockData || len === 0) { - console.warn(`Block at ${blockAddr} has zero length, skipping`); - return; - } - - let retries = 0; - - while (!checkCRC(blockData) && retries < MAX_RETRIES) { - console.warn(`CRC failed at ${blockAddr}, retrying (${retries + 1}/${MAX_RETRIES})`); - - if (activeRetries.has(blockAddr)) { - console.warn(`Already retrying block at ${blockAddr}, skipping`); - return; - } - - activeRetries.add(blockAddr); - const retryBlock = await requestBlock(St, blockAddr); - activeRetries.delete(blockAddr); - - if (!retryBlock || getBlockLength(retryBlock) === 0) { - console.error(`Retry failed or returned empty for block at ${blockAddr}`); - break; - } - - blockData = retryBlock; - retries++; - } - - if (!checkCRC(blockData)) { - console.error(`Skipping block at ${blockAddr} after ${retries} retries`); - return; - } - - try { - St.process_data(blockData); - } catch (err) { - console.error(`Processing error at ${blockAddr}:`, err); - } -} + function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + // PR objective: Ignore CRC errors and write whatever data we receive + if (chunkDataView && chunkDataView.byteLength > 0) { + const blob = new Blob([chunkDataView]); + FileSystem.writeChunck(openedFile, blob).then(() => { + nextAddress += chunkDataView.byteLength; + + if (!isNaN(bytesCompressed)) { + totalBytesCompressed += bytesCompressed; + } + + $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + }).catch((err) => { + console.error("File write failed:", err); + // Continue anyway to maintain forward progress (ignore CRC/write errors) + nextAddress += chunkDataView.byteLength; + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + }); + } else { + // Skip blocks with no data (CRC errors return null/empty) + console.warn(`Skipping block at address ${nextAddress} due to CRC error or no data`); + nextAddress += self.blockSize; + + if (nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + } + }
🧹 Nitpick comments (2)
src/js/tabs/onboard_logging.js (2)
492-503: Fix incorrect indentation throughout the function.The entire function has incorrect indentation that violates the project's ESLint configuration (20 spaces for function body).
-const activeRetries = new Set(); // Tracks blocks currently retrying -const MAX_RETRIES = 3; // Maximum number of retries per block - -// Safely get length of any block type -function getBlockLength(block) { + const activeRetries = new Set(); // Tracks blocks currently retrying + const MAX_RETRIES = 3; // Maximum number of retries per block + + // Safely get length of any block type + function getBlockLength(block) {
564-564: Keep the original inline placement ofstartTime.Moving
startTimeinitialization outside the function changes its scope unnecessarily.-const startTime = new Date().getTime(); + const startTime = new Date().getTime();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 20 spaces but found 0.
(indent)
[error] 495-495: Expected indentation of 20 spaces but found 0.
(indent)
[error] 496-496: Expected indentation of 20 spaces but found 0.
(indent)
[error] 497-497: Expected indentation of 24 spaces but found 4.
(indent)
[error] 498-498: Expected indentation of 24 spaces but found 4.
(indent)
[error] 499-499: Expected indentation of 24 spaces but found 4.
(indent)
[error] 500-500: Expected indentation of 24 spaces but found 4.
(indent)
[error] 501-501: Expected indentation of 24 spaces but found 4.
(indent)
[error] 502-502: Expected indentation of 24 spaces but found 4.
(indent)
[error] 503-503: Expected indentation of 20 spaces but found 0.
(indent)
[error] 505-505: Expected indentation of 20 spaces but found 0.
(indent)
[error] 506-506: Expected indentation of 20 spaces but found 0.
(indent)
[error] 507-507: Expected indentation of 24 spaces but found 4.
(indent)
[error] 508-508: Expected indentation of 20 spaces but found 0.
(indent)
[error] 510-510: Expected indentation of 20 spaces but found 0.
(indent)
[error] 511-511: Expected indentation of 20 spaces but found 0.
(indent)
[error] 512-512: Expected indentation of 24 spaces but found 4.
(indent)
[error] 513-513: Expected indentation of 28 spaces but found 8.
(indent)
[error] 514-514: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 24 spaces but found 4.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 24 spaces but found 4.
(indent)
[error] 519-519: Expected indentation of 20 spaces but found 0.
(indent)
[error] 521-521: Expected indentation of 20 spaces but found 0.
(indent)
[error] 522-522: Expected indentation of 20 spaces but found 0.
(indent)
[error] 523-523: Expected indentation of 24 spaces but found 4.
(indent)
[error] 524-524: Expected indentation of 24 spaces but found 4.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 28 spaces but found 8.
(indent)
[error] 527-527: Expected indentation of 24 spaces but found 4.
(indent)
[error] 529-529: Expected indentation of 24 spaces but found 4.
(indent)
[error] 531-531: Expected indentation of 24 spaces but found 4.
(indent)
[error] 532-532: Expected indentation of 28 spaces but found 8.
(indent)
[error] 534-534: Expected indentation of 28 spaces but found 8.
(indent)
[error] 535-535: Expected indentation of 32 spaces but found 12.
(indent)
[error] 536-536: Expected indentation of 32 spaces but found 12.
(indent)
[error] 537-537: Expected indentation of 28 spaces but found 8.
(indent)
[error] 539-539: Expected indentation of 28 spaces but found 8.
(indent)
[error] 540-540: Expected indentation of 28 spaces but found 8.
(indent)
[error] 541-541: Expected indentation of 28 spaces but found 8.
(indent)
[error] 543-543: Expected indentation of 28 spaces but found 8.
(indent)
[error] 544-544: Expected indentation of 32 spaces but found 12.
(indent)
[error] 545-545: Expected indentation of 32 spaces but found 12.
(indent)
[error] 546-546: Expected indentation of 28 spaces but found 8.
(indent)
[error] 548-548: Expected indentation of 28 spaces but found 8.
(indent)
[error] 549-549: Expected indentation of 28 spaces but found 8.
(indent)
[error] 550-550: Expected indentation of 24 spaces but found 4.
(indent)
[error] 552-552: Expected indentation of 24 spaces but found 4.
(indent)
[error] 553-553: Expected indentation of 28 spaces but found 8.
(indent)
[error] 554-554: Expected indentation of 28 spaces but found 8.
(indent)
[error] 555-555: Expected indentation of 24 spaces but found 4.
(indent)
[error] 557-557: Expected indentation of 24 spaces but found 4.
(indent)
[error] 558-558: Expected indentation of 28 spaces but found 8.
(indent)
[error] 559-559: Expected indentation of 24 spaces but found 4.
(indent)
[error] 560-560: Expected indentation of 28 spaces but found 8.
(indent)
[error] 561-561: Expected indentation of 24 spaces but found 4.
(indent)
[error] 562-562: Expected indentation of 20 spaces but found 0.
(indent)
[error] 564-564: Expected indentation of 20 spaces but found 0.
(indent)
ctzsnooze
force-pushed
the
IgnoreCRCforDataFLashReadss
branch
from
834d083 to
d0471ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/js/tabs/onboard_logging.js (2)
515-520: Zero-length payload ≠ EOF — this can truncate dumpsTreating byteLength === 0 as EOF may stop early on CRC/timeouts. Handle as error/skip instead.
- } else if (chunkDataView && chunkDataView.byteLength === 0) { - // Zero-length block → EOF - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); + } else if (chunkDataView && chunkDataView.byteLength === 0) { + // Zero-length delivery: treat as error and skip ahead + console.warn(`Zero-length block at address ${chunkAddress}; skipping ${self.blockSize} bytes`); + nextAddress += self.blockSize; + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + }
492-513: Await writes; issue next read only after write completesNot awaiting FileSystem.writeChunck risks out‑of‑order writes and premature finalization.
Apply:
- const blob = new Blob([chunkDataView]); - FileSystem.writeChunck(openedFile, blob); - - nextAddress += chunkDataView.byteLength; - - if (!isNaN(bytesCompressed) && !isNaN(totalBytesCompressed)) { - totalBytesCompressed += bytesCompressed; - } else { - totalBytesCompressed = null; - } - - $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); - - if (saveCancelled || nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); - } + const blob = new Blob([chunkDataView]); + const continueOrFinalize = () => { + $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); + } + }; + const afterWrite = () => { + nextAddress += chunkDataView.byteLength; + if (!isNaN(bytesCompressed) && !isNaN(totalBytesCompressed)) { + totalBytesCompressed += bytesCompressed; + } else { + totalBytesCompressed = null; + } + continueOrFinalize(); + }; + FileSystem.writeChunck(openedFile, blob) + .then(afterWrite) + .catch((err) => { + console.error("File write failed:", err); + // Keep forward progress anyway + afterWrite(); + });
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
520-531: Honor cancel and improve logging in skip pathSkip branch ignores saveCancelled and logs nextAddress instead of the reported chunkAddress.
- console.warn(`Skipping null block at address ${nextAddress}`); + console.warn(`Skipping null block at address ${chunkAddress}`); nextAddress += self.blockSize; - if (nextAddress >= maxBytes) { + if (saveCancelled || nextAddress >= maxBytes) { mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); FileSystem.closeFile(openedFile); } else { mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 24 spaces but found 4.
(indent)
[error] 494-494: Expected indentation of 28 spaces but found 8.
(indent)
[error] 495-495: Expected indentation of 28 spaces but found 8.
(indent)
[error] 496-496: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 28 spaces but found 8.
(indent)
[error] 500-500: Expected indentation of 28 spaces but found 8.
(indent)
[error] 501-501: Expected indentation of 32 spaces but found 12.
(indent)
[error] 502-502: Expected indentation of 28 spaces but found 8.
(indent)
[error] 503-503: Expected indentation of 32 spaces but found 12.
(indent)
[error] 504-504: Expected indentation of 28 spaces but found 8.
(indent)
[error] 506-506: Expected indentation of 28 spaces but found 8.
(indent)
[error] 508-508: Expected indentation of 28 spaces but found 8.
(indent)
[error] 509-509: Expected indentation of 32 spaces but found 12.
(indent)
[error] 510-510: Expected indentation of 32 spaces but found 12.
(indent)
[error] 511-511: Expected indentation of 28 spaces but found 8.
(indent)
[error] 512-512: Expected indentation of 32 spaces but found 12.
(indent)
[error] 513-513: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 24 spaces but found 4.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 522-522: Expected indentation of 28 spaces but found 8.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 32 spaces but found 12.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 28 spaces but found 8.
(indent)
[error] 529-529: Expected indentation of 32 spaces but found 12.
(indent)
[error] 530-530: Expected indentation of 28 spaces but found 8.
(indent)
[error] 531-531: Expected indentation of 24 spaces but found 4.
(indent)
[error] 532-532: Expected indentation of 20 spaces but found 0.
(indent)
[error] 534-534: Expected indentation of 20 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/tabs/onboard_logging.js (1)
492-534: Fix lint indentation to pass ESLintESLint flags indentation errors in the onChunkRead block — re-indent per project rules and run the linter.
File: src/js/tabs/onboard_logging.js (lines 492–534)
Run locally: npx eslint --fix src/js/tabs/onboard_logging.js || npm run lint -- --fix
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/js/tabs/onboard_logging.js (1)
492-534: Serialize writes and honor cancel; remove zero-length-as-EOF.Fire-and-forget writes risk reordering/corruption; zero-length isn’t reliable EOF; cancel isn’t checked in the skip path. Make writes sequential, update progress in both paths, check cancel before next read, and treat null/empty as “skip and continue”.
Apply this diff:
-function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { - if (chunkDataView && chunkDataView.byteLength > 0) { - // Always write non-empty data, even if CRC mismatch - const blob = new Blob([chunkDataView]); - FileSystem.writeChunck(openedFile, blob); - - nextAddress += chunkDataView.byteLength; - - // Track total compressed bytes, if provided - if (typeof bytesCompressed === "number") { - if (totalBytesCompressed == null) totalBytesCompressed = 0; // initialize if previously unknown - totalBytesCompressed += bytesCompressed; - } - - $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); - - if (saveCancelled || nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); - } - - } else if (chunkDataView && chunkDataView.byteLength === 0) { - // Zero-length block → EOF - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - - } else { - // Null block → skip ahead (hard error) - console.warn(`Skipping null block at address ${nextAddress}`); - nextAddress += self.blockSize; - - if (nextAddress >= maxBytes) { - mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); - FileSystem.closeFile(openedFile); - } else { - mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); - } - } -} +function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) { + const blockSize = self.blockSize; + + const afterWrite = () => { + if (typeof bytesCompressed === "number") { + if (totalBytesCompressed == null) totalBytesCompressed = 0; + totalBytesCompressed += bytesCompressed; + } + $(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100); + if (saveCancelled || nextAddress >= maxBytes) { + mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); + FileSystem.closeFile(openedFile); + } else { + mspHelper.dataflashRead(nextAddress, blockSize, onChunkRead); + } + }; + + if (chunkDataView && chunkDataView.byteLength > 0) { + const blob = new Blob([chunkDataView]); + FileSystem.writeChunck(openedFile, blob) + .then(() => { + nextAddress += chunkDataView.byteLength; + afterWrite(); + }) + .catch((err) => { + console.error("File write failed:", err); + nextAddress += chunkDataView.byteLength; // keep forward progress + afterWrite(); + }); + } else { + console.warn(`Skipping block at address ${nextAddress} (CRC/mismatch/no data)`); + nextAddress += blockSize; + afterWrite(); + } +}
🧹 Nitpick comments (1)
src/js/tabs/onboard_logging.js (1)
492-534: Fix indentation to satisfy ESLint.Linter flags this block. Re-indent per project rules or run the formatter.
Tip:
- npm run lint -- --fix
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/onboard_logging.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/tabs/onboard_logging.js (1)
src/js/msp/MSPHelper.js (1)
mspHelper(2858-2858)
🪛 ESLint
src/js/tabs/onboard_logging.js
[error] 492-492: Expected indentation of 20 spaces but found 0.
(indent)
[error] 493-493: Expected indentation of 24 spaces but found 4.
(indent)
[error] 494-494: Expected indentation of 28 spaces but found 8.
(indent)
[error] 495-495: Expected indentation of 28 spaces but found 8.
(indent)
[error] 496-496: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 28 spaces but found 8.
(indent)
[error] 500-500: Expected indentation of 28 spaces but found 8.
(indent)
[error] 501-501: Expected indentation of 28 spaces but found 8.
(indent)
[error] 502-502: Expected indentation of 32 spaces but found 12.
(indent)
[error] 503-503: Expected indentation of 32 spaces but found 12.
(indent)
[error] 504-504: Expected indentation of 28 spaces but found 8.
(indent)
[error] 506-506: Expected indentation of 28 spaces but found 8.
(indent)
[error] 508-508: Expected indentation of 28 spaces but found 8.
(indent)
[error] 509-509: Expected indentation of 32 spaces but found 12.
(indent)
[error] 510-510: Expected indentation of 32 spaces but found 12.
(indent)
[error] 511-511: Expected indentation of 28 spaces but found 8.
(indent)
[error] 512-512: Expected indentation of 32 spaces but found 12.
(indent)
[error] 513-513: Expected indentation of 28 spaces but found 8.
(indent)
[error] 515-515: Expected indentation of 24 spaces but found 4.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 28 spaces but found 8.
(indent)
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 522-522: Expected indentation of 28 spaces but found 8.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 32 spaces but found 12.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 28 spaces but found 8.
(indent)
[error] 529-529: Expected indentation of 32 spaces but found 12.
(indent)
[error] 530-530: Expected indentation of 28 spaces but found 8.
(indent)
[error] 531-531: Expected indentation of 24 spaces but found 4.
(indent)
[error] 532-532: Expected indentation of 20 spaces but found 0.
(indent)
[error] 534-534: Expected indentation of 20 spaces but found 0.
(indent)
| FileSystem.writeChunck(openedFile, blob); | ||
|
|
||
| nextAddress += chunkDataView.byteLength; | ||
|
|
||
| // Track total compressed bytes, if provided | ||
| if (typeof bytesCompressed === "number") { | ||
| if (totalBytesCompressed == null) totalBytesCompressed = 0; // initialize if previously unknown | ||
| totalBytesCompressed += bytesCompressed; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Confirm write API returns a Promise.
The fix relies on FileSystem.writeChunck being Promise-based. Verify and adjust (use callbacks/await) if not.
🏁 Script executed:
#!/bin/bash
# Inspect FileSystem.writeChunck signature/usages
rg -n -C2 --type=js --type=ts 'writeChunck\s*\('
rg -n -C2 --type=js --type=ts 'export\s+default\s+class\s+FileSystem|class\s+FileSystem'Length of output: 1098
Await or handle FileSystem.writeChunck Promise
FileSystem.writeChunck is async and returns a Promise (src/js/FileSystem.js:101). Update the call in src/js/tabs/onboard_logging.js (line 496) to await the call or attach .catch to handle errors — e.g. await FileSystem.writeChunck(openedFile, blob); or FileSystem.writeChunck(openedFile, blob).catch(err => console.error(err));
🧰 Tools
🪛 ESLint
[error] 496-496: Expected indentation of 28 spaces but found 8.
(indent)
[error] 498-498: Expected indentation of 28 spaces but found 8.
(indent)
[error] 500-500: Expected indentation of 28 spaces but found 8.
(indent)
[error] 501-501: Expected indentation of 28 spaces but found 8.
(indent)
[error] 502-502: Expected indentation of 32 spaces but found 12.
(indent)
[error] 503-503: Expected indentation of 32 spaces but found 12.
(indent)
🤖 Prompt for AI Agents
In src/js/tabs/onboard_logging.js around lines 496 to 503, the call to
FileSystem.writeChunck(openedFile, blob) returns a Promise but is not awaited or
error-handled; update the call to either await
FileSystem.writeChunck(openedFile, blob) (making the enclosing function async if
it isn’t already) or append .catch(err => /* handle/log error */) to handle
rejections, and ensure any subsequent logic that depends on the write waits for
completion or handles failures accordingly.
| } else if (chunkDataView && chunkDataView.byteLength === 0) { | ||
| // Zero-length block → EOF | ||
| mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); | ||
| FileSystem.closeFile(openedFile); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Zero-length is not a safe EOF indicator.
MSP can deliver empty payloads on CRC/mismatch; rely on nextAddress >= maxBytes for completion.
🏁 Script executed:
#!/bin/bash
# Check MSPHelper.dataflashRead callback semantics for empty/partial deliveries
rg -n -C3 --type=js --type=ts 'dataflashRead\s*\(' src
rg -n -C5 --type=js --type=ts 'MSP_DATAFLASH_READ|crcError|compression' src/js/mspLength of output: 7908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### rg matches for dataflashRead and surrounding context:"
rg -n -C3 --hidden --no-ignore 'dataflashRead' src || true
echo
echo "### MSPHelper.js (around 2400-2525):"
sed -n '2400,2525p' src/js/msp/MSPHelper.js || true
echo
echo "### MSPHelper.js (process_data callback area around 1700-1788):"
sed -n '1700,1788p' src/js/msp/MSPHelper.js || true
echo
echo "### onboard_logging.js (around 480-540):"
sed -n '480,540p' src/js/tabs/onboard_logging.js || trueLength of output: 13388
Don’t treat zero-length chunks as EOF
In onChunkRead (src/js/tabs/onboard_logging.js lines 515–519), the else if (chunkDataView && chunkDataView.byteLength === 0) branch must not unconditionally signal end-of-file—MSP can return an empty DataView on CRC/mismatch. Remove or convert it to an error/skip case and rely solely on nextAddress >= maxBytes to terminate the read loop.
🧰 Tools
🪛 ESLint
[error] 515-515: Expected indentation of 24 spaces but found 4.
(indent)
[error] 516-516: Expected indentation of 28 spaces but found 8.
(indent)
[error] 517-517: Expected indentation of 28 spaces but found 8.
(indent)
[error] 518-518: Expected indentation of 28 spaces but found 8.
(indent)
🤖 Prompt for AI Agents
In src/js/tabs/onboard_logging.js around lines 515–519, the code treats a
zero-length chunkDataView as EOF and immediately calls mark_saving_dialog_done
and FileSystem.closeFile; change this so zero-length chunks are NOT treated as
EOF: remove the unconditional EOF branch and instead handle byteLength === 0 as
an error/skip case (log a warning or emit an error callback and do not close the
file or mark saving done), and let loop termination be driven only by
nextAddress >= maxBytes; ensure no file closure or completion signaling occurs
for a zero-length chunk so CRC/mismatch cases can be retried/handled upstream.
| } else { | ||
| // Null block → skip ahead (hard error) | ||
| console.warn(`Skipping null block at address ${nextAddress}`); | ||
| nextAddress += self.blockSize; | ||
|
|
||
| if (nextAddress >= maxBytes) { | ||
| mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed); | ||
| FileSystem.closeFile(openedFile); | ||
| } else { | ||
| mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cancel not honored in skip path; progress not updated.
Ensure saveCancelled gating and progress update also occur when skipping.
Note: Addressed by the unified afterWrite() flow in the suggested diff.
🧰 Tools
🪛 ESLint
[error] 520-520: Expected indentation of 24 spaces but found 4.
(indent)
[error] 521-521: Expected indentation of 28 spaces but found 8.
(indent)
[error] 522-522: Expected indentation of 28 spaces but found 8.
(indent)
[error] 523-523: Expected indentation of 28 spaces but found 8.
(indent)
[error] 525-525: Expected indentation of 28 spaces but found 8.
(indent)
[error] 526-526: Expected indentation of 32 spaces but found 12.
(indent)
[error] 527-527: Expected indentation of 32 spaces but found 12.
(indent)
[error] 528-528: Expected indentation of 28 spaces but found 8.
(indent)
[error] 529-529: Expected indentation of 32 spaces but found 12.
(indent)
[error] 530-530: Expected indentation of 28 spaces but found 8.
(indent)
[error] 531-531: Expected indentation of 24 spaces but found 4.
(indent)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/js/msp/MSPHelper.js (4)
2448-2519: Consider tracking and surfacing CRC errors to usersWhile ignoring CRC errors enables file saving to proceed, users should be informed when their saved data may be corrupted. Consider implementing error tracking and user notifications.
Add CRC error tracking to provide transparency:
MspHelper.prototype.dataflashRead = function(address, blockSize, onDataCallback) { + // Track CRC errors globally for user notification + if (!this.dataflashCrcErrorCount) { + this.dataflashCrcErrorCount = 0; + } + let outData = [ address & 0xFF, // ... rest of the array ];Then in the response handler, when CRC errors are detected:
// Fallback: deliver raw payload if we have data but couldn't parse it properly if (!payloadView && response && response.data && response.data.byteLength > 7) { + this.dataflashCrcErrorCount++; const headerSize = 7; // ... rest of the fallback handling }And update the callback to pass CRC error status:
- onDataCallback(address, payloadView); + const hasCrcError = response && response.crcError; + onDataCallback(address, payloadView, hasCrcError);Consider also adding a method to retrieve the error count:
MspHelper.prototype.getDataflashCrcErrorCount = function() { return this.dataflashCrcErrorCount || 0; }; MspHelper.prototype.resetDataflashCrcErrorCount = function() { this.dataflashCrcErrorCount = 0; };
2466-2467: Critical: AdddoCallbackOnErrorflag to enable CRC bypassThe MSP send_message call is missing the
doCallbackOnErrorparameter (5th argument). Without this flag set totrue, the callback won't be invoked when CRC errors occur, defeating the purpose of this PR to bypass CRC errors.- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) {Wait, this needs the 5th parameter. Apply this fix instead:
- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, true, function(response) {Actually, reviewing the MSP object's send_message signature, it appears to be:
send_message(code, data, callback_sent, callback_msp, doCallbackOnError). The correct fix is:- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { let payloadView = null; // ... rest of the callback - }); + }, true); // Enable callback on CRC error
2475-2493: Add bounds checking and handle unknown compression typesThe decompression path needs hardening to prevent crashes and handle edge cases properly.
if (chunkAddress === address) { try { if (dataCompressionType === 0) { - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); + const available = Math.max(0, Math.min(dataSize, response.data.byteLength - headerSize)); + payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, available); } else if (dataCompressionType === 1) { + const available = Math.max(0, Math.min(dataSize, response.data.byteLength - headerSize)); + if (available < 2) { + throw new Error('Insufficient data for compressed payload'); + } const compressedCharCount = response.data.readU16(); + const compLen = Math.max(0, available - 2); const compressedArray = new Uint8Array( response.data.buffer, response.data.byteOffset + headerSize + 2, - dataSize - 2 + compLen ); const decompressedArray = huffmanDecodeBuf( compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex ); payloadView = new DataView(decompressedArray.buffer); + } else { + console.warn(`Unknown compression type ${dataCompressionType} at address ${address}`); + payloadView = null; } } catch (e) {
2503-2519: Critical: Fix syntax error causing build failureThere's a syntax error with an extra closing brace and semicolon that's breaking the build. The function structure is malformed.
The current code has structural issues:
- Line 2518 has a stray closing brace and semicolon
- The function isn't properly closed
- The fallback CRC handling (lines 2503-2508) should be integrated into the main response handling
Apply this fix to resolve the syntax error and properly structure the function:
- } - -if (!payloadView && response && response.data) { - // CRC fail or decompression failed, but we still have raw data - const headerSize = 7; - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize); - console.warn(`CRC failed at address ${address}, delivering raw data anyway`); -} - -// Always deliver the block -onDataCallback(address, payloadView); - -if (payloadView) { - console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); -} else { - console.warn(`No data available at address ${address}`); - } - }; // end of dataflashRead + } else { + console.log(`Expected address ${address} but received ${chunkAddress}`); + } + } + + // Fallback: deliver raw payload if we have data but couldn't parse it properly + if (!payloadView && response && response.data && response.data.byteLength > 7) { + const headerSize = 7; + const availableData = response.data.byteLength - headerSize; + if (availableData > 0) { + payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, availableData); + console.warn(`Failed to parse data at address ${address}, delivering raw payload`); + } + } + + // Always deliver the block (even if null) + onDataCallback(address, payloadView); + + if (payloadView) { + console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); + } else { + console.warn(`No data available at address ${address}`); + } + }, true); // Enable callback on CRC error +};
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2445-2447: Fix typo in function documentationThe word "occurred" is misspelled as "occured" in line 2446.
/** * Send a request to read a block of data from the dataflash at the given address and pass that address and a dataview - * of the returned data to the given callback (or null for the data if an error occured). + * of the returned data to the given callback (or null for the data if an error occurred). */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 Biome (2.1.2)
src/js/msp/MSPHelper.js
[error] 2518-2518: expected , but instead found ;
Remove ;
(parse)
🪛 GitHub Actions: Preview Deployment
src/js/msp/MSPHelper.js
[error] 2518-2518: vite-plugin-pwa:build failed: MSPHelper.js (2518:2): Expected ',', got ';'.
🔇 Additional comments (1)
src/js/msp/MSPHelper.js (1)
2459-2464: LGTM: Robust MSP object resolutionGood defensive programming with the runtime MSP object resolution and early error handling.
| MspHelper.prototype.dataflashRead = function(address, blockSize, onDataCallback) { | ||
| let outData = [ | ||
| address & 0xFF, | ||
| (address >> 8) & 0xFF, | ||
| (address >> 16) & 0xFF, | ||
| (address >> 24) & 0xFF, | ||
| blockSize & 0xFF, | ||
| (blockSize >> 8) & 0xFF, | ||
| 1 // allow compression | ||
| ]; | ||
|
|
||
| const mspObj = this.msp || (typeof MSP !== 'undefined' ? MSP : null); | ||
| if (!mspObj) { | ||
| console.error('MSP object not found, cannot read dataflash.'); | ||
| onDataCallback(address, null); | ||
| return; | ||
| } | ||
|
|
||
| mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { | ||
| let payloadView = null; | ||
|
|
||
| // Compressed format uses 2 additional bytes as a pseudo-header to denote the number of uncompressed bytes | ||
| if (response && response.data) { | ||
| const headerSize = 7; | ||
| const chunkAddress = response.data.readU32(); | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| if (chunkAddress === address) { | ||
| try { | ||
| if (dataCompressionType === 0) { | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| } else if (dataCompressionType === 1) { | ||
| const compressedCharCount = response.data.readU16(); | ||
| const compressedArray = new Uint8Array( | ||
| response.data.buffer, | ||
| response.data.byteOffset + headerSize + 2, | ||
| dataSize - 2, | ||
| dataSize - 2 | ||
| ); | ||
| const decompressedArray = huffmanDecodeBuf( | ||
| compressedArray, | ||
| compressedCharCount, | ||
| defaultHuffmanTree, | ||
| defaultHuffmanLenIndex, | ||
| defaultHuffmanLenIndex | ||
| ); | ||
|
|
||
| onDataCallback(address, new DataView(decompressedArray.buffer), dataSize); | ||
| payloadView = new DataView(decompressedArray.buffer); | ||
| } | ||
| } else { | ||
| // Report address error | ||
| console.log(`Expected address ${address} but received ${chunkAddress} - retrying`); | ||
| onDataCallback(address, null); // returning null to the callback forces a retry | ||
| } catch (e) { | ||
| console.warn('Decompression or read failed, delivering raw data anyway'); | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| } | ||
| } else { | ||
| // Report crc error | ||
| console.log(`CRC error for address ${address} - retrying`); | ||
| onDataCallback(address, null); // returning null to the callback forces a retry | ||
| console.log(`Expected address ${address} but received ${chunkAddress}`); | ||
| } | ||
| }, | ||
| true, | ||
| ); | ||
| }; | ||
|
|
||
| MspHelper.prototype.sendServoConfigurations = function (onCompleteCallback) { | ||
| let nextFunction = send_next_servo_configuration; | ||
|
|
||
| let servoIndex = 0; | ||
|
|
||
| if (FC.SERVO_CONFIG.length == 0) { | ||
| onCompleteCallback(); | ||
| } else { | ||
| nextFunction(); | ||
| } | ||
|
|
||
| function send_next_servo_configuration() { | ||
| const buffer = []; | ||
|
|
||
| // send one at a time, with index | ||
|
|
||
| const servoConfiguration = FC.SERVO_CONFIG[servoIndex]; | ||
|
|
||
| buffer | ||
| .push8(servoIndex) | ||
| .push16(servoConfiguration.min) | ||
| .push16(servoConfiguration.max) | ||
| .push16(servoConfiguration.middle) | ||
| .push8(servoConfiguration.rate); | ||
|
|
||
| let out = servoConfiguration.indexOfChannelToForward; | ||
| if (out == undefined) { | ||
| out = 255; // Cleanflight defines "CHANNEL_FORWARDING_DISABLED" as "(uint8_t)0xFF" | ||
| } | ||
| buffer.push8(out).push32(servoConfiguration.reversedInputSources); | ||
|
|
||
| // prepare for next iteration | ||
| servoIndex++; | ||
| if (servoIndex == FC.SERVO_CONFIG.length) { | ||
| nextFunction = onCompleteCallback; | ||
| } | ||
|
|
||
| MSP.send_message(MSPCodes.MSP_SET_SERVO_CONFIGURATION, buffer, false, nextFunction); | ||
| } | ||
| }; | ||
|
|
||
| MspHelper.prototype.sendModeRanges = function (onCompleteCallback) { | ||
| let nextFunction = send_next_mode_range; | ||
|
|
||
| let modeRangeIndex = 0; | ||
|
|
||
| if (FC.MODE_RANGES.length == 0) { | ||
| onCompleteCallback(); | ||
| } else { | ||
| send_next_mode_range(); | ||
| } | ||
|
|
||
| function send_next_mode_range() { | ||
| const modeRange = FC.MODE_RANGES[modeRangeIndex]; | ||
| const buffer = []; | ||
|
|
||
| buffer | ||
| .push8(modeRangeIndex) | ||
| .push8(modeRange.id) | ||
| .push8(modeRange.auxChannelIndex) | ||
| .push8((modeRange.range.start - 900) / 25) | ||
| .push8((modeRange.range.end - 900) / 25); | ||
| if (!payloadView && response && response.data) { | ||
| // CRC fail or decompression failed, but we still have raw data | ||
| const headerSize = 7; | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize); | ||
| console.warn(`CRC failed at address ${address}, delivering raw data anyway`); | ||
| } | ||
|
|
||
| const modeRangeExtra = FC.MODE_RANGES_EXTRA[modeRangeIndex]; | ||
| // Always deliver the block | ||
| onDataCallback(address, payloadView); | ||
|
|
||
| buffer.push8(modeRangeExtra.modeLogic).push8(modeRangeExtra.linkedTo); | ||
| if (payloadView) { | ||
| console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); | ||
| } else { | ||
| console.warn(`No data available at address ${address}`); | ||
| } | ||
| }; // end of dataflashRead | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Update callback signature in onboard_logging.js to match
The dataflashRead function's callback signature should be consistent with what the onboard_logging.js tab expects.
Let me verify that the onboard_logging.js file properly handles the callback parameters:
🏁 Script executed:
#!/bin/bash
# Check how onboard_logging.js uses the dataflashRead callback
echo "=== Searching for dataflashRead usage in onboard_logging ==="
rg -n "dataflashRead|onChunkRead" src/js/tabs/onboard_logging.js --type js -C 3 | head -50
echo -e "\n=== Checking callback parameter usage ==="
ast-grep --pattern 'function onChunkRead($_) {
$$$
}'Length of output: 1358
Pass bytesCompressed to onChunkRead from dataflashRead
onChunkRead is declared as onChunkRead(chunkAddress, chunkDataView, bytesCompressed) in src/js/tabs/onboard_logging.js, but dataflashRead currently calls the callback with only (address, payloadView). Modify src/js/msp/MSPHelper.js:dataflashRead to invoke onDataCallback(address, payloadView, bytesCompressed) — set bytesCompressed = (dataCompressionType === 1 ? dataSize - 2 : 0) (and pass 0 on CRC/decompression-fallback).
🧰 Tools
🪛 Biome (2.1.2)
[error] 2518-2518: expected , but instead found ;
Remove ;
(parse)
🪛 GitHub Actions: Preview Deployment
[error] 2518-2518: vite-plugin-pwa:build failed: MSPHelper.js (2518:2): Expected ',', got ';'.
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2448 to 2519, dataflashRead currently
calls onDataCallback(address, payloadView) but onChunkRead expects three args;
compute bytesCompressed = (dataCompressionType === 1 ? dataSize - 2 : 0) when a
response is present and pass it as the third argument to onDataCallback; ensure
that in the decompressed path you pass the computed bytesCompressed, in the
uncompressed path pass 0, and in the CRC/decompression-fallback path pass 0 as
well.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/msp/MSPHelper.js (1)
2466-2466: Critical: pass doCallbackOnError=true so callback runs on CRC failuressend_message accepts a doCallbackOnError parameter; the call at src/js/msp/MSPHelper.js:2466 omits it, so the callback won't be invoked on CRC failures. Apply this change:
- mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { + mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, true, function(response) {
🧹 Nitpick comments (2)
src/js/msp/MSPHelper.js (2)
2475-2497: Add bounds checking and handle unknown compression typesThe decompression logic needs additional validation:
- Bounds checking: Validate
dataSizeto prevent negative array lengths- Unknown compression types: Add explicit handling for unexpected compression values
if (chunkAddress === address) { + // Validate dataSize to prevent negative/overflow issues + const availableData = Math.max(0, Math.min(dataSize, response.data.byteLength - headerSize)); try { if (dataCompressionType === 0) { - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); + payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, availableData); } else if (dataCompressionType === 1) { + if (availableData < 2) { + throw new Error('Insufficient data for compressed payload'); + } const compressedCharCount = response.data.readU16(); + const compressedSize = Math.max(0, availableData - 2); const compressedArray = new Uint8Array( response.data.buffer, response.data.byteOffset + headerSize + 2, - dataSize - 2 + compressedSize, ); const decompressedArray = huffmanDecodeBuf( compressedArray, compressedCharCount, defaultHuffmanTree, defaultHuffmanLenIndex, ); payloadView = new DataView(decompressedArray.buffer); + } else { + console.warn(`Unknown compression type ${dataCompressionType} at address ${address}`); } } catch (e) { - console.warn('Decompression or read failed, delivering raw data anyway'); - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); + console.warn(`Decompression failed at address ${address}:`, e); + payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, availableData); }
2448-2520: Consider making CRC bypass configurableSince bypassing CRC validation can result in corrupted data being saved, consider making this behavior opt-in through a configuration option or user preference. This would allow users to choose between reliability and the ability to recover partial data.
Example approach:
// Add to FC configuration or user preferences FC.DATAFLASH.bypassCrcErrors = false; // Default to safe behavior // In dataflashRead: if (response && response.crcError && !FC.DATAFLASH.bypassCrcErrors) { console.error(`CRC error at address ${address} - aborting`); onDataCallback(address, null); return; }This gives users control over the trade-off between data integrity and recovery capability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2456-2456: Missing trailing comma.
(comma-dangle)
[error] 2484-2485: Missing trailing comma.
(comma-dangle)
[error] 2490-2491: Missing trailing comma.
(comma-dangle)
[error] 2503-2503: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2504-2504: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2505-2505: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2506-2506: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2507-2507: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2508-2508: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2510-2510: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2511-2511: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2513-2513: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2514-2514: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2515-2515: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2516-2516: Expected indentation of 12 spaces but found 4.
(indent)
[error] 2517-2517: Expected indentation of 8 spaces but found 0.
(indent)
[error] 2519-2519: Expected indentation of 4 spaces but found 0.
(indent)
🔇 Additional comments (1)
src/js/msp/MSPHelper.js (1)
2448-2520: Track CRC errors but keep the callback signature compatible (pass CRC as a 4th arg)onChunkRead is declared as (chunkAddress, chunkDataView, bytesCompressed) in src/js/tabs/onboard_logging.js:492 — changing the 3rd parameter would break callers. Instead:
- Initialize and increment a CRC error counter (e.g., FC.DATAFLASH.crcErrorCount or this.crcErrorCount) when a CRC is detected and log a warning (e.g.,
CRC error #n at address X).- When compression is used, compute bytesCompressed (e.g., compressedArray.length) and pass it as the 3rd argument as currently expected.
- Pass the CRC state as an optional 4th argument: onDataCallback(address, payloadView, bytesCompressed, hasCrcError). Update onChunkRead to accept and handle the optional 4th parameter while preserving existing bytesCompressed logic.
- Do not change existing call sites' parameter ordering to avoid regressions.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/msp/MSPHelper.js (1)
2463-2535: Make CRC-ignore actually work; clamp sizes; return correct address; align callback arity; fix lints
- Callback never fires on CRC errors (missing doCallbackOnError=true).
- Uses unbounded dataSize and may rethrow inside catch.
- Returns requested address instead of chunkAddress.
- Downstream expects metadata (bytesCompressed), and UI should know when CRC was ignored.
- ESLint comma-dangle errors.
Apply unified, safe implementation:
-MspHelper.prototype.dataflashRead = function(address, blockSize, onDataCallback) { - let outData = [ - address & 0xFF, - (address >> 8) & 0xFF, - (address >> 16) & 0xFF, - (address >> 24) & 0xFF, - blockSize & 0xFF, - (blockSize >> 8) & 0xFF, - 1 // allow compression - ]; - - const mspObj = this.msp || (typeof MSP !== 'undefined' ? MSP : null); - if (!mspObj) { - console.error('MSP object not found, cannot read dataflash.'); - onDataCallback(address, null); - return; - } - - mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { - let payloadView = null; - - if (response && response.data) { - const headerSize = 7; - const chunkAddress = response.data.readU32(); - const dataSize = response.data.readU16(); - const dataCompressionType = response.data.readU8(); - - if (chunkAddress === address) { - try { - if (dataCompressionType === 0) { - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); - } else if (dataCompressionType === 1) { - const compressedCharCount = response.data.readU16(); - const compressedArray = new Uint8Array( - response.data.buffer, - response.data.byteOffset + headerSize + 2, - dataSize - 2 - ); - const decompressedArray = huffmanDecodeBuf( - compressedArray, - compressedCharCount, - defaultHuffmanTree, - defaultHuffmanLenIndex - ); - payloadView = new DataView(decompressedArray.buffer); - } - } catch (e) { - console.warn('Decompression or read failed, delivering raw data anyway'); - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); - } - } else { - console.log(`Expected address ${address} but received ${chunkAddress}`); - } - } - - if (!payloadView && response && response.data) { - // CRC fail or decompression failed, but we still have raw data - const headerSize = 7; - payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize); - console.warn(`CRC failed at address ${address}, delivering raw data anyway`); - } - - // Always deliver the block - onDataCallback(address, payloadView); - - if (payloadView) { - console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); - } else { - console.warn(`No data available at address ${address}`); - } - - }); // end of send_message callback -}; // end of dataflashRead +MspHelper.prototype.dataflashRead = function (address, blockSize, onDataCallback) { + const size = Math.max(0, Math.min(blockSize | 0, 0xffff)); + const outData = [ + address & 0xff, + (address >> 8) & 0xff, + (address >> 16) & 0xff, + (address >> 24) & 0xff, + size & 0xff, + (size >> 8) & 0xff, + 1, // allow compression + ]; + + const mspObj = this.msp || (typeof MSP !== "undefined" ? MSP : null); + if (!mspObj) { + console.error("MSP object not found, cannot read dataflash."); + onDataCallback(address, null, 0, true); + return; + } + + mspObj.send_message( + MSPCodes.MSP_DATAFLASH_READ, + outData, + false, + function (response) { + const headerSize = 7; + let payloadView = null; + let bytesCompressed = 0; + let deliverAddress = address; + + if (response && response.data && response.data.byteLength >= headerSize) { + const chunkAddress = response.data.readU32(); + const dataSize = response.data.readU16(); + const dataCompressionType = response.data.readU8(); + deliverAddress = Number.isFinite(chunkAddress) ? chunkAddress : address; + + const available = Math.max(0, Math.min(dataSize | 0, response.data.byteLength - headerSize)); + + if (dataCompressionType === 0) { + payloadView = new DataView( + response.data.buffer, + response.data.byteOffset + headerSize, + available, + ); + bytesCompressed = available; + } else if (dataCompressionType === 1) { + try { + if (available >= 2) { + const compressedCharCount = response.data.readU16(); + const compLen = Math.max(0, available - 2); + const compressedArray = new Uint8Array( + response.data.buffer, + response.data.byteOffset + headerSize + 2, + compLen, + ); + const decompressedArray = huffmanDecodeBuf( + compressedArray, + compressedCharCount, + defaultHuffmanTree, + defaultHuffmanLenIndex, + ); + payloadView = new DataView(decompressedArray.buffer); + bytesCompressed = compLen; + } + } catch (e) { + console.warn(`MSP_DATAFLASH_READ: decompression failed at addr=${address}`, e); + payloadView = new DataView( + response.data.buffer, + response.data.byteOffset + headerSize, + available, + ); + bytesCompressed = available; + } + } else { + console.warn(`MSP_DATAFLASH_READ: unknown compression type=${dataCompressionType} at addr=${address}`); + } + } + + if (response?.crcError) { + FC.DATAFLASH.crcErrorCount = (FC.DATAFLASH.crcErrorCount || 0) + 1; + } + onDataCallback(deliverAddress, payloadView, bytesCompressed, Boolean(response?.crcError)); + + if (!payloadView) { + console.warn(`MSP_DATAFLASH_READ: no data available at address ${address}`); + } else if (response?.crcError) { + console.warn(`MSP_DATAFLASH_READ: CRC error at ${address} (delivered anyway, ${payloadView.byteLength} bytes)`); + } else { + console.log(`MSP_DATAFLASH_READ: block at ${address} received (${payloadView.byteLength} bytes)`); + } + }, + true, // doCallbackOnError + ); +};Verification (checks for caller arity and usage):
#!/bin/bash set -euo pipefail echo "== dataflashRead call sites ==" rg -nP -C2 '\bdataflashRead\s*\(' --type=js echo echo "== consumers expecting bytesCompressed/hasCrcError ==" rg -nP -C3 'onChunkRead\s*\(|bytesCompressed|hasCrcError' --type=js
🧹 Nitpick comments (1)
src/js/msp/MSPHelper.js (1)
2444-2458: Remove duplicated setRawRx() definitionThis second, identical prototype assignment just redefines the same method. Drop the duplicate block.
-/** - * Set raw Rx values over MSP protocol. - * - * Channels is an array of 16-bit unsigned integer channel values to be sent. 8 channels is probably the maximum. - */ -MspHelper.prototype.setRawRx = function (channels) { - const buffer = []; - - for (let i = 0; i < channels.length; i++) { - buffer.push16(channels[i]); - } - - MSP.send_message(MSPCodes.MSP_SET_RAW_RC, buffer, false); -};
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (2)
src/js/msp.js (1)
MSP(5-467)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2471-2471: Missing trailing comma.
(comma-dangle)
[error] 2499-2500: Missing trailing comma.
(comma-dangle)
[error] 2505-2506: Missing trailing comma.
(comma-dangle)
ctzsnooze
force-pushed
the
IgnoreCRCforDataFLashReadss
branch
from
b91f9ad to
62ba9ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/js/msp/MSPHelper.js (3)
2445-2447: Update comment to reflect the new "ignore CRC errors" behavior.The comment still mentions that null is passed on CRC failure, but the implementation now delivers raw data even when CRC errors occur.
/** - * Send a request to read a block of data from the dataflash at the given address and pass that address and a DataView - * of the returned data to the given callback. If an error or CRC fail occurs, the raw data is delivered instead of null. + * Send a request to read a block of data from the dataflash at the given address and pass that address and a DataView + * of the returned data to the given callback. CRC errors are ignored - raw data is always delivered when available. */
2456-2456: Add missing trailing comma for consistency.ESLint flagged this missing trailing comma which should be added for consistency with the codebase style.
blockSize & 0xFF, (blockSize >> 8) & 0xFF, - 1 // allow compression + 1, // allow compression
2491-2492: Add missing trailing commas in function call arguments.ESLint flagged missing trailing commas that should be added for consistency.
response.data.buffer, response.data.byteOffset + headerSize + 2, - dataSize - 2 + dataSize - 2, ); const decompressedArray = huffmanDecodeBuf( compressedArray, compressedCharCount, defaultHuffmanTree, - defaultHuffmanLenIndex + defaultHuffmanLenIndex, );Also applies to: 2497-2498
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/msp/MSPHelper.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/msp/MSPHelper.js (4)
src/js/msp.js (1)
MSP(5-467)src/js/msp/MSPCodes.js (1)
MSPCodes(2-210)src/js/default_huffman_tree.js (4)
defaultHuffmanTree(3-261)defaultHuffmanTree(3-261)defaultHuffmanLenIndex(263-273)defaultHuffmanLenIndex(263-273)src/js/tabs/onboard_logging.js (1)
totalBytesCompressed(488-488)
🪛 ESLint
src/js/msp/MSPHelper.js
[error] 2456-2456: Missing trailing comma.
(comma-dangle)
[error] 2491-2492: Missing trailing comma.
(comma-dangle)
[error] 2497-2498: Missing trailing comma.
(comma-dangle)
🔇 Additional comments (1)
src/js/msp/MSPHelper.js (1)
2448-2526: Well-executed CRC error tolerance implementation with good logging.The implementation effectively addresses the PR objective of making dataflash reads more reliable by ignoring CRC errors. Key strengths:
- Robust error handling: Try-catch around decompression with fallback to raw data
- Comprehensive logging: Distinguishes between CRC errors, missing data, and successful reads
- Consistent API: Always calls
onDataCallbackwith meaningful data when available- Proper MSP integration: Uses
doCallbackOnError: trueto ensure callbacks fire even on CRC failures- Huffman decompression support: Maintains existing compression functionality while adding fallback
The approach of delivering potentially corrupted data is justified given the context of flight log recovery where partial data is often preferable to no data.
| mspObj.send_message(MSPCodes.MSP_DATAFLASH_READ, outData, false, function(response) { | ||
| let payloadView = null; | ||
| let bytesCompressed = 0; | ||
|
|
||
| const servoConfiguration = FC.SERVO_CONFIG[servoIndex]; | ||
| if (response && response.data) { | ||
| const headerSize = 7; | ||
| const chunkAddress = response.data.readU32(); | ||
| const dataSize = response.data.readU16(); | ||
| const dataCompressionType = response.data.readU8(); | ||
|
|
||
| buffer | ||
| .push8(servoIndex) | ||
| .push16(servoConfiguration.min) | ||
| .push16(servoConfiguration.max) | ||
| .push16(servoConfiguration.middle) | ||
| .push8(servoConfiguration.rate); | ||
|
|
||
| let out = servoConfiguration.indexOfChannelToForward; | ||
| if (out == undefined) { | ||
| out = 255; // Cleanflight defines "CHANNEL_FORWARDING_DISABLED" as "(uint8_t)0xFF" | ||
| } | ||
| buffer.push8(out).push32(servoConfiguration.reversedInputSources); | ||
| if (chunkAddress !== address) { | ||
| console.log(`Expected address ${address} but received ${chunkAddress}`); | ||
| } | ||
|
|
||
| // prepare for next iteration | ||
| servoIndex++; | ||
| if (servoIndex == FC.SERVO_CONFIG.length) { | ||
| nextFunction = onCompleteCallback; | ||
| try { | ||
| if (dataCompressionType === 0) { | ||
| // Raw, uncompressed | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| bytesCompressed = dataSize; | ||
| } else if (dataCompressionType === 1) { | ||
| // Compressed | ||
| const compressedCharCount = response.data.readU16(); | ||
| const compressedArray = new Uint8Array( | ||
| response.data.buffer, | ||
| response.data.byteOffset + headerSize + 2, | ||
| dataSize - 2 | ||
| ); | ||
| const decompressedArray = huffmanDecodeBuf( | ||
| compressedArray, | ||
| compressedCharCount, | ||
| defaultHuffmanTree, | ||
| defaultHuffmanLenIndex | ||
| ); | ||
| payloadView = new DataView(decompressedArray.buffer); | ||
| bytesCompressed = compressedArray.length; | ||
| } | ||
| } catch (e) { | ||
| console.warn('Decompression failed, delivering raw data anyway:', e); | ||
| // fallback to raw block | ||
| payloadView = new DataView(response.data.buffer, response.data.byteOffset + headerSize, dataSize); | ||
| bytesCompressed = dataSize; | ||
| } | ||
| } | ||
|
|
||
| MSP.send_message(MSPCodes.MSP_SET_SERVO_CONFIGURATION, buffer, false, nextFunction); | ||
| } | ||
| }; | ||
|
|
||
| MspHelper.prototype.sendModeRanges = function (onCompleteCallback) { | ||
| let nextFunction = send_next_mode_range; | ||
|
|
||
| let modeRangeIndex = 0; | ||
|
|
||
| if (FC.MODE_RANGES.length == 0) { | ||
| onCompleteCallback(); | ||
| } else { | ||
| send_next_mode_range(); | ||
| } | ||
|
|
||
| function send_next_mode_range() { | ||
| const modeRange = FC.MODE_RANGES[modeRangeIndex]; | ||
| const buffer = []; | ||
|
|
||
| buffer | ||
| .push8(modeRangeIndex) | ||
| .push8(modeRange.id) | ||
| .push8(modeRange.auxChannelIndex) | ||
| .push8((modeRange.range.start - 900) / 25) | ||
| .push8((modeRange.range.end - 900) / 25); | ||
| // Deliver payloadView even if CRC failed | ||
| onDataCallback(address, payloadView); | ||
|
|
||
| const modeRangeExtra = FC.MODE_RANGES_EXTRA[modeRangeIndex]; | ||
|
|
||
| buffer.push8(modeRangeExtra.modeLogic).push8(modeRangeExtra.linkedTo); | ||
| // Logging | ||
| if (!response || response.crcError) { | ||
| console.log(`CRC error or missing data at address ${address} - delivering raw data (${payloadView ? payloadView.byteLength : 0} bytes)`); | ||
| } else if (payloadView) { | ||
| console.log(`Block at ${address} received (${payloadView.byteLength} bytes)`); | ||
| } | ||
|
|
||
| // prepare for next iteration | ||
| modeRangeIndex++; | ||
| if (modeRangeIndex == FC.MODE_RANGES.length) { | ||
| nextFunction = onCompleteCallback; | ||
| // Track total compressed bytes globally, if needed | ||
| if (typeof bytesCompressed === "number") { | ||
| if (typeof totalBytesCompressed === "undefined" || totalBytesCompressed == null) totalBytesCompressed = 0; | ||
| totalBytesCompressed += bytesCompressed; | ||
| } | ||
| MSP.send_message(MSPCodes.MSP_SET_MODE_RANGE, buffer, false, nextFunction); | ||
| } | ||
| }; | ||
| }, true); // end of send_message |
There was a problem hiding this comment.
🧩 Analysis chain
Verify callback parameter consistency with consumers.
The callback now always provides (address, payloadView) but I noticed from past review comments that some consumers might expect a third bytesCompressed parameter.
Let me check the actual callback usage in the onboard logging module:
🏁 Script executed:
#!/bin/bash
# Check dataflashRead callback usage in onboard_logging.js
echo "=== Finding dataflashRead calls ==="
rg -n -A5 -B2 "dataflashRead" src/js/tabs/onboard_logging.js
echo
echo "=== Check onChunkRead function signature ==="
ast-grep --pattern 'function onChunkRead($$$) {
$$$
}'
echo
echo "=== Alternative onChunkRead patterns ==="
rg -n -A10 "onChunkRead.*=" src/js/tabs/onboard_logging.jsLength of output: 4161
Pass bytesCompressed to the dataflashRead callback
onChunkRead in src/js/tabs/onboard_logging.js is defined as (chunkAddress, chunkDataView, bytesCompressed) and uses bytesCompressed (lines ~492–504); MSPHelper currently calls onDataCallback(address, payloadView) (src/js/msp/MSPHelper.js lines ~2466–2525) — the third argument is missing. Change the call to onDataCallback(address, payloadView, bytesCompressed) and ensure bytesCompressed is a numeric value (e.g. default 0) when unavailable. Also audit other mspHelper.dataflashRead callers for the same expectation.
🧰 Tools
🪛 ESLint
[error] 2491-2492: Missing trailing comma.
(comma-dangle)
[error] 2497-2498: Missing trailing comma.
(comma-dangle)
🤖 Prompt for AI Agents
In src/js/msp/MSPHelper.js around lines 2466 to 2525, the onDataCallback is
invoked with only two arguments but callers (e.g. onChunkRead in
src/js/tabs/onboard_logging.js) expect a third bytesCompressed parameter; ensure
bytesCompressed is always a numeric value (default 0) and change the call to
pass it: invoke onDataCallback(address, payloadView, bytesCompressed). Also
initialize bytesCompressed = 0 at declaration and keep updating it as currently
implemented; finally search the repo for other mspHelper.dataflashRead callbacks
and update any that assume a bytesCompressed value or add handling for a missing
third arg.
ctzsnooze
force-pushed
the
IgnoreCRCforDataFLashReadss
branch
from
011919a to
fc6b059
Compare
|
|
Preview URL: https://fc6b059d.betaflight-configurator.pages.dev |
|
Seems to work OK. Some observations:
|
|
Closing in favour of #4627 because this became very messy because I could only build online.. |
Just a quick hack to see if ignoring CRC errors will allow the 'Save Flash to File function to work more reliably.
Currently the binary characters in the logfile seem to generate CRC errors.
Summary by CodeRabbit
Bug Fixes
New Features
Chores