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

More complex approach to the audio "clicks" #149

Closed
wants to merge 9 commits into from
Closed

More complex approach to the audio "clicks" #149

wants to merge 9 commits into from
+157 −215

Conversation

dkaukov
Copy link

@dkaukov dkaukov commented Dec 22, 2024 •
edited
Loading

  1. ADC bias injection ([PCB] Connecting Audio to ADC is Problematic #148)
  2. ADC_ATTEN_DB_12 to be in spec
  3. ADC dc removal
  4. AudioFormat.ENCODING_PCM_16BIT as some Android devices do not support 8bit PCM (https://developer.android.com/reference/android/media/AudioFormat#ENCODING_PCM_8BIT)

All of this reduces chances of buffer underruns and when it happens, the click is almost not noticeable.

@dkaukov
Patch buffers
bc41459
@dkaukov
Update RadioAudioService.java
9e2f1cc
@dkaukov
More complex approach to the audio "clicks"
018f644 
1) ADC bias injection (VanceVagell#148)
2) ADC_ATTEN_DB_12 to be in spec
3) ADC dc removal
4) AudioFormat.ENCODING_PCM_16BIT as some Android devices do not support 8bit PCM
@dkaukov dkaukov mentioned this pull request Dec 22, 2024
Closed
@VanceVagell
Copy link
Owner

VanceVagell commented Dec 22, 2024 via email

@dkaukov
Copy link
Author

dkaukov commented Dec 23, 2024

Yep, no more clicking for me.

@dkaukov
Copy link
Author

dkaukov commented Dec 25, 2024

ADC Bias injection:

    DRA818 AOUT-+---- GPIO34 (ADC Input)
                |
               [R1] 22kΩ
                |
 GPIO26 (DAC2) -+
                |
               [C1] 0.1µF
                |
               GND

Discussed #148

@dkaukov
Copy link
Author

dkaukov commented Dec 25, 2024 •
edited
Loading

ADC input after bias injection:
IMG_20241223_150452423.jpg

Which is almost in spec.

@VanceVagell
Copy link
Owner

Thanks for the hard work on this! Your example helped me identify the root cause, which was purely USB related. This is now fixed in commit cc1ee23. Declining this PR, but it was instrumental to fixing this. Thank you!

@VanceVagell
Copy link
Owner

If there are elements of this PR that you think are still useful, please break them up into smaller PRs that have limited functionality so I can review them each. Thanks again.

@dkaukov
Copy link
Author

dkaukov commented Dec 25, 2024

Hi @VanceVagell ,

Thanks for the update and for fixing the USB-related issue! I appreciate the recognition of this PR. However, after testing commit cc1ee23, the clicking issue still persists on my end. While reducing the sample rate lowers the click frequency, the issue hasn’t been fully resolved.

Initially, high bit rate serial transfer seemed like the primary suspect, so I conducted a series of error rate tests but didn’t detect any problems in my environment.

Regarding this PR, I believe it’s already atomic and relatively simple. If you’d like to test specific parts of the PR or need further details, feel free to let me know. I’d be happy to help with additional testing or adjustments.

Thanks again for your time and the work on this project!

@VanceVagell
Copy link
Owner

VanceVagell commented Dec 26, 2024 via email

@dkaukov
Copy link
Author

dkaukov commented Dec 26, 2024 •
edited
Loading

Hi @VanceVagell ,

Thanks for the detailed feedback. I think the issue you're seeing with 44.1kHz might be due to testing the new firmware with the older Android app. Since the PR changes the wire audio format to ENCODING_PCM_16BIT, the existing app won't handle the audio correctly without corresponding updates (and vice versa).

Regarding APRS – you're right, it’s broken with the current PR because of the format change. The fix is straightforward: we just need to replace convertPCM8ToFloatArray with convertPCM16ToFloatArray to handle the new 16-bit format. I can make that adjustment if you'd like. (Something like: dkaukov@0527053)

Also, here's the audio recording you requested (v1.5.0): https://www.youtube.com/shorts/QL5MdgGOt2E. As you can see clicks are happening with open and closed SQ.

Thanks again for taking a look at this!

BTW: To simplify testing, I've created pre-built binaries https://github.com/dkaukov/kv4p-ht/releases/tag/click-fix-01

