OSDP File Transfers #42
Conversation
This adds pieces to send file data to a device. It uses a functional core in `Jeff.Command.FileTransfer` to check each reply after a FILETRANSFER attempt to determine next steps for continuing the data transfer. The functional core allows for simpler testing
There was a problem hiding this comment.
Pull Request Overview
Adds OSDP file transfer support by implementing file transfer command chunking, FTSTAT reply decoding, and a high-level file_transfer API with tests.
- Introduce
Jeff.Command.FileTransferwithcommand_set/2,adjust_from_reply/2, and private chunking logic - Add
Jeff.Reply.FileTransferStatusdecoder and integrate it intoJeff.Reply.decode/2 - Provide
Jeff.file_transfer/3|4public API, progress callbacks, and end-to-end tests
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/command/file_transfer_test.exs | New tests for command_set/2 and adjust_from_reply/2 |
| lib/jeff/reply/file_transfer_status.ex | Define FileTransferStatus struct & decode/1 logic |
| lib/jeff/reply.ex | Register FTSTAT decode branch in the main reply dispatcher |
| lib/jeff/command/file_transfer.ex | Implement FileTransfer command struct, chunking, and adjust |
| lib/jeff/command.ex | Hook up FILETRANSFER encode path in the command router |
| lib/jeff.ex | Add `file_transfer/3 |
| .mise.toml | Pin Elixir/Erlang versions |
Comments suppressed due to low confidence (2)
test/command/file_transfer_test.exs:1
- [nitpick] Test module name doesn’t match its path; rename to
Jeff.Command.FileTransferTestto align withtest/command/file_transfer_test.exs.
defmodule Jeff.FileTransferTest do
lib/jeff/command/file_transfer.ex:74
- Typo in doc: change 'may need to been adjusted' to 'may need to be adjusted' for correct grammar.
next set of commands may need to be adjusted or prevented and this provides
- Add file_transfer function with progress callback support - Resolve conflicts with new mfg function from main - Add ErrorCode alias for compilation
LivInAbsurdism
force-pushed
the
liv/filetransfers
branch
from
cef2a27 to
c1fba4e
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def file_transfer(acu, address, data, progress_callback) when is_binary(data) do | ||
| case ACU.send_command(acu, address, CAP) |> handle_reply() do | ||
| caps when is_map(caps) -> | ||
| max = 128 |
There was a problem hiding this comment.
This hardcoded value should be extracted from the capabilities response rather than using a fixed value. The comment on line 194 suggests this should use the PD's receive buffer size from the capabilities.
| max = 128 | |
| # Extract the PD's receive buffer size from capabilities, default to 128 if not present | |
| max = Map.get(caps, :receive_buffer_size, 128) |
Copilot uses AI. Check for mistakes.
| progress_callback.(total_packets, total_packets, 100) | ||
| end | ||
|
|
||
| %Reply.FileTransferStatus{} |
There was a problem hiding this comment.
Returning an empty FileTransferStatus struct for successful completion is inconsistent with the documented return type. Consider returning a properly initialized struct with status: :ok or defining a specific success indicator.
| %Reply.FileTransferStatus{} | |
| %Reply.FileTransferStatus{status: :ok} |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Liv Cella <47287116+LivInAbsurdism@users.noreply.github.com>
LivInAbsurdism
force-pushed
the
liv/filetransfers
branch
from
cfc0365 to
b39c841
Compare
No description provided.