Feature/ftp - #157
Feature/ftp#157victorsowa12 wants to merge 5 commits into
Conversation
matt001k
left a comment
matt001k
left a comment
There was a problem hiding this comment.
For the most part it looks good, I think we should consider maybe doing something different with the endpoint logic although. Happy to discuss when you are back!
| case BcmpFTPStartMessage: { | ||
| BmFtpStart *start = (BmFtpStart *)buf; | ||
| swap_ftp_address(&start->addresses); | ||
| swap_32bit(&start->transfer_id); | ||
| swap_32bit(&start->total_size); | ||
| swap_16bit(&start->requested_chunk_size); | ||
| swap_16bit(&start->crc16); | ||
| swap_16bit(&start->sink_spec_len); | ||
| } break; | ||
| case BcmpFTPAckMessage: { | ||
| BmFtpAck *ack = (BmFtpAck *)buf; | ||
| swap_ftp_address(&ack->addresses); | ||
| swap_32bit(&ack->transfer_id); | ||
| swap_32bit(&ack->total_size); | ||
| swap_16bit(&ack->crc16); | ||
| swap_16bit(&ack->chunk_size); | ||
| } break; | ||
| case BcmpFTPChunkReqMessage: { | ||
| BmFtpChunkRequest *request = (BmFtpChunkRequest *)buf; | ||
| swap_ftp_address(&request->addresses); | ||
| swap_32bit(&request->transfer_id); | ||
| swap_32bit(&request->offset); | ||
| swap_16bit(&request->length); | ||
| swap_16bit(&request->reserved); | ||
| } break; | ||
| case BcmpFTPChunkMessage: { | ||
| BmFtpChunk *chunk = (BmFtpChunk *)buf; | ||
| swap_ftp_address(&chunk->addresses); | ||
| swap_32bit(&chunk->transfer_id); | ||
| swap_32bit(&chunk->offset); | ||
| swap_16bit(&chunk->payload_length); | ||
| swap_16bit(&chunk->reserved); | ||
| } break; | ||
| case BcmpFTPEndMessage: { | ||
| BmFtpEnd *end = (BmFtpEnd *)buf; | ||
| swap_ftp_address(&end->addresses); | ||
| swap_32bit(&end->transfer_id); | ||
| swap_16bit(&end->reserved); | ||
| swap_32bit(&end->bytes_received); | ||
| swap_16bit(&end->running_crc16); | ||
| swap_16bit(&end->reserved2); | ||
| } break; | ||
| case BcmpFTPAbortMessage: { | ||
| BmFtpAbort *abort = (BmFtpAbort *)buf; | ||
| swap_ftp_address(&abort->addresses); | ||
| swap_32bit(&abort->transfer_id); | ||
| } break; | ||
| case BcmpFTPFetchMessage: { | ||
| BmFtpFetch *fetch = (BmFtpFetch *)buf; | ||
| swap_ftp_address(&fetch->addresses); | ||
| swap_32bit(&fetch->transfer_id); | ||
| swap_16bit(&fetch->source_spec_len); | ||
| } break; |
There was a problem hiding this comment.
I think we should get rid of this endian swap logic, or move the logic for each swap to its own file, it adds a lot of noise to this file in general.
Should not be done now, but I will make an issue in the repo.
| } | ||
| } | ||
|
|
||
| static void bm_ftp_event_thread(void *parameters) { |
There was a problem hiding this comment.
I am a little hesitant to add more threads to bm_core, but maybe a later task can be to determine how we can reduce the number of threads? But there is the potential overhead of writing to/reading from flash that makes having another task good for this purpose.
Something to think about.
|
|
||
| // I'm not sure if I want these, or if I want | ||
| // the nodes to define them arbitrarily themselves. | ||
| typedef enum { |
There was a problem hiding this comment.
What are the point of the different endpoints?
Thinking about the Spotter Bridge, it has onboard flash and potential access to the Spotter's SD card.
Could all of these files be represented as a single entity agnostic to the endpoint it comes from?
Do nodes care if it is being accessed over bm_serial or internally on flash?
I can see the spec of a file on the Bridge being prepended with /bridge/{file_name} or /spotter/{file_name} as the way to delineate where the file is coming from (or going to), that way it leaves the transfer agnostic to the endpoint and everytime a new endpoint kind comes up this does not have to be updated.
| if (coordinator.state != BmFtpCoordinatorReceiving || | ||
| chunk->addresses.src_node_id != coordinator.peer_node_id || | ||
| chunk->transfer_id != coordinator.transfer_id || | ||
| chunk->offset != coordinator.bytes_received || chunk->payload_length == 0 || | ||
| chunk->payload_length > coordinator.chunk_size || | ||
| chunk->payload_length > coordinator.total_size - coordinator.bytes_received) { |
There was a problem hiding this comment.
This conditional is pretty long, it would be nice to clean this up into some boolean variables that can be condensed down, i.e.:
bool payload_len_invalid = chunk->payload_length > coordinator.chunk_size ||
chunk->payload_length > coordinator.total_size - coordinator.bytes_received;
bool transfer_invalid = chunk->transfer_id != coordinator.transfer_id ||
chunk->offset != coordinator.bytes_received || chunk->payload_length == 0;
if (coordinator.state != BmFtpCoordinatorReceiving ||
chunk->addresses.src_node_id != coordinator.peer_node_id ||
payload_len_invalid || transfer_invalid) {
return BmEBADMSG;
}This branch has not been deployed
2 participants
What changed?
How does it make Bristlemouth better?
Where should reviewers focus?
Checklist