Add optional args in chip-tool and end point optional arg #11642
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost,
kpschoedel,
LuDuda,
lzgrablic02,
msandstedt,
pan-apple,
robszewczyk,
sagar-apple and
saurabhst
pullapprove
bot
requested review from
selissia,
tcarmelveilleux,
tecimovic,
vivien-apple,
wbschiller and
woody-apple
|
PR #11642: Size comparison from 125ad20 to d0564fc Increases above 0.2%:
Increases (1 build for linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Nov 11, 2021
|
Fast tracking given this is a test tool. |
bzbarsky-apple
requested changes
Nov 11, 2021
| @@ -32,6 +32,10 @@ | |||
| #include <lib/support/ScopedBuffer.h> | |||
| #include <lib/support/logging/CHIPLogging.h> | |||
|
|
|||
| #define NO_OPTIONAL_ARGS 0 | |||
| #define OPTIONAL_ARGS_OFFSET 2 | |||
There was a problem hiding this comment.
Please document that this is because each optional arg is two entries in argv: name and value.
| if (optionalArgC == NO_OPTIONAL_ARGS) | ||
| return true; | ||
|
|
||
| // If too many optinoal args passed in, return false. |
There was a problem hiding this comment.
Suggested change
| // If too many optinoal args passed in, return false. | |
| // If too many optional args passed in, return false. |
| // If too many optinoal args passed in, return false. | ||
| if (optArgsCount * OPTIONAL_ARGS_OFFSET < optionalArgC) | ||
| { | ||
| ChipLogError(chipTool, "Too Many arguments passed in."); |
There was a problem hiding this comment.
Suggested change
| ChipLogError(chipTool, "Too Many arguments passed in."); | |
| ChipLogError(chipTool, "Too many arguments passed in."); |
|
|
||
| bool InitArguments(int argc, char ** argv); | ||
| size_t AddArgument(const char * name, const char * value); | ||
| bool InitOptionalArguments(int & argc, char ** argv); |
There was a problem hiding this comment.
Need to document the semantics, especially of the return value and the in/out parameter.
| bool isValidCommand = false; | ||
| int optArgsCount = static_cast<int>(mOptionalArgs.size()); | ||
| int argsCount = static_cast<int>(mArgs.size()); | ||
| int optionalArgC = argc - argsCount; |
There was a problem hiding this comment.
So this can end up negative, right?
| // Subract the total initialized optional args passed in and reset pointer. | ||
| argc -= optInits * OPTIONAL_ARGS_OFFSET; | ||
| argv[argsCount] = nullptr; | ||
| // If the expected optional args are not inittialized, return false. ie wrong names. |
There was a problem hiding this comment.
Suggested change
| // If the expected optional args are not inittialized, return false. ie wrong names. | |
| // If the expected optional args are not initialized, return false. ie wrong names. |
Comment on lines
+111
to
+115
| if (!command->InitOptionalArguments(argc, &argv[3])) | ||
| { | ||
| ShowCommand(argv[0], argv[1], command); | ||
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } |
There was a problem hiding this comment.
So here's a question. Does it make sense to init the required arguments first, then on success ask the command how many non-optional args it ended up with, decrement argc by that, offset by that into argv and call InitOptionalArguments? Then InitOptionalArguments won't need an in/out param and the indexing will be much simpler...
Comment on lines
+30
to
+32
| // Limits on endpoint values. | ||
| #define CHIP_ZCL_ENDPOINT_MIN 0x00 | ||
| #define CHIP_ZCL_ENDPOINT_MAX 0xF0 |
There was a problem hiding this comment.
Why are these here?
| @@ -115,7 +115,13 @@ class {{filename}}: public TestCommand | |||
| {{else if isWait}} | |||
| CHIP_ERROR {{>testCommand}}() | |||
| { | |||
| ChipLogError(chipTool, "[Endpoint: {{endpoint}} Cluster: {{cluster}} {{#if isAttribute}}Attribute: {{attribute}}{{else}}Command: {{wait}}{{/if}}] {{label}}"); | |||
| uint8_t endPoint = {{endpoint}}; | |||
| Argument arg = mOptionalArgs.at((size_t) 0); | |||
There was a problem hiding this comment.
Suggested change
| Argument arg = mOptionalArgs.at((size_t) 0); | |
| Argument arg = mOptionalArgs.at(0); |
| { | ||
| endPoint = mEndPointId; | ||
| } | ||
| ChipLogProgress(chipTool, "Was Provided: %s and Enpoint Value: %d", arg.isProvided ? "true" : "false", endPoint); |
There was a problem hiding this comment.
Suggested change
| ChipLogProgress(chipTool, "Was Provided: %s and Enpoint Value: %d", arg.isProvided ? "true" : "false", endPoint); | |
| ChipLogProgress(chipTool, "Was Provided: %s and Endpoint Value: %d", arg.isProvided ? "true" : "false", endPoint); |
|
@krypton36 it looks like @vivien-apple added something along these lines (but with different semantics? Not sure whether it works right?) in #11729, so at the very least this will need to get rebased on top of that.... :( |
|
I am closing this PR and enhancing Vivian's PR for the optional arguments to avoid conflict nightmares. |
|
Makes sense, but I wish we were talking to each other more....Ah, well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Change overview
In chip-tool add an argument that will allow adding optional arguments. Optional arguments. ZAP file updates to add the optional flag to false.
Testing