-
Notifications
You must be signed in to change notification settings - Fork 20
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
refactor(config_test): Added the test_app for various configurations (part 2/2) #111
base: master
Are you sure you want to change the base?
Conversation
0726ee3
to
db62192
Compare
c6d9052
to
73ea243
Compare
de1a18b
to
3345b90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving some comments
device/esp_tinyusb/test_apps/configuration_desc/main/test_config_desc.c
Outdated
Show resolved
Hide resolved
device/esp_tinyusb/test_apps/configuration_desc/main/test_config_desc.c
Outdated
Show resolved
Hide resolved
1bba7a2
to
2ed1169
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx for the update
* - HS configuration descriptor is not provided by user (legacy compatibility) and default HS config descriptor is using when possible. | ||
* - Qualifier descriptor. | ||
*/ | ||
TEST_CASE("descriptors_config_device_and_config", "[esp_tinyusb][usb_device][config]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on S3 and Win10 host. Could you please double check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really strange.
Can you describe exactly please, how it fails?
__test_tinyusb_set_config(&tusb_cfg); | ||
} | ||
|
||
#if (TUD_OPT_HIGH_SPEED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future: We will run this test on P4 for both FS and HS configurations, so we might have to update this #ifdefs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I will need to update this test during the run-time configuration.
Maybe after run-time will be available, we can provide an error for unsupported configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, maybe it is not that important to add these tests right now.
@tore-espressif, WDYT about moving them to the release, when we will add run-time configuration?
Does it make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
2ed1169
to
1bd6261
Compare
Description
Split the test application for cdc and disconnection verification.
Part2/2: Added the test_app for tinyusb various configurations
Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following: