feat: Add native esp-idf template #42
Conversation
WalkthroughThe changes extend support for the ESP-IDF framework in the svelteesp32 tool. A new command line option is added, allowing users to generate a C/C++ header using an ESP-IDF–specific template. The updates include modifications to the command line argument parsing, interface type definitions for engine options, modularizing template selection in the code generation process, and the addition of a new template file dedicated to ESP-IDF. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CommandLine.ts
participant CG as CppCode.ts
participant TP as Template Provider (template_espidf.ts)
U->>CLI: Execute command with '-e espidf'
CLI->>CLI: Parse arguments and set engine = 'espidf'
CLI->>CG: Invoke getCppCode with engine 'espidf'
CG->>CG: Call getTemplate(engine)
CG->>TP: Request the espidfTemplate
TP-->>CG: Return espidfTemplate
CG->>CLI: Generate C++ header using the ESP-IDF template
CLI-->>U: Output generated header file
Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
README.md (1)
242-243: Update engine description in command options tableThe description for the
-eoption in the options table still only mentions "psychic/psychic2/async" but should be updated to include "espidf" to match the new functionality.- | `-e` | The engine for which the include file is created (psychic/psychic2/async) | psychic | + | `-e` | The engine for which the include file is created (psychic/psychic2/async/espidf) | psychic |src/commandLine.ts (1)
30-30: Update the engine description in command line helpThe description string for the engine option still only mentions "psychic|psychic2|async" and should be updated to include "espidf" to match the new functionality.
- description: 'The engine for which the include file is created (psychic|psychic2|async)', + description: 'The engine for which the include file is created (psychic|psychic2|async|espidf)',src/template_espidf.ts (1)
171-175: Consider checking httpd_register_uri_handler return valuesThe function doesn't check the return values of
httpd_register_uri_handlercalls, which could fail if there are issues with the URI registration.Consider adding error handling for the registration process.
static inline void {{methodName}}(httpd_handle_t server) { {{#each sources}} - httpd_register_uri_handler(server, &route_{{this.datanameUpperCase}}); + esp_err_t err = httpd_register_uri_handler(server, &route_{{this.datanameUpperCase}}); + if (err != ESP_OK) { + // Handle error or at least log it + } {{/each}} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/commandLine.ts(2 hunks)src/cppCode.ts(2 hunks)src/template_espidf.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/cppCode.ts (2)
src/template_espidf.ts (1)
espidfTemplate(1-176)src/commandLine.ts (1)
cmdLine(19-95)
🔇 Additional comments (6)
README.md (1)
51-52: The new ESP-IDF option is well documented.The addition of the ESP-IDF command line example is clear and follows the same format as the existing examples for PsychicHttpServer and ESPAsyncWebServer.
src/commandLine.ts (2)
6-6: Interface update for the new engine option.The engine type in the ICopyFilesArguments interface has been correctly extended to include 'espidf'.
26-26: Correct implementation of espidf engine value.The parser function correctly handles the 'espidf' value, maintaining the same pattern as the existing engine options.
src/cppCode.ts (2)
5-5: Import for the new ESP-IDF template.The import statement for the new ESP-IDF template has been correctly added.
🧰 Tools
🪛 ESLint
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
599-599: Improved modularity with getTemplate function.Using the new getTemplate function improves modularity and makes it easier to add more engine types in the future.
src/template_espidf.ts (1)
1-176: Well-structured ESP-IDF template.The ESP-IDF template follows the same structure as the existing templates, making it consistent with the rest of the codebase. It includes proper error handling with
esp_err_treturn types and handles ETag and gzip options similar to other templates.
|
Thoughts while working on this, that you are probably already aware of:
|
Tested with generating a Vue3 app for esp32, using esp-idf v5.3.2
This is perfectly functional, but there's always room for improvement.
Published in the hope it may be useful for others.
Note: no examples for esp-idf. Not sure where best to put them.
mini example
```
#include <esp_http_server.h>
#include "your_generated_file.h"
void start_http_server(void)
{
httpd_handle_t httpd;
httpd_config_t config = HTTPD_DEFAULT_CONFIG();
config.max_uri_handlers = SVELTEESP32_COUNT + xxx;
LOGI("Starting server on port: '%d'", config.server_port);
ESP_ERROR_CHECK(httpd_start(&httpd, &config));
initSvelteStaticFiles(httpd);
// any other handlers you need....
}
```
Tested with and without etag, and with and without gzip.
Signed-off-by: Karl Palsson <karl.palsson@marel.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/template_espidf.ts (1)
145-154: 🛠️ Refactor suggestionAdd explicit type casting for httpd_resp_send calls.
Since you're defining the data as arrays for compatibility with other templates, you should add explicit casting to
(const char*)when passing the data tohttpd_resp_send, as mentioned in your feedback.- httpd_resp_send(req, datagzip_{{this.dataname}}, {{this.lengthGzip}}); + httpd_resp_send(req, (const char*)datagzip_{{this.dataname}}, {{this.lengthGzip}});And similar changes for the other
httpd_resp_sendcalls on lines 148, 152, and 154.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/commandLine.ts(2 hunks)src/cppCode.ts(2 hunks)src/template_espidf.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/commandLine.ts
- README.md
🧰 Additional context used
🧠 Learnings (1)
src/template_espidf.ts (1)
Learnt from: karlp
PR: BCsabaEngine/svelteesp32#42
File: src/template_espidf.ts:145-154
Timestamp: 2025-03-28T14:16:04.232Z
Learning: For ESP-IDF templates in svelteesp32, data is defined as `uint8_t` arrays for compatibility with other templates, and should be cast to `(const char*)` when passing to functions like `httpd_resp_send` that expect const char* parameters, to maintain type safety.
🔇 Additional comments (6)
src/cppCode.ts (3)
5-5: 👍 Added import for the new ESP-IDF template.The new import supports the addition of the ESP-IDF engine option, extending the tool's functionality to work with the ESP-IDF framework.
🧰 Tools
🪛 ESLint
[error] 1-5: Run autofix to sort these imports!
(simple-import-sort/imports)
583-594: Excellent modularization of template selection logic.Extracting the template selection into a dedicated function improves code organization and makes it easier to add new engine types in the future.
599-599: Good implementation of the template selection.The function now uses the extracted
getTemplatefunction with the engine type from command line arguments, making the code more maintainable.src/template_espidf.ts (3)
1-188: Great addition of ESP-IDF template.The template includes all necessary ESP-IDF specific code for serving static files, complete with proper headers, function signatures, and error handling.
161-173: 👍 Great implementation of root path handling.You've successfully addressed the "root index catchall" limitation mentioned in the PR description by adding a dedicated handler for the root path when a file is marked as default.
179-186: 👍 Properly registering both default and specific routes.The template correctly registers both the default (root) and specific routes with the ESP-IDF HTTP server, ensuring all paths are properly handled.
| {{#each sources}} | ||
| const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | ||
| {{/each}} | ||
| {{/case}} | ||
| {{#case "false"}} | ||
| {{#each sources}} | ||
| const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | ||
| {{/each}} | ||
| {{/case}} | ||
| {{#case "compiler"}} | ||
| #ifdef {{definePrefix}}_ENABLE_GZIP | ||
| {{#each sources}} | ||
| const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | ||
| {{/each}} | ||
| #else | ||
| {{#each sources}} | ||
| const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | ||
| {{/each}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Data type inconsistency with other templates.
The data arrays are defined as const char in this template while other templates use uint8_t. According to the learning from your feedback, you intended to keep the data as uint8_t arrays for compatibility.
-const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} };
+const uint8_t datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} };And similar changes for the other data definitions on lines 66, 72, and 76.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{#each sources}} | |
| const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| {{/each}} | |
| {{/case}} | |
| {{#case "false"}} | |
| {{#each sources}} | |
| const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| {{/each}} | |
| {{/case}} | |
| {{#case "compiler"}} | |
| #ifdef {{definePrefix}}_ENABLE_GZIP | |
| {{#each sources}} | |
| const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| {{/each}} | |
| #else | |
| {{#each sources}} | |
| const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| {{/each}} | |
| {{#each sources}} | |
| -const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| +const uint8_t datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| {{/each}} | |
| {{/case}} | |
| {{#case "false"}} | |
| {{#each sources}} | |
| -const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| +const uint8_t data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| {{/each}} | |
| {{/case}} | |
| {{#case "compiler"}} | |
| #ifdef {{definePrefix}}_ENABLE_GZIP | |
| {{#each sources}} | |
| -const char datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| +const uint8_t datagzip_{{this.dataname}}[{{this.lengthGzip}}] = { {{this.bytesGzip}} }; | |
| {{/each}} | |
| #else | |
| {{#each sources}} | |
| -const char data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| +const uint8_t data_{{this.dataname}}[{{this.length}}] = { {{this.bytes}} }; | |
| {{/each}} |
There was a problem hiding this comment.
lol :) correct! but no. it's fixed code for esp32, it might as well just use const char to avoid the casts.
There was a problem hiding this comment.
@karlp Understood! Makes perfect sense to use const char for the ESP-IDF template to avoid unnecessary type casting in the ESP32 code. Thanks for the clarification.
✏️ Learnings added
Learnt from: karlp
PR: BCsabaEngine/svelteesp32#42
File: src/template_espidf.ts:60-77
Timestamp: 2025-03-28T14:37:30.463Z
Learning: In the ESP-IDF template for svelteesp32, `const char` should be used for data arrays instead of `uint8_t` (which is used in other templates) because it avoids type casting when working with ESP32 HTTP server APIs, resulting in cleaner and more efficient code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Changes in v2:
|
|
Hi, welcome your PR! I will follow up your code and rabbit work. |
Tested with generating a Vue3 app for esp32, using esp-idf v5.3.2 This is perfectly functional, but there's always room for improvement. Published in the hope it may be useful for others.
Note: no examples for esp-idf. Not sure where best to put them.
Note: esp-idf doesn't handle the root index catchall the same way as psychic or espasync, so you probably won't have a great time without some extra handlers, or handler aliases. Not sure if that is in or out of scope.
mini example
Tested with and without etag, and with and without gzip.
Summary by CodeRabbit
New Features
Documentation