-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add initial chip structure #21
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
👋 Hello Dzarda7, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
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.
@Dzarda7 Very nice work, I like it!
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.
Pull Request Overview
This PR introduces a data-driven SoC structure to replace compile-time #ifdefs for handling differences between ESP chip variants. The new architecture uses a unified SoC information structure containing capabilities and peripheral definitions for each chip, aiming to provide better maintainability compared to the existing esp-stub-lib approach.
- Implements a unified SoC abstraction layer with chip-specific peripheral configurations
- Provides common functions for watchdog and UART operations that work across all chips
- Replaces conditional compilation with runtime NULL checks for unsupported features
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
soc/include/soc.h | Main SoC structure definitions and accessor functions |
soc/include/peripherals/*.h | Peripheral structure definitions for UART, USB, RTC, and system peripherals |
soc/targets/*.c | Chip-specific SoC configuration files for all ESP variants |
soc/common/*.c | Common peripheral functions using the unified SoC structure |
soc/CMakeLists.txt | Build configuration for SoC library |
CMakeLists.txt | Integration of SoC library into main build |
Comments suppressed due to low confidence (1)
soc/targets/esp32s3.c:1
- The esp32s3_capabilities structure is missing the
config_efuse_spi
field initialization that appears in other chip configurations. This inconsistency could lead to undefined behavior.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@dobairoland Do you still plan to improve esp-stub-lib with the common features such as flash support? After this structure it seems to me you changed your direction. Am I wrong? |
@erhankur we do. There are just things that are different across chips and have no use for others to be in esp-stub-lib. I have to say, I probably missed some communication about what should be in esp-stub-lib, so I considered only flashing and ROM functions (and UART? But not sure what plan is with that). So I might have created more dedinitions than necessary. Sorry for confusion. |
@erhankur I'm sorry for the misunderstanding. We are not deviating from the original plan. |
@Dzarda7 @dobairoland Thanks. Good to know. Some notes:
|
From an architectural standpoint, I think it would be better to avoid exposing internal registers and hardware details to the common stub. Instead, we could define a set of function pointers for essential operations, like init_uart(), read_byte, etc. This approach abstracts the target-specific implementation. The initialization style and register layouts are likely to change across different platforms, and encapsulating them within the target-specific file would make the common code more robust and easier to maintain. |
Yes @jedi7, ideal plan is to expose these structures only to the functions in common, but we will see how it goes. |
4d3de47
to
6ad3c90
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.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@dobairoland @jedi7 @radimkarnis PTAL again. I removed functions that will be used in esp-stub-lib, also helper macros will be taken from esp-stub-lib. As @fhrbata pointed out it might be better use structures instead of typedef for better readability. In this case I am okay with both variants. |
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 thanks!
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. But please check with @erhankur before merging so we won't go against the principles of esp-stub-lib.
I will modify this PR to contain bare minimum structure and keep it open as a draft (or just in a branch) as we might find use for it, but for now it is not necessary and all chip specific code will be in esp-stub-lib. Merge conflict should not be an issue as this is separate from the rest of the code. |
Description
This PR creates initial chip structure. It tries to utilize a bit more data driven structure than esp-stub-lib as I believe there will not be big differences when using each peripheral, so if it is supported, common function can be used. Aim is to have as much common function as possible, but if it cannot be done, there can be per chip function in the structure.
I am open to suggestions, this was my first idea how this can be done compared to esp-stub-lib, I believe it is even a bit more comparable and better when it comes to avoiding #ifdefs, even though there has to be NULL checks for unsupported features.
Checklist
Before submitting a Pull Request, please ensure the following: