Lint and write up the protocol into words #1
Conversation
dhalbert
left a comment
dhalbert
left a comment
There was a problem hiding this comment.
Nicely done, but I have some questions about the robustness and idempotency of the protocol packets.
| Given a full path, returns the full contents of the file. | ||
|
|
||
| The header is three fixed entries and a variable length path: | ||
|
|
||
| * Command: Single byte. Always `0x01`. | ||
| * Chunk size: 32-bit number encoding the amount of data that the client can handle in the first reply. | ||
| * Path length: 16-bit number encoding the encoded length of the path string. | ||
| * Path: UTF-8 encoded string that is *not* null terminated. (We send the length instead.) | ||
|
|
||
| The server will respond with: | ||
| * Command: Single byte. Always `0x01`. | ||
| * Status: Single byte. | ||
| * Total length: 32-bit number encoding the total file length. | ||
| * Chunk length: 32-bit number encoding the length of the read data up to the chunk size provided in the header. | ||
| * Chunk-length contents of the file starting from the current position. | ||
|
|
||
| If the chunk length is smaller than the total length, then the client will request more data by sending: | ||
| * Command: Single byte. Always `0x01`. | ||
| * Status: Single byte. Always OK for now. | ||
| * Chunk size: 32-bit number encoding the number of bytes to read. May be different than the original size. Does not need to be limited by the total size. |
There was a problem hiding this comment.
This seems like it assumes errorless transfer and no dropped packets or acknowledgments, etc. I am not sure how well this will work out in the presence of marginal connections or noise. I don't understand how this is idempotent without specifying the position of the chunk. It seems like the position is implied. based on the sizes of the previous chunks ("current position"). Would you also want a checksum, etc.?
There was a problem hiding this comment.
Ya, the protocol relies on the underlying BLE stack for reliability. This may be a fool's errand but I thought I'd try it first.
By idempotent I mean on a per-command basis (not per-packet.) Maybe it should be per packet... Primarily I wanted to compare against my previous design where two characteristics shared state. One was the file path and that'd influence the state of the second.
There was a problem hiding this comment.
It's just that if it gets out of sync things will go quite badly. I think passing a file offset for the chunk might make monitoring the stream and debugging problems easier. It will provide something to assert against (not literally assert()) when transferring.
I guess a checksum on an already checksummed thing is unnecessary. (The low level has a checksum already, right?)
There was a problem hiding this comment.
READ and WRITE could be modified not to assume that the full contents are being fetched, but just some chunk, specified as offset and length. That would give you easy partial reads and easy overwrites or appends.
There was a problem hiding this comment.
SO says there is a CRC: https://stackoverflow.com/questions/44526541/is-a-cyclic-redundancy-check-required-in-a-ble-service
I'll play with the offsets tomorrow. I think it is a good idea. Thanks!
dhalbert
left a comment
dhalbert
left a comment
There was a problem hiding this comment.
I really like new read/write chunk stuff and its statelessness. DId not have a chance to review the Python code yet, but I"m sure it will evolve.
Yup, doing the python now. |
|
@dhalbert any objection to the added padding? |
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
Sorry, I am late to the party, hopefully it would still be useful. In addition to 3 feedbacks for concurrent access, file appending, remove directory recursively. I think we should also have commands for
- copy file/directory
- move file/directory
- Format which is useful for recovering from corrupted state.
| ``0x20`` - Write a file | ||
| +++++++++++++++++++++++ | ||
|
|
||
| Writes the content to the given full path. If the file exists, it will be overwritten. Content may be written as received so an interrupted transfer may lead to a truncated file. |
There was a problem hiding this comment.
If the file exists, it will be overwritten
Should we have an option to append to existing file, it would be useful for logging.
There was a problem hiding this comment.
You can do this by setting the offset to the end of the file. You can't "leave it open" though. You'll need to know the next total file size.
There was a problem hiding this comment.
how about having -1 or using negative offset with negative mean from right-to-left, it make append easier without first reading the file size. This will help with multiple clients since file size could changes between file op (read size and write) e.g device syslog with multiple client appending.
negative offset could apply to read as well, though it is lot less useful.
There was a problem hiding this comment.
I'm trying to be pretty specific so that everything is clear between the server and client and also specific about memory requirements.
|
Thanks for the feedback @hathach. I think those are all good ideas for a new revision. I've made issues for each of the ideas. For now, I think this version should be v1 and we can implement it with the limitation of a single client. Once we have that going, then we can expand it where it'd be useful. |
5 participants
Comments welcome on the protocol. The test server implementation is in the examples directory and the client is in the library.