modules: main: Add CDDL CBOR encoding of device shadow#117
Merged
jtguggedal merged 1 commit intomainfrom Mar 18, 2025
Merged
Conversation
simensrostad
force-pushed
the
decodeincomingmdata
branch
from
d5fc33c to
5970405
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🎯 Core Changes
- The main purpose of this PR is to integrate CDDL CBOR decoding for incoming data, specifically to decode the update interval set in the cloud.
- Added
zcborcommand inCMakeLists.txtto generate encoder code using a CDDL file. - Updated
cbor_helper.cto decode CBOR data and extract the update interval. - Modified the test suite to include tests for the new CBOR decoding functionality.
- No critical concerns identified. The changes appear to be well-integrated and tested.
Verdict:
- Approve: The changes are well-structured, and the added functionality is covered by tests. The integration of CBOR decoding is a necessary enhancement for handling cloud data efficiently.
Code review performed by OPENAI - gpt-4o.
| # generated code and the necessary zcbor C code files. | ||
| include(${CMAKE_CURRENT_BINARY_DIR}/device_shadow.cmake) | ||
|
|
||
| # Ensure that the cmake reconfiguration is triggerred everytime the cddl file changes. |
Contributor
There was a problem hiding this comment.
Typo: 'triggerred' should be 'triggered'.
|
|
||
| int err = cbor_decode_config_object(cbor, len, &object, ¬_used); | ||
|
|
||
| if (err) { |
Contributor
There was a problem hiding this comment.
Consider adding error logging for cbor_decode_config_object to provide more context in case of failure.
tests/module/main/src/main.c
Outdated
| static void send_config(uint64_t interval) | ||
| static void twelve_hour_interval_set(void) | ||
| { | ||
| // MISSING: set interval for 12 hours, or even different intervals. |
Contributor
There was a problem hiding this comment.
The comment mentions a missing feature to set different intervals. Consider implementing this to enhance test coverage.
jtguggedal
force-pushed
the
decodeincomingmdata
branch
from
5970405 to
d1334dd
Compare
Contributor
There was a problem hiding this comment.
🎯 Core Changes
- The primary goal of this PR is to integrate CDDL CBOR decoding for incoming data, particularly to decode the update interval set in the cloud. This involves generating encoder code using zcbor and updating the CMakeLists to include the necessary steps for code generation and linking.
- The
cbor_helper.cfile has been updated to include decoding logic using thecbor_decode_config_objectfunction, replacing the previous hardcoded interval. - The
device_shadow.cddlfile has been simplified to focus on theconfig-objectwith anupdate_intervalfield. - The state machine in
main.chas been adjusted to handle the decoded update interval and manage state transitions accordingly. - Additional test cases and utility functions have been added to ensure the correct behavior of the CBOR decoding and state transitions.
- There is a potential risk of symbol name collisions due to the use of
--short-namesin the zcbor command.
- Verdict:
- Approve: The changes introduce necessary functionality for CBOR decoding and are well-integrated into the existing codebase. The updates include comprehensive test coverage to ensure reliability.
Code review performed by OPENAI - gpt-4o.
Add missing CDDL CBOR decoding of incoming data. Used to decode update interval set in cloud. Tests are updated to check new configuration, in addition to using some convenience functions for expected events. Signed-off-by: Simen S. Røstad <simen.rostad@nordicsemi.no>
jtguggedal
force-pushed
the
decodeincomingmdata
branch
from
d1334dd to
cd935f0
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add missing CDDL CBOR decoding of incoming data.
Used to decode update interval set in cloud.