Skip to content
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

Client example show payload on block1 completion #383

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Conversation

kb2ma
Copy link
Contributor

@kb2ma kb2ma commented Jul 26, 2019

In a fix to RIOT's block implementation, we found that the libcoap client example does not show the payload on the final response of a block1 exchange. However, the client does show the payload when there is not a block option present.

This PR simply outputs the response payload in the block1 case.

@@ -500,6 +500,10 @@ message_handler(struct coap_context_t *ctx,
if (payload.length <= (block.num+1) * (1 << (block.szx + 4))) {
coap_log(LOG_DEBUG, "upload ready\n");
ready = 1;

if (coap_get_data(received, &len, &databuf))
append_to_output(databuf, len);
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if this needs to be after the following check for duplicate block1? Otherwise, we might end up adding the payload multiple times in case of duplicates.

Copy link
Contributor Author

@kb2ma kb2ma Aug 4, 2019

Choose a reason for hiding this comment

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

We only want the message to print on receipt of the last block1 response. Even if the last block is duplicated, the output routine needs to be within this if (payload.length... conditional, right? In that case, I can verify ready has not been set, like below. Please confirm and I'll push the change.

        if (payload.length <= (block.num+1) * (1 << (block.szx + 4))) {
          coap_log(LOG_DEBUG, "upload ready\n");
          /* check 'ready' to ensure not duplicate ACK */
          if (!ready && coap_get_data(received, &len, &databuf))
            append_to_output(databuf, len);

          ready = 1;
          return;
        }

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is too much reliance on ready not being used elsewhere with your updated solution. I would prefer

diff --git a/examples/client.c b/examples/client.c
index edfe798..23cdd2f 100644
--- a/examples/client.c
+++ b/examples/client.c
@@ -497,11 +497,6 @@ message_handler(struct coap_context_t *ctx,
           }
         }
 
-        if (payload.length <= (block.num+1) * (1 << (block.szx + 4))) {
-          coap_log(LOG_DEBUG, "upload ready\n");
-          ready = 1;
-          return;
-        }
         if (last_block1_tid == received->tid) {
           /*
            * Duplicate BLOCK1 ACK
@@ -516,6 +511,14 @@ message_handler(struct coap_context_t *ctx,
         }
         last_block1_tid = received->tid;
 
+        if (payload.length <= (block.num+1) * (1 << (block.szx + 4))) {
+          coap_log(LOG_DEBUG, "upload ready\n");
+          if (coap_get_data(received, &len, &databuf))
+            append_to_output(databuf, len);
+          ready = 1;
+          return;
+        }
+
         /* create pdu with request for next block */
         pdu = coap_new_request(ctx, session, method, NULL, NULL, 0); /* first, create bare PDU w/o any option  */
         if (pdu) {

If the final BLOCK1 response is large enough to need BLOCK2, this is handled earlier in the message_handler() code with the appropriate data being output.

@mrdeep1
Copy link
Collaborator

mrdeep1 commented Aug 23, 2019

@kb2ma Can you make the updated suggested change and push the change (rebasing if needed to make it just one patch).

@kb2ma
Copy link
Contributor Author

kb2ma commented Aug 23, 2019

Will do. Apologies for delay.

@kb2ma
Copy link
Contributor Author

kb2ma commented Aug 25, 2019

Rebased and pushed the single change as described. I see the final payload for block1 and block2, as well as the payload without a blockwise exchange.

@mrdeep1
Copy link
Collaborator

mrdeep1 commented Aug 25, 2019

Thanks - appreciated

@obgm obgm merged commit 16b77c3 into obgm:develop Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants