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

Add essl to component manager #9

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Conversation

ginkgm
Copy link
Collaborator

@ginkgm ginkgm commented Feb 9, 2022

Moving this component from IDF to extra components, since it's upper layer component.

Tracking ESP-IDF commit: 4f1a9e436e

@ginkgm
Copy link
Collaborator Author

ginkgm commented Feb 9, 2022

@mahavirj @tore-espressif PTAL

Copy link
Member

@mahavirj mahavirj left a comment

Choose a reason for hiding this comment

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

@ginkgm Please add entry for this component in .github/workflows/upload_component.yml

esp_serial_slave_link/README.md Show resolved Hide resolved
esp_serial_slave_link/essl.c Outdated Show resolved Hide resolved
esp_serial_slave_link/idf_component.yml Outdated Show resolved Hide resolved
esp_serial_slave_link/idf_component.yml Outdated Show resolved Hide resolved
esp_serial_slave_link/README.md Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

Would it make sense to move tests here too?
e.g. https://github.com/espressif/esp-idf/blob/master/components/driver/test/test_spi_slave_hd.c
https://github.com/espressif/esp-idf/blob/master/components/driver/test/test_sdio.c

We are working on running unit tests in GitHub action, but it is not ready yet. As of now, we only build the tests

EDIT: See #13 . This PR runs test_app on our self-hosted runners

esp_serial_slave_link/idf_component.yml Outdated Show resolved Hide resolved
@tore-espressif
Copy link
Collaborator

tore-espressif commented Feb 17, 2022

Please rebase on top of #12 , it checks formating in GH action.

And let me know whether you are ready to merge

@tore-espressif
Copy link
Collaborator

@ginkgm

Would it make sense to move tests here too?

FYI, #13 was merged and we run tests in GH action now.

@ginkgm
Copy link
Collaborator Author

ginkgm commented Sep 19, 2022

I think the target-test is too complicated to be in this repo. So I will only add a simple build test, to make sure the component can be build fine.

esp_serial_slave_link/essl_spi.c Fixed Show fixed Hide fixed
esp_serial_slave_link/essl_spi.c Fixed Show fixed Hide fixed
@igrr
Copy link
Member

igrr commented Sep 19, 2022

@ginkgm could you please check the two static analysis warnings reported above?

@ginkgm ginkgm force-pushed the feat/essl_init branch 2 times, most recently from 63ba00a to 62a2c53 Compare September 19, 2022 19:09
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

@ginkgm Thank you for following this up.

I'm haven't worked with essl before, so I can't review properly.
Just make sure that the code compiles with esp-idf master

@ginkgm ginkgm force-pushed the feat/essl_init branch 3 times, most recently from 9b7ad51 to f270023 Compare September 21, 2022 12:43
@ginkgm
Copy link
Collaborator Author

ginkgm commented Sep 21, 2022

@tore-espressif @kumekay @mahavirj Please merge this MR if it's OK for you. There is basic build test for the source files.

For the compatibilities, I tried with ESP-IDF master examples with slight changes. Those changes will be added to IDF master after this is merged.

@mahavirj mahavirj merged commit b901876 into espressif:master Sep 21, 2022
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.

7 participants