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

sys/phydat: fix phydat_to_json precondition #18117

Merged
merged 1 commit into from
May 19, 2022

Conversation

LP-HAW
Copy link
Contributor

@LP-HAW LP-HAW commented May 19, 2022

Contribution description

RIOT kernel panics when calling phydat_to_json with dim=3:

sys/phydat/phydat_json.c:46 => 0x8049c14
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

The precondition/assertion for parameter 'dim' in phydat_to_json is to restrictive. Calling phydat_to_json with dim=3 should be valid as can be seen in the documentation of phydat_to_json itself:
https://doc.riot-os.org/group__sys__phydat.html#gad7a5978e5119e3976addffd2891c76fa

This pull request allows phydat_to_json to be called with dim=3.

Testing procedure

Just call phydat_to_json with dim=3. For example:

#include <stdio.h>
#include <phydat.h>

int main(void)
{
    char json[100];
    phydat_t data = {0};
    phydat_to_json(&data, 3, json);
    puts(json);
}

Expected output would be:

{"d":[0,0,0],"u":"undefined"}

Issues/PRs references

(no references)

@github-actions github-actions bot added the Area: sys Area: System label May 19, 2022
@benpicco benpicco requested review from haukepetersen and chrysn May 19, 2022 09:44
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels May 19, 2022
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Thanks, good catch and straightforward fix.

@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 19, 2022
@chrysn chrysn enabled auto-merge May 19, 2022 09:53
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 19, 2022
@maribu
Copy link
Member

maribu commented May 19, 2022

Congratulations and thanks for your first PR :)

@chrysn chrysn merged commit bb56b34 into RIOT-OS:master May 19, 2022
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants