Skip to content

Shell: String to numeric conversion utils #44021

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 5, 2022
Merged

Shell: String to numeric conversion utils #44021

merged 2 commits into from
Apr 5, 2022

Conversation

@Andrewpini
Copy link
Contributor

@Andrewpini Andrewpini commented Mar 21, 2022 •
edited
Loading

Adds string to numeric conversion utility for shell.
Applies these functions to the Bluetooth Mesh Shell module.

Signed-off-by: Anders Storrø [email protected]

@github-actions github-actions bot added area: Bluetooth area: Bluetooth Mesh area: Tests Issues related to a particular existing or missing test labels Mar 21, 2022
@Andrewpini Andrewpini requested review from PavelVPV and ludvigsj March 21, 2022 11:16
@Andrewpini
Copy link
Contributor Author

Andrewpini commented Mar 21, 2022

For any questions regarding if this belongs here on basis of its generic nature, ref: #43289

@Andrewpini
Copy link
Contributor Author

Andrewpini commented Mar 21, 2022 •
edited
Loading

@alwa-nordic I know that you argued that this should not be moved back here, but I have spent way more time than intended for this task, and the discussion in the original PR does not seem to give a finite solution any time soon. It is a blocker for other tasks that are important for the Mesh team and it has to move forward now. I have cleared this with @carlescufi.

jhedberg
jhedberg requested changes Mar 21, 2022
View reviewed changes
Copy link
Member

@jhedberg jhedberg left a 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 think it makes sense to introduce such generic functionality in a mesh shell-only context. If necessary (to make progress introducing this as a generic API) we can discuss this in some appropriate Zephyr project meeting (like the API one).

ludvigsj
ludvigsj suggested changes Mar 21, 2022
View reviewed changes
ludvigsj
ludvigsj reviewed Mar 21, 2022
View reviewed changes
ludvigsj
ludvigsj reviewed Mar 21, 2022
View reviewed changes
ludvigsj
ludvigsj reviewed Mar 21, 2022
View reviewed changes
ludvigsj
ludvigsj reviewed Mar 21, 2022
View reviewed changes
@Andrewpini
Copy link
Contributor Author

Andrewpini commented Mar 21, 2022

I really don’t think it makes sense to introduce such generic functionality in a mesh shell-only context. If necessary (to make progress introducing this as a generic API) we can discuss this in some appropriate Zephyr project meeting (like the API one).

Well @jhedberg that would be great. All I know at this point is that this PR is evaluated as to generic to reside in mesh or shell module, while simoutaniously not qualifying as a legit contribution to the general libraries in zephyr, ref: #43289. So I'm not quite sure what to do whit this at this point. Is there anything that you can see which I can do to progress this so that I actually can start utilizing thisfunctionality? I currently do not have the time required to start from scratch on a generic library addition (e.g. sscanf).

@zephyrbot zephyrbot requested a review from alxelax March 21, 2022 14:33
@Andrewpini Andrewpini requested review from jhedberg and ludvigsj March 22, 2022 07:39
ludvigsj
ludvigsj previously approved these changes Mar 22, 2022
View reviewed changes
PavelVPV
PavelVPV reviewed Mar 23, 2022
View reviewed changes
ludvigsj
ludvigsj previously approved these changes Mar 29, 2022
View reviewed changes
ludvigsj
ludvigsj reviewed Mar 31, 2022
View reviewed changes
@jhedberg
Copy link
Member

jhedberg commented Mar 31, 2022

@jakub-uC the helpers presented in this PR are not in any way Mesh specific. Would it be possible to move them as part of the shell subsystem, so that any shell module can use them?

carlescufi
carlescufi requested changes Mar 31, 2022
View reviewed changes
subsys/bluetooth/mesh/shell.c Outdated
Copy link
Member

@carlescufi carlescufi Mar 31, 2022

Choose a reason for hiding this comment

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

Please move these 3 functions to subsys/shell/shell_util.c, that way other shell users can reuse them.

@Andrewpini Andrewpini dismissed stale reviews from ludvigsj and PavelVPV via 478188e March 31, 2022 13:51
@github-actions github-actions bot added area: API Changes to public APIs area: Shell Shell subsystem labels Mar 31, 2022
@Andrewpini Andrewpini changed the title Bluetooth: Mesh: Add checking of shell input args Shell: String to numeric conversion utils Mar 31, 2022
PavelVPV
PavelVPV previously approved these changes Apr 1, 2022
View reviewed changes
@jhedberg
Copy link
Member

jhedberg commented Apr 1, 2022

You need to split the shell subsystem changes into a separate commit. Otherwise looks fine.

@Andrewpini
Shell: String to numeric conversion utils
d9411bd 
Adds string to numeric conversion utility for shell.

Signed-off-by: Anders Storrø <[email protected]>
@Andrewpini
Bluetooth: Mesh: Add err check to shell input
a66f786 
Changes parsing of input string args  to provide error checking.
This is to prevent unintentional command execution on garbage input
strings.

Signed-off-by: Anders Storrø <[email protected]>
@Andrewpini Andrewpini force-pushed the mesh_shell_util_v2 branch from 478188e to a66f786 Compare April 1, 2022 12:29
@Andrewpini Andrewpini requested a review from PavelVPV April 1, 2022 12:53
@carlescufi carlescufi merged commit e4aed1c into zephyrproject-rtos:main Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PavelVPV PavelVPV PavelVPV approved these changes

@jhedberg jhedberg jhedberg approved these changes

@carlescufi carlescufi carlescufi approved these changes

@nordic-krch nordic-krch nordic-krch approved these changes

@jakub-uC jakub-uC jakub-uC approved these changes

@de-nordic de-nordic de-nordic approved these changes

@Vudentz Vudentz Awaiting requested review from Vudentz

@trond-snekvik trond-snekvik Awaiting requested review from trond-snekvik

@LingaoM LingaoM Awaiting requested review from LingaoM

@alxelax alxelax Awaiting requested review from alxelax

@alwa-nordic alwa-nordic Awaiting requested review from alwa-nordic

+1 more reviewer

@ludvigsj ludvigsj ludvigsj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@trond-snekvik trond-snekvik

Labels

area: API Changes to public APIs area: Bluetooth Mesh area: Bluetooth area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Andrewpini @jhedberg @PavelVPV @ludvigsj @carlescufi @nordic-krch @jakub-uC @de-nordic @trond-snekvik