Skip to content

Update mbed-client-c version 3.0.4 #3500

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
Merged
merged 1 commit into from
Dec 30, 2016
Merged

Update mbed-client-c version 3.0.4 #3500

merged 1 commit into from
Dec 30, 2016

Conversation

kuggenhoffen
Copy link
Contributor

Description

Valgrind error fixes, CoAP library bugfixes and CoAP message id randomization. Added compile time flag to enable new memory optimized API.

Status

READY

Migrations

NO

@kuggenhoffen
Copy link
Contributor Author

don't merge yet, let's wait for our CI jobs to finish

@kuggenhoffen kuggenhoffen force-pushed the update-mbed-client-c-3.0.4 branch from f68ecf0 to d80f863 Compare December 23, 2016 11:53
@0xc0170 0xc0170 requested review from sg- and adbridge December 23, 2016 13:25
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 23, 2016

don't merge yet, let's wait for our CI jobs to finish

Let us know once ready

@sg-
Copy link
Contributor

sg- commented Dec 23, 2016

/morph test-nightly

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1324

All builds and test passed!

@kuggenhoffen
Copy link
Contributor Author

@0xc0170 @sg-
CI looks happy, this should be OK now

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 28, 2016

@kuggenhoffen Thanks, LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 28, 2016

@adbridge or @sg- - one more review to be done please

adbridge
adbridge approved these changes Dec 30, 2016
View reviewed changes
Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

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

There are a considerable number of formatting issues (I wonder if tabs have gotten embedded along with spaces?) . There are also a considerable number of magic numbers scattered throughout the code. If this was anything other than a temporary patch I would block this coming in until it was tidied up. Please ensure that when this is re-done properly for 5.4 that comments are addressed.

features/FEATURE_COMMON_PAL/mbed-client-c/module.json
@@ -1,6 +1,6 @@
{
"name": "mbed-client-c",
"version": "3.0.2",
"version": "3.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has the version jumped 2 patch versions instead of one ?

features/FEATURE_COMMON_PAL/mbed-client-c/nsdl-c/sn_nsdl_lib2.h
uint8_t type_len;
uint8_t lifetime_len;
uint8_t location_len;

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 big gaps between type and struct member ?

features/FEATURE_COMMON_PAL/mbed-client-c/nsdl-c/sn_nsdl_lib2.h
uint8_t *type_ptr; /**< Endpoint type */
uint8_t *lifetime_ptr; /**< Endpoint lifetime in seconds. eg. "1200" = 1200 seconds */
uint8_t *location_ptr; /**< Endpoint location in server, optional parameter,default is NULL */
} sn_nsdl_ep_parameters_s;
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 big gaps between type and struct member ?

features/FEATURE_COMMON_PAL/mbed-client-c/nsdl-c/sn_nsdl_lib2.h
*/
typedef struct sn_nsdl_sent_messages_ {
uint8_t message_type;
uint16_t msg_id_number;
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 big gaps between type and struct member ?

features/FEATURE_COMMON_PAL/mbed-client-c/nsdl-c/sn_nsdl_lib2.h
uint8_t (*sn_grs_dyn_res_callback)(struct nsdl_s *,
sn_coap_hdr_s *,
sn_nsdl_addr_s *,
sn_nsdl_capab_e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Some weird spacing here?

...URE_COMMON_PAL/mbed-client-c/test/nsdl-c/unittest/sn_coap_protocol/libCoap_protocol_test.cpp
@@ -582,7 +582,7 @@ TEST(libCoap_protocol, sn_coap_protocol_parse)
memset(list, 0, sizeof(sn_coap_options_list_s));
sn_coap_parser_stub.expectedHeader->options_list_ptr = list;
sn_coap_parser_stub.expectedHeader->options_list_ptr->block1 = 1;
sn_coap_parser_stub.expectedHeader->msg_id = 4;
sn_coap_parser_stub.expectedHeader->msg_id = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers

...URE_COMMON_PAL/mbed-client-c/test/nsdl-c/unittest/sn_coap_protocol/libCoap_protocol_test.cpp
@@ -843,12 +843,13 @@ TEST(libCoap_protocol, sn_coap_protocol_parse)

retCounter = 20;
sn_coap_builder_stub.expectedInt16 = 1;
tmp_hdr.payload_ptr = (uint8_t*)malloc(3);
int buff_size = SN_COAP_MAX_BLOCKWISE_PAYLOAD_SIZE + 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers

...URE_COMMON_PAL/mbed-client-c/test/nsdl-c/unittest/sn_coap_protocol/libCoap_protocol_test.cpp
@@ -891,13 +892,14 @@ TEST(libCoap_protocol, sn_coap_protocol_parse)

retCounter = 21;
sn_coap_builder_stub.expectedInt16 = 1;
tmp_hdr.payload_ptr = (uint8_t*)malloc(3);
int buff_len = SN_COAP_MAX_BLOCKWISE_PAYLOAD_SIZE + 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers

features/FEATURE_COMMON_PAL/mbed-client-c/test/nsdl-c/unittest/sn_nsdl/test_sn_nsdl.c
@@ -279,6 +279,7 @@ bool test_sn_nsdl_register_endpoint()
sn_grs_stub.expectedInfo->resource_parameters_ptr->interface_description_ptr[1] = '\0';
sn_grs_stub.expectedInfo->resource_parameters_ptr->interface_description_len = 1;
sn_grs_stub.expectedInfo->resource_parameters_ptr->observable = 1;
sn_grs_stub.expectedInfo->resource_parameters_ptr->coap_content_type = 0; // XXX: why was this left uninitialized? what was point of this test?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a comment querying the code left in ?

features/FEATURE_COMMON_PAL/mbed-client-c/test/nsdl-c/unittest/sn_nsdl/test_sn_nsdl.c
return false;
}
free(handle->nsp_address_ptr->omalw_address_ptr->addr_ptr);
handle->nsp_address_ptr->omalw_address_ptr->addr_ptr = NULL;

if( SN_NSDL_FAILURE != set_NSP_address(handle, addr, 0, SN_NSDL_ADDRESS_TYPE_IPV6) ){
// Note: the set_NSP_address() will read 16 bytes of source address
uint8_t* addr6 = (uint8_t*)calloc(16, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be named addr16 if keeping with previous convention?

@0xc0170 0xc0170 merged commit 2f6f3c6 into ARMmbed:master Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@adbridge adbridge adbridge approved these changes

@sg- sg- Awaiting requested review from sg-

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

Successfully merging this pull request may close these issues.
5 participants
@kuggenhoffen @0xc0170 @sg- @mbed-bot @adbridge