Skip to content
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

feat: Add board support for ThingPulse ePulse Feather #9256

Merged
merged 10 commits into from
Feb 21, 2024
Merged

feat: Add board support for ThingPulse ePulse Feather #9256

merged 10 commits into from
Feb 21, 2024

Conversation

matthias-bs
Copy link
Contributor

@matthias-bs matthias-bs commented Feb 16, 2024

By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes

Checklist

  1. Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
  2. Please provide related links (eg. Issue which will be closed by this Pull Request)
  3. Please update relevant Documentation if applicable
  4. Please check Contributing guide
  5. Please confirm option to "Allow edits and access to secrets by maintainers" when opening a Pull Request

This entire section above can be deleted if all items are checked.


Description of Change

Add support for ThingPulse ePulse Feather. The configuration is based on Adafruit ESP32 Feather, which already worked for my basic needs. The only pain point was a different (unusual) voltage divider ratio for vbatt measurement. (see matthias-bs/BresserWeatherSensorTTN#55)

Changed the pin configuration according to the schematic and the Flash / PSRAM configuration according to the ePulse Feather datasheet and the ESP32-WROVER-E-N8R8 datasheet.

Tests scenarios

Trying to compile an example sketch with this board selection gives

/home/mp/.arduino15/packages/esp32/hardware/esp32/2.0.14/tools/sdk/esp32/include/newlib/platform_include/assert.h:20:10: fatal error: sdkconfig.h: No such file or directory
 #include <sdkconfig.h>
          ^~~~~~~~~~~~~
compilation terminated.

Did I miss anything? A build step?

Related links

--

Copy link
Contributor

github-actions bot commented Feb 16, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Added missing flash_type, removed unavailable partition schemes":
    • summary looks empty
    • type/action looks empty
  • the commit message "Set some unspecified pins to -1":
    • summary looks empty
    • type/action looks empty
  • the commit message "Update boards.txt":
    • summary looks empty
    • type/action looks empty
  • the commit message "epulse_feather: Modified PSRAM definitions":
    • type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test]

Please fix these commit messages - here are some basic tips:

  • follow Conventional Commits style
  • correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue
  • allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test
  • sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter
  • avoid Jira references in commit messages (unavailable/irrelevant for our customers)

TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

⚠️ Please consider squashing your 10 commits (simplifying branch history).

👋 Hello matthias-bs, we appreciate your contribution to this project!


Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 35d7687

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Feb 16, 2024

@me-no-dev any clues about the CI error? The board itself is defined correctly, did not found any issue there.

@Jason2866
Copy link
Collaborator

Jason2866 commented Feb 16, 2024

Looks like the variant path (name) is wrong thingpulse_epulse_feather the variant is epulse_feather
variants/thingpulse_epulse_feather/pins_arduino.h -> variants/epulse_feather/pins_arduino.h

@P-R-O-C-H-Y
Copy link
Member

Looks like the variant path (name) is wrong thingpulse_epulse_feather the variant is epulse_feather

The variant looks fine. It doesn't have to be the name of board itself.
epulse_feather.build.variant=thingpulse_epulse_feather

@Jason2866
Copy link
Collaborator

Jason2866 commented Feb 16, 2024

@P-R-O-C-H-Y Tried? Do not see an example for what you said. Maybe this part is not working as it should?

@P-R-O-C-H-Y
Copy link
Member

@P-R-O-C-H-Y Tried? Do not see an example for what you said.

@Jason2866 I know that some 3rd party boards uses variants not same as the name. Few examples:

esp32doit-espduino.build.board=ESP32_DEV
vintlabs-devkit-v1.build.board=ESP32_DEV
intorobot-fig.build.board=INTOROBOT_ESP32_DEV

I am sure that it will print different error that it was not able of find the pins_arduino.h file

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Feb 16, 2024

I may run a CI to test all boards to see if its this board specific or more boards are affected by some new issue now.

@matthias-bs
Copy link
Contributor Author

I can easily rename it if this helps.

@Jason2866
Copy link
Collaborator

This ones will work for sure, since there is the variant and path ESP32_DEV

esp32doit-espduino.build.board=ESP32_DEV
vintlabs-devkit-v1.build.board=ESP32_DEV

@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Feb 16, 2024

I have run the CI and no other board have this issue.
https://github.com/P-R-O-C-H-Y/arduino-esp32/actions/runs/7930133353
Some of them failed, but for other reasons. So the variant name should not be a problem I assume.

@P-R-O-C-H-Y
Copy link
Member

@matthias-bs May I ask you which board did you use for a "template" to add this ThingPulse board? From what I see as it uses ESP32 SOC, you are defining stuff that is used for other chips.

@matthias-bs
Copy link
Contributor Author

@P-R-O-C-H-Y I used Adafruit ESP32 Feather as a template. It's my first attempt at creating such a board description, so some things might be wrong... Which parts are not correct? Is there a kind of manual/recipe?

@P-R-O-C-H-Y
Copy link
Member

@P-R-O-C-H-Y I used Adafruit ESP32 Feather as a template. It's my first attempt at creating such a board description, so some things might be wrong... Which parts are not correct? Is there a kind of manual/recipe?

There is no specific manual or recipe on how to do it. If you can please compare your board addition with the Adafruit ESP32 Feather and check to not add more stuff, that is not there for the Adafruit board, like:

epulse_feather.build.psram_type=qspi
epulse_feather.build.memory_type={build.flash_type}_{build.psram_type}

As your board have PSRAM, I suggest you to use as the template the Adafruit Feather ESP32 V2 :)

@P-R-O-C-H-Y
Copy link
Member

@matthias-bs If you will have any issues or won't be sure, I can just review that and point what is wrong ;)

@matthias-bs
Copy link
Contributor Author

matthias-bs commented Feb 16, 2024

@P-R-O-C-H-Y O.k., then I would copy

adafruit_feather_esp32_v2.menu.PSRAM.enabled=Enabled
adafruit_feather_esp32_v2.menu.PSRAM.enabled.build.defines=-DBOARD_HAS_PSRAM -mfix-esp32-psram-cache-issue -mfix-esp32-psram-cache-strategy=memw
adafruit_feather_esp32_v2.menu.PSRAM.disabled=Disabled
adafruit_feather_esp32_v2.menu.PSRAM.disabled.build.defines=

Does -mfix-esp32-psram-cache-issue -mfix-esp32-psram-cache-strategy=memw apply? What is the background?

@matthias-bs
Copy link
Contributor Author

matthias-bs commented Feb 16, 2024

Found it: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/external-ram.html#chip-revisions
According to the "ESP32­-WROVER-­E & ESP32­-WROVER-­IE Datasheet", this should no longer be needed, right?

@P-R-O-C-H-Y
Copy link
Member

Found it: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/external-ram.html#chip-revisions According to the "ESP32­-WROVER-­E & ESP32­-WROVER-­IE Datasheet", this should no longer be needed, right?

According to the Thinkpulse website, it should use the V3 (rev.03), so it shouldn't be necessary.

Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

Just small fix needed, else looks good :)

boards.txt Show resolved Hide resolved
@P-R-O-C-H-Y P-R-O-C-H-Y added the Status: Pending Merge Pull Request is ready to be merged label Feb 20, 2024
@me-no-dev me-no-dev merged commit 05e2abc into espressif:master Feb 21, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Pending Merge Pull Request is ready to be merged Type: 3rd party Boards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants