Skip to content

Refactor boolean assignments from cJSON - #205

Open
Tarik2142 wants to merge 2 commits into
espressif:mainfrom
Tarik2142:main
Open

Refactor boolean assignments from cJSON#205
Tarik2142 wants to merge 2 commits into
espressif:mainfrom
Tarik2142:main

Conversation

@Tarik2142

Copy link
Copy Markdown
Contributor

Description

SecurityPolicy flags in the active Thread dataset cannot be set to
false. They are always forced to true, regardless of the input.

Steps to reproduce:

  1. Set an active dataset with all five optional SecurityPolicy flags
    (CommercialCommissioning, AutonomousEnrollment, NetworkKeyProvisioning,
    TobleLink, NonCcmRouters) explicitly set to false via PUT /node/dataset/active with a JSON body containing
    "false" for all five fields.
  2. Wait for the changes to take effect.
  3. Read back the dataset GET /node/dataset/active

The actual bool value is mapped to item->valueint (1 for true, 0 for false).

Related

Testing


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@gytxxsy

gytxxsy commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Thanks for sharing this! There are two suggestions:

For the fields already guarded by cJSON_IsBool(), please assign with cJSON_IsTrue(value) instead of value->valueint. That is the usual way to read a cJSON boolean.

Please leave defaultRoute in network_formation_param_json_convert2_struct as-is. The Web GUI and docs send 1/0 (a number), so the original cJSON_GetNumberValue() is already correct. Switching it to valueint does not fix a bug. If we want this field to be a JSON bool, that should be done together with the frontend (and docs); otherwise keep the current number handling.

value = cJSON_GetObjectItemCaseSensitive(jsonTimestamp, "Authoritative");
if (cJSON_IsBool(value)) {
aTimestamp->mAuthoritative = (bool)cJSON_GetNumberValue(value);
aTimestamp->mAuthoritative = value->valueint;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
aTimestamp->mAuthoritative = value->valueint;
aTimestamp->mAuthoritative = JSON_IsTrue(value);

Please do the same elsewhere.

temp = cJSON_GetObjectItem(root, "defaultRoute");
if (temp && temp->type != cJSON_NULL)
param->default_route = (bool)cJSON_GetNumberValue(temp);
param->default_route = temp->valueint;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep it as is; otherwise, the frontend should also be fixed (defaultRoute should be a boolean rather than a number).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand what the problem is here.
The frontend uses 1/0 numeric values

defaultRoute: form.defaultRoute.checked ? 1 : 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

defaultRoute represents whether the prefix is advertised as a default route, so it should be a boolean. When we implemented the frontend, we used 0/1. Although it works, it's not a reasonable representation.

If we want to fix this, we should fix both the frontend and the backend together: change the frontend to use true/false, and change the backend to use cJSON_IsTrue(), instead of keeping 0/1 in the frontend and replacing cJSON_GetNumberValue(value) with value->valueint in the backend.

Also, for numbers, cJSON_GetNumberValue() is actually better than value->valueint.

By the way, since these changes are all part of the same fix, it's better to squash them into a single commit instead of adding a new commit for every code change.

keep default_route as number
@gytxxsy

gytxxsy commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@Tarik2142
Thank you for your contribution. This change has been merged into our internal codebase and synchronized to GitHub, so this PR can now be closed.
ca6b923

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.

2 participants