Adopt PR #2189: a JSON en/decoder #8312

smlng · 2017-12-22T10:56:59Z

Contribution description

I adopted PR #2189 to get it merged, as IMHO the original contribution is still valuable to RIOT but @Kijewski (original author) hasn't found the time to work on it (no blame for that 😉). I rebased to current master and squashed reasonably, authorship was retained to stay with @Kijewski.

The original PR also included a minor adaption to embUnit, which still is a separate commit and can be dropped, if someone objects.

Otherwise this PR (was and) is in good shape and hence ready to merge.

Issues/PRs references

Adopts sys: add JSON serialization and deserialization module #2189
Requires (and includes) tests: adapt embunit TEST_ASSERT_EQUAL_STRING #10538

sys/include/embUnit/AssertImpl.h

miri64

Preliminary review up until the end of json.h

sys/include/json.h

miri64 · 2017-12-29T16:31:10Z

sys/include/json.h

+ */
+json_result_t json_write_float(json_write_cookie_t *cookie, float value);
+
+#ifndef MODULE_ATMEGA_COMMON


Why is this hardware dependent?

ATMEGA did not support int64_t values at all. (In 2014, maybe my toolchain was to blame for that.)

will check and adapt as needed

In any case I would prefer a NOP or error message instead of excluding this function completely from the API.

sys/include/json.h

smlng · 2018-01-09T09:31:14Z

general question: how to proceed with the requested changes? I'd like to retain the original author ship of @Kijewski, but also squashing was needed (already done here) and now need to add the fixes. Just add them as separate commits, what do you think? @miri64

miri64 · 2018-01-09T09:44:55Z

general question: how to proceed with the requested changes? I'd like to retain the original author ship of @Kijewski, but also squashing was needed (already done here) and now need to add the fixes. Just add them as separate commits, what do you think? @miri64

If you squash correctly, @Kijewski's authorship is retained (since ideally his commits are the base commits of your squash commits). If you want to make sure the authorship is retained, you can even use the --author=<author> parameter for git commit to set the authorship for your fixup commits. If there is a significant change (i.e. removing the platform dependency of the API) you can have it as a separate PR with yourself as an author.

smlng · 2018-01-09T09:46:05Z

If you squash correctly

I'll try that 😉

smlng · 2018-01-09T14:00:08Z

for now I reverted my initial squashing and made some fixes to documentation and coding style. Still need to look into this atmega stuff.

smlng · 2018-01-10T09:59:20Z

a major problem encountered by Murdock is, that the JSON unittest increase the unittests binary size by >640K (bc it uses a JSON test suite with lots of (really large) tests strings). Thus, rendering most boards incompatible because of low FLASH/ROM size - basically we could change from blacklisting boards to whitelist native only.

Any suggestions on that: @RIOT-OS/maintainers ?

cladmi · 2018-01-10T12:41:55Z

I would say, put it in a separate test in directly in the tests directory.

cladmi · 2018-01-10T13:41:40Z

Hmm, it may also be too big for one test alone, so maybe split it into different tests types, tests float, tests strings, tests arrays, tests dicts, and one with mixed data types.
Pthreads, threads, xtimer also have several tests folders.

kYc0o · 2018-01-10T13:41:42Z

@smlng what's the current size of this feature? When I reviewed the code of the former PR I found it extremely large, a lot of code for something "simple" especially the encoding. For decoding I agree it might need more lines of code.

biboc · 2018-01-11T07:56:16Z

What is the difference with pkg/jsmn? Is it necessary to have two json library?

kaspar030 · 2018-01-11T09:00:36Z

What is the difference with pkg/jsmn? Is it necessary to have two json library?

jsmn only offers very basic parsing.

tcschmidt · 2018-07-11T18:11:25Z

@smlng @miri64 is this a quick finish?

smlng · 2018-07-12T06:20:57Z

@tcschmidt code-wise: yes, otherwise not so much, because this PR adds unittests for json which are rather huge - meaning that no board can handle the complete unittests binary anymore. There is already a (yet inconclusive) discussion in #8941 (split unittests) on this matter.

