Memory optimizations #101

TeroJaasko · 2016-12-12T15:48:10Z

No description provided.

TeroJaasko · 2016-12-12T15:49:45Z

@anttiylitokola @kuggenhoffen @kjbracey-arm @karsev

kseverinkangas-zg · 2016-12-13T08:58:46Z

nsdl-c/sn_nsdl_lib.h

+                                                 otherwise block messages are passed to application */
+    unsigned    mode:2;                     /**< STATIC etc.. */
+    bool        free_on_delete:1;           /**< 1 if struct is dynamic allocted --> to be freed */
+} sn_nsdl_static_resource_parameters_s;


Verify the comments if they are valid.

kseverinkangas-zg · 2016-12-13T10:04:42Z

nsdl-c/sn_nsdl_lib.h

+    unsigned                                    access:4;           /**< Allowed operation mode, GET, PUT, etc,
+                                                                         TODO! This should be in static struct but current
+                                                                         mbed-client implementation requires this to be changed at runtime */
+    unsigned                                    registered:2;       /**< Is resource registered or not */


Would be safer to use uint8_t.

AnttiKauppila · 2016-12-15T14:09:20Z

@artokin or @terhei Check from coap-service point of view

AnttiKauppila · 2016-12-15T14:23:42Z

source/libNsdl/src/sn_grs.c

@@ -23,7 +23,7 @@
 */
 #include <string.h>
 #include <stdlib.h>
-
+#include <assert.h>


Do not use assert for portability reasons in Mbed-client-c.

Why not? Assert.h is part of C99 and we're already using it all over codebase without any problems so far.

It's not used at all in any of the portable C/Nanomesh components. Is it getting used in the C++ bits?

We can't really assume we have a full "hosted" C99 library. But the minimum for a "freestanding" implementation is too tight, so we go a bit beyond that. So we take memcpy() and sprintf() from "hosted", but not fprintf(). I think assert() falls into the slightly-more-than-we-can assume fprintf() area.

My main concern is - does it actually do what you want on any given embedded platform, and do you end up accidentally pulling in a whole chunk of code to give you stderr for its output?

If you really want it, we can certainly create an NS_ASSERT in ns_types.h - it would map to MBED_ASSERT in mbedOS.

Actually, some sort of tr_assert() extension to the trace library might make sense. You do really want the output going to the trace location, so that's presumably what a default ns_types.h implementation would do, in which case maybe it should just be in the trace to start with?

Don't forget the trace location isn't necessarily just a UART - it could be some sort of internal log-storage-for-later-extraction system. assert()'s default behaviour of "write to stderr" would not be helpful in that scenario.

Up to now, I've usually manually done tr_error() and return myself.

I thought that the NDEBUG should get rid of any extra crap that assert() brings and that define should be anyway set on the release build versions.

That tr_assert() is a really good idea. Currently mbed-os' assert() is mostly visible only on debugger. If the assert fail or any other mbed os exception happens on tests, the test framework does not get any output and logs just "Dut1 timed out" or something equally useful.

True about NDEBUG, but if we were only thinking about NDEBUG, then there's never any issue.

The essence of the problem is "where does the output go", and so if we use tr_xxx rather than printf, we should use tr_error or a new tr_assert rather than assert. (Presumably a release build should also have the trace disabled?)

A secondary question to consider is "what happens after the output"? Standard assert() calls abort(), but what does that do in an embedded system? I believe MBED_ASSERT infinite loops, and a few of our own error handlers do the same.

That feels a bit more like a platform HAL thing, which isn't just covered by the existing trace. We do have a fairly-widely-implemented board reset call, but is halting better?

Ok, removed asserts in #107.

AnttiKauppila · 2016-12-15T14:37:54Z

source/libNsdl/src/sn_grs.c

-        if (temp_resource->resource_parameters_ptr) {
-            if (temp_resource->resource_parameters_ptr->registered == SN_NDSL_RESOURCE_REGISTERING) {
-                temp_resource->resource_parameters_ptr->registered = SN_NDSL_RESOURCE_REGISTERED;
+        if (temp_resource->static_resource_parameters) {


What is the reason for this check? (Only do what function name indicates if static resources are used?)
Is non-static resources handled elsewhere?

Fixed in #108

AnttiKauppila · 2016-12-15T14:38:42Z

source/libNsdl/src/sn_nsdl.c

@@ -21,6 +21,7 @@
 */

 #include <string.h>
+#include <assert.h>


Don't use assert for portability reasons

AnttiKauppila

Remove assert calls from code, otherwise seemed to be ok.

terhei · 2016-12-15T16:45:06Z

nsdl-c/sn_nsdl_lib.h

@@ -583,7 +497,7 @@ extern int8_t set_NSP_address(struct nsdl_s *handle, uint8_t *NSP_address, uint1
 * \return  0   Success
 * \return  -1  Failed to indicate that internal address pointer is not allocated (call nsdl_init() first).
 */
-extern int8_t set_NSP_address_2(struct nsdl_s *handle, uint8_t *NSP_address, uint8_t address_length, uint16_t port, sn_nsdl_addr_type_e address_type);
+extern int8_t set_NSP_address(struct nsdl_s *handle, uint8_t *NSP_address, uint8_t address_length, uint16_t port, sn_nsdl_addr_type_e address_type);


If you are changing API, can we also update this? set_DS_address( )?

Good point, we added a IOTCLT-1322 task for this and the other cleanup items you suggested.

terhei · 2016-12-16T07:55:39Z

@peksaa01 - please check these modifications.

yogpan01 · 2017-02-14T17:16:46Z

@TeroJaasko Can you close this , now that you have this branch merged with the master ?

mbed-ci · 2017-02-24T12:56:13Z

Can't set status; build succeeded.

mbed-ci · 2017-02-24T12:57:53Z