Add tinyCBOR external library and apply mynewt upstream patch from intel/tinycbor#83

[vrahane]

• Add tinycbor as an external library commit: intel/tinycbor@497066e
• This is needed by MCUmgr and config subsystem in zephyr
• Pulling in code from Tinycbor mynewt upstream intel/tinycbor#83 as a patch to zephyr's ext/tinycbor. This is to facilitate the
  use of chained buffers functionality for tinycbor while it is in development on
  https://github.com/intel/tinycbor
• Bring tinycbor up to date with mynewt tinycbor
• Changing implementation of cbor_encoder_get_extra_bytes_needed() and
  cbor_encoder_get_buffer_size() as part of cbor_buf_writer APIs
• Move bytes_needed field from CborEncoder to the buf writer, this is needed by the tests mainly.
• Fix cbor_buf_cmp to do memcmp and return complemented result iterate_string_chunks(): fixing
  NULL compare at the end of string and moving it out of the iterate_string_chunks(). This is to
  avoid buffer specific parser calls in the function
• cbor_value_get_next_byte() is removed in mynewt version of tinycbor, so, we track offsets of the
  buffer which can be used for comparison in the parser tests instead of calculating the offset
• Making the decoder and parser APIs backwards compatible
• Adding encoder writer and parser reader as part of the encoder and parser structure. This is to
  make the encoder and parser use new function of encoder_writer and decoder_reader without
  breaking backwards compatibility.
• Making the old API use flat buffers by default
• Adding APIs for initializing encoder and parser with custom writer and reader
• Make the default writer and reader conditional based on NO_DFLT_READER/WRITER define. This
  is because we want a default reader/writer to avoid API changes.
• Use it->offset instead of it->ptr to track buffer offsets
• Update resolve_indicator() static api parameters to use cbor value and access offsets instead of
  taking pointers as input parameters
• In validate_container() do a byte by byte comparison instead of memcmp since we no longer
  have access to the buffer directly
• Also, use offsets instead of pointers to validate sorted maps
• Added a new define for conditionally compiling in float support (NO_FLOAT_SUPPORT).
• This is because we want the float support to be compiled in by default.
• Use static_assert macro instead of Static_assert. Changed to avoid build failures.
• Add api to get string chunk, this is a callback which can be used by buffer implementations to
  grab a string that is divided in chunks which spans across multiple chained buffers

Fixes #3081

Signed-off-by: Vipul V Rahane [email protected]

[carlescufi]

@vrahane I think we should also include integration with Kconfig and CMake in this PR. Otherwise this will not be usable by Zephyr modules. We cannot build with make because that does not work on Windows. Something like this: https://github.com/zephyrproject-rtos/zephyr/blob/master/ext/hal/nordic/CMakeLists.txt#L7

[carlescufi]

@vrahane in the first commit message, can you remove - This is needed by MCUmgr and config subsystem in zephyr and replace it with a very brief description of what tinyCBOR is/does?

[codecov-io]

Codecov Report

Merging #5912 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5912   +/-   ##
=======================================
  Coverage   52.63%   52.63%           
=======================================
  Files         410      410           
  Lines       40041    40041           
  Branches     7779     7779           
=======================================
  Hits        21074    21074           
  Misses      15767    15767           
  Partials     3200     3200

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41db766...d57e9bd. Read the comment docs.

[carlescufi]

I understand having the full repo makes it easier later to rebase, but I'm not sure we should include things like .gitignore and similar.

[carlescufi]

This should live in ext/lib/tinycbor and not in ext/tinycbor

[vanwinkeljan]

Should mynewt changes not first be upstreamed to intel/tinycbor before taking it in to zephyr repo?
Now you would be taking in a fork instead of the upstream project.

[carlescufi]

@vanwinkeljan they should, but they won't make it in time for Zephyr 1.11. This is a stopgap solution until @thiagomacieira merges those.

[thiagomacieira]

I'll join zephyr-devel and ask questions. I want to have native support for net_buf and net_pkt in TinyCBOR, but I need to understand a few things first.

[carlescufi]

@thiagomacieira thanks, although the work so far has been done by Mynewt developers. I'll ask them to join zephyr-devel as well.

[vrahane]

@carlescufi I have made changes as per the review and the builds are now successful as well. Thank you.

[mbolivar]

@vrahane Cool!

One thing, though. You need to use interactive rebase to edit the pull request history into a coherent set of patches, rather than addressing review with commits at the end of the series, like the "Make changes as per PR review" one that is present right now.

Here is the workflow document: http://docs.zephyrproject.org/contribute/contribute_guidelines.html#contribution-workflow

[vrahane]

@mbolivar I generally prefer rebasing as well. The only issue here is that the first commit is the vanilla version of tinycbor, second applies the mynewt patch on top of it and the third one specifically adds Kconfig, changes directories and deletes git metadata files from the library. If the patch(second commit) were to be ever reverted, minimal changes would have to be made to the CMake files, Kconfig and menuconfig which is added by the third commit. Hence, the third commit. Maybe I should name it differently instead of just a PR review changes, Any thoughts appreciated ?

[carlescufi]

@vrahane That is fine but you cannot name a commit "Make changes as per PR review". You need to name commits after what they contain.

[carlescufi]

@vrahane also the removal of non-needed files should not really be in this 3rd commit. Those files should really never have been included in the first place, we can always trace back the diffs from the imported files to the next commit changes with the patch from the TinyCBOR upstream PR regardless of these files being included or not. In fact, we only need the diffs on the relevant files for Zephyr.

