search

toverainc / willow

Open source, local, and self-hosted Amazon Echo/Google Home competitive Voice Assistant alternative
https://heywillow.io/
Apache License 2.0
2.61k stars 96 forks source link

Initial modification of screen and LCD. #324

Closed espressif2022 closed 1 year ago

espressif2022 commented 1 year ago

Try to reuse existing components resources.

stintel commented 1 year ago

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:

Aside from that, you are not supposed to commit the components to this repository. See f9bbc40232f966d01556fae7ebfe3c30d57fb49c 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 commented 1 year ago

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 commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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.