Re: [PR] Feature/esp dedicated gpio [nuttx]
nuttxpr commented on PR #16223: URL: https://github.com/apache/nuttx/pull/16223#issuecomment-2808600905 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description mostly meets the NuttX requirements, but needs a few improvements: **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the changes. * **Detailed Impact:** The impact section covers most of the required areas, although some could be more detailed. * **Testing Information:** Good inclusion of build instructions, sample app code, and expected output. The performance comparison is a nice addition. **Weaknesses:** * **Missing Issue References:** The summary should link to any related NuttX or NuttX Apps issues. Even if there isn't a directly related issue, consider creating one to track the feature request or bug that prompted this PR. * **Incomplete Documentation Impact:** You state there is a documentation impact but don't describe *what* the update is. Specify which files were added/modified and the nature of the changes. * **Insufficient "Testing Logs Before Change":** You provide *expected* output after the change, but don't show what the behavior was *before* the change. This makes it difficult to assess the impact of the PR. Ideally, provide logs showing the slower performance of the existing GPIO implementation. * **Target Details in Testing:** The testing section should be more specific about the target hardware used. E.g., "esp32c6-devkitc on ESP-C6-DevKitC-1 board". * **Build Host Information Missing:** You've documented the *targets*, but not the *host* used for the build. Include OS, CPU architecture, and compiler version. * **Overuse of Bolding:** The excessive bolding in the tables makes them harder to read. Reserve bolding for key information or headings. **Suggested Improvements:** 1. **Add Issue References:** Create and link to a NuttX issue describing the need for dedicated GPIO support. 2. **Detail Documentation Changes:** Explain what documentation was added (e.g., "Added a new section to the ESP32 driver documentation describing the dedicated GPIO interface and usage."). 3. **Provide "Before" Logs:** Show the output of the sample app (or a similar test) *before* the dedicated GPIO changes were implemented. This demonstrates the performance improvement. 4. **Specify Build Host Details:** Add the build host information (OS, CPU, compiler) to the Testing section. For example: "Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0" 5. **Refine Target Descriptions:** Be more precise about the target boards used. For example: "Target: esp32c6-devkitc on ESP-C6-DevKitC-1 board". 6. **Reduce Bolding in Tables:** Remove the excessive bolding in the result tables. Consider using just a header row for bolding. Simple Markdown tables are fine. **Example of Improved Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0 * Target(s): * esp32c6-devkitc on ESP-C6-DevKitC-1 board * esp32s3-devkit on ESP32-S3-DevKitC-1 board * esp32s2-saola-1 on ESP32-S2-Saola-1 board `esp32c6-devkitc:gpio`, `esp32s3-devkit:gpio` and `esp32s2-saola-1:gpio` config used with `CONFIG_ESPRESSIF_DEDICATED_GPIO` option enabled. Testing logs before change (using standard GPIO): ``` ``` Testing logs after change (using dedicated GPIO): ``` By addressing these points, your PR will be much stronger and easier for reviewers to assess. Remember, a well-written PR significantly increases its chances of being merged quickly and smoothly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] Feature/esp dedicated gpio [nuttx]
eren-terzioglu opened a new pull request, #16223:
URL: https://github.com/apache/nuttx/pull/16223
## Summary
Add dedicated gpio support to control pins with faster response time and
control multiple pins at the same time to applications requires faster response
time (.e.g SPI bitbang)
* arch/xtensa: Update common layer version for esp32s3
* Documentation/xtensa: Add dedicated GPIO docs for esp32[-s2|-s3]
* Documentation/risc-v: Add dedicated GPIO documentation for
esp32[-c3|-c6|-h2]
* boards/xtensa: Add dedicated GPIO board level support for esp32[-s2|-s3]
* arch/xtensa: Add dedicated GPIO support for esp32[-s2|-s3]
* boards/risc-v: Add dedicated GPIO board level support for
esp32[-c3|-c6|-h2]
* arch/risc-v: Add Dedicated GPIO support for esp32[c3|c6|h2]
* drivers/gpio: Add bundle ioctl commands
## Impact
Impact on user: No, new feature added
Impact on build: No, old defconfigs can be used without any issue
Impact on hardware: Yes, dedicated GPIO feature added
Impact on documentation:
Impact on security: No
Impact on compatibility: No, it is compatible with old defconfigs
## Testing
`esp32c6-devkitc:gpio`, `esp32s3-devkit:gpio` and `esp32s2-saola-1:gpio`
config used with `CONFIG_ESPRESSIF_DEDICATED_GPIO` option enabled.
### Building
Commands used for building:
esp32c6
```
make distclean && ./tools/configure.sh esp32c6-devkitc:gpio && kconfig-tweak
-d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO && make
olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0
ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins
```
esp32s2
```
make distclean && ./tools/configure.sh esp32s2-saola-1:gpio && kconfig-tweak
-d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO && make
olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0
ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins
```
esp32s3
```
make distclean && ./tools/configure.sh esp32s3-devkit:gpio && kconfig-tweak
-d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO && make
olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0
ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins
```
Sample app I used:
```
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#define ESP_DEDIC_GPIO_PATH "/dev/gpio3"
#define COUNT_LIMIT 5
int main(int argc, char *argv[])
{
int fd;
int count = 0;
int rd_val = 0;
struct gpio_bundle_wr_arg_s wr_arg =
{
0
};
fd = open(ESP_DEDIC_GPIO_PATH, O_RDWR);
wr_arg.mask = 0x;
wr_arg.value = 1;
while (count < COUNT_LIMIT)
{
ioctl(fd, GPIOC_BUNDLE_WR, &wr_arg);
ioctl(fd, GPIOC_BUNDLE_RD, &rd_val);
printf("Read value: %d\n", rd_val);
wr_arg.value = !wr_arg.value;
count++;
usleep(100);
}
close(fd);
return OK;
}
```
### Results
Output should look like this:
```
esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
esp_dedic_gpio_bundle_write: Writing 0 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 0
esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
esp_dedic_gpio_bundle_write: Writing 0 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 0
esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
nsh>
```
I tested blink test without any delay and these are the results
C6 results with using POSIX calls
| Freq\Number of GPIO's | 1 | 2 |
||---|---|
| Dedicated GPIO Blink Freq(khz) | 285 - 290 | 285 |
| Normal GPIO Blink Freq(khz)| 240 | 120 - 123 |
S3 results with using POSIX calls
| Freq\Number of GPIO's | 1 | 2 |
||---|---|
| Dedicated GPIO Blink Freq(khz) | 475 | 475 |
| Normal GPIO Blink Freq(khz)| 421 | 210 |
S3 results with using arch calls (esp32s3_gpiowrite, ...)
| Freq\Number of GPIO's | 1 | 2 |
||---|---|
| Dedicated GPIO Blink Freq(khz) | 5300 | 5300 |
| Normal GPIO Blink Freq(khz)| 4000 | 2000 |
--
This is an automated message from the Apache Git Service.
To respond
