Add support for TLV binary integer #463

JaniSuonpera · 2017-04-06T12:07:38Z

If resource type is INTEGER then value will convert to 64-bit binary buffer. Format is two's complement big endian word. See, OMA-TS-LightweightM2M-V1_0-20170208-A, Appendix C, data Types, Integer, TLV Format.

JaniSuonpera · 2017-04-06T12:11:14Z

This change will be broken TLV test cases for interger resoureces. So this should not merge until test cases has been fixed.

kjbracey · 2017-04-06T12:15:43Z

source/m2mresourceinstance.cpp

 }

-int M2MResourceInstance::get_value_int()
+int64_t M2MResourceInstance::get_value_int() const


This still can't return 64-bit numbers, because it uses int and atoi.

Is _value zero-terminated now? It if isn't, this might be about 96% reliable.

Null termination is handle in setter. See set_value. Yes, it cannot be 64-bit when use atoi. By the way setter is 64-bit. Maybe this should fix to use atol or atoll.

kjbracey · 2017-04-06T12:16:57Z

source/m2mtlvserializer.cpp

+    }
+}
+
+template void M2MTLVSerializer::set_value_int<int8_t>(int8_t value, uint8_t *buffer, uint32_t &size);


Utilities for these already exist in common_functions.h.

common_functions.h is not available for unit tests.

It should be. Not using common_functions because of unit tests not making it available makes as much sense as not using memcpy because of unit tests not making it available.

For unit test I need to copy/ make stub for common_functions.h for common_write_64_bit.

I do not like way how common_write_64_bit has been implement and how it must use. It is working but I do not like it. Future plan is also to use automatic or manual sizing of integer. This will be decided later. For different sizing I want to use my common loop against of common_write_64_bit, common_write_32_bit or common_write_16_bit.

I agree that if you're not fixed size, those functions may not be for you anyway.

But those simple utility functions should never be stubbed for unit tests, they should be pulled in to the build and working, just like you have working pointer accesses, linked lists and memcpy.

Doing anything else would be insane.

What you stub should be a matter of common sense, not the totally arbitrary rule "is it in libc or not".

@JanneKiiskila - this keeps coming up, due to bonkers definition of "unit test", which is apparently "It must compile only 1 C file, and use no libraries, but libc is permitted".

I once saw a unit tests for packet parsing where all the 8-bit values are read from the packet, as 8-bit accesses are not stubbed, but the 16-bit values were not because common_read_16_bit() was stubbed. If anyone with a test hat is still trying to force this nonsense, they need a firm kick.

Basically, if you're going to insist on stubbing common_read_16_bit, then I'm going to insist on stubbing memcpy.

Would agree with you Kevin - the tests should run anything and everything possible to run. Any function that does just math, string manipulations, memory operations, logic etc. w/o need for EXTERNAL dependencies (say board specific stuff, etc.) should use anything it can.

Mainly it's the frequently-used "language + standard library extensions" in libService need to be always available. (Not sure if there are other bits in the mbed Client base that are similar?)

So just as we get memcpy and strcpy, we should also have our added bitcopy, and just as we have native pointer accesses, we should have the big-endian unaligned accesses, That sort of thing. And ns_list needs to be functional.

Some things are available kind of by accident - eg if they're a macros, or if they get inlined. Basically if the H file happens to work without the C file, there's a quiet pass.

There's a separate argument about utility of unit tests versus module tests - I feel we have too much of the former and not enough of the latter. But for the unit tests, those core utilities need to work. Stubbing them is counterproductive - they always work and cannot fail unless you crash with an invalid pointer. They have no error returns that you might want to exercise.

I think correct interface functions for 16 and 32 bits in this case are htons and htonl http://man.yolinux.com/cgi-bin/man2html?cgi_command=htons&cgi_section=3. These functions seem to be available under mbed-os. For Linux there is also available common 64-bit version http://man.yolinux.com/cgi-bin/man2html?cgi_command=htobe64&cgi_section=.

