-
Notifications
You must be signed in to change notification settings - Fork 8.3k
boards: seeed: Add support for XIAO ESP32S3 Sense #79334
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
boards: seeed: Add support for XIAO ESP32S3 Sense #79334
Conversation
31239d3 to
abf5142
Compare
|
Kindly asking - is anyone going to have a look at this? |
|
Great work. I was looking forward for this feature as well. However I see that you have used the generic zephyr, camera driver while there is a specific one for Espressif boards espressif,esp32-lcd-cam |
I'm not sure what do you mean by generic zephyr,camera, could you elaborate on this? As you can see in boards/seeed/xiao_esp32s3/seeed_xiao_connector.dtsi xia_sense_lcd_cam points to lcd_cam which is defined here: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/xtensa/espressif/esp32s3/esp32s3_common.dtsi which as you can see ueses espressif,esp32-lcd-cam that was recently added to zephyr. |
Thank you for working on this! I have this board, and would love to see support for it in Zephyr. I think this should just be a variant of the existing XIAO ESP32S3 board, placed in the same folder. This too allows reusing the base devicetree files etc. between the two variants without the "overhead" of adding a board-specific shield. |
Hi, thanks, will explore this and rework the solution. // EDIT |
abf5142 to
0effa60
Compare
d92d3e6 to
3936759
Compare
josuah
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.
FYI: There need to be at least 2 reviews before Kartben being able to merge it.
You could ask extra reviews on the #pr-help Discord channel.
In this case, the _sense variant only adds features, so mechanisms like this might still work:
If both the common
plank_defconfigfile and one or more board qualifiers specificplank_<qualifiers>_defconfigfiles exist, then all matching files will be used. This allows you to place configuration which is common for all board SoCs, CPU clusters, and board variants in the baseplank_defconfigand only place the adjustments specific for a given SoC or board variant in theplank_<qualifiers>_defconfig. -- doc
I am a bit confused, but having a "no-variant variant" is actually how it is done in Zephyr, like the Raspberry Pi Pico (rpi_pico_defconfig) vs Rasberry Pi Pico W (rpi_pico_rp2040_w_defconfig).
Still, to help users discovering that if they order the XIAO ESP32S3, they would get a missing connector, some documentation changes could help.
Many thanks for porting this very compact computer vision capable board!
uLipe
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.
Besides on @josuah comments, LGTM so far.
50f6e58 to
3d9c579
Compare
henrikbrixandersen
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.
One minor nit, rest looks good!
3d9c579 to
c5f3356
Compare
The Seeed Studio XIAO ESP32S3 Sense board is a board based on the XIAO ESP32S3 board with a soldered B2B connector that allows to connect an extension board with a camera sensor, microphone and the sdcard slot. Signed-off-by: Patryk Biel <[email protected]>
cfd8821
c5f3356 to
cfd8821
Compare
| cam-clk = <10000000>; | ||
| pinctrl-0 = <&lcd_cam_default>; | ||
| pinctrl-names = "default"; | ||
| source = <&ov2640>; |
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.
The source device can be retrieved from the remote-endpoint-label (you can see aea820d) so no need to do direct reference via phandle here. This is just a recommendation, perhaps for next time.
| dma-names = "rx"; | ||
| port { | ||
| dvp_ep_in: endpoint { | ||
| remote-endpoint-label = "ov2640_ep_out"; |
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.
We should also use the video-interfaces binding in the lcd-cam yaml. If not, declaring remote-endpoint-label or remote-endpoint or anything else will not have any effect (although it still compiles).
I am not sure which is the binding yaml file of the lcd_cam. Is it espressif,esp32-lcd-cam.yaml ? If so, by using the video-interfaces binding, you can also avoid declaring a number of vendor customized DVP properties for example, data-width, hsync, vsync, etc.
This can be addressed next time.
The Seeed Studio XIAO ESP32S3 Sense board is a board based on the XIAO ESP32S3 board with a soldered B2B connector that allows to connect extension board with a camera sensor, microphone and the sdcard slot.
Tested by executing samples/drivers/video/capture with some modifications needed to save an image on the sdcard.
Btw. sorry for the typo in the source branch name, but there is no way to change the source branch once the PR has been created.