I can either split of the json unittests right away or wait for consensus in #8941.

smlng · 2018-12-01T18:57:16Z

hey @miri64 care to have a look at this one again?

miri64 · 2018-12-02T11:22:03Z

Is there a way maybe to have this PR less than 6000 additions? Maybe split it up in encoder and decoder? Also, I still question the necessaty of changing embunit.

miri64

Review on some build system stuff and the first half of the documentation. While reviewing I tried to look at the code as little as possible for now, since I wanted to understand the API for now before digging into the code.

sys/json/Makefile

sys/json/json_write.c

sys/include/json.h

miri64 · 2018-12-06T09:05:48Z

sys/include/json.h

+/**
+ * @brief End a JSON serialization stream.
+ *
+ * @note An empty stream without any data is invalid.


Where is the stream in this context? What I mean is: How is this note to be understood? Can't I do e.g.

json_write_init(&cookie, _write); json_write_finish(&cookie);

without crashing?

sys/include/json.h

miri64 · 2018-12-06T21:49:52Z

sys/include/json.h

+ * JSON does not differentiate between integers and floating point numbers.
+ *
+ * @warning The number must be finite, i.e. it must not be NaN, +Inf or -Inf.
+ *          Illegal inputs are not caught but the stream will be broken.


Is there a way to write those regardless?

In JSON as of RFC 7159 § 6 it is not possible:

Numeric values that cannot be represented in the grammar below (such
as Infinity and NaN) are not permitted.

sys/include/json.h

miri64 · 2018-12-06T21:53:58Z

sys/include/json.h

+ * @brief   The cookie with all the information needed to deserialize JSON data.
+ *
+ * @see json_read_init()
+ * @see @ref container_of


Why is this reference here?

You'll most likely use this struct as a member of your own struct to keep track of it's state. If struct json_write_cookie is the first member, then you could simply cast the pointer, but if not you'll have to use the macro. If you think that's already obvious, then the reference may be removed.

I think such behavior should not be encouraged, so it should be removed. See also this talk ;-).

stale · 2019-08-29T17:01:59Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

smlng · 2019-10-15T08:02:50Z

do we (still) want this in or not? I would squash and make changes if necessary to make it work, but wouldn't "waste" more time on this for restructuring and fixing some not so nice handling. I adopted bc it looked like a nice addition.

An open point that I would address is to move the tests out of unittests bc its very big.

miri64 · 2019-10-15T08:07:52Z

TBH, I'm not really sure. On one hand it would be nice to have a JSON encoder (though one could also do that by hand). With jsmn we already have a decoder however, that is less fat. On the other hand, as you said, there is definitely some fixing and rework required to get this in. I'd say: your work, your decision :-).

stale · 2020-04-17T08:41:57Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 22, 2017

smlng added this to the Release 2018.01 milestone Dec 22, 2017

smlng requested review from Kijewski and kaspar030 December 22, 2017 10:57

smlng mentioned this pull request Dec 22, 2017

sys: add JSON serialization and deserialization module #2189

Closed

smlng changed the title ~~Adopt/pr2189~~ Adopt PR #2189 Dec 22, 2017

smlng changed the title ~~Adopt PR #2189~~ Adopt PR #2189: a JSON en/decoder Dec 22, 2017

miri64 reviewed Dec 27, 2017

View reviewed changes

sys/include/embUnit/AssertImpl.h Outdated Show resolved Hide resolved

miri64 requested changes Dec 29, 2017

View reviewed changes

smlng force-pushed the adopt/pr2189 branch from 0f87bd6 to 2ebc713 Compare January 9, 2018 13:58

smlng force-pushed the adopt/pr2189 branch from 2ebc713 to d9fe82d Compare January 9, 2018 15:37

smlng removed this from the Release 2018.01 milestone Jan 15, 2018

smlng force-pushed the adopt/pr2189 branch 2 times, most recently from dc4ba87 to 155f093 Compare December 1, 2018 17:44

RIOT-OS deleted a comment Dec 2, 2018

miri64 requested changes Dec 6, 2018

View reviewed changes

RIOT-OS deleted a comment Dec 7, 2018

RIOT-OS deleted a comment Jan 18, 2019

smlng force-pushed the adopt/pr2189 branch from b322ca9 to 22dfb60 Compare February 25, 2019 16:44

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 29, 2019

Kijewski and others added 16 commits September 19, 2019 10:54

Add JSON serialization and deserialization module

d3a3927

fixup, documentation

d283226

fixup, headerguards

48e11e0

Add unittests for the JSON module

ee140d0

json: make ATMEGA happy

9357b94

SQUASH ME: ATMEGA & MSP430

e920086

json: Reduce .text size of unittest

9f22356

json: don't skip whitespaces on illegal input

082441c

.

b57d83f

tests

76bd40a

FIXUP: some findings via https://github.com/nst/JSONTestSuite

2ddc4d6

json: Add JSON Parsing Test Suite tests

ae5341d

FIXUP

29d1f29

fixup, coding style

e78ee4f

fixup, whitespace error

13a96b6

fixup, fix header guards

c1cef12

smlng force-pushed the adopt/pr2189 branch from 22dfb60 to c1cef12 Compare September 19, 2019 08:55

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 19, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 17, 2020

stale bot closed this May 18, 2020

keestux mentioned this pull request Jun 11, 2020

tests/xtimer_mutex_lock_timeout: don't assume number of threads at start #14271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt PR #2189: a JSON en/decoder #8312

Adopt PR #2189: a JSON en/decoder #8312

smlng commented Dec 22, 2017 •

edited

miri64 left a comment

miri64 Dec 29, 2017

Kijewski Dec 29, 2017

smlng Dec 29, 2017

miri64 Dec 29, 2017

smlng commented Jan 9, 2018

miri64 commented Jan 9, 2018

smlng commented Jan 9, 2018

smlng commented Jan 9, 2018

smlng commented Jan 10, 2018

cladmi commented Jan 10, 2018

cladmi commented Jan 10, 2018

kYc0o commented Jan 10, 2018

biboc commented Jan 11, 2018

kaspar030 commented Jan 11, 2018

tcschmidt commented Jul 11, 2018

smlng commented Jul 12, 2018

smlng commented Dec 1, 2018

miri64 commented Dec 2, 2018

miri64 left a comment

miri64 Dec 6, 2018

miri64 Dec 6, 2018

Kijewski Dec 19, 2018 •

edited

miri64 Dec 6, 2018

Kijewski Dec 19, 2018

miri64 Jan 9, 2019 •

edited

Kijewski Jan 9, 2019

stale bot commented Aug 29, 2019

smlng commented Oct 15, 2019

miri64 commented Oct 15, 2019

stale bot commented Apr 17, 2020

Adopt PR #2189: a JSON en/decoder #8312

Adopt PR #2189: a JSON en/decoder #8312

Conversation

smlng commented Dec 22, 2017 • edited

Contribution description

Issues/PRs references

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smlng commented Jan 9, 2018

miri64 commented Jan 9, 2018

smlng commented Jan 9, 2018

smlng commented Jan 9, 2018

smlng commented Jan 10, 2018

cladmi commented Jan 10, 2018

cladmi commented Jan 10, 2018

kYc0o commented Jan 10, 2018

biboc commented Jan 11, 2018

kaspar030 commented Jan 11, 2018

tcschmidt commented Jul 11, 2018

smlng commented Jul 12, 2018

smlng commented Dec 1, 2018

miri64 commented Dec 2, 2018

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kijewski Dec 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 Jan 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Aug 29, 2019

smlng commented Oct 15, 2019

miri64 commented Oct 15, 2019

stale bot commented Apr 17, 2020

smlng commented Dec 22, 2017 •

edited

Kijewski Dec 19, 2018 •

edited

miri64 Jan 9, 2019 •

edited