-
Notifications
You must be signed in to change notification settings - Fork 39
Fix/add frame checks for uvc #340
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
base: master
Are you sure you want to change the base?
Conversation
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 PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
host/class/uvc/usb_host_uvc/examples/basic_uvc_stream/main/basic_uvc_stream.c
Show resolved
Hide resolved
host/class/uvc/usb_host_uvc/examples/basic_uvc_stream/main/basic_uvc_stream.c
Show resolved
Hide resolved
29d392e to
747a7ea
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.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
host/class/uvc/usb_host_uvc/examples/basic_uvc_stream/main/basic_uvc_stream.c
Show resolved
Hide resolved
tore-espressif
left a comment
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.
@TDA-2030 thank you very much for you contribution!
I'm only worried about moving usb_types_uvc.h to private include dirs....
@tore-espressif I moved this file to the private folder because a test project used both usb_stream and usb_host_uvc, but some of their structure definitions were duplicated, causing compilation errors. Further investigation revealed that this file is not publicly accessible, so I felt moving it to the private folder was more appropriate. |
|
@TDA-2030 I can see that only failing check is the Linux host test. I'll have a look |
I tried running |
747a7ea to
20cd10a
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 20
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| uvc_stream->single_thread.current_frame_id = payload_header->bmHeaderInfo.frame_id; | ||
| uvc_stream->single_thread.skip_current_frame = payload_header->bmHeaderInfo.error; // Check for error flag | ||
| // Validate header before accessing it | ||
| if (!uvc_frame_payload_header_validate(payload_header, transfer->actual_num_bytes)) { |
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.
Bug: Header accessed before validation in bulk SOF handler
In the UVC_STREAM_BULK_PACKET_SOF case, the code accesses payload_header->bmHeaderInfo.frame_id and payload_header->bmHeaderInfo.error on lines 88-89 before the header is validated by uvc_frame_payload_header_validate on line 91. If transfer->actual_num_bytes is 0 or 1, this reads memory beyond the valid payload data since uvc_payload_header_t is 2 bytes. The validation call and the field accesses need to be reordered so validation happens first.
|
here is a readme how to rung the host tests. AFAIK, it is working only on linux/mac os, not on windows. I tried running the tests locally on you branch, and the tests are failing, because you added some more checks to the uvc driver. Now the host tests expects different output (aka, it expects to pass in some scenarios, but your checks are making some tests to "fail" because of set expectations). The tests will have to be modified. |
|
@ This test case fails
This is part of the error log This means: For bulk streams, if the frame contains error in EoF header, the driver must not call frame callback. But after you changes in https://github.com/espressif/esp-usb/blob/20cd10a5b3b73cae375940aefb0fc342a6d9816d/host/class/uvc/usb_host_uvc/uvc_bulk.c it does call the frame callback even for frames with error flag. This is a real regression that the test caught. Could you please fix it? Running the host_tests
|
|
I ran the test according to your command, but I received the following error: └─[1] <git:(fix/add_frame_checks_for_uvc 20cd10a) > idf.py --preview set-target linux build
Adding "set-target"'s dependency "fullclean" to list of commands with default set of options.
Executing action: fullclean
Build directory '/home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build' not found. Nothing to clean.
Executing action: set-target
Set Target to: linux, new sdkconfig will be created.
Running cmake in directory /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build
Executing "cmake -G Ninja -DPYTHON_DEPS_CHECKED=1 -DPYTHON=/home/bot/esp/.espressif/python_env/idf5.5_py3.10_env/bin/python -DESP_PLATFORM=1 -DIDF_TARGET=linux -DCCACHE_ENABLE=0 /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test"...
-- Found Git: /usr/bin/git (found version "2.34.1")
-- Minimal build - OFF
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test ver_gnu17_supported
-- Performing Test ver_gnu17_supported - Success
-- Performing Test ver_gnu++2b_supported
-- Performing Test ver_gnu++2b_supported - Success
-- Building ESP-IDF components for target linux
NOTICE: Dependencies lock doesn't exist, solving dependencies.
NOTICE: Using component placed at /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc for dependency "usb_host_uvc", specified in /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/main/idf_component.yml
..NOTICE: Skipping optional dependency: usb
..NOTICE: Updating lock file at /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/dependencies.lock
NOTICE: Processing 3 dependencies:
NOTICE: [1/3] espressif/catch2 (3.7.0)
NOTICE: [2/3] usb_host_uvc (2.4.1) (/home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc)
NOTICE: [3/3] idf (5.5.1)
-- building full USB HOST MOCKS
NOTICE: Skipping optional dependency: usb
-- Project sdkconfig file /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/sdkconfig
Loading defaults file /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/sdkconfig.defaults...
-- Performing Test HAVE_FLAG__ffile_prefix_map__home_bot_esp_esp_usb_host_class_uvc_usb_host_uvc_host_test_managed_components_espressif__catch2_Catch2__
-- Performing Test HAVE_FLAG__ffile_prefix_map__home_bot_esp_esp_usb_host_class_uvc_usb_host_uvc_host_test_managed_components_espressif__catch2_Catch2__ - Success
-- building full USB HOST MOCKS
-- Component idf::main will be linked with -Wl,--whole-archive
-- Components: cmock esp_common esp_hw_support esp_rom esp_system espressif__catch2 freertos hal heap linux log main soc spi_flash unity usb usb_host_uvc
-- Component paths: /home/bot/esp/esp-idf-v5.5/components/cmock /home/bot/esp/esp-idf-v5.5/components/esp_common /home/bot/esp/esp-idf-v5.5/components/esp_hw_support /home/bot/esp/esp-idf-v5.5/components/esp_rom /home/bot/esp/esp-idf-v5.5/components/esp_system /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/managed_components/espressif__catch2 /home/bot/esp/esp-idf-v5.5/components/freertos /home/bot/esp/esp-idf-v5.5/components/hal /home/bot/esp/esp-idf-v5.5/components/heap /home/bot/esp/esp-idf-v5.5/components/linux /home/bot/esp/esp-idf-v5.5/components/log /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/main /home/bot/esp/esp-idf-v5.5/components/soc /home/bot/esp/esp-idf-v5.5/components/spi_flash /home/bot/esp/esp-idf-v5.5/components/unity /home/bot/esp/esp-usb/host/usb/test/mocks/usb_host_full_mock/usb /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc
-- Configuring done
-- Generating done
-- Build files have been written to: /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build
Executing action: all (aliases: build)
Running ninja in directory /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build
Executing "ninja all"...
[166/215] Try to find ruby. If this fails, you need to install ruby
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux-gnu]
[175/215] Generating mocks/Mockusb_host.c, mocks/Mockusb_phy.c, mocks/Mockusb_host.h, mocks/Mockusb_phy.h
FAILED: esp-idf/usb/mocks/Mockusb_host.c esp-idf/usb/mocks/Mockusb_phy.c esp-idf/usb/mocks/Mockusb_host.h esp-idf/usb/mocks/Mockusb_phy.h /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build/esp-idf/usb/mocks/Mockusb_host.c /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build/esp-idf/usb/mocks/Mockusb_phy.c /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build/esp-idf/usb/mocks/Mockusb_host.h /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build/esp-idf/usb/mocks/Mockusb_phy.h
cd /home/bot/esp/esp-usb/host/class/uvc/usb_host_uvc/host_test/build/esp-idf/usb && /usr/bin/cmake -E env UNITY_DIR=/home/bot/esp/esp-idf-v5.5/components/unity/unity ruby /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb -o/home/bot/esp/esp-usb/host/usb/test/mocks/usb_host_full_mock/usb/mock/mock_config.yaml /home/bot/esp/esp-usb/host/usb/include/usb/usb_host.h /home/bot/esp/esp-idf-v5.5/components/esp_hw_support/include/esp_private/usb_phy.h
/home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:46:in `read': No such file or directory @ rb_sysopen - /home/bot/esp/esp-idf-v5.5/components/esp_hw_support/include/esp_private/usb_phy.h (Errno::ENOENT)
from /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:46:in `generate_mock'
from /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:30:in `block in setup_mocks'
from /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:29:in `each'
from /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:29:in `setup_mocks'
from /home/bot/esp/esp-idf-v5.5/components/cmock/CMock/lib/cmock.rb:102:in `<main>'
Creating mock for usb_host...
Creating mock for usb_phy...
[180/215] Building CXX object esp-idf/espressif__catch2/Catch2/src/CMakeFiles/Catch2.dir/catch2/matchers/catch_matchers_string.cpp.o
ninja: build stopped: subcommand failed.Is it because the IDF version is incorrect? |
|
Yes, it works only with latest master branch of esp-idf |
Description
basic_uvc_streamexample to automatically open the first available formatRelated
Testing
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Add MJPEG SOI and UVC payload header validation, improve bulk/isoc EOF handling, auto-select first available format in example, move UVC types header to private, and bump to 2.4.1.
uvc_frame_payload_header_validate()and use it in bulk (uvc_bulk.c) and isoc (uvc_isoc.c) paths.uvc_desc_get_streaming_intf_and_ep()to continue scanning alternates when a candidate isn’t the best match.examples/basic_uvc_stream):device_count.esp32p4/esp32s2/esp32s3.usb_types_uvc.htoprivate_includeand update includes.2.4.1; extendidf_component.yml(targets, file excludes).host_testto new header path.Written by Cursor Bugbot for commit 20cd10a. This will update automatically on new commits. Configure here.