New CDD API #172

sgunes-wirepas · 2024-12-09T08:33:03Z

Support for setting/getting config data items
Support for optional gateway feature flags

Existing error codes can be reused, for example:
INVALID_ROLE: trying to set an item if the role is not sink
INVALID_PARAM: invalid endpoint
SINK_OUT_OF_MEMORY: too large payload, too many items added

MauriKuorilehto

In general, I think that this takes now a bit too simplistic approach that tries to benefit from the existing API a bit too much. The intention (as far as I understood) was to make CDD management adapt a bit like a dictionary type of access pattern (endpoint being the key and the content being the value) so that each item could be managed separately. The current propasal is not directly supporting that, or at least it is bit hidden. For example, being able to delete an individual item is missing completely, getting only those items that of interest for the given backend is not available, and also it is not clear whether you can add just one item at a time or are you expected to provide the "whole list / content".

MauriKuorilehto · 2025-01-13T14:07:23Z

gateway_to_backend/protocol_buffers_files/config_message.proto

@@ -39,6 +39,11 @@ message NetworkKeys {
    required bytes authentication = 2;
 }

+message ConfigurationDataItem {
+    required uint32 endpoint = 1;
+    required bytes payload = 2;


Should the payload length be given as parameter here as well? Or is that something that we can expect to be achievable (in all the relevant client libraries / languages) via the payload itself?

bytes proto type holds the length already, it should be possible to obtain it from the libraries

MauriKuorilehto · 2025-01-13T14:09:27Z

gateway_to_backend/protocol_buffers_files/config_message.proto

@@ -72,6 +77,9 @@ message SinkReadConfig {
    optional ScratchpadInfo processed_scratchpad = 21;
    optional uint32 firmware_area_id = 22;
    optional TargetScratchpadAndAction target_and_action = 23; // Unset if sink doesn't support it
+
+    // Only present if CDD_API feature flag is set
+    repeated ConfigurationDataItem configuration_data_content = 24;


Hmm, what is included in this?

Based on the initial discussion, I thought that we agreed that this kind of "listing" all is a bit questionable. There might be different backends managing different items in the overall CDD list. Should we allow all backends to read all, or should be per request (i.e. one can ask for the endpoints of interest and receives only those).

All of the items would be included here, and there is no possibility of requesting to read a single item.

This means all of the information related to CDC would be already available in the status event messages. Maybe @GwendalRaoul can tell more about the reasoning behind having only a list based on his experience?

Yeap, I was mainly raising the point that this is not exactly according to the (initial) discussion. In case of CDD there will be multiple masters for the managed data and I would think that it would make sense to try to not mix those. If we want to have it in status message, then I guess everything needs to be listed there.

I'm not against lists as such, those would be beneficial in any case (meaning also with the "list of endpoints" case), but just wanted to raise the concern on "coupling" all the CDD items.

MauriKuorilehto · 2025-01-13T14:11:59Z

gateway_to_backend/protocol_buffers_files/config_message.proto

+
+    // Only processed if CDD_API feature flag is set
+    // 0-length payloads are used to clear an entry
+    repeated ConfigurationDataItem configuration_data_content = 11;


Is this intended to be used to set single item?

I was thinking that these operations used for managing the CDD content could warrant own messages / topics. The intention was to have one for adding, one for getting, and then one for deleting individual items (or a list of items). There should not be a need to manage the whole list of CDD items at once (instead, combining these should be done only at the sink).

It can be any number of items. In any case the sink accepts one set request at a time, and the gateway sends them one by one if multiple items are received on MQTT.

At least for the deleting items, it was agreed to use empty payload to simplify the interface (similar clearing the scratchpad on sink by uploading an empty scratchpad).

Right, thanks for reminding me about the deletion.

Now this is (if I understood correctly) quite much different from other content of this SinkNewConfig message, so would be good to consider if it makes sense to combine this 🤔 . Other fields are such that the value provided would then override the old value, where as this would be then used for per item management (if I understood correctly). So you could call this ten times in a row and give different endpoints, and then sink would combine those and send all the CDD message?

At least the differing usage pattern would need to be documented clearly, but I would still consider, if SinkNewConfig is the proper place for this.

I think these cdc items are quite similar to the app config functionality (app config, scratchpad target, and diagnostics interval would each be one if these items). When one of these is set, it's updated on the sink immediately like the other parameters.

And I agree all of this should be documented clearly somewhere.

Yeap, at the low level those would be similar (all coupled to NPD/CDD when sent in Mesh). However, what I meant, at the gateway-to-backend API this now introduces a whole new operation pattern for the SinkNewConfig. None of the other messages has such, instead in all of the other cases write is "atomic" (or whatever you want to call it), i.e. newly provided value replaces the old value (instead of appending or removing parts of it).

sgunes-wirepas · 2025-01-16T15:54:17Z

gateway_to_backend/README.md

+sink, meaning that the gateway must report the new complete configuration data
+content by generating a new [GetConfigsResp][message_GetConfigsResp] message
+with a request id 0, and a new [StatusEvent][message_StatusEvent] message._
+


I suppose having nodes with non-sink role connected as sinks to the gateway is not a supported scenario, so no need to define what happens if the CDC contents change on such a system?

I guess not for that specific case. However, I do think that either the naming of the topic or alternative this documentation would need to be tuned somehow to explain what does these actually configure. Now the topic name is very close to set_config, so it kind of makes you to assume that you are setting part of that config.

This is anyways mesh level CDD content item, so I wonder should these be set_mesh_config_item or set_mesh_config_data_item. Or if we want to be more explicit, could be set_cdc_item, which would not have possibility to misunderstanding, but would require explanation what it means.

Yeah this is a tricky one, we decided to use naming that is consistent with DECT-2020 specification (page 59), but I agree on it's own the naming is not very self explanatory in this interface.

leppaott · 2025-01-17T09:17:20Z

gateway_to_backend/README.md

+
+:warning:
+
+_Successfuly setting an item must be treated as a configuration change on the


Successfully

MauriKuorilehto · 2025-01-17T11:14:41Z

gateway_to_backend/README.md

+sink, meaning that the gateway must report the new complete configuration data
+content by generating a new [GetConfigsResp][message_GetConfigsResp] message
+with a request id 0, and a new [StatusEvent][message_StatusEvent] message._
+


I guess not for that specific case. However, I do think that either the naming of the topic or alternative this documentation would need to be tuned somehow to explain what does these actually configure. Now the topic name is very close to set_config, so it kind of makes you to assume that you are setting part of that config.

This is anyways mesh level CDD content item, so I wonder should these be set_mesh_config_item or set_mesh_config_data_item. Or if we want to be more explicit, could be set_cdc_item, which would not have possibility to misunderstanding, but would require explanation what it means.

MauriKuorilehto · 2025-01-17T11:16:29Z

gateway_to_backend/protocol_buffers_files/config_message.proto

+    required RequestHeader header = 1;
+
+    // 0-length payloads are used to clear an entry
+    required ConfigurationDataItem configuration_data_item = 2;


I wonder whether this should be repeated list still? That would at least then leave the decision whether to update one or more at a time to the client side.

If it's repeated, I think we would need to send all of the CDC items in the reply. Since they would be set one by one on the sink, some of the sets might be successful and some not.

Right indeed, this is tricky part as each item might be handled in the end by different entity in the sink and naturally there is no "transactionality" with these. I guess it is not too big of a drawback, but maybe something to worth discussing with wider audience as well next week.

MauriKuorilehto · 2025-01-17T11:17:56Z

gateway_to_backend/protocol_buffers_files/error.proto

@@ -47,4 +47,6 @@ enum ErrorCode {
    // If scratchpad chunk size is bigger than
    // the max value supported by the gateway
    INVALID_SCRATCHPAD_CHUNK_SIZE = 29;
+    INVALID_CDC_ENDPOINT = 30;
+    INVALID_CDC_PAYLOAD = 31;


I wonder if we could have used INVALID_DEST_ENDPOINT and INVALID_DATA_PAYLOAD for these?

Yeah definitely, didn't notice the endpoint one. Probably it's better to reuse existing codes since we explain what each error code means in the readme.

MauriKuorilehto · 2025-01-17T11:23:46Z

gateway_to_backend/README.md

@@ -518,6 +545,7 @@ definition)

    gw-request/otap_set_target_scratchpad/<gw-id>/<sink-id>

+    gw-request/set_configuration_data_item/<gw-id>/<sink-id>


Now thinking of it, I do think that we should probably have similar get API for this. Then it would be more analogical with other setter type of API methods (you can read all of those with corresponding get method). While that is to some extent available at the moment as well, there is just extra overhead if we would need to query whole sink configs if some backend component is just interested on its own appconfig type of application configuration etc.

MauriKuorilehto · 2025-01-17T11:24:38Z

gateway_to_backend/protocol_buffers_files/config_message.proto

@@ -124,6 +132,15 @@ message GatewayInfo {
 * Commands definition
 */
 message StatusEvent {
+    enum OptionalGatewayFeature {


Should this be made available also in GetGwInfoResp as well?

I think that would be good, yes. Thanks!

sgunes-wirepas requested review from GwendalRaoul and MauriKuorilehto December 9, 2024 08:33

LG-75 CDD API and optional feature flags

44531af

sgunes-wirepas force-pushed the feature/LG-75-cdd_api branch from 4b93602 to 44531af Compare January 10, 2025 11:22

sgunes-wirepas marked this pull request as ready for review January 10, 2025 11:25

MauriKuorilehto reviewed Jan 13, 2025

View reviewed changes

sgunes-wirepas added 4 commits January 16, 2025 17:27

LG-75 Modify the CDD API to have a dedicated message for setting an item

f0ec509

LG-75 Documentation for the new SetConfigurationDataItem message

3b48139

Fix errors in readme

57d4386

LG-75 Define error conditions for .SetConfigurationDataItemReq

82ad619

sgunes-wirepas commented Jan 16, 2025

View reviewed changes

leppaott reviewed Jan 17, 2025

View reviewed changes

Fix typo

2b3b385

MauriKuorilehto reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New CDD API #172

New CDD API #172

sgunes-wirepas commented Dec 9, 2024 •

edited

Loading

MauriKuorilehto left a comment

MauriKuorilehto Jan 13, 2025

sgunes-wirepas Jan 14, 2025 •

edited

Loading

MauriKuorilehto Jan 13, 2025

sgunes-wirepas Jan 14, 2025

MauriKuorilehto Jan 14, 2025

MauriKuorilehto Jan 13, 2025

sgunes-wirepas Jan 14, 2025

MauriKuorilehto Jan 14, 2025

sgunes-wirepas Jan 14, 2025

MauriKuorilehto Jan 15, 2025

sgunes-wirepas Jan 16, 2025

MauriKuorilehto Jan 17, 2025

sgunes-wirepas Jan 17, 2025

leppaott Jan 17, 2025

MauriKuorilehto Jan 17, 2025

MauriKuorilehto Jan 17, 2025

sgunes-wirepas Jan 17, 2025

MauriKuorilehto Jan 17, 2025

MauriKuorilehto Jan 17, 2025

sgunes-wirepas Jan 17, 2025

MauriKuorilehto Jan 17, 2025

MauriKuorilehto Jan 17, 2025

sgunes-wirepas Jan 17, 2025


		:warning:

		_Successfuly setting an item must be treated as a configuration change on the

		@@ -518,6 +545,7 @@ definition)

		gw-request/otap_set_target_scratchpad/<gw-id>/<sink-id>

		gw-request/set_configuration_data_item/<gw-id>/<sink-id>

New CDD API #172

Are you sure you want to change the base?

New CDD API #172

Conversation

sgunes-wirepas commented Dec 9, 2024 • edited Loading

MauriKuorilehto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgunes-wirepas Jan 14, 2025 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgunes-wirepas commented Dec 9, 2024 •

edited

Loading

sgunes-wirepas Jan 14, 2025 •

edited

Loading