Add remaining UCI app config parameters to nocli uwb range start #196
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
… update GetValueMap()
…onfig, and StaticStsInitializationVector
|
Output: |
|
|
💯 I've never liked it. |
I'm also hoping we can make some of the primary parameters positional such that switches don't always need to be provided. This is mostly for commands that only take 1 or 2 arguments. |
abeltrano
approved these changes
Apr 1, 2023
| @@ -483,6 +545,40 @@ NearObjectCli::AddSubcommandUwbRangeStart(CLI::App* parent) | |||
| m_cliData->appConfigParamsData.destinationMacAddresses.value().insert(uwb::UwbMacAddress::FromString(m_cliData->destinationMacAddressString, macAddressType).value()); | |||
| } | |||
|
|
|||
| // Parse and validate ResultReportConfig | |||
| constexpr int resultReportConfigurationSize = 4; // TODO: Find a way to calculate this number | |||
There was a problem hiding this comment.
Suggested change
| constexpr int resultReportConfigurationSize = 4; // TODO: Find a way to calculate this number | |
| constexpr int resultReportConfigurationSize = magic_enum::enum_count<ResultReportConfig>(); |
| if (IsValidResultReportConfigurationString(m_cliData->resultReportConfigurationString)) { | ||
| m_cliData->appConfigParamsData.resultReportConfig.emplace(); | ||
|
|
||
| const std::bitset<4> resultReportConfigurationBits(m_cliData->resultReportConfigurationString); |
There was a problem hiding this comment.
Should this be resultReportConfigurationSize instead?
Suggested change
| const std::bitset<4> resultReportConfigurationBits(m_cliData->resultReportConfigurationString); | |
| const std::bitset<resultReportConfigurationSize > resultReportConfigurationBits(m_cliData->resultReportConfigurationString); |
| m_cliData->appConfigParamsData.resultReportConfig.emplace(); | ||
|
|
||
| const std::bitset<4> resultReportConfigurationBits(m_cliData->resultReportConfigurationString); | ||
| if (resultReportConfigurationBits[0]) { |
There was a problem hiding this comment.
FYI, std::bitset also has a test() function is does the same thing as the subscript operator here. I like that test() more directly indicates what's being done, but I'm mostly bringing this up for informational purposes. Feel free to close.
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.
Type
Goals
This PR adds the remaining UCI app config params to nocli. This includes CLI options, some additional validation (not all), and additional code to print out parameters.
Technical Details
Test Results
nocli output looks correct
Reviewer Focus
Apologies for the long PR, but there are so many UCI app config parameters!
Also, in this line:
oss << "0x" << std::setw(2) << std::internal << std::setfill('0') << std::hex << +value << " ";, I had to add"0x"instead ofstd::showbasebecause it interferes withstd::setw(2)andstd::setfill('0'), since the added base is considered part of the width, thus truncating the added 0.Future Work
Additional validation for the newly added parameters.
I also really don't like how CLI11 adds "UINT" or "ENUM" when displaying the options. Perhaps this can be changed.
Checklist
allcompiles cleanly.