Skip to content

Add initial Google CTC component#8689

Merged
cujomalainey merged 1 commit intothesofproject:mainfrom
eddy1021:ctc
Aug 6, 2024
Merged

Add initial Google CTC component#8689
cujomalainey merged 1 commit intothesofproject:mainfrom
eddy1021:ctc

Conversation

@eddy1021
Copy link

@eddy1021 eddy1021 commented Jan 3, 2024

Add CTC component

@eddy1021 eddy1021 force-pushed the ctc branch 5 times, most recently from 6cb6a6a to 2cc2bad Compare January 3, 2024 22:10
lgirdwood
lgirdwood reviewed Jan 8, 2024
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@eddy1021 can you update to us the module API instead of component API (now deprecated) ? Module API works with both IPC3 & 4, the changes are mainly in registration/init with some functions using changed names and args. @singalsu can give you more details if needed. Thanks !

@eddy1021 eddy1021 force-pushed the ctc branch 2 times, most recently from a0a249b to 70aa53d Compare January 10, 2024 21:24
@eddy1021 eddy1021 force-pushed the ctc branch 3 times, most recently from 92446b7 to 747468c Compare January 24, 2024 20:52
@eddy1021 eddy1021 force-pushed the ctc branch 4 times, most recently from ec965f5 to e0b55d3 Compare February 6, 2024 23:04
@eddy1021 eddy1021 changed the title [WIP] CTC Add initial Google CTC component Feb 6, 2024
@eddy1021 eddy1021 marked this pull request as ready for review February 6, 2024 23:07
andyross
andyross reviewed Feb 7, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

I guess I can declare myself expert enough to start pontificating a bit. A few spots seem like they might be errors, most are nitpicks.

cujomalainey
cujomalainey reviewed Feb 7, 2024
View reviewed changes
lgirdwood
lgirdwood reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@eddy1021 good to see a mock as part of this PR :)
@singalsu pls review.

singalsu
singalsu reviewed Feb 19, 2024
View reviewed changes
lyakh
lyakh previously requested changes Jul 16, 2024
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

simple to fix, but better fix it before merge

@lyakh
Copy link
Collaborator

lyakh commented Jul 18, 2024

@eddy1021 BTW, this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

kv2019i
kv2019i approved these changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I think this is starting to be ready. Having a version in the tree could help with maintainance and get changes like the recent UUID registry and cleanup of the xtos/zephyr makefiles, "for free". These need to be addressed, but I'm ok to do these as follow-ups.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 19, 2024

this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I think you mean "open-source CI". I think (and hope) there are many closed CIs we don't all know about.

@lyakh
Copy link
Collaborator

lyakh commented Jul 19, 2024

this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I think you mean "open-source CI". I think (and hope) there are many closed CIs we don't all know about.

@marc-hb I mean the SOF GitHub PR CI - the one we all see results of when submitting our PRs.

@eddy1021
Copy link
Author

eddy1021 commented Jul 24, 2024

@eddy1021 BTW, this PR adds a mock, but all Kconfig options are off by default, so this will never be tested by the CI, not even for compilation, right?

I turned the Mock to default on now. Thanks for pointing this out!

@lgirdwood
Copy link
Member

lgirdwood commented Jul 26, 2024

@eddy1021 CI has some scheduled downtime, I'll manually rerun CI on Monday.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 26, 2024 •
edited
Loading

@eddy1021 CI has some scheduled downtime, I'll manually rerun CI on Monday.

There was a downtime but that is not the main reason Jenkins did not test this. The main reason is because I had to add the magic keywords [SKIP SOF-TEST] in the past because of rapid-fire force pushes that were saturating our scarce hardware devices. It seems more quiet now; dropping it.

EDIT: Friday's downtime was actually cancelled for complex reasons.

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 26, 2024

SOFCI TEST

1 similar comment
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 29, 2024

SOFCI TEST

andyross
andyross approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Sneak in and refresh my vote

cujomalainey
cujomalainey suggested changes Jul 30, 2024
View reviewed changes
@lgirdwood
Copy link
Member

lgirdwood commented Aug 5, 2024

@cujomalainey good for you now ?

cujomalainey
cujomalainey suggested changes Aug 5, 2024
View reviewed changes
comp: Add initial Google CTC component
0f72a19 
Introduce a new component to perform crosstalk-cancellation on
the playback path.

Signed-off-by: Eddy Hsu <eddyhsu@google.com>
@cujomalainey
Copy link
Contributor

cujomalainey commented Aug 5, 2024

I'll push submit tomorrow unless anyone else fines any issues.

@lgirdwood
Copy link
Member

lgirdwood commented Aug 6, 2024

I'll push submit tomorrow unless anyone else fines any issues.

I've rerun failing GH actions (now passed) and CI is showing expected results (this module not tested in CI to date). Pls merge when ready.

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

Reviewers

@marc-hb marc-hb marc-hb left review comments

@andyross andyross andyross approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@kv2019i kv2019i kv2019i approved these changes

@lyakh lyakh lyakh left review comments

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@johnylin76 johnylin76 Awaiting requested review from johnylin76

@singalsu singalsu Awaiting requested review from singalsu

+1 more reviewer

@cujomalainey cujomalainey cujomalainey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.11

Development

Successfully merging this pull request may close these issues.

9 participants

@eddy1021 @marc-hb @singalsu @lgirdwood @cujomalainey @lyakh @andyross @johnylin76 @kv2019i