Maybe PAL should offer these common functions by above for all targets.

kjbracey · 2017-04-06T12:19:13Z

source/m2mtlvserializer.cpp

-    return serialize_TILV(TYPE_RESOURCE_INSTANCE, id, resource->value(), resource->value_length(), data, size);
+    bool success;
+
+    if ( resource->resource_instance_type() == M2MResourceInstance::INTEGER) {


Odd to see this code repeated from serialize_resource, slightly differently. Feels like there should be a common subroutine.

Is the int32_t difference intentional?

kjbracey · 2017-04-06T12:25:33Z

source/m2mtlvserializer.cpp

+        if ( resource->resource_instance_type() == M2MResourceInstance::INTEGER) {
+            int64_t valueInt = resource->get_value_int();
+            uint32_t buf_size;
+            /* max len 8 bytes */


Size is fixed, isn't it? Or is it supposed to be variable? Spec suggests there are 4 fixed-width integer types, this only does 64-bit.

It might be variable in future. It depends how we going to implement support for TLV integer. In ideal case these should be supported 8, 16, 32 and 64-bit. If we are going use fixed size, e.g. 64-bit always, then we can simplify this later.

Specs says that. "Represented as a binary signed integer in network byte order, and in two‟s complement representation. The value may be 1 (8-bit), 2 (16-bit), 4 (32-bit) or 8 (64-bit) bytes long as indicated by the Length field. When transmitted over network, the data is represented in network byte order (big endian)."

Ah, I was potentially misreading the spec. Still not totally clear to me whether integer TLVs are intended to be autosized based on value (like CoAP options) or be fixed for each specific resource. The fact that they spell out 8,16,32 and 64 suggests to me at least they're not thinking of autosizing. If autosizing by value I'd expect it to allow any number of bytes from 0-8.

I guess we could handle all cases for writing with 5 distinct types for the 4 fixed sizes and 1 for autosize.

kjbracey · 2017-04-06T12:26:29Z

source/m2mtlvserializer.cpp

+ * Data Types, Integer, TLV Format 
+ * 
+ * This is easy to change to support 8, 16 or 32 interger. */
+template <typename T> void M2MTLVSerializer::set_value_int(T value, uint8_t *buffer, uint32_t &size)


size parameter is pointless here, unless it could be variable-width, which I don't think is the intent.

Size is bound type of value. This is not mandatory parameter an could handle different way because parameter value size it known before execution of set_value_int. This for avoiding magic numbers. Now sizeof() return always correct size and it has been called only in one place.

If resource type is INTEGER then value will convert to 64-bit binary buffer. Format is two's complement big endian word. See, OMA-TS-LightweightM2M-V1_0-20170208-A, Appendix C, data Types, Integer, TLV Format.

JaniSuonpera · 2017-04-20T11:59:23Z

Finally test case changes are ready in https://github.com/ARMmbed/mbed-clitest-mbed-client/pull/430

kuggenhoffen · 2017-04-20T12:50:37Z

source/m2mresourceinstance.cpp

+int64_t M2MResourceInstance::get_value_int() const
+{
+    int64_t value_int = 0;
+    uint8_t* buffer = _value;


Isn't this unnecessary temporary variable?

Yes, it is.

kuggenhoffen · 2017-04-24T11:23:42Z

source/m2mtlvserializer.cpp

 }

+template bool M2MTLVSerializer::serialize_TLV_binary_int<const M2MResourceInstance>(const M2MResourceInstance *resource, uint8_t type, uint16_t id, uint8_t *&data, uint32_t &size);
+template bool M2MTLVSerializer::serialize_TLV_binary_int<const M2MResource>(const M2MResource *resource, uint8_t type, uint16_t id, uint8_t *&data, uint32_t &size);


Does this even need to be templated? Isn't M2MResourceInstance version enough since M2MResource inherits from that?

You are right. I will fix this.

JaniSuonpera · 2017-05-02T04:34:12Z

Please review latest changes. There is now unit tests fixes correct way without stubs.

yogpan01 · 2017-05-02T08:38:05Z

Looks good. Do you also plan to do similar modification for Deserializer ? Might be good idea to do it at the same time.

JaniSuonpera · 2017-05-02T08:53:48Z

I have understood that @anttiylitokola has already done some fixes in deserialization side in R1.2 branch for binary data. Would @anttiylitokola confirm?

I think that deserialization should check later with own task. It would be better do changes via smaller tasks. We should get merged this review and test cases changes too https://github.com/ARMmbed/mbed-clitest-mbed-client/pull/430.

yogpan01 · 2017-05-02T09:33:05Z

@anttiylitokola Can you tell the PRs where you did the fixes for Deserializer? @JaniSuonpera I think it makes sense to keep these changes in pair (Serializer-Deserializer) should work the same way. Its currently fresh in this PR so deserializer work shouldn't be too different to fix. Also, would be nice to fix the test cases at the same time for deserializer.

anttiylitokola · 2017-05-02T10:25:37Z

Here are the changes done in 1.2 branch. #446

JaniSuonpera · 2017-05-03T05:20:40Z

I have checked 1.2 branch #446. It cannot directly cherry-pick/merge without other changes. E.g. every serialization test case and unit will be broken and unit test compilation will be broken. I propose that we

Merge these deserialization commits to master branch.
Merge 1.2. branches to master branches (or cherry-pick/merge serialization changes to 1.2. branch.)
Finalize deserialization changes in master (or 1.2) branch.

This support TLV binary formats Integer, Boolean and Time. If resource type is INTEGER then value will convert to 64-bit binary buffer. Format is two's complement big endian word. See, OMA-TS-LightweightM2M-V1_0-20170208-A, Appendix C, data Types, Integer, TLV Format.

JaniSuonpera requested review from JanneKiiskila, anttiylitokola, kuggenhoffen and yogpan01 April 6, 2017 12:08

kjbracey reviewed Apr 6, 2017

View reviewed changes

JaniSuonpera force-pushed the TLV_binary_integer branch 2 times, most recently from 29f819f to b55211f Compare April 10, 2017 10:18

JaniSuonpera mentioned this pull request Apr 10, 2017

In TLV (multiple instance resource) Integer or Boolean types are in string format. #469

Closed

Jani Suonpera added 4 commits April 19, 2017 12:54

Add support for TLV binary integer

d5dab0b

If resource type is INTEGER then value will convert to 64-bit binary buffer. Format is two's complement big endian word. See, OMA-TS-LightweightM2M-V1_0-20170208-A, Appendix C, data Types, Integer, TLV Format.

Change atoi to atoll

5ca417d

Add support for TLV binary boolean

0e7590c

Add support for TLV binary time

ae2509c

JaniSuonpera force-pushed the TLV_binary_integer branch from d7aedfe to ae2509c Compare April 19, 2017 09:58

kuggenhoffen reviewed Apr 24, 2017

View reviewed changes

Jani Suonpera added 3 commits April 24, 2017 15:53

Review fixes

7c1bafe

Review fixes.

d543dc3

Fix unit tests coverage

940339e

JaniSuonpera merged commit e157707 into master May 4, 2017

JaniSuonpera mentioned this pull request May 4, 2017

Add support for TLV binary integers (#463) #488

Merged

Add support for TLV binary integer #463

Add support for TLV binary integer #463

Uh oh!

Conversation

JaniSuonpera commented Apr 6, 2017

Uh oh!

JaniSuonpera commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaniSuonpera Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaniSuonpera commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaniSuonpera commented May 2, 2017

Uh oh!

yogpan01 commented May 2, 2017

Uh oh!

JaniSuonpera commented May 2, 2017

Uh oh!

yogpan01 commented May 2, 2017

Uh oh!

anttiylitokola commented May 2, 2017

Uh oh!

JaniSuonpera commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JaniSuonpera Apr 26, 2017 •

edited

Loading

JaniSuonpera commented May 3, 2017 •

edited

Loading