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.

Sign up for GitHub

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

Jump to bottom

Initial modification of screen and LCD. #324

Closed
wants to merge 1 commit into from
Closed

Initial modification of screen and LCD. #324

wants to merge 1 commit into from

Conversation

espressif2022
Copy link

Try to reuse existing components resources.

@stintel
Copy link
Collaborator

stintel commented Nov 3, 2023 •
edited
Loading

Thanks for your interest in improving Willow. While the idea to use BSP is interesting and would indeed reduce Willow code, this PR is unreviewable and unacceptable in its current form. The first 3 changes in your PR are completely unrelated to the commit title:

  • adding dependencies.lock to .gitignore is proposed in Update .gitignore #184 but we've not decided on this yet, it is unrelated to this PR, and you do not explain why you do it
  • you revert 79c34f9 without any explanation
  • you disable MULTINET support with no explanation

Aside from that, you are not supposed to commit the components to this repository. See f9bbc40 for an example how to properly add a component.

Based on these issues, I'm not going to further review this PR until all of the above is addressed.

@espressif2022
Copy link
Author

espressif2022 commented Nov 6, 2023 •
edited
Loading

Thanks for your interest in improving Willow. While the idea to use BSP is interesting and would indeed reduce Willow code, this PR is unreviewable and unacceptable in its current form. The first 3 changes in your PR are completely unrelated to the commit title:

  • adding dependencies.lock to .gitignore is proposed in Update .gitignore #184 but we've not decided on this yet, it is unrelated to this PR, and you do not explain why you do it
  • you revert 79c34f9 without any explanation
  • you disable MULTINET support with no explanation

Aside from that, you are not supposed to commit the components to this repository. See f9bbc40 for an example how to properly add a component.

Based on these issues, I'm not going to further review this PR until all of the above is addressed.

Hello, glad to get your reply, here is my answer:

 adding dependencies.lock to .gitignore is proposed in Update .gitignore #184 but we've not decided on this yet, 
 it is unrelated to this PR, and you do not explain why you do it

dependencies. lock is an intermediate temporary file for compilation.
If the project wants to use fixed version, I think it's better to modify the idf_component. yml to lock the version.

You reverse 79c34f9 without any explanation
I have communicated with ADF's brother, and you will rely on esp-adf.
If cmake is used in this way, it'll cause idf_component.yml under audio_board cann't be pulled.

You disable MULTINET support with no explanation
This is my mistake, but DWILLOW_ SUPPORT_ MULTINET seems to be preventing me from compiling properly, let me confirm it again.

I am an AE of Espressif. I hope to know your follow-up plans and how we can better cooperate here.
May I have a question here：
What are your BSP's follow-up plans, mainly LCD and touch. Depend on esp-adf's audio-board, or others?

@stintel
Copy link
Collaborator

stintel commented Nov 8, 2023

dependencies. lock is an intermediate temporary file for compilation.
If the project wants to use fixed version, I think it's better to modify the idf_component. yml to lock the version.

Thanks. This is valuable info. Ideally this goes in a separate commit with the above explanation in the commit message.

You reverse 79c34f9 without any explanation
I have communicated with ADF's brother, and you will rely on esp-adf.
If cmake is used in this way, it'll cause idf_component.yml under audio_board cann't be pulled.

Can you further elaborate on that? It's not entirely clean what you mean.

You disable MULTINET support with no explanation
This is my mistake, but DWILLOW_ SUPPORT_ MULTINET seems to be preventing me from compiling properly, let me confirm it again.

Can you show the error? This should not be a problem since dc2f713.

I am an AE of Espressif. I hope to know your follow-up plans and how we can better cooperate here.
May I have a question here：
What are your BSP's follow-up plans, mainly LCD and touch. Depend on esp-adf's audio-board, or others?

We're definitely interested in using BSP for LCD and touch, as using ESP-ADF for this has proven to be not very flexible, and as you noticed requires a lot of extra code.

If there is a way to use es7210 and es8311 from BSP with ADF's audio_pipeline and ESP Audio, we'd also be interested in that.

@espressif2022
Copy link
Author

Okay, thank you very much for your reply.
I've communicated with our ADF colleague and he'll porting the new attributes here and provide you with a new ADF patch.
He'll reply to you later under this PR, I will close this PR later, thank you.

@ESP-Mars
Copy link

Hi @stintel We fully understand your current reliance on ESP-ADF and identified issues with the existing ESP-ADF. We prepare another patch (It has been provided to @kristiankielhofner via email), which can greatly improve screen display. Feel free to test. We plan to officially integrate this patch into the upcoming release of ESP-ADF Master branch this month.

Regarding audio_pipeline and ESP Audio, we also hope to separate these as independent modules from ESP-ADF to better coordinate with ESP-BSP. We are actively working on this internally, which involves collaboration across several departments. While there will be some work involved, any significant progress will be promptly communicated to your team.

stintel
stintel reviewed Nov 17, 2023
View reviewed changes
main/audio.c
@@ -536,7 536,7 @@ static esp_err_t init_ap_to_api(void)
cfg_hs.event_handle = hdl_ev_hs;
cfg_hs.task_stack = 8 * 1024; // default 6 * 1024
cfg_hs.type = AUDIO_STREAM_WRITER;
cfg_hs.user_agent = WILLOW_USER_AGENT;
// cfg_hs.user_agent = WILLOW_USER_AGENT;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This requires espressif/esp-adf#1029.

stintel added a commit that referenced this pull request Nov 22, 2023
@stintel
gitignore: add dependencies.lock
b892412 
As explained by Espressif in #324, dependencies.lock is an intermediate
temporary file used during compilation. To use fixed versions, they
should be locked in idf_component.yml, which we already do.

Closes #184
@stintel stintel mentioned this pull request Nov 22, 2023
Merged
kristiankielhofner pushed a commit that referenced this pull request Nov 22, 2023
@stintel @kristiankielhofner
gitignore: add dependencies.lock
c49200b 
As explained by Espressif in #324, dependencies.lock is an intermediate
temporary file used during compilation. To use fixed versions, they
should be locked in idf_component.yml, which we already do.

Closes #184
stintel added a commit that referenced this pull request Nov 22, 2023
@stintel
gitignore: add dependencies.lock
982b4c9 
As explained by Espressif in #324, dependencies.lock is an intermediate
temporary file used during compilation. To use fixed versions, they
should be locked in idf_component.yml, which we already do.

Closes #184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stintel stintel stintel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@espressif2022 @stintel @ESP-Mars