• Status Closed
  • Percent Complete
  • Task Type Bugs
  • Category Music playback
  • Assigned To No-one
  • Operating System PortalPlayer-based
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Version 3.1
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by pondlife - 2008-04-01
Last edited by pondlife - 2008-12-11

FS#8836 - Short pcmbuf_beep() (AKA Keyclick) doesn't work properly on PortalPlayer targets

The attached patch revises pcmbuf_beep() in an attempt to kill the beeeeep of death that occurs on PortalPlayer targets when keyclick is enabled.

With this patch, on c200, the unpleasent beep seems to have gone, but the keyclick isn’t very reliable:
- Before playback is started, the keyclick has an extra click (probably when pcm playback begins)
- While playback is in progress, the keyclick sounds correct, but is delayed (could the 1/8th second delay be removed?)
- After playback then STOP, the very first keyclick works, but subsequent ones are almost inaudible.

I’ve no idea why, but if the un-needed “memset(bufstart, 0, samples * 4);” is removed, then the keyclick after playback rarely sounds at all. I guess it’s something to do with memory caching, but am not a low-level expert.

Also, it would be good if the pcmbuf static variable audiobuffer could be renamed, to avoid potential confusion with the audiobuffer declared in buffer.c.

Closed by  pondlife
2008-12-11 08:10
Reason for closing:  Fixed
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

No need to disable the option now. Thanks, jhMikeS!

Of course, the bug itself probably lies in the lower level PCM/DMA code. If it were in pcmbuf.c then I’d expect similar problems on all SWCODEC targets.

I’ve committed the above patch, minus the un-needed memset (and corresponding comment). Seems to kill the problem on PP, but sadly also the keyclick. H300 works fine with the simpler code.

I’ll change this back to a bug report for some PP guru to look at.

Enabling voice and keyclick and then jumping around the menus very quickly will make the beep of death happen eventually.

ncm commented on 2008-09-30 02:39

Duplicate #9430 - crash when keyclick on, multiple button presses

ncm commented on 2008-09-30 02:45

Shouldn’t the keyclick setting option be disabled on players where it doesn’t work?

There were plans to disable it in v3.0 (for PP players) but apparently it was forgotten.

ncm commented on 2008-09-30 23:58

Isn’t that what a tracking system is for? I have entered #9432.

ncm commented on 2008-10-01 03:22

That was quick; Llorean up and closed #9432. So much for using the tracking system so plans wouldn’t be forgotten. Me, I would have suggested putting it on the “Roadmap” instead of closing it.

ncm commented on 2008-10-01 07:17

I have attached a patch to make the bug self-documenting.

That’s not the right solution (and it breaks translation). Better just to disable the feature on PP (as per the attached patch), although if that’s committed the underlying problem will probably never be fixed.

The correct solution is, of course, to fix the PP implementation of pcmbuf_beep()…

ncm commented on 2008-10-01 19:00

Here’s another try at fixing the keyclick settings without, this time, breaking translation. It seems less intrusive than noppkeyclick.patch, and leaves open the possibility that the underlying bug might someday be fixed.

ncm commented on 2008-10-01 19:01

Sorry, that patch:

ncm commented on 2008-10-01 19:43

Sorry, I suppose that should be:

Erm, that patch just removes the options (apart from “off”), right? So the keyclick still can’t be anabled, but there’s a useless menu option left behind.

I maintain that the correct solution is to find out why the problem occurs in PP pcm playback, and then fix it. Could it be that the buffer length has to be a minimum number of samples, or a multiple of 2^n bytes? If so, a workaround may be just to mix appropriate silence after the beep. I don’t have a working PP target, so can’t test this here.

And if we can’t fix it, then the keyclick should just be removed as per my patch (IMHO).

ncm commented on 2008-10-02 09:38

The point is that, until it’s fixed, the present menu options are a nasty trap for anybody who doesn’t know that turning them on will make playback crash on random button presses. Of course the underlying bug should be fixed, but in the meantime we shouldn’t be tricking people into turning on something that has never worked correctly.

We leave the menu option in place because the manual mentions it. Anybody curious why the other options are missing will find this FS task and be enlightened, and maybe motivated to fix the underlying bug. When the underlying bug gets fixed, then we turn the menu options back on just by removing the #if/#else.

The manual should be kept in sync, and also suppress keyclick for PP targets. That should be doable, but I’ll need to ask a manual guru on how to do it.

ncm commented on 2008-10-02 21:31

All it need is a one-line note that keyclick is not yet supported on PP targets. It doesn’t need anything tricky.

MikeS commented on 2008-12-11 01:52

The PCM bug has been fixed but are all issues addressed here now? If so, please close.


Available keyboard shortcuts


Task Details

Task Editing