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.

Sign up for GitHub

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

Jump to bottom

[mbed-os-5.15] Update mbed-coap to version v5.1.10 #14682

Merged
merged 1 commit into from
Jun 10, 2021
Merged

[mbed-os-5.15] Update mbed-coap to version v5.1.10 #14682

merged 1 commit into from
Jun 10, 2021

Conversation

anttiylitokola
Copy link
Contributor

@anttiylitokola anttiylitokola commented May 19, 2021 •
edited
Loading

Summary of changes

  • Fix regression from previous release concerning 1024 byte blocksize operations.
  • Do not store ACK's into duplicate list.
  • Code size optimizations

Impact of changes

Migration actions required

Documentation

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@teetak01 @AnttiKauppila

[mbed-os-5.15] Update mbed-coap to version v5.1.10
ee201bc 
- Fix regression from previous release concerning 1024 byte blocksize operations.
- Do not store ACK's into duplicate list.
- Code size optimizations
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested a review from a team May 19, 2021 12:00
@0xc0170
Copy link
Contributor

0xc0170 commented May 21, 2021

CI started

@ciarmcom ciarmcom added the stale Stale Pull Request label May 26, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels May 28, 2021
adbridge
adbridge approved these changes Jun 4, 2021
View reviewed changes
features/frameworks/mbed-coap/mbed-coap/sn_config.h
#define SN_COAP_CONSTANT_NEEDED_SIZE MBED_CONF_MBED_CLIENT_SN_COAP_CONSTANT_NEEDED_SIZE
#endif

//#define SN_COAP_CONSTANT_NEEDED_SIZE 1024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove redundant code rather than comment out

features/frameworks/mbed-coap/source/include/sn_coap_protocol_internal.h
uint16_t count_resent_msgs;
#endif
#if SN_COAP_DUPLICATION_MAX_MSGS_COUNT
uint16_t count_duplication_msgs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the massive spacing here ?

features/frameworks/mbed-coap/source/include/sn_coap_protocol_internal.h
uint8_t *payload_ptr;
unsigned int use_size1:1;

uint32_t timestamp; /* Tells when Payload is stored to Linked list */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the massive spacing? Are you using tabs instead of spaces ?

@mergify mergify bot added needs: CI and removed needs: review labels Jun 4, 2021
@adbridge
Copy link
Contributor

adbridge commented Jun 4, 2021

I've approved based on the fact that presumably you maintain coap outside of Mbed-os so may not be able to address the comments I provided. But it would be good to do so if possible.

@adbridge adbridge requested a review from andypowers June 4, 2021 12:12
andypowers
andypowers approved these changes Jun 4, 2021
View reviewed changes
Copy link
Collaborator

@andypowers andypowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 4, 2021
@ciarmcom
Copy link
Member

ciarmcom commented Jun 7, 2021

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

@ciarmcom ciarmcom added the stale Stale Pull Request label Jun 7, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 7, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Jun 7, 2021

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_example-test-lts

@mergify mergify bot added needs: work and removed needs: CI labels Jun 7, 2021
@ciarmcom ciarmcom removed the stale Stale Pull Request label Jun 7, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 10, 2021

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jun 10, 2021

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit 54b8523 into ARMmbed:mbed-os-5.15 Jun 10, 2021
@mergify mergify bot removed the ready for merge label Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbridge adbridge adbridge approved these changes

@teetak01 teetak01 teetak01 approved these changes

@andypowers andypowers andypowers approved these changes

Assignees
No one assigned
Labels
release-version: 5.15.8
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anttiylitokola @ciarmcom @0xc0170 @adbridge @mbed-ci @teetak01 @andypowers