Updates for ThingSet Protocol draft v0.5#24
Conversation
The ThingSet specification v0.5 introduces a different data layout compared to previous versions where the data is grouped by entities of the device (like battery, actuator) instead of the data type (e.g. measurement, configuration). The data type is described by a single character prefix in the item name. The new grouping allows to have same item names in different groups, so for unambigous description of the data the nested structure has to be maintained in statements. This option is introduced to maintain compatibility with legacy firmware and will be enabled by default in the future.
Reduces the required RAM and flash memory by 4 bytes * number of data objects. Drawback: Only 8 instead of 16 different subsets can be defined (which is considered plenty) and the maximum length of strings (specified in "detail" struct member) is 2^11.
Previously, configuration values had to be stored all in a single category with a callback, which could be used to store data in the NVM after an update. If data items in all groups can be stored values (as planned for ThingSet v0.5) the callback method is not feasible anymore. Instead, the new flag can be used to identify if items were updated. Currently it is not checked if the new value was actually different from the previously stored value. Any patch request received via ThingSet will set the updated flag of all included items to true. As we are now changing the data object, some const specifiers had to be removed. The number of allowed subsets in the bit set had to be reduced to 7 to keep the overall memory footprint.
If nested JSON is used, subset elements must contain the path instead of just the names of the data items, as the names may be ambiguous.
src/thingset.h
Outdated
| const uint32_t type : 4; | ||
|
|
||
| /** | ||
| * Exponent (10^exponent = factor to convert to SI unit) for decimal fraction type, |
There was a problem hiding this comment.
Could you improve the documentation a little bit in the course of the change. It took me quite a time to understand that this are three different usages of the detail information depending on the type of data object. And I'm stiill not shure how to handle the exponent of the decimal fraction type. It seems you expect this to be a fixed value and not something that can be adapted acc. to the value.
For doxygen it would be nice to have a brief description and a detailed description.
There was a problem hiding this comment.
Good point. Will improve the docs here.
The idea was to have it fixed so that e.g. the firmware could store values in mV as integers, but send out the data in V without any numeric conversion to float. In that case the exponent would be -3. But so far I've never actually used the decimal fraction type (I can't even remember if it's implemented...).
There was a problem hiding this comment.
I have updated the documentation, please have a look.
src/thingset.h
Outdated
| * @param subsets Flags to select which subset(s) of data items should be considered | ||
| * @param clear If set to true, the updated status is cleared after checking. | ||
| */ | ||
| bool ts_check_updated(struct ts_context *ts, const uint16_t subsets, bool clear); |
There was a problem hiding this comment.
Do we really need that? Isn't there anyway a change callback that may be used by an application?
There was a problem hiding this comment.
The change callback is currently assigned to a group. If any of the data objects in that group are changed, the callback is called. But if we change data in multiple groups, a separate callback for every group would have to be assigned and multiple callbacks would have to be called.
Maybe a better approach would be to assign a callback for every patch request instead. I'll think about this, thanks for the pointer.
There was a problem hiding this comment.
Maybe a better approach would be to assign a callback for every patch request instead. I'll think about this, thanks for the pointer.
A ThingSet context may have an update counter. On every patch request the update counter may be incremented and every data object that is changed will have it's own update counter set to the context's update count. The general callback can then search for updated data objects by the update count. As the update count in the data objects should be of small size (e.g. 2 bits) all object update counts that are equal to the current one shall be invalidated before a patch request is applied. By this the last (e.g. 4) "transactions" on the object database can be retrieved by an application. This may help for multi patch request updates to the object database.
The general patch callback should have a void(*cb)(uintxx_t update_count, other???) signature to care for update count races in the execution of several patch requests (the context update count may already be incremented again when the callback is executed).
Note: Commented again - last comment got somehow lost in GitHub
There was a problem hiding this comment.
I'm not sure if I understand correctly. How would the counter value be transferred over the wire?
I don't think we should support multiple patch requests in parallel. If one patch request is still in progress, the next requester will have to wait until the request is finished.
There was a problem hiding this comment.
I'm not sure if I understand correctly. How would the counter value be transferred over the wire?
Never !-) The counter is just a way to mark a data object was changed. This way the callback function can search the database for changed data objects. The counter just helps to in case of asynchronous callback function execution and a new patch executed at the same time the callback function is still active from the last patch request. If we serialise patch -> callback -> patch a data changed flag would be sufficient.
I don't think we should support multiple patch requests in parallel. If one patch request is still in progress, the next requester will have to wait until the request is finished.
Yes - patch requests to the database have to be serialised. The design decision is whether a patch callback that is executed after the patch is applied can block further patch request to be executed until it finishes. In my opinion this should not be the case.
There was a problem hiding this comment.
I have reverted the ts_check_updated function and introduced a callback for the patch functions. This avoids to iterate through all data objects for each patch request and is quite similar to the previous approach where the callback was assigned per group.
Now the application can decide what to do after a patch request. It can either store the updated data in NVM synchronously in the callback (which would delay the ThingSet response) or it stores the data asynchronously and takes care of any race conditions. Handling asynchronous data processing inside the library will make it too complicated for very simple devices with low processing power, I think.
This reverts commit a2910b5. Reason: Iterating twice through the entire data objects list after every call to ts_process is not very efficient. A callback should be called directly from the patch function if something was changed.
martinjaeger
force-pushed
the
thingset-v0.5
branch
from
83eef50 to
b9d4590
Compare
This is a new attempt for the reverted method using a single update flag per data object. Prior to v0.5 spec, configuration values had to be stored all in a single group with a callback assigned to it. This callback could be used to store data in the NVM after an update. If data items in all groups can be stored values (as planned for ThingSet v0.5) the group callback method is not feasible anymore. Instead, an update notification callback per patch request as introduced in this commit is necessary. Currently it is not checked if a new value was actually different from the previously stored value. Any valid patch request received via ThingSet will call the specified update callback function.
martinjaeger
force-pushed
the
thingset-v0.5
branch
from
b9d4590 to
cd5c514
Compare
|
As this PR provides backwards compatibility and does not break existing firmware I'll also merge it to benefit from smaller firmware size. |
2 participants
Background
While developing a mobile phone app that translates ThingSet data objects read from a device into a user interface, I realized several shortcomings with the approach of structuring the data into categories like
info,conf,meas, etc. Most importantly, it was not semantically clear which data items could be written, so an app would have to try writing a value, process the response and if it is allowed to write the value, display it in a text box so that it can be changed via the UI.The v0.5 spec introduces prefixes for data items which define if an item is write-able, read-only, executable, stored in RAM or flash, etc. This makes the previous categories obsolete and data can be structured more logically based on features of the device.
An example using the new data structure can be seen in ThingSet Spec v0.5 (draft). Also consider the section "User interface processing" at the end.
As data item names don't have to be globally unique anymore (e.g. there can be a measured current for the battery
Bat/rMeas_Aand the loadLoad/rMeas_A) the JSON in the statement messages needs to be nested.What does this PR change
TS_NESTED_JSON(orCONFIG_THINGSET_NESTED_JSONfor Zephyr) is introduced which activates nested JSON for statements (see also comments in the code for the option). For compatibility reasons, the option is disabled by default.TS_NESTED_JSONis enabled, references in subsets use the path to an item (e.g.Bat/rMeas_A) instead of just the name (rMeas_A).updatedbit is introduced for each data item.struct ts_data_objectsis updated to use bitsets for reduced memory footprint. By doing this, we save 4 bytes for each data object in flash and RAM even though we added above mentionedupdatedbit parameter to be stored.In order to reduce noise in the PR, the new naming schema with prefixes is not fully adapted for the unit tests. I might update the test data in a dedicated PR without functional changes.
Additional information
@daniel-connectedenergy Commit 7ce2093 will be most interesting for you as it reduces memory footprint with zero effort.