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

Feature/wakaama update on new version #20930

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

moritzholzer
Copy link
Contributor

Contribution description

This patch updates the wakaama library to a new version (Commit ee0c98da7495e1ab43358b49e970dbc97f73ce3b).
It updates the RIOT code to the new interfaces.

Testing procedure

Test with the lwm2m example, with leshan demo server.

@github-actions github-actions bot added Area: build system Area: Build system Area: pkg Area: External package ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Oct 21, 2024
@leandrolanzieri
Copy link
Contributor

leandrolanzieri commented Oct 21, 2024

Hey @moritzholzer, thanks for working on this!

For an easier reviewing process, I'd suggest you try to squash unnecessary commits (e.g., the merges). Please, make sure you check our commit conventions https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#commit-conventions

Regarding the version bump, do you know whether there are plans from Wakaama on releasing a version 2.0? With this update, which is the highest LwM2M version that is supported?

@moritzholzer
Copy link
Contributor Author

I wil squash the commits later.

The lwm2m version is 1.0, some features 1.1.
See Which LWM2M version Wakaama Client supports?

From what i can get from the issues in the wakaama repo there is not a fixed plan for a new version but it was discussed in the past without an conclusion.

@moritzholzer moritzholzer force-pushed the feature/wakaama_update branch from daf550d to 0af25ba Compare October 21, 2024 18:02
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Here's a first round of comments

pkg/wakaama/Makefile Outdated Show resolved Hide resolved
pkg/wakaama/Makefile.include Outdated Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_client.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_client.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/lwm2m_client_connection.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/objects/device.c Show resolved Hide resolved
pkg/wakaama/contrib/objects/device.c Show resolved Hide resolved
pkg/wakaama/contrib/objects/ipso_sensor_base.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/objects/light_control.c Outdated Show resolved Hide resolved
pkg/wakaama/contrib/objects/security.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Area: doc Area: Documentation label Oct 23, 2024
@moritzholzer
Copy link
Contributor Author

I'v worked through the comments.

@leandrolanzieri
Copy link
Contributor

I tested it on native and works well

@leandrolanzieri leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 23, 2024
@riot-ci
Copy link

riot-ci commented Oct 23, 2024

Murdock results

✔️ PASSED

97acd2e examples/lwm2m: changes on example application

Success Failures Total Runtime
10215 0 10215 17m:54s

Artifacts

@leandrolanzieri
Copy link
Contributor

There seems to be a problem with a printf on object_server.c. Check the CI output

@leandrolanzieri
Copy link
Contributor

You may want to remove the Makefile.ci and run make generate-Makefile.ci in the example (IT TAKES SOME TIME) to exclude boards that can't fit the application anymore.

@leandrolanzieri
Copy link
Contributor

Looks good. There's only one style comment pending. Please squash directly, so that we keep only meaningful commits in the history.

@moritzholzer moritzholzer force-pushed the feature/wakaama_update branch from 2aa1f8d to 863f0f6 Compare October 25, 2024 08:39
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels Oct 25, 2024
@moritzholzer moritzholzer force-pushed the feature/wakaama_update branch from 863f0f6 to abed01c Compare October 25, 2024 08:45
@github-actions github-actions bot removed the Area: tests Area: tests and testing framework label Oct 25, 2024
@github-actions github-actions bot removed the Area: drivers Area: Device drivers label Oct 25, 2024
@moritzholzer
Copy link
Contributor Author

Is it enought squashed or are less commits better?

@leandrolanzieri
Copy link
Contributor

Thanks @moritzholzer. I think there are some mix ups on the commit history. I see, for example, various changes of the pkg version? (ec5d90f and e0fe270)

I would suggest having the following commits:

  • version bump (updates on pkg makefiles, updates of the patches)
  • changes on config and OS services (addition of random seed, Kconfig)
  • adaption of contrib to new interface (changes on our object implementations and their docs)
  • changes on example application

Also please check the messages. They should always start with the area of changes (e.g., pkg/wakaama, examples/lwm2m). Also, we use present tense. If you want to elaborate on large changes, do so by leaving a blank line after the short description.

@moritzholzer moritzholzer force-pushed the feature/wakaama_update branch from abed01c to 0829293 Compare October 31, 2024 09:27
@moritzholzer
Copy link
Contributor Author

Thank you @leandrolanzieri for the help:)
I did my best to restructure the commits.

Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Looks good!

@leandrolanzieri
Copy link
Contributor

@moritzholzer moritzholzer force-pushed the feature/wakaama_update branch from 0829293 to 97acd2e Compare November 12, 2024 07:47
@moritzholzer
Copy link
Contributor Author

I removed the whitespaces.

@maribu
Copy link
Member

maribu commented Nov 12, 2024

Before the merge queue triggers a timeout, I'll drop and requeue this one.

@maribu maribu removed this pull request from the merge queue due to a manual request Nov 12, 2024
@maribu maribu added this pull request to the merge queue Nov 12, 2024
Merged via the queue into RIOT-OS:master with commit 5f661f4 Nov 13, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants