[Test] Creating TC_CADMIN_1_3 and TC_CADMIN_1_4 python test modules #35818
+428
−923
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
- Current WIP script creation following test steps in PR: CHIP-Specifications/chip-test-plans#4689
|
Review changes with Changed Files
|
github-actions
bot
added
tests
matter-1.4-te2-script-change
|
PR #35818: Size comparison from 1b4fa25 to 5a48f12 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
arkq
reviewed
Sep 27, 2024
- Updated TC_CADMIN_1_3 to latest CI format
|
PR #35818: Size comparison from 1b4fa25 to 28ba101 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35818: Size comparison from 941c1d2 to b2d9a67 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Resolving linting errors
|
PR #35818: Size comparison from 941c1d2 to 75e368c Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35818: Size comparison from 9ae53c5 to 45c711e Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Appended WIP CADMIN_1_4 test script to TC_CADMIN_1_3 test module - Renamed TC_CADMIN_1_3 to TC_CADMIN_1_3_4 - Added issue in a TODO comment since current workaround in place to make CADMIN_1_3 pass
- Updated test step 14 for TC_CADMIN_1_3 to contain correct information
- Minor change to attempt resolving restylizer issue
|
PR #35818: Size comparison from af3727b to 6d59815 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Updating to resolve some linting issues
|
PR #35818: Size comparison from af3727b to 7a74ef3 Full report (20 builds for cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
arkq
reviewed
Oct 9, 2024
Updating to reflect change in master for "factory-reset", thank you @arkq Co-authored-by: Arkadiusz Bokowy <[email protected]>
|
PR #35818: Size comparison from af3727b to dabe8b7 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Now using get_setup_payload_info() to gather discriminator and passcode for BCM commissioning in TC_CADMIN_1_4
cecille
reviewed
Dec 11, 2024
| current_fabric_index = await self.read_single_attribute_check_success(dev_ctrl=th, endpoint=0, cluster=cluster, attribute=attribute) | ||
| return current_fabric_index | ||
|
|
||
| def pics_TC_CADMIN_1_3(self) -> list[str]: |
There was a problem hiding this comment.
Looking at these two tests, from step 1 to step 7, the steps are basically identical with the exception of the command sent (open commissioning window vs. open basic commissioning window. Can definitely re-use some of this between tests.
- Removed unneeded get_commissioning_rcac_data_async() - Removed commented out python bindings - Removed fail safes as no longer needed with new method of gathering RCAC in place
- Created combined_commission_val_steps function in order to combine test steps 1-7 to be used for both CADMIN_1_3 and CADMIN_1_4 tests.
cecille
reviewed
Jan 30, 2025
| *rcacSize = rcacData.size(); | ||
|
|
||
| // Copy the data from C++ to Python's allocated memory | ||
| std::memcpy(rcacDataPtr, rcacData.data(), *rcacSize); |
There was a problem hiding this comment.
Does the python caller pass in the size of the allocated buffer? ie, how do you know that the rcacDataPtr buffer is large enough for the data you're coping into it?
There was a problem hiding this comment.
So I recall when gathering the RCAC data initially I had printed out the size of the data returned here to make sure that the allocated pointer was large enough to house the data stored and then made the rcacDataPtr buffer a similar size in the ChipDeviceCtrl python3 module which is set in the following line:
rcac_buffer = (ctypes.c_uint8 * rcac_size)()
I ran several iterations to make sure the size didn't change for the data in C++.
I also had tinkered around with it a bit at the time to make sure it was returning the expected data we needed returned.
There was a problem hiding this comment.
I know it's not a guarantee, but if you have the caller pass in the buffer size then you can check before you memcpy. It's not foolproof, but at least you can say you tried.
The API here is public, so you're not guaranteed to be the only user. If the memcpy is too large, you'll segfault.
There was a problem hiding this comment.
Ah ok, understood, sorry I did not realize the API was public.
I have updated to passing in the buffer size to the C++ function now, I have an allocation check now to make sure the rcac_size does not overflow the buffer we created in python3, then updated the bindings and that appears to be working at this time for returning the RCAC data.
There was a problem hiding this comment.
can you log an error and return a zero sized buffer if the value sent in is too small instead? This will give back a partial certificate, which is not useful.
There was a problem hiding this comment.
Sorry, good point, I have updated the code here to include a check, in the event that the buffer is smaller than the RCAC data size it should now return a zero sized buffer if the check fails.
| fabric_info = await self.read_single_attribute_check_success(dev_ctrl=th, fabric_filtered=True, cluster=OC_cluster, attribute=OC_cluster.Attributes.Fabrics) | ||
| return fabric_info | ||
|
|
||
| async def get_txt_record(self): |
There was a problem hiding this comment.
would you mind just getting Raul to take a peek here and make sure this API is correct? It looks correct to me, but he wrote it so I want to get a second set of eyes on it.
There was a problem hiding this comment.
I can respectfully request Raul when he has some time to take a look at this section of the code to make sure it looks right for the mdns_discovery module integration
|
PR #35818: Size comparison from e0792a4 to e4a7688 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Update to OpCredsBinding C++ module was to add a missing bracket - Update ChipDeviceCtrl python module was to change to using 400 instead of 650 for buffer size to store RCAC data
|
PR #35818: Size comparison from e0792a4 to 4b5c071 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Including change to pass in set buffer size for allocation check before passing RCAC data back to python3 from C++ module
|
PR #35818: Size comparison from e0792a4 to 02fc183 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Updated to include reference to key sizes in spec 6.1.5
cecille
reviewed
Feb 4, 2025
| rcac_data = bytearray(rcac_buffer[:actual_rcac_size.value]) | ||
| rcac_bytes = bytes(rcac_data) | ||
| else: | ||
| raise Exception("RCAC data is empty") |
There was a problem hiding this comment.
Q - why raise an exception then catch it and return None? Why not just log and return here?
There was a problem hiding this comment.
Sorry, I think I was only attempting to return a value due to needing something to be returned to the test script for validation. Not thinking about doing it without the try and except block, the code has been modified quite a bit from the original form, I believe the try and except block had included more code before.
However, I do apologize you are correct I should updated this section of code to catch and return here without adding in the try and except block being utilized here now that we have been able to make this code much cleaner.
| *rcacSize = rcacData.size(); | ||
|
|
||
| // Copy the data from C++ to Python's allocated memory | ||
| std::memcpy(rcacDataPtr, rcacData.data(), *rcacSize); |
There was a problem hiding this comment.
can you log an error and return a zero sized buffer if the value sent in is too small instead? This will give back a partial certificate, which is not useful.
- Removed try and except block from get_rcac() - Changed method to make sure that we aren't getting returned a partial cert
- Updating to include better logger message when RCAC returns 0 sized data - Updating to remove prior commented out no longer needed code block
- Moved comment into correct indent area
|
PR #35818: Size comparison from 93114c7 to 2af3bdb Full report (3 builds for cc32xx, stm32)
|
- Resolving linting errors for log message if RCAC data size greater than allocated buffer size
- Updating Logging message for allocated buffer being too small to house RCAC data size, this should hopefully not happen.
|
PR #35818: Size comparison from 93114c7 to acf18da Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
approved these changes
Feb 11, 2025
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.
Testing