Skip to content

Ingore CRC on dataFlash Reads #4619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Ingore CRC on dataFlash Reads #4619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 49 additions & 122 deletions src/js/msp/MSPHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2443,147 +2443,74 @@ MspHelper.prototype.setRawRx = function (channels) {

/**
* 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).
*/
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]);

// 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();
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();
}
// Deliver payloadView if defined, otherwise pass null
onDataCallback(address, payloadView);

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);

const modeRangeExtra = FC.MODE_RANGES_EXTRA[modeRangeIndex];

buffer.push8(modeRangeExtra.modeLogic).push8(modeRangeExtra.linkedTo);

// prepare for next iteration
modeRangeIndex++;
if (modeRangeIndex == FC.MODE_RANGES.length) {
nextFunction = onCompleteCallback;
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)`);
}
MSP.send_message(MSPCodes.MSP_SET_MODE_RANGE, buffer, false, nextFunction);
}
};
}, true); // end of send_message
}; // end of dataflashRead


MspHelper.prototype.sendAdjustmentRanges = function (onCompleteCallback) {
let nextFunction = send_next_adjustment_range;
Expand Down
85 changes: 43 additions & 42 deletions src/js/tabs/onboard_logging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,49 +489,50 @@ onboard_logging.initialize = function (callback) {

show_saving_dialog();

function onChunkRead(chunkAddress, chunkDataView, bytesCompressed) {
if (chunkDataView !== null) {
// Did we receive any data?
if (chunkDataView.byteLength > 0) {
nextAddress += chunkDataView.byteLength;
if (isNaN(bytesCompressed) || isNaN(totalBytesCompressed)) {
totalBytesCompressed = null;
} else {
totalBytesCompressed += bytesCompressed;
}

$(".dataflash-saving progress").attr("value", (nextAddress / maxBytes) * 100);

const blob = new Blob([chunkDataView]);
FileSystem.writeChunck(openedFile, blob).then(() => {
if (saveCancelled || nextAddress >= maxBytes) {
if (saveCancelled) {
dismiss_saving_dialog();
} else {
mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed);
}
FileSystem.closeFile(openedFile);
} else {
if (!self.writeError) {
mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead);
} else {
dismiss_saving_dialog();
FileSystem.closeFile(openedFile);
}
}
});
} else {
// A zero-byte block indicates end-of-file, so we're done
mark_saving_dialog_done(startTime, nextAddress, totalBytesCompressed);
FileSystem.closeFile(openedFile);
}
} else {
// There was an error with the received block (address didn't match the one we asked for), retry
mspHelper.dataflashRead(nextAddress, self.blockSize, onChunkRead);
}
}
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;
Comment on lines +496 to +503
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

}

$(".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);

Comment on lines +515 to +519
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 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/msp

Length 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 || true

Length 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);
}
}
Comment on lines +520 to +531
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)

}

const startTime = new Date().getTime(); // Start timestamp

const startTime = new Date().getTime();
// Fetch the initial block
FileSystem.openFile(fileWriter).then((file) => {
openedFile = file;
Expand Down