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

Update mbed-coap to version 5.1.0 #11359

Merged
merged 3 commits into from Aug 29, 2019

Conversation

@anttiylitokola
Copy link
Contributor

commented Aug 28, 2019

Description

  • Reduce heap footprint by storing only single block when receiving a blockwise message.
    • Feature is controlled through SN_COAP_REDUCE_BLOCKWISE_HEAP_FOOTPRINT configuration flag. Flag is disabled by default to keep the backward compatibility in place. If flag is enabled, application must NOT free the payload when it gets the COAP_STATUS_PARSER_BLOCKWISE_MSG_RECEIVED status. And application must call sn_coap_protocol_block_remove() instead.
  • Bug fix: Request blockwise transfer if incoming payload length is too large and when it comes without block indication.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@yogpan01, @teetak01

- Reduce heap footprint by storing only single block when receiving a blockwise message.
    * User is now responsible of freeing the data by calling sn_coap_protocol_block_remove() and must not free the payload separately.
- Bug fix: Request blockwise transfer if incoming payload length is too large and when it comes without block indication.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Is this breaking change here: * User is now responsible of freeing the data by calling sn_coap_protocol_block_remove() and must not free the payload separately.
?

Or what is breaking and why? Please add release notes section above. Isn't this just functional change ?

@yogpan01

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

@0xc0170 Yes, you are right that this is a breaking change as the API behaviour has changed. Earlier using this API, this library handled the memory allocation but it was not memory optimized and caused inflated memory heap usage. In this version, we have given that control to the application which means application will have to implement the freeing logic on their end.
That's why we have also bumped the coap library version info to 5.0.0 reflecting this break.
@anttiylitokola Please update the CHANGELOG to clearly reflect that this will be a breaking change for previous users of the given blockwise API.

@ciarmcom ciarmcom requested review from teetak01, yogpan01 and ARMmbed/mbed-os-maintainers Aug 28, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@anttiylitokola, thank you for your changes.
@yogpan01 @teetak01 @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 requested a review from bulislaw Aug 28, 2019
@anttiylitokola

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@0xc0170 Yes, you are right that this is a breaking change as the API behaviour has changed. Earlier using this API, this library handled the memory allocation but it was not memory optimized and caused inflated memory heap usage. In this version, we have given that control to the application which means application will have to implement the freeing logic on their end.
That's why we have also bumped the coap library version info to 5.0.0 reflecting this break.
@anttiylitokola Please update the CHANGELOG to clearly reflect that this will be a breaking change for previous users of the given blockwise API.

Changelog has been updated.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

@anttiylitokola also add release section (it was in the PR template) and add details there (we use these for real release notes).

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 28, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

CI started

Copy link
Member

left a comment

waiting for the release notes section

@anttiylitokola

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

@anttiylitokola also add release section (it was in the PR template) and add details there (we use these for real release notes).

@0xc0170, is the release note section ok now?

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 28, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Restarting the build, wifi example failed but looks like timeout to me

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 28, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Error ToolException: Compile did not finish in 10 minutes ? -11 from ARM again.

cc @ARMmbed/mbed-os-test Please review

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Restarted CI

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 28, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Failure not related

@ARMmbed/mbed-os-crypto updated the example for upcoming crypto, trying to fix it now

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Ci restarted (should be fixed now)

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 28, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

cloud client restarted

@anttiylitokola

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

cloud client restarted

cloud client tests will fail due to this breaking change.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@anttiylitokola Please talk to @OPpuolitaival how to fix this. I would assume the test would be fixed for this PR and master (update, restart, integrate).

@OPpuolitaival

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Waiting for pelion device management client release candidate for testing in CI

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Waiting for Release candidate for testing in CI

I don't follow. What shall we do here?

@yogpan01

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@OPpuolitaival there wont be any release candidate since it will require this fix to be merged to mbed-os. This is a typical "Catch22" situation.
I am asking @anttiylitokola to check if we can make this change as backward compatible to make your tests GREEN.
Although I fail to understand if we clearly mention that this is a breaking change and we are proposing it as part of a major release, why such bureaucracy around it.

By default CoAP will create a copy of the whole data to be passed to application and it keeps the backward compatibility.

If enabled, application must NOT free the payload when it gets the COAP_STATUS_PARSER_BLOCKWISE_MSG_RECEIVED status.
And application must call sn_coap_protocol_block_remove() instead.
@anttiylitokola anttiylitokola changed the title Update mbed-coap to version 5.0.0 Update mbed-coap to version 5.1.0 Aug 29, 2019
@anttiylitokola

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Feature is now behind a flag which is disabled by default to maintain backward compatibility.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 29, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 29, 2019
@0xc0170 0xc0170 merged commit 40a84fc into ARMmbed:master Aug 29, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8518 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.