[nashif]

Can you please just import the files we need to build the library and drop everything else? We should not include any of the CI, IDE, project scripts and such, those will just trigger all kind of violations when we do license scanning and IP management. Just keep the C and header files we need for building and drop everything else. Maybe later when we have better repository management we can import complete tree, but not now.

[vrahane]

Thank you @nashif and @carlescufi for going through it. As per your reviews and zephyr policies, I have done the following:

• Added KConfig and CMakeList files for configuration and build
• Added tinycbor to ext/lib/encoding
• Deleted .gitignore and .gitattributes
• Removed tools, tests, examples, CI scripts, Makefiles as they are not really needed for building the library with Zephyr
• Rebased and squashed unnecessary commits made for cleaning up the library

[carlescufi]

@vrahane I think it would be cleaner if we did this in a slightly different way:

Commit 1: Add the required TinyCBOR files only, without the 83 patch
Commit 2: Patch according to 83, again only the required files
Commit 3: Add Kconfig and CMakeLists

That said I am OK with merging this as-is if @nashif is as well

[carlescufi]

@vrahane also can you please remove everything that is not required for building? Files like Doxyfile and TODO, etc.

[carlescufi]

@vrahane Also please change the first commit so it only contains the src/ folder, and not the whole of TinyCBOR.

[nashif]

@thiagomacieira do you have c based tests for tinycbor that we could adapt for Zephyr? All I can see in the tree are QT based tests

[thiagomacieira]

Not yet, no. That's why I asked you last Friday for how to use Travis with Zephyr. I need to write a couple, though I will focus only on the Zephyr-specific code (yet to be written) that integrates with net_buf. I won't re-do the entire suite, though I realise the current runs don't test any 32-bit build.

[thiagomacieira]

Better text: This option controls whether the CborEncoder checks whether the user passed valid arguments.

[vrahane]

It is a prompt message, hence you cannot put a long descriptive text there.

[thiagomacieira]

Maybe find a better name for this? At least get rid of the acronym.

I've been calling this the ability to encode to a linear buffer, as opposed to a net_buf.

[vrahane]

Any suggestion ?

[thiagomacieira]

This option should control the decode_half function we've been having problems with too.

[thiagomacieira]

Unnecessary, since there is no open_memstream support in Zephyr. It's a Glibc extension to POSIX, though I think Apple recently added to their libc too.

[vrahane]

Are you suggesting we should remove open_memstream.c altogether from the sources ?

[thiagomacieira]

Are you sure? Note that the #define is negative ("without'), so this makes a double negative and you're saying with open_memstream, which Zephyr doesn't have.

[vrahane]

Yes, this needs to get fixed. Thank you.

[vrahane]

I discussed this with @ccollins476ad and we tested a few things out on the Zephyr-SDK. Based on the findings, I have made two commits above:

2635b19: ext: lib: tinycbor: make HF type conditional
This commit does the following:

• Make half float encode/decode conditional based on CBOR_NO_HALF_FLOAT_TYPE
• Changes defaults for CBOR_WITHOUT_OPEN_MEMSTREAM and
  CBOR_NO_HALF_FLOAT_TYPE to y so that half float support and memstream support does
  not get compiled in as it is not needed for normal usage of the library

2edd586: ext: lib: tinycbor: Fix build issues

• This fixes build issues that we have been discussing on this PR wrt newlib libc and float support
• src/cborpretty.c, src/cbortojson.c and src/cborvalidation.c
  conditionally include math.h and half float type support.
  They are not specified in the CMakeList files because of the following reasons:
  i. The following source files requires cbor_value_to_pretty_advance(), which is hosted-only.
• src/cborpretty_stdio.c
• src/cbortojson.c

ii. The following source file requires glibc which is not part of Zephyr and hence it is compiled
out by default:

• src/open_memstream.c

• Change default settings for floating type support and half
  float type support to be disabled by default to avoid newlib libc from
  getting compiled in by default CBOR_NO_HALF_FLOAT_TYPE and
  CBOR_NO_FLOATING_TYPE is set to y.

• Conditionally include math.h in src/compilersupport_p.h to avoid
  newlib libc from getting compiled in using CBOR_NO_FLOATING_TYPE

• Conditionally compile src/cborparser_dup_string.c if newlib libc is
  compiled in using CBOR_NO_FLOATING_TYPE

This solves most of the issues if not all. I have also changed prompt messages to make it more aesthetic. Please let me know if this works. I have kept it as separate commits so that everybody can take a look at it. I can further combine it into a single commit or maybe split it into multiple based on what feedback I get.

[thiagomacieira]

The changes look good, with minor changes. I'd like the half and full-FP support changes in TinyCBOR.

[carlescufi]

@vrahane Thanks for all the changes related to building with and without newlib. These commits that modify src/ need to go go to the official Zephyr upstream fork like you did for the commit with all the changes: https://github.com/zephyrproject-rtos/tinycbor/tree/zephyr, so we can keep all changes to TinyCBOR itself there. Finally I would prefer if you could squash the last 2 commits together, we really don't want a commit that says "Fix the build".

[vrahane]

Sorry @SebastianBoe, did not mean to add you as a reviewer.

[vrahane]

@carlescufi I have made the above half/full FP changes to the PR to https://github.com/zephyrproject-rtos/tinycbor/tree/zephyr after squashing.

[carlescufi]

Looks good to me now

[carlescufi]

@nashif mind taking another look at this?