Skip to content
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

Enable SENML_JSON support in lightclient #520

Merged
merged 1 commit into from
Jan 20, 2021

Conversation

qleisan
Copy link

@qleisan qleisan commented Jan 12, 2021

Lightclient example now works out-of-the-box with wakaama server example.

Signed-off-by: Leif Sandstrom leif.sandstrom@husqvarnagroup.com

Wakaama client / server examples should work together after a "vanilla build" to provide a good user experience.

@sbertin-telular
Copy link
Contributor

The server should support all formats. I think the JSON formats were added to the other client to support command line enhancements.

This makes me wonder what format the lightclient was using before this change. I lost everything on my hard disk recently and haven't gotten set back up for building Wakaama to check. If it supported no formats, we should probably add something to liblwm2m.h to give a reasonable default for people writing their own client.

@sbertin-telular
Copy link
Contributor

This makes me wonder what format the lightclient was using before this change. I lost everything on my hard disk recently and haven't gotten set back up for building Wakaama to check. If it supported no formats, we should probably add something to liblwm2m.h to give a reasonable default for people writing their own client.

I checked. The lightclient only supported plain text and probably opaque. Nothing that could represent any structured data.

@qleisan
Copy link
Author

qleisan commented Jan 19, 2021

TLV was supported until ab776be "Make TLV format optional.". The 1.1 spec states

"LwM2M Clients supporting LwM2M TS 1.1 or newer SHOULD NOT use the legacy TLV or JSON formats"

Although the lightclient has no CLI we still need to support a valid format for the payload, right? (any finer points about "structured data" that needs to be highlighted?)

The logic for TLV support or not is handled differently (header file logic) from SENML/JSON (cmake file logic). I used the pattern from the regular client to adapt the lightclient. Any thoughts on this?

if(LWM2M_VERSION VERSION_GREATER "1.0")
add_definitions(-DLWM2M_SUPPORT_SENML_JSON)
else()
add_definitions(-DLWM2M_SUPPORT_JSON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLV will be supported by default for LWM2M 1.0. For a lightweight client I don't think we need to add JSON also.

@sbertin-telular
Copy link
Contributor

For LWM2M 1.1 there is no clear default once SENML CBOR support is added. I'm good with having it in the cmake file. This solves it for lightclient and I created #523 for the general case.

@qleisan
Copy link
Author

qleisan commented Jan 20, 2021

@sbernard31 @sbertin-telular Is this good to merge now? (or shall I remove LWM2M_SUPPORT_JSON for the 1.0 case since the lightclient doesn't have a CLI)

@sbernard31
Copy link
Contributor

(I let you decide about this, I merge it as soon as the PR is approved 🙂)

@sbertin-telular
Copy link
Contributor

(or shall I remove LWM2M_SUPPORT_JSON for the 1.0 case since the lightclient doesn't have a CLI)

I'd like to see it removed. It is supposed to be a lightweight example. Adding unnecessary features is counter to that.

@sbernard31
Copy link
Contributor

(@qleisan, if you adapt the PR, could you rebase+push force)

Lightclient example now works out-of-the-box with wakaama server example.

Signed-off-by: Leif Sandstrom <leif.sandstrom@husqvarnagroup.com>
@qleisan
Copy link
Author

qleisan commented Jan 20, 2021

Agree! New version pushed

@sbernard31 sbernard31 merged commit 5e5b718 into eclipse-wakaama:master Jan 20, 2021
@qleisan qleisan deleted the fixlc branch January 20, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants