-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Lwm2m block transfer #54761
Lwm2m block transfer #54761
Conversation
8700ab0
to
ac6d68c
Compare
ac6d68c
to
b5e4e5c
Compare
I'm working on some small improvements and fixes. So please be aware that some details will change. But the basic idea and the overall code structure will still be the same. |
8beae95
to
ce6b592
Compare
This PR (draft) is ready for review. |
@LukasWoodtli I'm not able to find out the reason, so why are we not using the big buffer for all the messages? I cannot seem to be able to trigger this feature just using the I wonder if we just always render the packet into a big buffer, the split it into multiple BLOCK* CoAPs. |
The choice where to request a big buffer is somewhat arbitrary. I had a look at the code to see where a big message body could potentially be created. |
To trigger this feature you need to set |
Yes this would be a possibility. But it would possibly require a bigger buffer pool. On the other hand it would reduce the complexity of the code a bit. |
In my mind, if we should support this, it should be supported for all packets, not just some. I tried to trigger this feature, but it was not supported for READ requests, and I believe I also failed to trigger this on SEND command. So it is a bit hard to test. We also need to take a look that how many ongoing requests we even could have.. Is it going to block a lot if there is just one big buffer to be used. |
I have added some complexity which could be considered as premature optimization. I’m happy to simplify this part of the code. Regarding number of big buffers: if we activate this feature for all messages we will probably need more big buffers in the pool. I’ll have a look at it asap. But it won’t be this week anymore. |
subsys/net/lib/lwm2m/lwm2m_object.h
Outdated
#define LWM2M_COAP_BLOCK_TRANSFER_ENABLED \ | ||
(CONFIG_LWM2M_COAP_BLOCK_SIZE < CONFIG_LWM2M_COAP_MAX_MSG_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.
I think this should simply be a Kconfig option. Plus probably another one to determine the actual size of the buffer for large message serialization.
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.
I'm happy to add a Kconfig for LWM2M_COAP_BLOCK_TRANSFER_ENABLED
. Maybe anyone has a better name for the config.
But I'm a bit confused about the config for the big buffer. Then we would have tree different sizes for buffers:
- block size
- message size
- big buffer size for serialization
Probably I don't understand the meaning of the message 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.
Message size represents the maximum CoAP payload size the message can carry. In most cases, it should be lower than network MTU (minus CoAP/UDP/IP header sizes) so that the message can fit a single UDP datagram. In case IP fragmentation is supported though, one could try to use even larger message sizes, however I don't think we've ever tested such case.
So to sum up:
LWM2M_COAP_BLOCK_SIZE
represents the CoAP block size size used by LwM2M, this is pretty straightforwardLWM2M_COAP_MAX_MSG_SIZE
represents the maximum payload a single CoAP packet can carry. This should be no lower thanLWM2M_COAP_BLOCK_SIZE
, and block transfer should only be involved if the desired payload size to transmit would exceedLWM2M_COAP_MAX_MSG_SIZE
- Plus, as you've noticed, we'd need a third config for the serialization buffer. This should be higher than
LWM2M_COAP_MAX_MSG_SIZE
, as otherwise it'd make little sense (whole serialization buffer would fit a single CoAP message, so there's no really need for block transfer).
I second such approach. I think this is the most feasible way to determine the actual payload size w/o rewriting content encoders heavily. We could have a preallocated buffer, mutex protected, used only for encoding. Then, if it the message size would exceed the predefined packet size, allocate a dedicated buffer for the block transfer, and send block by block. I don't think we should split into multiple coap messages prematurely, as this kind of blows the idea of block size negotiation. |
My PR is not handling block size negotiation yet. But it would be quite easy to introduce. Currently, the content of a new block to send is determined by the 'fixed' block size and the block number in the response with the CONTINUE code. But with some bookkeeping, it would be possible to handle the case of block size negotiation. Otherwise, the approach is as described. Splitting blocks out of the buffer as needed. |
fba6428
to
d9fb81b
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.
Sorry for late review. Looks like a step in the right direction (at least to me) but I still have some concerns wrt memory management or CON handling (pendings).
d9fb81b
to
59fb027
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.
Thanks for the update @LukasWoodtli. I think we're on par with the actual design, but it'd be good to get a second review on the actual changes.
@SeppoTakalo Please have a look at convenience. |
59fb027
to
fe2c6b5
Compare
The function adds a block option to a CoAP message. If the message is a request the BLOCK1 option is added. Otherwise (response) the BLOCK2 option is added. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
The function gets the the BLOCK1 option from a received message and extracts the block number, block size and has-more flag from it. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
Allow to use a buffer for the complete message body if block transfer is enabled. This buffer is used when serializing the message. For sending the message the body is split into blocks. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
fe2c6b5
to
ff6f6b9
Compare
Add functionality that creates blocks from a big payload body. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
The tests check that a message is correctly split into multiple blocks. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
After sending a CoAP block and receiving the CONTINUE response code the next block is sent. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
The continue code is not considered an error in case of block-wise transfer. Signed-off-by: Lukas Woodtli <lukas.woodtli@husqvarnagroup.com>
ff6f6b9
to
671cd6e
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.
No more comments from my side, thanks.
I experimented with block transfers as introduced here. A few observations:
|
Thank you for the feedback!
I'll have a look at that ASAP.
I will also need to investigate this.
On the fly block-wise transfer would be nice. But I think I'll not have any resources for implementing it in the near future. |
This is not entirely true. The block-wise write only works for opaque binary data. So only when you write a binary blob into one resource. It does not work when the data is encoded as our encoders expects to get the full payload and parse it completely. For example CBOR parsers expects this. Some LwM2M specific parsers might be able to be tweaked to support partial data, but none are currently able to. For example getting LwM2M composite write to work with CoAP block-wise would require this. Any write that has more than one resource requires encoded payload. |
@lodup29 Could you please check if #60764 solves your problem? |
When initializing a lwm2m message it's possible to request a big buffer for serializing the message body. Before sending a message with the big buffer in use, the body is split into blocks.
This approach was chosen due to several reasons.:
There is still some (or a lot) room for improvement:
How to decide if a big buffer is needed for serializing the message? Currently, it's the responsibility of the developer. But there could be a heuristic or another algorithm for deciding if a big buffer should be requested. It could also be set by one or multiple configurations.
There is some copying of buffers involved. This could probably be improved in some cases.
This implementation is just for lwm2m even if block-transfer is a CoAP feature.