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

Lwm2m block transfer #54761

Conversation

LukasWoodtli
Copy link

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.:

  • The implementation is mostly decoupled from the existing code. If block transfer is not needed, the corresponding code is excluded by preprocessor defines (conditional compilation).
  • No need to adjust any code that generates message content (headers, options, body). The existing code just writes to the message buffer as before. But the buffer can be bigger (if requested) than the actual lwm2m packet size.

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.

@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from 8700ab0 to ac6d68c Compare February 13, 2023 08:44
@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from ac6d68c to b5e4e5c Compare February 13, 2023 15:03
@LukasWoodtli
Copy link
Author

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.

@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch 3 times, most recently from 8beae95 to ce6b592 Compare February 15, 2023 15:52
@LukasWoodtli
Copy link
Author

This PR (draft) is ready for review.
Please give me some feedback. Thanks!

@SeppoTakalo
Copy link
Collaborator

@LukasWoodtli I'm not able to find out the reason, so why are we not using the big buffer for all the messages?
Why only some (registration, Notify)?

I cannot seem to be able to trigger this feature just using the native_posix build.

I wonder if we just always render the packet into a big buffer, the split it into multiple BLOCK* CoAPs.

@LukasWoodtli
Copy link
Author

I'm not able to find out the reason, so why are we not using the big buffer for all the messages?

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.
How to decide where a big buffer is requested is definitely a topic for discussion and there is room for improvement.

@LukasWoodtli
Copy link
Author

LukasWoodtli commented Feb 17, 2023

I cannot seem to be able to trigger this feature just using the native_posix build.

To trigger this feature you need to set CONFIG_LWM2M_COAP_MAX_MSG_SIZE to a value that is a multiple of CONFIG_LWM2M_COAP_BLOCK_SIZE.

@LukasWoodtli
Copy link
Author

I wonder if we just always render the packet into a big buffer, the split it into multiple BLOCK* CoAPs.

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.

@SeppoTakalo
Copy link
Collaborator

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.
I think we should aim for less complexity and create packets always on big buffer, instead of just some selected messages.

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.

@LukasWoodtli
Copy link
Author

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. I think we should aim for less complexity and create packets always on big buffer, instead of just some selected messages.

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/coap/coap.c Show resolved Hide resolved
subsys/net/lib/lwm2m/Kconfig Outdated Show resolved Hide resolved
Comment on lines 138 to 139
#define LWM2M_COAP_BLOCK_TRANSFER_ENABLED \
(CONFIG_LWM2M_COAP_BLOCK_SIZE < CONFIG_LWM2M_COAP_MAX_MSG_SIZE)
Copy link
Contributor

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.

Copy link
Author

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:

  1. block size
  2. message size
  3. big buffer size for serialization

Probably I don't understand the meaning of the message size.

Copy link
Contributor

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 straightforward
  • LWM2M_COAP_MAX_MSG_SIZE represents the maximum payload a single CoAP packet can carry. This should be no lower than LWM2M_COAP_BLOCK_SIZE, and block transfer should only be involved if the desired payload size to transmit would exceed LWM2M_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).

@rlubos
Copy link
Contributor

rlubos commented Feb 22, 2023

I wonder if we just always render the packet into a big buffer, the split it into multiple BLOCK* CoAPs.

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.

@LukasWoodtli
Copy link
Author

I wonder if we just always render the packet into a big buffer, the split it into multiple BLOCK* CoAPs.

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.
I suggest adding that in a separate PR.

Otherwise, the approach is as described. Splitting blocks out of the buffer as needed.

@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch 3 times, most recently from fba6428 to d9fb81b Compare March 8, 2023 07:32
@LukasWoodtli LukasWoodtli requested review from rlubos and removed request for SeppoTakalo March 8, 2023 08:11
Copy link
Contributor

@rlubos rlubos left a 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).

subsys/net/lib/lwm2m/lwm2m_object.h Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/Kconfig Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/Kconfig Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/Kconfig Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/Kconfig Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_rd_client.c Outdated Show resolved Hide resolved
@rlubos rlubos requested a review from SeppoTakalo March 21, 2023 12:07
@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from d9fb81b to 59fb027 Compare April 20, 2023 14:16
@LukasWoodtli LukasWoodtli marked this pull request as ready for review April 20, 2023 14:28
Copy link
Contributor

