-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CBOR support #709
Add CBOR support #709
Conversation
tests/cbor_tests.c
Outdated
"Unknown")))))))) | ||
#endif | ||
|
||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this disabled. Should probably not be committed. At least it should be configurable by a proprocessor define.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just imported the code and my IDE conveniently hides the section. I will remove it.
data/senml_cbor.c
Outdated
@@ -139,6 +139,8 @@ static int prv_parseItem(const uint8_t *buffer, size_t bufferLen, senml_record_t | |||
bvSeen = true; | |||
memcpy(baseValue, &data, sizeof(lwm2m_data_t)); | |||
/* Convert explicit 0 to implicit 0 */ | |||
_Pragma("GCC diagnostic push"); | |||
_Pragma("GCC diagnostic ignored \"-Wfloat-equal\""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing floats for equality is not a very good idea.
Could use fpclassify
(since C99) or something like fabsf(a - b) < FLT_EPSILON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO we can keep it this way for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like the compiler specific _Pragma. Not everyone uses gcc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are just 3 equality checks in there outside of the examples and tests. I'm more concerned with the base library than those.
- Check for NaN which could be replaced by isnan(), I was just targeting C90 when I wrote it.
- Check whether or not anything would be lost converting double to float (and back). If not, then it can be encoded single precision. Another method would probably need to break it down into the exponent and mantissa and check those. That may need to make assumptions about the underlying hardware representation, although that assumption is probably already being made.
- Explicitly check for 0. Something like fabsf(a - b) < FLT_EPSILON would involve extra floating point computations and classify anything less than FLT_EPSILON as 0, which would be incorrect. Again, checks on the mantissa and exponent could be done, but be careful that -0 and +0 are treated the same. That would also likely be more computation than a simple compare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use fpclassify
since we rely on C11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use fpclassify since we rely on C11.
Will do this in a separate PR. There are already some Pragma in the code to mute floating point comparison compiler warnings. They can be removed as well.
data/cbor.c
Outdated
@@ -14,12 +14,14 @@ | |||
* Scott Bertin, AMETEK, Inc. - Please refer to git log | |||
* | |||
*******************************************************************************/ | |||
|
|||
#define _DEFAULT_SOURCE | |||
#include <endian.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endian.h is glibc specific and may not exist with other compilers or libraries.
data/senml_cbor.c
Outdated
static int prv_serializeTlv(const lwm2m_data_t *tlvP, const uint8_t *baseUriStr, size_t baseUriLen, | ||
uri_depth_t baseLevel, const uint8_t *parentUriStr, size_t parentUriLen, uri_depth_t level, | ||
bool *baseNameOutput, uint8_t *buffer, size_t bufferLen) { | ||
static int prv_serializeCbor(const lwm2m_data_t *tlvP, const uint8_t *baseUriStr, size_t baseUriLen, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the name change? It is serializing TLV (lwm2m_data_t) to CBOR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware that the TLV term is also used for the internal data representation lwm2m_data_t
. I will change it back.
080be99
to
278dbd7
Compare
6538ba9
to
b43ed07
Compare
This commit contains the contribution from Scott Bertin to support SenML-CBOR with only changes resolving conflicts. Signed-off-by: Scott Bertin <sbertin@telular.com> Signed-off-by: Pascal Brogle <pascal.brogle@husqvarnagroup.com> Signed-off-by: Marc Lasch <marc.lasch@husqvarnagroup.com>
b43ed07
to
94d896a
Compare
94d896a
to
16d7a7c
Compare
#ifdef LWM2M_SUPPORT_SENML_CBOR | ||
// defined in cbor.c | ||
int cbor_get_type_and_value(const uint8_t *buffer, size_t bufferLen, cbor_type_t *type, uint64_t *value); | ||
int cbor_get_singular(const uint8_t *buffer, size_t bufferLen, lwm2m_data_t *dataP); | ||
int cbor_put_type_and_value(uint8_t *buffer, size_t bufferLen, cbor_type_t type, uint64_t val); | ||
int cbor_put_singular(uint8_t *buffer, size_t bufferLen, const lwm2m_data_t *dataP); | ||
#if !defined(LWM2M_VERSION_1_0) && !defined(LWM2M_VERSION_1_1) | ||
int cbor_parse(const lwm2m_uri_t *uriP, const uint8_t *buffer, size_t bufferLen, lwm2m_data_t **dataP); | ||
int cbor_serialize(const lwm2m_uri_t *uriP, int size, const lwm2m_data_t *dataP, uint8_t **bufferP); | ||
#endif | ||
|
||
// defined in senml_cbor.c | ||
int senml_cbor_parse(const lwm2m_uri_t *uriP, const uint8_t *buffer, size_t bufferLen, lwm2m_data_t **dataP); | ||
int senml_cbor_serialize(const lwm2m_uri_t *uriP, int size, const lwm2m_data_t *tlvP, uint8_t **bufferP); | ||
#endif | ||
|
||
// defined in senml_common.c | ||
#if defined(LWM2M_SUPPORT_JSON) || defined(LWM2M_SUPPORT_SENML_JSON) || defined(LWM2M_SUPPORT_SENML_CBOR) | ||
int senml_convert_records(const lwm2m_uri_t *uriP, senml_record_t *recordArray, int numRecords, | ||
senml_convertValue convertValue, lwm2m_data_t **dataP); | ||
lwm2m_data_t *senml_extendData(lwm2m_data_t *parentP, lwm2m_data_type_t type, uint16_t id); | ||
int senml_dataStrip(int size, lwm2m_data_t *dataP, lwm2m_data_t **resultP); | ||
lwm2m_data_t *senml_findDataItem(lwm2m_data_t *listP, size_t count, uint16_t id); | ||
uri_depth_t senml_decreaseLevel(uri_depth_t level); | ||
int senml_findAndCheckData(const lwm2m_uri_t *uriP, uri_depth_t baseLevel, size_t size, const lwm2m_data_t *tlvP, | ||
lwm2m_data_t **targetP, uri_depth_t *targetLevelP); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not really done at all in Wakaama, but having (at least) the return values documented would be reeeeeeeally nice. How is success encoded? >0? How an error? <0?
(Feel free to ignore/"resolve conversation" this input. Will approve anyway.)
I'm not involved in any projects using LWM2M anymore, so don't wait for my review. My only objection remains the use of the gcc specific _Pragma. |
This change only contains cosmetic changes to comply with the current code format style for the SenML-CBOR contribution enforced by clang-format and cmake-format.
With -Wstrict-prototypes enforced, all functions need a prototype.
because zephyr does not support float16
Silence the existing, benign looking code, but ensure we get a warning for new code with such issues.
With -Wstrict-aliasing enforced, simple type casting between different lvalue is not allowed.
Convert endianness to also support big-endian systems.
This will silence -Wsign-compare warnings.
Add capability to log data types to improve logging in CBOR code.
16d7a7c
to
389bf5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested/used the code, but looks sane enough to me.
Add CBOR support to LwM2M server and client introduced with the LwM2M 1.1 standard. This PR is based on work from @sbertin-telular with changes to work with the current build system. Thanks for the contribution.
CBOR support can be enabled by compiling the client with
LWM2M_SUPPORT_SENML_CBOR
flag enabled. The server will support SenML-CBOR when compiled with LwM2M 1.1 support.