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

feat(validation): Add helper macros for common validatiors #65

Open
wants to merge 2 commits into
base: feature/validation
Choose a base branch
from

Conversation

TjazVracko
Copy link
Collaborator

Description

Adds helper macros for common validators.
Adds a new sample that demonstrates the use of these helper macros.

(Also updates the readme in a separate commit)

Related #47

Areas of interest for the reviewer

All

Checklist

  • My code follows the style guidelines as defined by IRNAS.
  • I have performed a self-review of my code.
  • My changes generate no new warnings.
  • I added/updated source code documentation for all newly added or changed functions.
  • I updated all customer-facing technical documentation.

After-review steps

  • I will merge PR by myself.

@github-actions github-actions bot added the pull request Pull request, added automatically by CI. label Jan 13, 2025
Copy link
Collaborator Author

TjazVracko commented Jan 13, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@TjazVracko TjazVracko force-pushed the feature/validation-helpers branch from 14169ec to ab13285 Compare January 13, 2025 08:50
@TjazVracko TjazVracko force-pushed the feature/validation-helpers branch from ab13285 to 92a83bf Compare January 13, 2025 09:02
@TjazVracko TjazVracko force-pushed the feature/validation-helpers branch 2 times, most recently from 19c9585 to 2c19248 Compare January 13, 2025 10:01
@TjazVracko TjazVracko force-pushed the feature/validation-helpers branch from 2c19248 to b163eb3 Compare January 13, 2025 10:13
Copy link

Coverage after merging feature/validation-helpers into feature/validation will be

24.21%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
library/protocol/binary
   user_settings_protocol_binary.c83.33%72.97%100%87.18%113, 116–118, 118, 118–119, 121–124, 135–136, 173–174, 66–67, 71–72, 80
library/user_settings
   user_settings.c46.44%28.19%72%60.82%100, 107, 107, 110, 114, 131–132, 137–138, 138, 138, 138, 138–141, 141, 141, 141, 141–142, 145, 145, 168, 176–177, 177, 177, 177, 177–178, 183–184, 184, 184, 184, 184–185, 190–191, 191, 191, 191, 191–192, 197–198, 198, 198, 198, 198–199, 209–211, 218–219, 227, 233–234, 234, 234, 234, 234–235, 240–241, 241, 241, 241, 241–242, 247–248, 248, 248, 248, 248–249, 259, 259, 259, 265, 265, 265, 265, 271, 271, 271, 271, 278–279, 279, 279, 279, 279–280, 287, 287, 287, 287, 299–300, 300, 300, 300, 300–301, 306–307, 307, 307, 307, 307–308, 314, 316, 316, 316, 318–319, 319, 319, 321, 326, 326, 326, 329, 329, 329, 344, 350, 350, 360–361, 361, 361, 361, 361–362, 368–369, 369, 369, 369, 369–370, 378, 384, 384, 384, 384, 390, 390, 398, 398, 398, 398, 408–409, 409, 409, 409, 409–410, 415–416, 416, 416, 416, 416–417, 422–423, 423, 423, 423, 423–424, 433–434, 441, 443, 443, 443, 449, 451, 451, 451–452, 454, 456, 458, 458, 458, 460–461, 461, 461, 463–464, 469, 469, 469, 47, 472, 472, 472, 48, 480, 488, 488, 488, 496, 499, 506, 509, 516, 52, 523, 523, 523–524, 524, 524–525, 527, 53, 530, 533, 535, 535, 535, 537–538, 538, 538, 540, 545, 548, 555–556, 562, 565, 567, 567, 567, 569–570, 570, 570, 572, 577, 577, 577, 58, 580, 580, 580, 587, 59, 59, 59, 59, 59, 592, 594, 594, 594, 596–597, 597, 597, 599, 60, 600, 604, 607, 61, 612, 614, 614, 614, 616–617, 617, 617, 619, 62, 62, 62, 62, 62, 620, 623, 623, 623, 626, 626, 626, 632, 634, 634, 634, 636–637, 637, 637, 639, 64, 643, 645, 645, 645, 647–648, 648, 648, 650, 653, 655, 655, 655, 657–658, 658, 658, 660, 663, 665, 665, 665, 667–668, 668, 668, 670, 675, 675, 675, 678, 678, 678, 685, 685, 685, 688, 688, 688, 693, 695, 695, 695, 697–698, 698, 698, 700, 705, 705, 705, 708, 708, 708, 715, 718, 725, 725, 725, 728, 728, 728, 769, 771, 771, 771, 773–774, 774, 774, 776–777, 779, 781, 781, 781, 783–784, 784, 784, 786–787, 791, 791, 791, 794, 794, 794, 801, 801, 801, 804, 804, 804, 811, 83–84, 88–89, 94–95, 95, 95, 95, 95–98, 98, 98, 98, 98
   user_settings_json.c45.99%34.75%100%52.44%100–101, 103, 106, 114, 127–128, 128, 128, 128, 128–129, 133–134, 143–144, 160, 160, 160, 160, 160, 160, 160, 160, 162, 165, 169–171, 173–175, 181–183, 185–187, 189–191, 193–195, 197–199, 216–217, 217, 217, 217, 217, 221–222, 222, 222,

Copy link
Collaborator

@MarkoSagadin MarkoSagadin left a comment

Choose a reason for hiding this comment

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

I have some minor requests :)

Comment on lines +2 to +15

cmake_minimum_required(VERSION 3.20)

# create compile_commands.json for clang
set(CMAKE_EXPORT_COMPILE_COMMANDS on)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})

project(user-settings-sample-validation)

zephyr_compile_options(-fdiagnostics-color=always)

zephyr_include_directories(src)
target_sources(app PRIVATE src/main.c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clean up the cmakelists file:

Suggested change
cmake_minimum_required(VERSION 3.20)
# create compile_commands.json for clang
set(CMAKE_EXPORT_COMPILE_COMMANDS on)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(user-settings-sample-validation)
zephyr_compile_options(-fdiagnostics-color=always)
zephyr_include_directories(src)
target_sources(app PRIVATE src/main.c)
cmake_minimum_required(VERSION 3.20)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(user-settings-sample-validation)
zephyr_include_directories(src)
target_sources(app PRIVATE src/main.c)

Please check if other files need similar cleanup.

Comment on lines +27 to +29
# disable the settings shell to not confuse the user with
# 2 similar top level commands
CONFIG_SETTINGS_SHELL=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be n, based on the comment?

@@ -0,0 +1,6 @@
# Validation

Sample to demonstrate the validation helpers of the user_settings lib.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please briefly describe how the sample does this. I see that the shell is enabled, so this file should atleast say that is expected that the user should check the source code to see what is the expected range (or expected exact size) and then try changing that setting over the shell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull request Pull request, added automatically by CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants