-
Notifications
You must be signed in to change notification settings - Fork 1
OSDP File Transfers #42
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
cef2a27 to
c1fba4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
3f88514 to
9844455
Compare
- Add file_transfer function with progress callback support - Resolve conflicts with new mfg function from main - Add ErrorCode alias for compilation
e2b32c5 to
6769f4a
Compare
fb6fa97 to
e8d7f76
Compare
Signed-off-by: Liv Cella <47287116+LivInAbsurdism@users.noreply.github.com>
kybishop
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm admittedly glossing over this a bit but the general architecture seems sound. It seems to me that we'd know very quickly if something is wrong with the implementation during test so I'm not too concerned about reviewing with a fine toothed comb.
jjcarstens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some thoughts. Looks great!
9408284 to
6d9d6fa
Compare
…us additional tweaks
1b57fea to
7ea1ec3
Compare
jjcarstens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
No description provided.