Skip to content

Commit

Permalink
#15 Fix user-defined header types w/fragmentation
Browse files Browse the repository at this point in the history
Since a fragment_id and fragment type is sent, we can include and
restore the user-defined header type with fragmented payloads by
including it with the final fragment, since we know its expected
fragment_id and that this should be the last expected fragment.
  • Loading branch information
TMRh20 committed Sep 11, 2014
1 parent a4b6076 commit 6a79f9b
Showing 1 changed file with 11 additions and 7 deletions.
18 changes: 11 additions & 7 deletions RPi/RF24Network/RF24Network.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,15 +193,15 @@ bool RF24Network::enqueue(RF24NetworkFrame frame) {
bool result = false;


if (frame.header.fragment_id > 1 && (frame.header.type == NETWORK_MORE_FRAGMENTS || frame.header.type== NETWORK_FIRST_FRAGMENT)){
if ((frame.header.fragment_id > 1 && (frame.header.type == NETWORK_MORE_FRAGMENTS || frame.header.type== NETWORK_FIRST_FRAGMENT)) ){
//Set the more fragments flag to indicate a fragmented frame
IF_SERIAL_DEBUG_FRAGMENTATION(printf("%u: FRG fragmented payload of size %i Bytes with fragmentID '%i' received.\n\r",millis(),frame.message_size,frame.header.fragment_id););

//Append payload
appendFragmentToFrame(frame);
result = true;

} else if (frame.header.fragment_id == 1 && frame.header.type == NETWORK_LAST_FRAGMENT) {
result = true;
}else if ( frame.header.type == NETWORK_LAST_FRAGMENT) {

This comment has been minimized.

Copy link
@reixd

reixd Sep 11, 2014

Contributor

I think we should nevertheless check if (frame.header.fragment_id == 1), otherwise it would be a bad (fragmentation-)protocol behavior. Mostly it is redundant but could also be used to avoid obscure problems like a bit inversion during tx and this inversion does not affect the NRF24 CRC. A small chance is there ;)

This comment has been minimized.

Copy link
@TMRh20

TMRh20 Sep 11, 2014

Author Member

Due to the confusing method of replacing the fragment_id with the type on the last fragment, I think this is a small sacrifice to make, given some of the other options for maintaining the header types.

This comment has been minimized.

Copy link
@reixd

reixd Sep 11, 2014

Contributor

Then a comment for this code should be made to explain this decision.

//Set the last fragment flag to indicate the last fragment
IF_SERIAL_DEBUG_FRAGMENTATION(printf("%u: FRG Last fragment with size %i Bytes and fragmentID '%i' received.\n\r",millis(),frame.message_size,frame.header.fragment_id););

Expand Down Expand Up @@ -262,6 +262,10 @@ void RF24Network::appendFragmentToFrame(RF24NetworkFrame frame) {
//Append payload
RF24NetworkFrame *f = &(frameFragmentsCache[ std::make_pair(frame.header.from_node,frame.header.id) ]);

if(frame.header.type == NETWORK_LAST_FRAGMENT){
f->header.type = frame.header.fragment_id;

This comment has been minimized.

Copy link
@reixd

reixd Sep 11, 2014

Contributor

We need to comment this "feature" in the code. It is not easy to see, that this is not an error.

frame.header.fragment_id = 1;
}
//Error checking for missed fragments and payload size
if(frame.header.fragment_id != f->header.fragment_id-1){
if(frame.header.fragment_id > f->header.fragment_id){
Expand Down Expand Up @@ -407,7 +411,7 @@ bool RF24Network::write(RF24NetworkHeader& header,const void* message, size_t le

IF_SERIAL_DEBUG_FRAGMENTATION(printf("%u: FRG Total message fragments %i\n\r",millis(),fragment_id););

bool firstFrag = 1;

//Iterate over the payload chuncks
// Assemble a new message, copy and fill out the header
// Try to send this message
Expand All @@ -422,10 +426,10 @@ bool RF24Network::write(RF24NetworkHeader& header,const void* message, size_t le

if (fragment_id == 1) {
fragmentHeader.type = NETWORK_LAST_FRAGMENT; //Set the last fragment flag to indicate the last fragment
fragmentHeader.fragment_id = header.type;

This comment has been minimized.

Copy link
@reixd

reixd Sep 11, 2014

Contributor

We need to comment this "feature" in the code. It is not easy to see, that this is not an error.

} else {
if(firstFrag == 1){
if(msgCount == 0){
fragmentHeader.type = NETWORK_FIRST_FRAGMENT;
firstFrag = 0;
}else{
fragmentHeader.type = NETWORK_MORE_FRAGMENTS; //Set the more fragments flag to indicate a fragmented frame
}
Expand Down

2 comments on commit 6a79f9b

@reixd
Copy link
Contributor

@reixd reixd commented on 6a79f9b Sep 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you commited the header file with the new defined types?

@TMRh20
Copy link
Member Author

@TMRh20 TMRh20 commented on 6a79f9b Sep 11, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only new type is the NETWORK_FIRST_FRAGMENT, and yeah, it was added on the previous commit.

Please sign in to comment.