@dkaukov
Fix for APRS encoder
0527053
@dkaukov
Merge branch 'main' into clicks-fix
e345f0c 
# Conflicts:
#	android-src/KV4PHT/app/src/main/java/com/vagell/kv4pht/radio/RadioAudioService.java
#	microcontroller-src/kv4p_ht_esp32_wroom_32/kv4p_ht_esp32_wroom_32.ino
@dkaukov
Fixed RSSI
29e6fb7
@dkaukov
cleanup
a91e995
@dkaukov
Copy link
Author

dkaukov commented Jan 3, 2025 •
edited
Loading

Hi @VanceVagell,
Since the proposed changes depend on each other, I created "progressive" PRs in my fork:

PR 1 – ADC Bias Injection
PR 2 – ADC auto DC removal
PR 3 – Android: Switch AudioTrack to the AudioFormat.ENCODING_PCM_16BIT
PR 4 – More robust command extractor state machine

Let me know if you'd like to review or discuss further.

@VanceVagell
Copy link
Owner

Thanks so much Dmitry, this will make it easier for me to take a look, and test each part.

In the meantime, could you please test the latest build (v1.6.1 of app and v5 of firmware), and confirm if you still have the audio problems?

@dkaukov
Copy link
Author

dkaukov commented Jan 4, 2025

Hi @VanceVagell,

Thanks for the quick response! Unfortunately, I can't test the released v1.6.1 as I only have UHF modules, and I need to patch both the firmware and the Android app. After building from git, I can confirm the following:

  1. The clicks are much less frequent but still occur occasionally.
  2. The audio on RX seems distorted to me.
  3. The latency is noticeably high.

These are the reasons behind the proposed patches. Let me know if you need more details or if I can assist further with testing.

@VanceVagell
Copy link
Owner

Reopening now that this is split out. Will test.

Dmitry, does this still require the PCB modifications you previously outlined? If so, @SmittyHalibut should take a look at that to confirm if your change is compatible with the new PCB 2.x designs.

@VanceVagell VanceVagell reopened this Jan 6, 2025
@VanceVagell
Copy link
Owner

VanceVagell commented Jan 6, 2025 •
edited
Loading

I tested this PR on both a v1.x PCB, and a v2.x PCB, with both the new Android app in this PR and the new firmware installed on the test device.

Unfortunately, I'm getting severe audio distortion on both PCB versions. I have not made the hardware modifications you mentioned in the other PR, would that cause these audio issues? We can't expect everyone to change their PCB, so any software change needs to work with the stock kv4p HT hardware.

Please let me know what you think is going on so we can move this PR forward. I'd love to merge it if it helps some people with other audio issues, but currently it's a regression for my test devices.

PCB v1.x:
https://github.com/user-attachments/assets/c5d8cb8d-67a9-4a1c-8e4b-cc9fa9f93340

PCB v2.x:
https://github.com/user-attachments/assets/ea900c2c-7f53-49b7-bac8-40fb4f942b1d

@dkaukov
Copy link
Author

dkaukov commented Jan 6, 2025

Hi @VanceVagell,

Thanks for testing on both PCB versions and sharing your feedback. I completely understand your concern – software updates should work with stock kv4p HT hardware whenever possible.

That said, we can’t avoid physics – feeding an AC signal directly into the ESP32 ADC isn’t ideal, and staying within spec would require schematic changes. Apologies for that.

It looks like the samples you tested may have come from an incorrect merge. Could you try the pre-built binaries here to help narrow things down?
https://github.com/dkaukov/kv4p-ht/releases/tag/click-fix-01

Let me know how it goes. I appreciate your time!

@dkaukov
Copy link
Author

dkaukov commented Jan 6, 2025

Hi @VanceVagell,

In my opinion, it’s better to keep this PR closed, as the code changes here are no longer mergeable (I didn’t expect it to be re-opened). Let’s focus on new discussions, starting with:
#159

Thanks for your understanding!

@dkaukov
Copy link
Author

dkaukov commented Jan 7, 2025

Code changes here are no longer mergeable

@dkaukov dkaukov closed this Jan 7, 2025
@dkaukov dkaukov deleted the clicks-fix branch January 7, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkaukov @VanceVagell