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

coap: zephyr: implement server-negotiated block size during block upload #570

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

szczys
Copy link
Contributor

@szczys szczys commented Sep 3, 2024

  • Add a negotiated_blocksize context member to store desired block size from server
  • Test the negotiated_blocksize and update blockwise_context accordingly
  • Use the negotiated_blocksize when appending the block1 szx (block size) on future coap packets.
    • At this point in the code we don't ave access to blockwise_context so this is the next best option. This is a bit convoluted and could be cleaned up in future work.

resolves https://github.com/golioth/firmware-issue-tracker/issues/628

Copy link

github-actions bot commented Sep 3, 2024

Visit the preview URL for this PR (updated for commit 86adf2d):

https://golioth-firmware-sdk-doxygen-dev--pr570-szczys-blocksi-ie16jg7s.web.app

(expires Mon, 23 Sep 2024 15:21:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a9993e61697a3983f3479e468bcb0b616f9a0578

@szczys szczys force-pushed the szczys/blocksize-server-suggests branch from 858afc7 to b8f8694 Compare September 3, 2024 19:51
@szczys szczys changed the title Szczys/blocksize server suggests coap: zephyr: implement server-negotiated block size during block upload Sep 3, 2024
@szczys szczys force-pushed the szczys/blocksize-server-suggests branch 2 times, most recently from 91e1a29 to 20ed573 Compare September 3, 2024 21:49
Copy link

github-actions bot commented Sep 3, 2024

Code Coverage

Type Coverage
lines 57.8% (1302 of 2251 lines)
functions 66.2% (129 of 195 functions)

@szczys szczys force-pushed the szczys/blocksize-server-suggests branch 2 times, most recently from d350687 to fc504e7 Compare September 4, 2024 02:39
@szczys szczys marked this pull request as draft September 4, 2024 14:08
@szczys szczys force-pushed the szczys/blocksize-server-suggests branch from fc504e7 to 05afcb7 Compare September 4, 2024 18:52
@szczys szczys force-pushed the szczys/add-blocksize-negotiation branch 4 times, most recently from 4b38e9d to 7ed09a6 Compare September 14, 2024 00:32
Base automatically changed from szczys/add-blocksize-negotiation to main September 14, 2024 20:21
Add SZX_TO_BLOCKSIZE macro to convert from coap enum value to bytes

Signed-off-by: Mike Szczys <mike@golioth.io>
Remove macro that is more broadly available from coap_client.h

Signed-off-by: Mike Szczys <mike@golioth.io>
When performing blockwise upload, the server may negotiate a smaller
block size than the device begins with.

- Add a struct member to track this using post_block_ctx which is
  accessible both in the block upload functions as well as the coap thread
  functions. Use the _szx suffice to indicate coap block size enum value
  (as opposed to bytes).
- When the server requests a smaller size, update the blockwise_transfer
  context so the value is used on subsequent blocks.
- When the block_size is updated to a smaller value, the number of the next
  block needs to be recalculated because the previous transfer actually
  included more than one block of the newly negotiated size.

Signed-off-by: Mike Szczys <mike@golioth.io>
When the server responds to a block upload request with a smaller
block size, store the value to inform the negotiated block size.

Signed-off-by: Mike Szczys <mike@golioth.io>
Use the negotiated block size when initializing the block1 option of the
coap packet.

This moves the use of the compiled-in
CONFIG_GOLIOTH_BLOCKWISE_UPLOAD_MAX_BLOCK_SIZE value to the
initialization step of the negotiated value, and allows for the negotiated
value to be updated based on the server preference, then used for all
subsequent coap requests in the block upload process.

The Kconfig value was previously masked by a bitwise operation. This should
not be necessary as that symbol was updated to a 'choice' in Kconfig so we
can be confident that the value is valid coap size or it would have
triggered a static assert.

Signed-off-by: Mike Szczys <mike@golioth.io>
@szczys szczys force-pushed the szczys/blocksize-server-suggests branch from 05afcb7 to 86adf2d Compare September 16, 2024 15:20
@szczys szczys marked this pull request as ready for review September 16, 2024 15:25
Copy link

Code Coverage (Linux)

Type Coverage
lines 65.4% (1404 of 2148 lines)
functions 77.7% (139 of 179 functions)

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.

1 participant