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

anc: Fix enable/disable from slave bud #74

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

Haxk20
Copy link
Contributor

@Haxk20 Haxk20 commented Nov 5, 2023

Switching ANC on or off from master bud has always worked since we could just straight up enable it and no command had to be sent.

When switching from slave bud we had to send the command to enable or disable it via app_anc_key. Issue is the handling of this request is from app_anc_cmd_receive_process which is not called without APP_ANC being defined.

This commit cleans up the way ANC is enabled and also fixes slave bud ANC switching by implementing the actual code path :)

Please note that the ANC testing branch should be rebased on top of main with this commit added.

I did my testing with main merged with anc_testing branch thus why the key_handling.cpp file :)

@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

This PR should be targeted at anc_testing branch, not main. Voting to close in favour of a new PR based off anc_testing, which can then be merged cleanly when anc_testing branch is ready.

@FintasticMan
Copy link
Contributor

The base branch of a PR can be changed without needing to close and re-open.

@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

The base branch of a PR can be changed without needing to close and re-open.

@FintasticMan Yes, this is true. I can't do it myself though.

@Ralim Ralim changed the base branch from main to anc-testing November 5, 2023 23:05
@Ralim
Copy link
Collaborator

Ralim commented Nov 5, 2023

I changed the PR target branch, though looks like this wasnt a clean base off that branch.
Could you please rebase your patch onto the anc-testing branch?

@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

I can do it from my end.

Switching ANC on or off from master bud has always worked
since we could just straight up enable it and no command had to be
sent.

When switching from slave bud we had to send the command to enable
or disable it via app_anc_key. Issue is the handling of this request
is from app_anc_cmd_receive_process which is not called without
APP_ANC being defined.

This commit cleans up the way ANC is enabled and also fixes slave
bud ANC switching by implementing the actual code path :)
@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

Done, need approval to push.

@shymega
Copy link
Collaborator

shymega commented Nov 5, 2023

Done, discussed with @Haxk20 on channel, and I've updated the PR.

@shymega shymega requested a review from Ralim November 5, 2023 23:39
@Ralim
Copy link
Collaborator

Ralim commented Nov 6, 2023

This looks great, very happy for this fix to go in.
Though I should probably update the anc_testing branch as well.
Ill try and get that done tonight which will probably make this neater to merge; apologies.

@Ralim Ralim merged commit fdfb820 into pine64:anc-testing Nov 6, 2023
1 check passed
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.

4 participants