Adds lib/support/tests to the ESP32 unit-test target. Makes platform-specific fixes to ParseArgs and EncodeTlvElement. #36847
Conversation
This file contains hidden or 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
…dress test failures.
|
Review changes with Changed Files
|
github-actions
bot
added
esp32
lib
vscode
github
workflows
support
core
test driver
labels
|
PR #36847: Size comparison from cfdaf79 to c320ab5 Full report (42 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36847: Size comparison from cfdaf79 to 37bd427 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36847: Size comparison from cfdaf79 to da758f2 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36847: Size comparison from cfdaf79 to ebaafcf Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
feasel0
changed the title
|
@feasel0 , can you please post a list of the test logs before and after your changes. If they are many in number, can you please post one log for each type: for e.g.: 1.ParseArgs tests and 2. EnocdeTLV tests in PR description. |
dhrishi
reviewed
Jan 15, 2025
|
Added logs to the description:
|
Co-authored-by: shripad621git <[email protected]>
|
PR #36847: Size comparison from 1739c88 to fc8eb8f Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
dhrishi
approved these changes
Mar 29, 2025
|
Approving as per the review from @shripad621git |
andy31415
reviewed
Mar 31, 2025
src/lib/support/CHIPArgParser.cpp
Outdated
| @@ -332,48 +394,115 @@ bool ParseArgs(const char * progName, int argc, char * const argv[], OptionSet * | |||
| goto done; | |||
| } | |||
|
|
|||
| #ifdef CONFIG_NON_POSIX_GETOPT_LONG | |||
There was a problem hiding this comment.
use #if instead of #ifdef to force defines to exist as need.
This does mean that th define will have to be set somewhere else than just a specific file.
andy31415
reviewed
Mar 31, 2025
| @@ -315,7 +315,13 @@ CHIP_ERROR EncodeTlvElement(const Json::Value & val, TLV::TLVWriter & writer, co | |||
|
|
|||
| Platform::ScopedMemoryBuffer<uint8_t> byteString; | |||
| byteString.Alloc(BASE64_MAX_DECODED_LEN(static_cast<uint16_t>(encodedLen))); | |||
| VerifyOrReturnError(byteString.Get() != nullptr, CHIP_ERROR_NO_MEMORY); | |||
|
|
|||
| // On a platform where malloc(0) is null (which could be misinterpreted as "out of memory") | |||
There was a problem hiding this comment.
Would it make sense to have the check for encodedLen on all platforms?
Is there extra benefit to check for non-null even if encoded length is 0 ? If encoded length is 0, does it make sense to short-circuit and set decoded length 0 immediately?
There was a problem hiding this comment.
I updated this to break out of the case and skip setting decodedLen and calling writer.PutBytes if it's a zero length string. For now I only have this "check and break" performed if it's a platform like esp32 (where we have to do that because malloc(0)==null).
case TLV::kTLVType_ByteString: {
VerifyOrReturnError(val.isString(), CHIP_ERROR_INVALID_ARGUMENT);
const std::string valAsString = val.asString();
size_t encodedLen = valAsString.length();
VerifyOrReturnError(CanCastTo<uint16_t>(encodedLen), CHIP_ERROR_INVALID_ARGUMENT);
//BEGIN NEW CODE
#if CONFIG_MALLOC_0_IS_NULL
// Skip PutBytes for 0-length strings.
if (encodedLen == 0)
break;
#endif
//END NEW CODE
// Check if the length is a multiple of 4 as strict padding is required.
VerifyOrReturnError(encodedLen % 4 == 0, CHIP_ERROR_INVALID_ARGUMENT);
Platform::ScopedMemoryBuffer<uint8_t> byteString;
byteString.Alloc(BASE64_MAX_DECODED_LEN(static_cast<uint16_t>(encodedLen)));
VerifyOrReturnError(byteString.Get() != nullptr, CHIP_ERROR_NO_MEMORY);
auto decodedLen = Base64Decode(valAsString.c_str(), static_cast<uint16_t>(encodedLen), byteString.Get());
VerifyOrReturnError(decodedLen < UINT16_MAX, CHIP_ERROR_INVALID_ARGUMENT);
ReturnErrorOnFailure(writer.PutBytes(tag, byteString.Get(), decodedLen));
break;
}
However we have the option of performing the "check and break" on all platforms if we just get rid of the #if directive and drop the CONFIG_MALLOC_0_IS_NULL macro entirely. This makes the code slightly larger on all platforms (an extra "if" statement and an extra "break" statement), but it saves some execution time every time we need to encode an empty string (an extra 6 lines of code). Is this worth it?
There was a problem hiding this comment.
@andy31415
If the way I have it now looks good, then I have no more changes to add to this PR.
andy31415
reviewed
Mar 31, 2025
src/test_driver/esp32/clean.sh
Outdated
| # | ||
|
|
||
| # | ||
| # This script cleans the files generated by building the unit tests. |
There was a problem hiding this comment.
This seems to apply only if using build_examples (since that determines the output directory in /out.
We should add this info to the comment as this seems to be designed to be specific to build_examples usage with exactly esp32-qemu-tests target.
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.
Fixes errors that were causing
libSupportTests(lib/support/tests) unit tests to fail.Changes
src/lib/support/testsinto theesp32-qemu-teststarget.ArgParser::ParseArgs()to work with platforms with a non-POSIXgetopt_long(like ESP32). This was causing failures in some of the test cases inTestCHIPArgParser.cpp.EncodeTlvElement()to work with platforms wheremalloc(0)returns null (like ESP32). This was causing a failure when building zero-length strings, since the null response was being interpreted as "out of memory"..vscode/tasks.jsonto include Build and Run tasks for the ESP32 unit tests.clean.shscript for removing the files generated by the ESP32 unit test build.How is ESP32's
getopt_longdifferent from the POSIX version?As a result, ParseArgs needs to do a little more work when interpreting the results of
getopt_long. This code is only executed ifCONFIG_NON_POSIX_GETOPT_LONGis defined, which happens automatically insrc/lib/support/BUILD.gnwhenchip_device_platformis "esp32".Testing
Automated testing. Used the
ESP32_QEMUCI check, which now includeslibSupportTests. Also manually built theesp32-qemu-teststarget and ran all images.