-
Notifications
You must be signed in to change notification settings - Fork 2k
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
nanocoap: add serverside block1 support #8788
nanocoap: add serverside block1 support #8788
Conversation
ad95389
to
9558aab
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.
Initial review, I'll do a bit more thorough review as soon as #8772 is merged
unsigned szx; /**< szx value */ | ||
int more; /**< -1 for no option, 0 for last block, | ||
1 for more blocks coming */ | ||
} coap_block1_t; |
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.
Can be reused for block2? If so, can it be changed to coap_block_t
?
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.
Probably. I propose renaming at that point.
bea5296
to
eaea004
Compare
|
I'm doing some extensive testing (feeding gigabyte size data to the sha256 tester) and I'm seeing some weird things:
snip
This is produced with the nanocoap_server example on native using libcoaps coap-client to feed it data: I've wiresharked the packets, they all have size 64 there. |
/* must be sorted by path (alphabetically) */ | ||
const coap_resource_t coap_resources[] = { | ||
COAP_WELL_KNOWN_CORE_DEFAULT_HANDLER, | ||
{ "/sha256", COAP_POST | COAP_PUT, _sha256_handler, NULL }, |
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.
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.
fixed
sys/include/net/nanocoap.h
Outdated
int coap_get_blockopt(coap_pkt_t *pkt, uint16_t option, uint32_t *blknum, unsigned *szx); | ||
|
||
/** | ||
* @brief Block1 helper |
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.
"Block1 helper" sounds a bit too generic to me. Maybe "Block1 option getter" is more clear.
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.
ok
sys/include/net/nanocoap.h
Outdated
* @param[out] block1 ptr to preallocated coap_block1_t structure | ||
* | ||
* @returns 0 if block1 option not present | ||
* @returns 1 if structure has been filles |
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.
filles -> filled
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.
fixed
int coap_get_block1(coap_pkt_t *pkt, coap_block1_t *block1); | ||
|
||
/** | ||
* @brief Insert block1 option into buffer |
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.
"into the buffer"?
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.
To me "the buffer" implies a specific buffer, but the function can insert the option in any buffer.
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.
Ok
size_t coap_put_option_block1(uint8_t *buf, uint16_t lastonum, unsigned blknum, unsigned szx, int more); | ||
|
||
/** | ||
* @brief Insert block1 option into buffer (from coap_block1_t) |
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.
"into the buffer"?
sys/include/net/nanocoap.h
Outdated
* | ||
* @param[in] szx SZX value to convert | ||
* | ||
* @returns size |
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.
Maybe expand a bit to "size in bytes" to clarify a bit.
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.
fixed
* @param[in] pkt pkt to work on | ||
* @param[in] option actual block option number to get | ||
* @param[out] blknum block number | ||
* @param[out] szx SZX value |
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.
Maybe clarify here that these parameters are modified to zero if the option is not found.
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.
did
Does this happen a lot? I've been feeding q couple hundred jiggabytes through the tester and so far, I cannot reproduce. |
After testing a bit more this appears to be a variant of #6123, except that I'm hammering the instance here with CoAP packets instead of ping floods. This results in sock returning a far too large packet size (several times more than the mtu size). So this issue is unrelated to this PR. |
Ack, please squash |
8aef938
to
b42159d
Compare
@kaspar030 It looks like the commits are in the wrong order, could you rebase it to first the block1 support and then the commit adding example. Otherwise people are going to complain when bisecting. |
@bergzand github often messes up the order. the actual commit order in the branch is as you suggested. |
Ah, you're right, never mind then |
…e_block1 nanocoap: add serverside block1 support
Contribution description
This PR makes use of the new option handling (introduced in #8772) and adds support for server-side blockwise (block1) support.
Issues/PRs references
Waiting for #8772.