search

ThingSet / thingset-device-library

ThingSet library for resource-constrained devices written in C/C++
https://thingset.io/thingset-device-library/
Apache License 2.0
13 stars 6 forks source link

Updates spec v0.4 #11

Closed martinjaeger closed 2 years ago

martinjaeger commented 3 years ago

This PR contains some bugfixes and updates for the new specification v0.4 as suggested in https://github.com/LibreSolar/thingset/pull/18.

See commit messages and protocol spec updates for details.

@b0661 I would appreciate your feedback aswell.

b0661 commented 3 years ago

Data nodes are now called data objects (as it was already the case in a very early version of the protocol) in order to avoid confusion with IoT devices, which are also often called "nodes" Leaf nodes containing actual data (like Bat_V) are called data items.

Why not also adapt type names to new nomenclature?

martinjaeger commented 3 years ago

Data nodes are now called data objects (as it was already the case in a very early version of the protocol) in order to avoid confusion with IoT devices, which are also often called "nodes" Leaf nodes containing actual data (like Bat_V) are called data items.

Why not also adapt type names to new nomenclature?

  • struct ts_data_nodes -> struct ts_data_objects
  • node -> data_object
  • endpoint -> data_item

Good point. I was planning to do that, but thought it might be better to wait with this find & replace action until your remaining changes are included aswell, as such changes make rebasing very difficult. Or do you think I should add it to this PR right away?

b0661 commented 3 years ago

such changes make rebasing very difficult. Or do you think I should add it to this PR right away?

I was a general remark. Please wait, I will update PR #10 today or tomorrow.

b0661 commented 3 years ago

Please also update zephyr/tests/src/main.c to changed and new tests.

b0661 commented 3 years ago

LGTM

martinjaeger commented 2 years ago

Thanks for the reviews. I'm currently working on a few more updates to match v0.4 spec which I'm going to add to this PR. Will be ready for final review probably by end of today.

martinjaeger commented 2 years ago

Quick summary of the new commits:

  1. The renaming of data nodes to data objects had an issue with how the old functions were deprecated (it failed to compile). This has been fixed now.
  2. The pub/sub functions have been reworked as the naming caused a lot of confusion. Messages that are not confirmed and just broadcast via the network are now called statements. The process of broadcasting them is called publishing, but not the message itself. As this library does not implement the UART or CAN interfaces themselves, it can only generate the messages, but not subscribe to received messages.
  3. If an application firmware uses the ThingSet library to store data in the EEPROM (as all Libre Solar devices do) there are now new functions which export or import a specified subset (previously called pubsub channel) of all data items. These should be used for such purposes now. They do not contain the first ThingSet byte and an endpoint/path.

This is now ready to be merged from my side. I will open some issues for features that are still missing from the specification (e.g. retrieving names for given data object IDs).

b0661 commented 2 years ago

I created test coverage data using the the Zephyr tests:

The following functions are not tested at all:

Some are obviously not complex and the functionality may be covered by other tests (ts_bin_statement_by_id, ts_txt_statement_by_id). cbor_deserialize_decimal_fraction() is a noop always returning 0.

I think ts_dump_json() and cbor_deserialize_int64() are good candidates for additional tests.

Screenshot_20210803_082634

martinjaeger commented 2 years ago

Some comments regarding missing test coverage:

  • ts_bin_statement_by_id()

I will add a quick test.

  • ts_dump_json()

I did not really consider this function part of the library, but used it only for testing / debugging purposes (e.g. in the example application). But we could still add some tests. I will open a separate issue to improve test coverage.

  • ts_txt_statement_by_id()

I will add a quick test.

  • cbor_deserialize_int64()

This function is covered in the following tests, but they are currently not enabled by default: https://github.com/LibreSolar/thingset-device-library/blob/0e695f719bb8cf10de212b2b9f41fba65d9327df/test/test_common.c#L192-L197

  • cbor_deserialize_decimal_fraction()

Would you suggest to add a test even though it's currently a noop? Just so that we have one and once it's implemented the test is not forgotten?

b0661 commented 2 years ago

cbor_deserialize_int64()

This function is covered in the following tests, but they are currently not enabled by default:

Sorry, forgot to enable. Now it is tested. Much better coverage !-)

grafik

Would you suggest to add a test even though it's currently a noop? Just so that we have one and once it's implemented the test is not forgotten?

Yes, you may add an additional message in case the number of bytes processed != 0. This makes the test fail in case the function is implemented in some way.

// preset data with decimal fraction float (e.g. 1.5)
int num_bytes =  cbor_deserialize_decimal_fraction(data, mantissa, exponent);
TEST_ASSERT_EQUAL_INT_MESSAGE(0, num_bytes, "Please improve test for cbor_deserialize_decimal_fraction()!")
martinjaeger commented 2 years ago

I started to actually implement the decimal fraction function instead of adding a dummy test. But it's taking a bit longer than expected. As it's not really the scope of this PR, I will create a separate PR for that.

@b0661 Can you have a final look at the new commit where I added the missing unit tests for ts_txt_statement_by_id() and ts_bin_statement_by_id()?