@rlubos rlubos left a 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.

subsys/net/lib/coap/coap.c Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Outdated Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
subsys/net/lib/lwm2m/lwm2m_message_handling.c Show resolved Hide resolved
tests/net/lib/lwm2m/block_transfer/src/main.c Show resolved Hide resolved
@rlubos
Copy link
Contributor

rlubos commented Apr 21, 2023

@SeppoTakalo Please have a look at convenience.

@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from 59fb027 to fe2c6b5 Compare May 4, 2023 12:15
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>
@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from fe2c6b5 to ff6f6b9 Compare May 4, 2023 14:39
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>
@LukasWoodtli LukasWoodtli force-pushed the lwm2m-block-transfer-full-body-buffer branch from ff6f6b9 to 671cd6e Compare May 4, 2023 14:58
@LukasWoodtli LukasWoodtli requested a review from rlubos May 8, 2023 08:06
Copy link
Contributor

@rlubos rlubos left a 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.

@carlescufi carlescufi merged commit fb924e2 into zephyrproject-rtos:main May 15, 2023
24 checks passed
@lodup29
Copy link
Contributor

lodup29 commented May 20, 2023

I experimented with block transfers as introduced here. A few observations:

  • CoAP response code 2.31 (COAP_RESPONSE_CODE_CONTINUE) is not handled everywhere. do_send_reply_cb is called after the first block with this code. As a result, the callback registered by lwm2m_send_cb is called with a failure code but on the line, everything looks good:
    send_ok
  • Reading the resource fails. The client keeps responding with block # 0 even if block # 1 is requested:
    read_fail
  • I think it would make sense to be able to build the payload block by block on the fly with block aware callbacks for read and send operations as is it the case for the write operation.

@LukasWoodtli
Copy link
Author

Thank you for the feedback!

* CoAP response code 2.31 (COAP_RESPONSE_CODE_CONTINUE) is not handled everywhere. [do_send_reply_cb](https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/net/lib/lwm2m/lwm2m_message_handling.c#L3197) is called after the first block with this code. As a result, the callback registered by lwm2m_send_cb is called with a failure code but on the line, everything looks good:

I'll have a look at that ASAP.

* Reading the resource fails. The client keeps responding with block # 0 even if block # 1 is requested:

I will also need to investigate this.

* I think it would make sense to be able to build the payload block by block on the fly with block aware callbacks for read and send operations [as is it the case for the write operation](https://github.com/zephyrproject-rtos/zephyr/blob/d01780fc9403166d250ded3d02372a7e1a9db4fd/include/zephyr/net/lwm2m.h#L290).

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.
I also think that we need to keep the current case where the whole body is encoded beforehand in a big buffer.

@SeppoTakalo
Copy link
Collaborator

I think it would make sense to be able to build the payload block by block on the fly with block aware callbacks for read and send operations as is it the case for the write operation.

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.

@LukasWoodtli
Copy link
Author

LukasWoodtli commented Jul 25, 2023

I experimented with block transfers as introduced here. A few observations:

* CoAP response code 2.31 (COAP_RESPONSE_CODE_CONTINUE) is not handled everywhere. [do_send_reply_cb](https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/net/lib/lwm2m/lwm2m_message_handling.c#L3197) is called after the first block with this code. As a result, the callback registered by lwm2m_send_cb is called with a failure code but on the line, everything looks good:
  ![send_ok](https://user-images.githubusercontent.com/6696893/239693702-d6ded685-1469-409a-9660-853d188a56c8.png)

* Reading the resource fails. The client keeps responding with block # 0 even if block # 1 is requested:
  ![read_fail](https://user-images.githubusercontent.com/6696893/239693690-3a534df6-31d7-42d9-a21a-77ad0e5d5d08.png)

* I think it would make sense to be able to build the payload block by block on the fly with block aware callbacks for read and send operations [as is it the case for the write operation](https://github.com/zephyrproject-rtos/zephyr/blob/d01780fc9403166d250ded3d02372a7e1a9db4fd/include/zephyr/net/lwm2m.h#L290).

@lodup29 Could you please check if #60764 solves your problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants