Skip to content

feat: Add native esp-idf template #42

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

Merged
merged 1 commit into from
Mar 31, 2025

Conversation

karlp
Copy link
Contributor

@karlp karlp commented Mar 28, 2025

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

 #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.

Summary by CodeRabbit

  • New Features

    • Introduced support for an ESP-IDF option in the command-line tool, enabling the generation of header files tailored for ESP-IDF projects.
    • Added an ESP-IDF template that offers configurable options for generating C/C++ code.
  • Documentation

    • Updated usage instructions to include an example for utilizing the new ESP-IDF option.

Copy link

coderabbitai bot commented Mar 28, 2025

Walkthrough

The 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

File(s) Change Summary
README.md Added a new command line usage example for generating a header for the native ESP-IDF environment.
src/commandLine.ts Updated the ICopyFilesArguments interface to include 'espidf' in the engine options and added corresponding conditional logic in the command line parsing.
src/cppCode.ts Introduced a new getTemplate function for modularized template selection (including support for 'espidf'), updated the header generation logic, and added an import for the new template.
src/template_espidf.ts Added a new file defining the espidfTemplate constant that produces ESP-IDF specific C/C++ code with conditional compilation based on features such as etag and gzip.

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
Loading

Possibly related PRs

  • feat: support PsychicHttp v2 #29: Updates similar command line option enhancements by adding a new engine type, establishing a direct code-level connection.
  • Implement cache max age #37: Involves modifications to the ICopyFilesArguments interface and related parsing logic, aligning closely with the changes introduced here.
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
README.md (1)

242-243: Update engine description in command options table

The description for the -e option 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 help

The 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 values

The function doesn't check the return values of httpd_register_uri_handler calls, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4b6ef34 and fcb67fb.

📒 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_t return types and handles ETag and gzip options similar to other templates.

@karlp
Copy link
Contributor Author

karlp commented Mar 28, 2025

Thoughts while working on this, that you are probably already aware of:

  • The templates have a lot of redundancy in them!
  • the Svelte name is.... not at all descriptive here :) I'm glad I decided to even look at this project :)
  • the generation output seems to imply that some files will be gzip'ed, others not, but it's really all the files or none of the files?

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>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/template_espidf.ts (1)

145-154: 🛠️ Refactor suggestion

Add 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 to httpd_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_send calls on lines 148, 152, and 154.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcb67fb and d9196fd.

📒 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 getTemplate function 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.

Comment on lines +60 to +77
{{#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}}
Copy link

@coderabbitai coderabbitai bot Mar 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
{{#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}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol :) correct! but no. it's fixed code for esp32, it might as well just use const char to avoid the casts.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@karlp
Copy link
Contributor Author

karlp commented Mar 28, 2025

Changes in v2:

  • typo fix
  • default handling now improved (thanks coderabbit!)
  • native esp32 style char * used to avoid casting, given that the templates don't share the data file defines anyway.

@BCsabaEngine
Copy link
Owner

Hi, welcome your PR! I will follow up your code and rabbit work.

@BCsabaEngine BCsabaEngine merged commit d901141 into BCsabaEngine:main Mar 31, 2025
1 check passed
@coderabbitai coderabbitai bot mentioned this pull request Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants