add FC21 - #616
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Modbus function code 21 Write File Records support, including request construction, response parsing and dispatch, and promise-based API exposure. ChangesFC21 Write File Records
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant PromiseAPI
participant ModbusRTU
participant Port
participant FC21Parser
PromiseAPI->>ModbusRTU: writeFileRecords(...)
ModbusRTU->>Port: send FC21 request with file and record data
Port-->>ModbusRTU: FC21 response
ModbusRTU->>FC21Parser: parse response
FC21Parser-->>PromiseAPI: parsed response data
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
index.js (4)
252-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect parameter name and typos in the JSDoc comment.
The parameter is named
data, but the JSDoc documentsbuffer. Also, correct the "fro" typo.📝 Proposed fix
/** - * Parse the data fro Modbus - + * Parse the data from Modbus - * Write File Records * - * `@param` {Buffer} buffer + * `@param` {Buffer} data * `@param` {Function} next */ function _readFC21(data, next) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 252 - 259, Update the JSDoc for _readFC21 to document the parameter as data instead of buffer, and correct the “fro” typo in the description to “from”; leave the function implementation unchanged.
1267-1270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the typo in the variable name.
If you apply the variable rename
requestDataLengthfrom the previous comment, update its usage here as well.📝 Proposed fix
const buf = Buffer.alloc(codeLength + 2); // add 2 crc bytes buf.writeUInt8(address, 0); buf.writeUInt8(code, 1); - buf.writeUInt8(reqeustDataLength, 2); + buf.writeUInt8(requestDataLength, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 1267 - 1270, Rename the misspelled reqeustDataLength reference in this buffer-writing code to requestDataLength, matching the corrected variable declaration and preserving the existing writeUInt8 behavior.
1275-1279: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptimize copying data to the buffer.
If
datais aBuffer, using.copy()is significantly faster and more idiomatic than iterating byte-by-byte. Ifdatamight also be an array of bytes, you can provide a fallback.⚡ Proposed fix
- let pos = 10; - for(const byte of data) { - buf.writeUInt8(byte, pos); - pos += 1; - } + if (Buffer.isBuffer(data)) { + data.copy(buf, 10); + } else { + let pos = 10; + for(const byte of data) { + buf.writeUInt8(byte, pos); + pos += 1; + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 1275 - 1279, Optimize the data copy in the buffer-writing block by using Buffer.copy when data is a Buffer, starting at offset 10. Preserve support for byte arrays with a fallback that retains the existing byte-wise write behavior, and keep the resulting buffer contents unchanged.
615-620: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate semicolon.
There is an accidental double semicolon after the
breakstatement.📝 Proposed fix
case 17: _readFC17(data, next); break case 20: _readFC20(data, transaction.next); - break;; + break; case 21: _readFC21(data, transaction.next);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 615 - 620, Remove the duplicate semicolon after the break statement in the case 20 branch of the transaction dispatch logic, leaving a single semicolon and preserving the surrounding _readFC20 call and control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.js`:
- Around line 1256-1260: Validate that data.length is even before calculating
recordLength, and reject or otherwise handle odd-length input before any
writeUInt16BE call. Keep recordLength an integer representing 16-bit registers,
and rename reqeustDataLength to requestDataLength while updating all references.
---
Nitpick comments:
In `@index.js`:
- Around line 252-259: Update the JSDoc for _readFC21 to document the parameter
as data instead of buffer, and correct the “fro” typo in the description to
“from”; leave the function implementation unchanged.
- Around line 1267-1270: Rename the misspelled reqeustDataLength reference in
this buffer-writing code to requestDataLength, matching the corrected variable
declaration and preserving the existing writeUInt8 behavior.
- Around line 1275-1279: Optimize the data copy in the buffer-writing block by
using Buffer.copy when data is a Buffer, starting at offset 10. Preserve support
for byte arrays with a fallback that retains the existing byte-wise write
behavior, and keep the resulting buffer contents unchanged.
- Around line 615-620: Remove the duplicate semicolon after the break statement
in the case 20 branch of the transaction dispatch logic, leaving a single
semicolon and preserving the surrounding _readFC20 call and control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
index.js (2)
615-620: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the transaction callback wrapper for FC20 and FC21.
The local
nextwrapper adds optionaltransaction.requestandtransaction.responsesmetadata before invokingtransaction.next. Passingtransaction.nextdirectly bypasses that contract, so debug-enabled callers lose request/response data. Route both parsers throughnext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 615 - 620, Update the FC20 and FC21 branches in the transaction dispatch switch to pass the local next wrapper to _readFC20 and _readFC21 instead of transaction.next directly. Preserve the wrapper’s request and responses metadata behavior for debug-enabled callers.
1261-1273: 🎯 Functional Correctness | 🔴 CriticalUse
requestDataLengthconsistently.Line 1262 declares
requestDataLength, but Line 1273 references the misspelledreqeustDataLength. Every valid FC21 write therefore throws aReferenceErrorbefore transmission. This repeats the previous review finding; update the use torequestDataLength.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@index.js` around lines 1261 - 1273, In the transaction buffer setup around the FC21 write logic, update the misspelled reqeustDataLength reference in the buf.writeUInt8 call to use the declared requestDataLength variable consistently.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@index.js`:
- Around line 1261-1263: In the FC21 frame-writing path, correct the
request-data length variable name and validate payload size before constructing
the frame so data.length values above 248 are rejected. Call next with an
appropriate error for oversized payloads, while preserving normal frame
generation for valid payloads.
---
Outside diff comments:
In `@index.js`:
- Around line 615-620: Update the FC20 and FC21 branches in the transaction
dispatch switch to pass the local next wrapper to _readFC20 and _readFC21
instead of transaction.next directly. Preserve the wrapper’s request and
responses metadata behavior for debug-enabled callers.
- Around line 1261-1273: In the transaction buffer setup around the FC21 write
logic, update the misspelled reqeustDataLength reference in the buf.writeUInt8
call to use the declared requestDataLength variable consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-authored-by: Yaacov Zamir <[email protected]>
Co-authored-by: Yaacov Zamir <[email protected]>
|
Thank you for the pull request @enthusapp |
2 participants
add function write file records
Summary by CodeRabbit
writeFC21method for sending FC=21 requests.writeFileRecordsmethod on Modbus instances for FC=21 operations.