Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Drivers
  • Assigned To No-one
  • Operating System iPod 5G
  • Severity Low
  • Priority Very Low
  • Reported Version
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by obo - 2006-04-12
Last edited by jdgordon - 2011-11-16

FS#5111 - Ipod piezo driver

First attempt at a piezo driver for ipods - works, but needs a lot more done to it yet.

Closed by  jdgordon
2011-11-16 10:26
Reason for closing:  Accepted
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

Holy crap! commited in r30995.

thanks everyone!

obo commented on 2006-04-15 20:22

Add approx Hz conversion and test plugin

obo commented on 2006-05-26 19:11

To emulate the clicker sound produced by RetailOS:

Sound needs to have a period of 26 or 27, for a duration of 0.004 or 0.005 seconds. I don’t know any way of making Rockbox sleep for that short a period (also the sleep function always seems to do a sleep + 1), without changing the definition of HZ. Setting HZ to 1000 produces some very funky button handling.

Whether we should be slavishly trying to emulate the sound of RetailOS is another matter… but I think a default beep and click would be good, just not sure what settings to use.

I have worked a little more on the patch, it sleeps with udelay for RetailOS like sound. The sleep frequency and length may be configured in the menu (when the length is 0 it is silent). (patch against cvs from 2006.7.24). (does not include the test_piezo.c plugin code).

New version of piezo.diff without the keyboard junk and another patch (to be aplied after tracker - the old piezo.diff) for sleep using timer instead of udelay which should be snappier.

obo commented on 2006-08-01 22:11

Doesn’t beep now during button repeats. Also remove what I think are some unused calls to button_beep.

It also beeps during volume change in the WPS - I haven’t yet looked at trying to stop this.

senab commented on 2006-08-02 08:37

Thanks Rob ;)

senab commented on 2006-08-02 08:44

Infact obo, piezo.c and piezo.h seem to be missing in your latest patch. Have they been altered since you’re old patch?

obo commented on 2006-08-02 09:02

Oops - sorry about that - I was using yet another source tree and had forgotten to “cvsdo add” them. piezo.[c|h] are unchanged from freqmods last diff, but I’m attaching a new file for completeness :)
I’ve also wrapped all the piezo functions in button.c with #ifdef HAVE_PIEZO - makes it compile cleanly on other platforms. Could have just done a (void)tpe, but didn’t see the point in calling something that did nothing.

senab commented on 2006-08-03 13:21

Obo, freqmod’s piezo.c/h still aren’t working on target. I’ve gone back to using your old piezo.c for now.

We’ve had some more fun testing the piezo with Mikachu this afternoon.

Looks like the piezo commands can be decomposed as:
0×80000000 : sound on if set / off if 0
0x7f000000 : doesn’t seem to have any effect
0x00ffe000 : seems to be some kind of wave form modifier (not sure about 0x0000e000 though)
0x00001fff : frequency

You can find a small test plugin here: http://people.videolan.org/~dionoea/ipodtest.c

senab commented on 2006-09-21 11:52

This patch no longer compiles, I’m guessing it has something to do with the thread scheduler.

lini commented on 2006-09-22 11:51

I have updated the piezo patch for the new scheduler. The priority of the piezo thread is set to PRIORITY_USER_INTERFACE (same as the rockbox UI) because the clicks need to be in sync with the actual scrolling/screen update.

obo commented on 2006-10-05 22:28

Updated for the ipod target tree.

where r the settings for this?

obo commented on 2006-10-10 17:02

General Settings → Display Settings → LCD Settings

Button click duration
Button click frequency

Has anyone had experience with this conflicting with other patches? I’m trying to figure out which one it is.

ok, it conflicts with patch http://www.rockbox.org/tracker/task/5744

I apologize but my last comment was incorrect. It does NOT conflict with the USB connection patch. What the issue is if you have the iPod LCD brightness patch (http://www.rockbox.org/tracker/task/5594) that setting must be all the way to 15 or the piezo will not work.

obo commented on 2006-10-26 11:24

Add support for the “wave form modifier” - can be used to adjust volume.

obo commented on 2006-11-11 22:47

Remove the microsecond timer.
Add check on piezo_button_beep to prevent the queue overflowing - any better ways to do this?

lini commented on 2006-11-13 06:43

using this new patch, what values should the duration and frequency vars have so the click sounds like the original ipod?

Anyone object to applying this patch to trunk? (If not, I’ll apply it)

idak commented on 2007-09-02 12:41

resync

idak commented on 2007-09-02 12:43

resync

I’d really like to have this stuff commited soon, but I have two gripes with it right now:
1. Beeps are too long. Yes, I know I told you not to use the user timer for shorter durations, and we still can’t do that, thanks to backlight fading also using it. Any other ways of doing it?
2. I don’t think the options are necessary. A retailos-ish beep should really be enough.

obo commented on 2007-09-28 21:25

1 - is better now, but maybe not perfect.
2 - changed the options to a bool only. Retailos pitch and duration may need tweaking.

Lots better, the bool option we can’t really get rid of, so that ok :) The beep sound is lots better now, but now it’s perhaps a bit on the inaudible side? I also have some weird effects where the sounds changes a bit if you do successive beeps.

There seems to be an issue with the current patch version and the most recent svn checkout. Patching english.lang fails the hunk. After patching by hand, everything compiles, but there are no menu items relating to the piezo, and no beeps. Thoughts? Maybe just a sync would fix this.

does this patch conflict with slider progress bar?

Please re-syc the patch, repeared !

Is there any way to change the way piezo sounds, on my Ipod it dosent sounds like the original apple OS, and the sound is very low.
Maybe a menu option or just to say what and where to edit the patch.

Please re-sync the patch?

When you scroll a little faster, it starts to sound little different, and not cool at all.

This is a shameless bump to the thread, as I have no coding skills of my own, but would really like to see this patch picked up again by someone (hopefully to the point where it could be merged with the main build). Is there any chance that any coders with an ipod find this feature as good an idea as I do?

I’ve updated the patch to r17730

Sweet, someone else at least interested enough to update for compatibility! I assume it’s working the same as the old version did? I don’t suppose you’re interested in working on the patch more?

Either way, thanks so much for syncing it. Much appreciated!

Yes, very very much thanks for syncing it. When will this patch be in SVN, because in my opinion it’s stable enough. I think this is also very useful for blind people (the current keyclick function in rockbox using the headphones doesn’t work well on the ipods (at least on my nano)).

I dont tihnk it should be commit as is just yet.
1) it should probably be HAVE_HARDWARE_CLICK or something instead of piezo
2) the setting should be similar to the ipod OD where you can click in the piezo/headphones/both
3) it should be handled in the same way and places as the software click

I agree with jgordon, it’s not ready for svn yet, but I would love to see someone passionate about the feature take a look into his suggestions. It’s definitely important that it gets handled similarly to the current software click, and the two would probably get grouped together in the same location to toggle one or both on, as he said in 2.

Changing to hardware_click makes sense to me on more than on level, since before I actively searched for this topic I had no idea what piezo was… maybe that’s not necessarily keping people from realizing this feature has been worked on, but it kept me in the dark for a while.

And of course, calling it hardware click makes sense with the other kind being a software click.

I wish that I could do more to help see this in svn, but I have trouble even applying patches, let alone writing them. I don’t suppose there’s anything non-code related I could do to help out?

wpyh commented on 2008-06-19 15:24

I’m actually interested in this patch too, just for completeness. I think one big problem is that it doesn’t sound ‘nice’, according to one of the comments above. I hope someone can step up and try to fix the issue.

Disclaimer: I haven’t tried the patch yet, so I don’t know how “different” it is from the OF. We certainly need confirmation on this one.

I’ve changed the define to be HAVE_HARDWARE_CLICK, moved the options in the Keyclick menu as a bool to “Enable Speaker”, and placed the beep with the software keyclick in actions.c

It does sound different than the standard iPod click, but personally I don’t mind it. I might play with it later.

That should be “action.c” above, and on closer listening between the two - the difference is barely noticeable (or I’m imagining it)

The most ironclad test would probably be to have an ipod and a rockbox’D ipod side by side, to hear if there’s any pitch/duration difference (rather than trying it, rebooting it in the other firmware and trying it again).

And once again, thanks for taking the time to work on this. It’s honestly the only thing I miss about the normal firmware.

Okay. I did it. I finally, finally figured out how to make my own build, and apply patches. Sweet. I’ve tested the latest patch, and while the click (when it works) sounds pretty okay, I’m seeing a few problems on my end.

1) Is there a possibility of hearing the click only through the speaker, or through the headphones? It would be nice to have the option to select none, either, or both (even though the OF doesn’t allow this, to my knowledge, it would be kinda neat).
2) Is there a way to disable/adjust the click when adjusting the volume/doing other WPS things? The click can disrupt the sound a bit, and with crossfade turned on, having a click sounding on track change becomes semi-unnecessary (unless you definitely want to know that the track change was registered without looking at it). I know that for the OF, the clicks still occur when changing volume, but they seem to have made them less frequent when doing so.

Those two are more of a personal preference, but the last one is an actual problem. I’m still getting weird beeps when I scroll too fast, only now it’s coming from the speaker as well as the headphones. It’s not particularly loud, about the same volume as the clicks, but more piercing and annoying. I guess the cause of this is still unknown? Is there anything that can be done about it at this point?

To explain myself before any comments come in, the reason I think it would be nice to have the option of hearing the click from speaker or heaphone individually is that when I use my player in the car, the clicks through the headphone jack are more distracting than the softer clicks I could potentially hear from the ipod speaker (and yes, I do realize that I can turn the headphone click down). It would be useful to me to have the click in this fashion.

The beep is caused by the piezo thread yielding for too long. If your feeling more adventurous, in file “firmware\target\arm\ipod\piezo.c” change line 106 from :-
yield();
to :-
if (duration >= 5000) yield();

If not, I’ll make a patch file up for you soon - I’m just playing with another patch and have the source code in bits. I can look at reducing the frequency of the clicks, but at moment its in line with the rockbox software keyclick code.

At the moment, you can set keyclick off but set enable speaker to only hear it through the piezo speaker - or disable the speaker but set the keyclick from off to enable software keyclick through headphones. The options do need cleaning up.

My bad on the last part… I should have remembered that keyclick was already there pre-patch. The options do not need cleaning up, as you said. Although I still do think that since both options produce a click, it might make sense to simply use the “keyclick” option, and upon selecting it you would see something like
-headphones
-speaker
-both
-off

It’s perfectly fine as-is though, so unless anyone else feels that way/my wording of it just changed your feelings on it, I wouldn’t worry about it.

As to feeling adventurous, I may look into what you were talking about… it sounds like whatever you’re doing with the source now is bigger than that, though, and I may just wait to see what you’ve got up your sleeve. Thanks again for the hard work.

Here is a new patch - I’ve just changed the option wording a bit. See what you guys think of it now. I plan to borrow my friends iPod nano to check the click sound next to each other.

I’ll check this patch later today and let you know how it goes. Also, I’d just like to apologize for mistyping jdgordon earlier, and also that I could’ve sworn you said the options did not need cleaning up, when obviously you did say they needed cleaning up. I’ll try and be a more careful reader in the future, haha.

This new patch is a crazy awesome improvement, if not simply for the killing of the beeps/reworking of the menu wording. My thoughts/observations:

1) With keyclick repeat on, holding down any button will make the device start clicking rapidly. This should probably be altered so that a single button press, even a held one, still produces a single click. This wouldn’t be applied to the scroll wheel, since it needs to continue to click when doing things like changing volume (albeit less frequently, as I suggested) and scrolling through lists. I know that right now using the repeat mode probably isn’t so conditional, but to add those tweaks would probably be useful, if they’re doable.

2) Since ‘keyclick’ is the name of the overall setting menu, ‘headphone click’ and ‘speaker click’ are probably simpler and less repetitive than saying ‘keyclick’ again in full. probably works for ‘click repeats’ as well. Besides that, I think the new wording and setup looks much better.

3) When scrolling through lists (again, with repeat enabled), I was thinking about another handy tweak that might be likeable, if it could be done… on my super-duper cool phone (the newer chocolate), when I’m scrolling through lists, it makes a beep noise instead of a click when wrapping from the bottom back to the top (or vice-versa). This is another thing that, if implemented, would assist in navigating without needing to look down at the screen, as you can tell when you’ve wrapped around and can do things like scroll immediately to the bottom of a list and then four up to the song you want, just by hearing the beep and four clicks (the tone wouldn’t need to be a beep, of course). Does anyone else think that would be useful, and is it doable?

I’m very impressed with all of the work. You’re a machine! I’ll be interested to hear how the current click stacks up against the nano’s side by side. Please let us know, and I hope my comments are more helpful than annoying…

1) no… it shold keep beeping… I like how in the OF the pitch changes (havnt tryed this patch so dunno if we are doing that also)
3) yes this is doable, sort of… possibly the best thing we can do is beep when you get to the top/bottom of the list because in most lists it wont let you wrap untill you release the button

1) I’m not exactly sure what you’re referring to in the OF. I still don’t think the clicks should repeat in that manner… it’s especially weird when doing something like shutting off the player (where holding down a button is necessary) and the ipod sounds like a mini-rattlesnake. Fast-forwarding a rewinding by holding prev or next in the OF also only make a single click, though changing volume does make multiple clicks (as previously stated).

3) Currently if you continue to scroll after reaching the bottom, the ipod continues to click away as if you’re still scrolling through something. Ideally, this is where something should just tell the click to stop, rather than make a beep that could be lost in the current series of clicks. Once it stops clicking here, you would know to stop scrolling for the moment so one could wrap the list, and once the wheel is pressed again, when the code sees you can’t go any further and loops back to the top, I think that a beep should be made (and scrolling up instead would just return to clicking). There has to be code in there telling the list to loop, so it sounds like it should be doable to make that loop produce the beep.

I’m not sure about the pitch change in the OF on the iPod nano - I notice the duration of the click changes on some actions and there was provision for this in an earlier patch. I’ll have a look into the top/bottom list beep but would this also want applying to the software keyclick?

I think it would make sense to apply it to the software click as well, yes. I think that for all intents and purposes, it makes the most sense to keep the same features for both the speaker and software click (and hopefully take less work to maintain?). That will mean that people will choose the type of click based solely on where they want to hear it, and not because one has features the other does now. Not to mention it would be pretty weird to have both on and hear two different behaviors at once.

… “and not because one has features the other does NOT” is what I meant to say.

I think the beep when scrolling up/down is moving away from this task, however here is a patch with it roughly implemented for the hardware click only. “Keyclick Repeat” can now be turned off and the clickwheel will only repeat clicks as suggested by DancemasterGlenn for 1).

As for 2) - I quite like the word Keyclick, I don’t think Click “looks” right. My personal opinion.

I like this patch, but there’s something really annoying me namely when at the end or beginning of a list it beeps very loud with a high pitch. It’s getting me a headache. Also when piezo keyclick is turned off, the high beep still remains to beep. I’d like to see an option to turn that beep off. I’ve attached the current version of this patch including Dutch Translations (I hope this is correct, this is the first time I’m working with patches).

Oops attached wrong, this version should be fine.

Haha, that was an amazingly fast response to turn off something that was just added to the latest patch… having tested it, I would agree that a final implementation would definitely need a less high-pitched sound than that current beep. Also, the way it was implemented was more like jdgordon’s idea of having the beep upon reaching the bottom or top of a list, rather than during the action of moving from bottom straight to the the top, or vice versa. I would at this point agree with you (Craig) that the whole deal with the beep is probably best left for another task, which I’ll probably open if/when the speaker patch does get committed. Thank you for working with it for my sake, though.

Back to the patch, the change causing a button hold to not click like crazy is REALLY GREAT. Is it doable to make the device stop clicking when you’ve scrolled to the bottom of a list, as well? That will probably be more useful than the beep idea anyway. If the device isn’t clicking anymore, I would know I’ve reached the top or bottom of the list anyway, and could go from there (the same idea could also be applied to changing volume, so one knows they can’t turn the volume up/down anymore?).

Finally, your opinion on liking ‘keyclick’ over ‘click’ is totally justifiable, and it’s definitely not something I feel the need to defend any further. ‘Keyclick’ sounds fine to me.

Lol, didn’t saw the beep was just implemented… Anyways, yes I agree with Glenn it’s really great it doesn’t click like earlier when holding a button.
I think it isn’t too difficult to let it stop clicking when at the bottom of a list or menu. Later this day I’m gonna try to remove that beep by myself (I have some basic C skills, still learning more and more).

Well, Craig knows exactly what code he used to put the beep in, and assuming he wasn’t really all for pursuing it in this particular patch (which seemed implied, especially since it was only implemented in the speaker click and not the headphone click), I’m sure it’ll be really easy for him to remove the beep himself. I appreciate your enthusiasm a lot, though… I wish I knew anything about coding in C so that I could help out too.

It’s really great to see people this interested in getting this up and running so well! I’m simply overjoyed.

Hehe, yeah the beep staying on when the keyclick option is off is my fault - I forgot to include the option itself! To remove the beep code - its in “\apps\gui\list.c”, remove everything that’s “#ifdef HAVE_HARDWARE_CLICK”. You can also make it beep when it reaches the bottom (not when tries to wrap) in the same function - but I think its definitely for another task.

To make it stop clicking when at bottom of menu seems a bit more tricky, the keyclick is produced on response to the buttons in “\apps\action.c” without knowing where it is in the GUI. I think some information/opinion from jdgordon on this is needed, as (again) its going from more than an iPod thing and could be an option to change behaviour of software keyclick too.

Here the patch with no beep and the dutch translation for people to use.

This patch seems to be fine (no beep in it). When at the bottom or beginning in a menu / list it just clicks instead of beeping.

Offtopic: Is it normal that crosscompiling in Cygwin takes almost an half our? I’m running on a Celeron M 1,5 Ghz.
I also noticed after crosscompiling my memory usage is increased from approx. 300 mb to almost 1000 mb, and it just stays at that height. I have to reboot to get it back to normal. I’m thinking to install Debian or so for Rockbox compiling on my laptop.

More offtopic: WOW, installed linux/debian on my machine and the needed crosscompilers, it compiles full rockbox in just 3-5 minutes!

I made a little comparison between the OF piezo and the rockbox piezo. The clicks after the first beep are from the OF, after the second beep are from Rockbox. As you can hear the rockbox clicks are a little bit higher, but it doesn’t much differ from OF.

Here it is:

If at all possible, I’d definitely like to see the Rockbox click match the OF click more closely. Finally hearing the two by side, I would definitely make the argument that the OF click is a much “nicer” sound, and definitely more subdued. It’s definitely a noise that will not attract quite as much attention when navigating in public, if you need a real world reason to make the attempt. How is the current Rockbox pitch determined, and is it something that can be adjusted to mimic the OF in this way?

Also, is there any code in the list source that returns a list length? That would possibly be helpful in stopping clicks at the end of the lines.

list→nb_items

Try this one - the keyclick is made with the action handling code if not in a list context, otherwise its made by the list handling code - so hopefully no clicks at top or bottom of list.

I’ve also reduced the pitch of the clicks to try and make it match better. I’ve got my friends 1GB iPod nano, and his OF click sounds quite a bit different to my 4GB nano OF click - both are same firmware versions, maybe damage to one of the piezo or just natural variation.

This patch is great! The way you coded it stops the clicks perfectly. I was worried an initial implementation wouldn’t remember to start clicking again after reaching the beginning/end and then looping, but you totally took care of it. Good job! The click also sounds better… Though the OF has this very distinct sound that I can’t put my finger on. In Stijn’s test, it almost sounds like someone hitting something very tiny and hollow. Anyway, no biggie if no one wants to work much further on it. I would say that the click could actually be a little quieter? But that’s pure opinion, and is neither here nor there. The sound is a massive improvement, either way.

Some observations/other thoughts:

1) Some testing revealed that keyclick now repeats no matter what. Personally, my question is: who needs this option? To me, turning off repeat was only really important to me when the thing was still beeping when used too quickly, and I never really wanted the click just to know I’ve made a general action. If people think this is still useful, it will need to be fixed, but I’d appreciate it if someone would tell me its purpose. In addition, now that it’s working very nicely, would anyone object to ‘keyclick repeats’ defaulting to ‘on’ (this is not asking for the keyclick to be defaulted to on, just the repeat function)? Once again, I think this was defaulted to ‘off’ because of the beeping. Just thought I’d throw that out there.

Actually, I just found out what turning on ‘keyclick repeats’ does: it seems to give the exact same behavior, but it brings back the crazy clicking when holding down a button. So right now, setting this feature to ‘off’ is actually giving me the behavior I was looking for. Weird. So actually, if you want to get technical, the ‘off’ feature is correct: going through a list is not holding down a button, so that’s not a click repeat. Same with changing volume. So actually… I guess that as-is, I’m not seeing any need for the ‘on’ function. Interesting. If this is what everyone else wants, do we need this option anymore?

2) Some more testing (actually plugging in headphones) revealed that the headphone click is now tied to the speaker click. When I turn on the speaker click, it enables the headphone click even when the headphone click is still off. This isn’t as big a deal at the moment, since the click noise is much easier to deal with in its lower pitch, but should still probably be cleaned up. Also, turning on headphone click and disabling speaker click gives me no clicks at all. As opposed to the repeating options, I do think that it is a good idea to be able to toggle the headphone and speaker clicks independently. I don’t necessarily understand needing to adjust the volume of the headphone click, but that should probably be left up to people who use it.

3) As I’ve said before, I still think that it would be very simple to have this in the ‘Keyclick’ menu:
-headphone keyklick
-speaker keyclick
-both
If the volume of the headphone click was sufficient for everyone (and the repeat thing was deemed to be properly implemented), then we’ve knocked out a whole set of further settings to navigate through, in favor of three simple options (basically, “where do you want your clicks coming from?”). I’d like to hear people’s thoughts on this, as well.

4) Finally, having seen that we can use action handling code to make the clicks work in specific ways in lists, can the same thing be done with the volume handling? For example, is there a piece of code that can retrieve the current volume (I’m assuming there is, so that the gui can be updated), and if so, can that be used like the list code was used to stop clicks at the minimum and maximum volume limits?

To take this a step further, in an attempt to try and understand how the OF slows its clicks when changing volume… perhaps a click could only be given every five decibels, or something like that? I don’t know if the code could be made to work this way, but it’s a thought.

Once again, sorry to unload a ton of information all at once. I just don’t want to forget anything, and I’m all giddy about all the work that’s been done lately. Keep up the amazing work.

Sorry, in number three I of course forgot an ‘off’ option. So four options. It would still be much simpler, though, if people were into it.

I can’t reproduce your problems in 2) - have you cleaned your rockbox code and started with a fresh rockbox install? There is an active bug with the keyclick on PP-based targets  FS#8836 , the speaker click disappears and works mainly when music is playing.

I can look at reducing the frequency of clicks whilst using clickwheel in CONTEXT_WPS to give the desired clicks for volume changing. At the moment, I don’t know about stopping it clicking on min and max volume though.

I believe the version I compiled this morning was 17806. An absolutely fresh rockbox (the way I learned to patch, I’m not sure if there’s an easier way, but it sounds like an easier way would be wrong). I’m using an ipod video 5.5 80 gig, though I’m under the impression that shouldn’t make a difference. I can try compiling again if you want, but I don’t think I’ll get different results. Shisken, JDGordon, did you experience any of the problems in 2?

Also, a penny for your thoughts regarding the other stuff I blabbered on about?

As for min and max volume clicks, something like “if rockbox.volume() > 6 then keyclick==null” (haha, yay for fake code). I think 6 is the max, it is on my ipod. If there is currently a way to retrieve decibel values, I feel like it should owrk similarly to however you got it to stop clicking in lists.

I think there are several issues here and they’re getting muddled - here is how I understand it, anyone please correct and help make progress :-)

The ‘Headphone click’, as my patch now calls it, is Rockbox default software keyclick - which on iPods doesn’t work correctly, and on my iPod nano only really works when I have music playing or before I press play after a restart - related to bug  FS#8836  .

On the iPod, the clickwheel motion of scrolling until you stop behaves as a single keypress, ie. holding down the button - so default behaviour is no repeated clicking when keyclick repeat off - the patch overrides the “Keyclick repeats” option for the clickwheel only (coming from the speaker - it doesn’t alter the behaviour of the software click). The “Keyclick repeats” is there for other keys and other players. The crazy clicking on button press is the keyclick repeat working correctly. Perhaps a new task (or this one??) should be started to change the keyclick for the ipod in general.

The volume options for the software click come from the Rockbox normal menus - when the software click does work, they do make a difference and I can see people having different preferences.

Its the headphone click tied to the speaker click that I’m interested in - I don’t know if there is there a way to make the piezo drive to the headphones?? The software and hardware options are different - I’ve double checked, but I did change the parameters to the piezo to try and improve the click. On my nano it behaves as it should do, I’ve made some more small changes to the patch in preparation to try looking at the volume magic later.

I believe I understand what you’re saying… that essentially, this task should be kept more specifically to the piezo driver work, and that work on the headphone click is probably more appropriately kept to a separate task. In that case, it would make sense to work out the remaining quirks in the speaker click before subsequently tackling a headphone click rework in a separate task. And in the meantime, until a point at which they’re essentially the same and possibly a particular headphone volume is decided upon, a point could possibly be made to streamline the options. In the meantime, work must continue on this task.

Hopefully that’s what you meant.

I would make the point that whether they use the same code or not, the two clicks should probably eventually end up doing the same thing (or at least have the same sound). Like I said a while ago, it would be weird to have them both on and doing different things. We’ll see where it goes when we come to it. I’ll admit I’m constantly forgetting that this needs to work in some other targets besides ipods, so I’ll need to be more conscious of that when it comes to brainstorming in the future.

I’ll say one last thing in this task in regards to the headphone click: it’s nowhere near as sophisticated as you’ve gotten the speaker click. I’m probably never going to use it, but I hope when this task is done you’ll think about cleaning up the headphone click for all ports to enjoy.

Okay, I’m definitely getting headphone clicking with the speaker click on. I’m not crazy!!! It even sounds like the speaker click… the headphone click doesn’t sound anything like the speaker click anymore. It’s not just bleedthrough, because my headphones are noise cancelling and if I hold the ipod way out of reach with my fan on, I can’t hear the speaker click with the headphones off. And I can hear it with them on. You’re really not hearing this? It’s only audible with no music playing… When music starts, the louder the music, the more the click fades into the background. Which I know sounds like I’m hearing bleedthrough that’s being drowned out, but I’m serious. This has to be coming from the headphones.

To reproduce:
1) Apply patch
2) Start Rockbox
3) Enable speaker click
4) Cover your ipod with something, I guess. or drown it out some other way. You’ve got to be able to hear this. Just scroll up and down a list.
5) Play a music file. Scroll the volume up and down. When it gets low, the click can be clearly heard. As you up the volume, the song fades in, but the click fades out (or maybe stays the same and is drowned out, I can’t tell).

Please, someone confirm this. If it’s really not just me, then we have a headphone click already for some reason… it’s just not audible when the volume is up for some reason. Tested on a fresh source.

Thats pretty much what I was getting at, and yeah I think the software click should end up doing the same thing, but its really up to jdgordon if this is getting worthy of committing, and integrating the keyclick mods.

All I can suggest is to try this patch - I’ve put the click back to how it was but still at a lower pitch. Remember that we do have different hardware, maybe I’ve stumbled on the hardware headphone click for the ipod video (like the OF on headphone setting)??? - but for my nano I get nothing. Also, the volume click in the WPS is now every 6 dB.

All right. Two things.

First of all, this patch still gives me the magic headphone click on my firmware. If you’re trying to reproduce, use the steps in my last post, but for 5: “Play a music file. scroll the volume up and down, THEN GO BACK INTO A MENU AND SCROLL UP AND DOWN A LIST AGAIN (before you could test it by just continuing to turn the volume down, but now that it doesn’t click after reaching the bottom, this isn’t the easiest way to test). When volume is low, the click can be heard. When the volume is up, it sort of fades away.” Apparently this doesn’t work on the nano, so if anyone at all has a non-nano ipod, please test this for me and tell me if I’m nuts or not.

Second of all (and most importantly), this patch pretty much does everything that the OF click does, at this point. The click stops at the end/beginning of a line, and stops at the minimum and maximum volumes. Clicking every 6 dB sounds pretty good (I might go even higher, maybe seven or eight? Hard to tell without hearing it), and the fact that you accomplished all of this at all is just great. The click is sounding good, too (especially comparing it to the original rockbox click). Is there anything else that should be set as a goal for this specific type of keyclick? JDGordon, what are your thoughts on what this patch still needs before it would be considered for committing?

The only reason I picked 6 dB is that 4 clicks with my earphones is the difference between loud and quiet - I made this patch click every 12 dB, see what you think.

This patch also includes the test plugin from dionoea near the start of this task. Glenn, if you could first of all test the previous patch that didn’t give the magic headphone clicks, to make sure it still doesn’t, then apply this patch and start up the piezotest plug-in under applications. The piezo sounds if enabled is greater than 0×80. I changed the waveform from 0×80 to 0xF0 before when you first said the problem, and reduced the freq from 0×32 to 0x5B (lower freq is an increase in the value). I guess its a case of see if the buzzing comes through your headphones.

Well, to be perfectly honest, I tested most of the initial patches once I started actually using them with no headphones on. I’ll try a couple of the patches out, but it’ll be slow going. Am I patching wrong? I always download a new source, run ./rockboxdev.sh, patch, configure and make. Every time I want to try a new patch, I’m deleting all of that and starting completely fresh. That’s right, right? Or can I un-apply a patch on a source so I can re-use it again slightly more quickly?

Either way, I’ll go back and test right after we removed the beep when looping. I’m still not quite sure what you’re asking me to do with the plugin, but hopefully I’ll know when I actually see it. And if not I’ll ask.

Myquestions to everyone else still stand! We need more than just my feedback…

Project Manager

Glenn, A) rockboxdev.sh install the dev environment and you basically never have to re-run it. B) you _can_ unapply a patch by simply doing the exact same command line but adding -R

Dan, thank you so much! That speeds this up greatly.

Craig, I was rolling backwards through your patches, when I realized I should start from your initial commits and move forward. Your initial sync doesn’t compile for me, and the one after that (when you moved all the settings to where they are now)… gives me magic headphone click. So I have no idea. We really need someone else to confirm this. It’s definitely there… I’m literally holding my ipod under a pillow, and I can only hear it clicking with my headphones on. I have no explanation as to why it only seems to be happening on my video, though.

And don’t get me started on how weird it sounds when I enable both (this is on more recent patches)… without repeat on, I hear the different-pitch headphone click with every first click, followed by little subsequent (different pitch) clicks (the magic ones). With repeat ON, it gets kinda buggy. The headphone click beeps sometimes, but the magic clicks don’t. Curiouser and curiouser.

What would happen if we commented out all the headphone click code? Could you make a patch that does that? I’m wondering if disabling all headphone click junk may disable something that’s been magically tied to the speaker click on my build. If it does, then I would know that something in that code is the culprit.

In the meantime, I’m going to test out the newest patch, with the plugin. I’ll let you know my results.

Yes, the buzzing does come through my headphones with this plugin.

Having spent the day with the patch, I’d say clicking every 12 is probably higher than it needs to be (especially with what you said about it only taking 4 clicks for it to get very loud for you). I’m always clicking all the way up for when I’m in the car, since I like to get it up to 0 dB and I’m runnning it through the headphones, so it’s good to hear a few more clicks. I’d go with every 8 or 10, probably. 10 would be a nice round number (hah), but 8 gives people who don’t turn the player all the way up all the time (you and probably most other people) more clicks. And clicks are good. So 8 would probably be good.

You know what? I just realized the OF doesn’t even mess with the clicking in volume. I mean, it stops at the min/max values, but it just kinda chugs away when changing volume. Maybe it’s clicking less than in a list, but it’s a bit hard to tell… the volume ramps up a bit faster in the OF than in rockbox. I’m a bit embarassed, I’m not sure why it sounded so different before. So I guess whatever you want to do is up to you… personally I like it not clicking so much (I’d rather be hearing the song than a bunch of clicks), but if you want to change it back to the way it was before we started messing with it I’d understand.

Testing against r17865, the newest patch now fails (something related to the plugin), but the patch before it works fine.

I’ve changed the volume click to every 8 dB so only have to count 3 clicks now. From what you say about the piezo coming through your headphones with the plug-in suggests something else - the test plug-in drives the piezo (as known at the moment) directly, have you played around with it to see if any command stops the headphone buzz? Without hardware to play with I don’t think I can help anymore, could do with help from an PP/iPod guru. Another thought I’ve had, is what happens when you load the ipod firmware, change its clicker setting and then reboot to rockbox?

The only way I could get it to stop in the headphones was to play in a low volume (like 1) and a lower frequency (higher pitch, if I remember correctly). Not really optimal for a click noise, and it probably faded out because the headphones simply couldn’t handle the tone anymore. I can give you more concrete values if it would help.

Really, I would say this is an issue similar to the beep experienced in the software click… it’s a weird little quirk, but it shouldn’t stop the task from being committed. Maybe adding this to the main build will inspire someone with more knowledge to work out what it is that’s happening (which is what happened when software click was sent out into the wild, I think). I’d be perfectly fine seeing this committed, even with my phantom clicks. That’s just my opinion, though.

So, two final things before I turn in for the evening… turning off the click in OF and rebooting didn’t change anything (unfortunately… it was a good idea, though). Secondly, I’m wondering if you might be able to make me a special patch… one that lets me choose the frequency of the volume click. I keep testing these builds and thinking I’ll know the right number of dB to click to, but I really can’t figure this one out. if it’s possible to make it an option for a patch (one I would NOT want to see in a final commit, just one so it can be tweaked to find the right value for this one), that would be awesome. Maybe it’s just that this volume thing is the last real issue bugging me about this task, but I’d like to get this one just right before I’m 100% behind it being committed.

If a patch for that can’t be easily made, I would say stick with your original click every 6 dB. maybe it’s just me having finally realized that the OF doesn’t do this, but the more I’m using these high number intervals, the less they sit right with me. Might go even lower than 6 at this point. Sorry for being so wishy-washy.

I figured I might as well extend the question again, as to whether anyone has goals this patch must meet before it is truly considered for a commit. For me, the biggest remaining issue is to get a volume click we can all agreee on (I’m once again okay with lower values, 6 was probably fine). I think some of the problem for me is that sometimes it seems like the volume change lags, and volume doesn’t go up/down as fast as usual. I think I sometimes see these as the norm, and the difference between these spaced out clicks and the normal rapid clicks are large. This is probably a gui boost issue. It’s all very complicated, and frankly, if we just set it back to click every 6, or or maybe even lower, I’d be fine with it.

I think that looking back at JDGordon’s initial thoughts on what the patch needed, the only thing still really missing is his number three: “it should be handled in the same way and places as the software click.” Because the piezo click has now (in my opinion) surpassed the headphone click in functionality, it is doing some things differently than the piezo click does. Are these things this patch should work to reconcile? If so, it would make sense to work on it and get that obstacle out of the way. If it is something that should not be addressed by this patch, like I said before, is there anything else that would block this from being committed?

Any chance this could be submitted for Rockbox V3?

obo commented on 2008-08-20 18:38

Rockbox is currently in feature freeze before the release of 3.0 - this means no new features (including this patch) will be added, only bug fixes.

I’ll start the post by apologizing, because I feel like my extensive comments and wishy-washy behavior have contributed to this patch not seeing any activity in some time. I’m not surprised in the slightest that this isn’t making it into 3.0, because as we’ve gone over, it’s not quite done yet (done currently meaning “meets jdgordon’s original 3 suggested criteria”). There’s also the additional question of how many people will use this once implemented, but I think that once it is offered that number can only grow. It’s definitely a nifty feature.

So I guess what I’m wondering, once again, is if any higher-up people would like to revise the “to do” for this patch, to get it up to the level where it can be considered for submission (post-3.0). Anything besides click frequency in changing volume (which should definitely go back down to every 4 db, or something around that, after having lived with the higher one for this time), and possibly getting the features of this patch matching the current headphone click functionality (in this task or a new one)?

Either way, Craig, don’t give up on the patch. You’ve done really solid work bringing this patch to the level it’s at, and I know I appreciate it very much. I hope you’ll be back at some point to show this task some more love. Thanks again.

Yeah please don’t give up on this patch.. I would definitely use this but for 1 thing, on my Ipod Nano 1st gen 2 gb in rockbox it breaks video playback… It says “Cannot create buffer thread”. I do not know if this affects any1 else since noone else mentioned it, but this message is persistent.. I dont really know wat this means? If this could be fixed it would be an awesome patch.. Thanxs in advance for making a working piezo patch.. :)

Confirm the “Cannot create buffering thread!” error at video playback on iPod Color :-(

Yeah, I also got the buffering thread error on my Nano 2gb while using this patch.

Updated for r19748. Also increased BASETHREADS to (hopefully) fix the buffering thread error.

Video playback works now :-)

A must-have patch! Thanx!

Totally Fixed now! thank you! In my opinion, it is ready for inclusion into rockbox permanetly.. Thanks again…

The patch enables the USB stack for ipodnano and doesn’t build for ipodmini2g. Haven’t checked if other ipod models are affected. Updated patch fixes the two mentioned issues. Make the file use unix line endings while I’m at it.

I love This patch, thanks again… Hmph, its out-of-date again, though, with a hunk error, may someone please fix this wonderful patch again please..

Would be amazing if somebody could fix it…. Tried yesterday but I don’t understand all the syntax even though i can program in delphi^^
+ my first patch ever :D But from what I understand the error is in “settings_list” at some point :)
Greetz

I’ve updated it for r20564 - the main patch is FS5111-piezo-r20564.patch and I’ve included a 2nd version which alters the headphone click to match the piezo click behaviour with regards volume and list sounds.

Nice! I’ll test this out when I have a spare moment. I remember that headphone and piezo click working the same was one of the hurdles for committal, so perhaps we’re one step closer…

Thanks Craig!

oooh!!! very nice, thank you.. The Headphone Keyclick works perfectly now (even if its a little painful, i dont know, the sounds a little high pitched i guess) and the speaker works again.. Thanks a lot.!

The patch still apply to firmware r21947, but it breaks the simulator. Probably some ifdefs are missing.
It works very fine, the note in last Glenn’s comment is still valid :-)

I’ve updated the patch to include checks for the defines SIMULATOR and HAVE_SCROLLWHEEL, so it shouldn’t have any negative effects on other target builds or the simulator build, but admittedly I haven’t had chance to try it yet. This patch introduces the new click behaviour (for lists and volume) for those targets with a scrollwheel. What work would be required for this patch to be committed?

I will try it asap.
Thank you for the work.

Unfortunately there are still compilation errors (miss some header?).

CC apps/gui/option_select.c
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1488: error: LANG_KEYCLICK_SOFTWARE undeclared here (not in a function)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1488: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1488: warning: (near initialization for (anonymous)[0].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1488: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1488: warning: (near initialization for settings[167].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1492: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1492: warning: (near initialization for settings[168].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1499: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1499: warning: (near initialization for settings[169].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1507: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1507: warning: (near initialization for settings[170].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1511: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1511: warning: (near initialization for settings[171].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1515: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1515: warning: (near initialization for settings[172].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1533: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1533: warning: (near initialization for settings[173].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1536: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1536: warning: (near initialization for settings[174].<anonymous>)
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1539: warning: missing initializer
/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/apps/settings_list.c:1539: warning: (near initialization for settings[175].<anonymous>)
make: *** [/home/asettico-9.04/Sviluppo/rockbox/rockbox-2009-07-22/build_sim/apps/settings_list.o] Errore 1

There is also a fuzz factor 2 when applying the patch (I normally don’t mind the simple line offsets).

patching file apps/action.c
patching file apps/action.h
patching file apps/features.txt
patching file apps/gui/list.c
patching file apps/lang/english.lang
patching file apps/main.c
Hunk #1 succeeded at 112 with fuzz 2 (offset 2 lines).
Hunk #2 succeeded at 612 (offset 4 lines).
patching file apps/menus/settings_menu.c
patching file apps/settings.h
patching file apps/settings_list.c
patching file firmware/export/config-ipod4g.h
patching file firmware/export/config-ipodcolor.h
patching file firmware/export/config-ipodmini.h
patching file firmware/export/config-ipodmini2g.h
patching file firmware/export/config-ipodnano.h
patching file firmware/export/config-ipodvideo.h
patching file firmware/export/thread.h
patching file firmware/SOURCES
Hunk #1 succeeded at 943 (offset -9 lines).
Hunk #2 succeeded at 962 (offset -9 lines).
Hunk #3 succeeded at 980 (offset -9 lines).
Hunk #4 succeeded at 998 (offset -9 lines).
Hunk #5 succeeded at 1050 (offset -9 lines).
Hunk #6 succeeded at 1069 (offset -9 lines).
patching file firmware/target/arm/ipod/piezo.c
patching file firmware/target/arm/ipod/piezo.h

HTH

A second attempt - I’d misplaced a check for the define in settings_list.c

The simulator builds perfectly, now. Thank you.
Just for my curiosity, I did not see the differences between the earlier FS5111-piezo-with-ipod-soft-click-r20564.patch and FS5111-piezo-r21997.patch.

Thanks Craig, I’m charging my ipod at the moment, but I just compiled a build with this patch, so I’ll be taking it for a test spin in a few hours. I’ll let you know how she’s running. New patch gave me no errors, for the record.

Wall of text, as usual. My apologies if I repeat myself near the end.

Just to get it out of the way at the start:
-I would make clicks a bit more frequent in volume changing, still (yes, I have a feeling I’m never going to be satisfied…).

(Possible) bugs:
-Clicks can get out of sync when both are enabled, headphone click comes first (only seems to happen in scrolling, not in button presses). During playback, clicks are in sync (whether changing volume or navigating). I don’t know how big a deal it is, since I at least will not be using both at once, but I figured I’d mention it.
-If music is paused (not stopped) only speaker click happens (volume change or navigation). Stopping the music brings headphone click back to navigation.

Most of my thoughts have to do with click repeat, whether it be possibly implementing some of its functions into the non-repeating behavior, or working out what might be possible kinks in its present functionality.

-Would it be objectionable to make the click repeat behavior when fast forwarding/rewinding the norm (as in, click repeats even with click repeat off)? It might just be me, but that behavior is the one I expect when doing those actions, as the slow-to-fast clicks remind me the these behaviors accelerate over time, and can help give me an approximation of time skipped without having to look down while driving.
-On a similar note, I had another behavior in mind, but I don’t know if it’s even possible… when fast forwarding through a song, the behavior does speed up, but as you are nearing the end of the song, the behavior slows again. Would it be possible to have the clicks slow down as well? I don’t know how the repeating click behavior works at the moment, so like I said, that may not be possible. It would be cool if it is, though.
-Possible weird behavior… on the main menu (possibly in some other places), a tap of the right button will open the menu in question (same as using the select button, not a bug), but a long press to the right will not open those folders (also not a bug, to my knowledge). In this case, if repeat click is off, a click will sound (which makes sense, since the click happens on the button press, rather than the action, so unless clicks can be made to happen on an action rather than a button press here, that’s no big deal). The odd behavior I’m referring to is that if repeat click is on, the player just sits in the main menu, clicking away. Actually, the same behavior happens when you hold down said button and fast forward to the end of a song, as well. in situations like these, is it possible to cancel clicking, to show that navigation is not happening/the song is over (or you’re back at the beginning), respectively? Another possible use for this behavior would be so that holding down play to shut down the player won’t make this same rapid clicking occur. I may be not recognizing the reasons for the click repeat behavior, again, but it seems like the only times one should hear repeating clicks are when rewinding/fast forwarding, or when backing out of a series of options/directories (note, I’m speaking here specifically of the repeating clicks currently only available with this option on… obviously a series of clicks should be heard when changing volumes, scrolling through lists, etc).

To summarize that last part… and keep in mind this is brainstorming, and completely open to both opinion and feasibility…

Rapid keyclicks should occur when:
-starting fast forward/rewind (slow then fast, and if possible back to slow at the end)
-quickly navigating backward through a series of directories/options (holding left)
These should stop when:
-reaching the end/beginning of a song
-going as far back in the directory tree as possible
Multiple clicks should not happen when:
-holding down play
-holding down right button while navigating (since folders/options cannot be navigated this way)
-holding down select (for context menus)
-holding down menu (for quickscreen)

Essentially what I’m suggesting is, if these changes were doable and desirable, what we would have is a smarter keyclick behavior that yes, does have repeats, but in a smarter way. If this could be implemented, I would imagine it replacing the current repeat click functionality, and possibly would default to on. Turning it off would offer the current non-repeat behavior. This is, of course, something that would have to be discussed, as I seem to recall this keyclick feature is shared between many targets, and the things I’ve suggested here may not work well for non-ipods. Still, I’d like to know people’s thoughts, particularly Craig and any devs.

Finally, thanks again to Craig. Thank you for keeping up with this patch, and although my mini discussions with myself and apparent inability to be satisfied with the volume click behavior, I hope I’m being of some service on this patch.

Oh, and just so we’re clear, the volume click frequency is absolute bottom of the list in importance to me, at the moment. I just thought I’d mention it offhand. If I was going to adjust it I’d only make it slightly more frequent at this point, but I certainly wouldn’t hold this patch back from committal because of it, or anything.

Since I have that great power :P

Sorry… I had one other outlier odd behavior that I just remembered. If I’m natvigating through the file browser, and I’m in a directory with only one folder/item, moving the scrollwheel clicks once, though there’s obviously nowhere to scroll to. It’s incredibly minor, but if lists with a length of 1 could have the scroll click disabled, it would probably make sense to do so.

Resync to r22406.

No functional changes, yes? I’ll check it out as soon as I’ve updated. Thanks!

Absolutely not, I’m not able to perform functional changes (at least, not willingly ;-) ), because I haven’t enough knowledge about the internals.
I just fixed a failed hunk in the patch.

Heh, totally understandable. The patch works great, no errors, thanks for the sync.

I joined this patch with  FS#10311 , because this one prevents the latter to apply.

Interesting. We had one version a while back that beeped at the end of lists, but it ended up getting removed because the patch stops clicking at the bottom of a list anyway. I could see this being useful for some, though. I’ll be sticking to “vanilla” click, personally.

The cumulative patch doesn’t add the end of list beep. It doesn’t depend on the “Headphone keyclick” setting.
I will try with just  FS#10311 , reports will continue there.

Re-sync to r22699.

I’m a bit confused by what you’re doing. I’m under the impression that you’ve added no features, and in addition you have actually kept the features of fs10311 out of these updates. If that is the case (and please correct me if I’m wrong), why again are you adding the name of said patch to your updates? Am I following correctly that this patch is needed for your other patch to apply? Why does that mean your patch number is added to this patch’s title? And secondly, if all you’re doing is resyncing, why on earth are you bumping the version number?

Sync to r23221.
I did not merge  FS#10311  any more, since it seems that it doesn’t add any effect (should be investigate).

Sync to r23894 (mainly new config file locations/names).
Readded missing piezo.h/piezo.c files.
Doesn’t compile for Sim.

I don’t like the whole apps/ changes. I think it should rather replace (or co-exist since the piezo can’t be heard with headphones) with the existing sw keyclick feature (clicking at the same times etc) instead of implementing a new totally different layer.

St. commented on 2010-01-06 23:22

Hello all,

Firstly, thanks for the great patch, just what I was looking for.

However,

Could someone (as I don’t know how, nor where to start) please sync this with r24191 (current as of right now), currently I’m getting a LOT of complaints whilst compiling.
Was last sync’d to r23894, so I don’t blame it for spitting errors at me.
I’d really appreciate it if anyone could bring this up to date, thanks.

[St.]

Sync’d against 24194.

Bear in mind kugel’s comments though - this is in no shape to get committed currently.

St. commented on 2010-01-11 02:30

This patch needs syncing *AGAIN* :’(

If this patch were to be modified rather radically so as to only initiate hw keyclicks when headphones were unplugged, and if it were integrated with the current sw keyclick settings I believe it would be ideal.

It’s fine as is, but I believe it would stand a higher chance of being commited with the above changes made.
In saying that, I have no idea how to do this.

However, these changes are not necessary, but a re-sync (r24213 at least) is.
Many, *many* thanks to anyone kind enough to do so.

[St.]

I use it when headphones are plugged in, so that would not be ideal for me. I do hope the patch will end up refined to the point where it can be included… only time (and an interested coder) will bring that about, of course.

The piezo patch does not work for me on ipod mini 1G. I mean I can hear “piezo like” clicks in headphones but not from piezo itself.

St. commented on 2010-01-12 02:19

Glenn (DancemasterGlenn),

“I use it when headphones are plugged in, so that would not be ideal for me.”

Software Keyclicks are available for this purpose, I see no reason to have hardware keyclicks enabled when you have the headphones plugged in, as the software keyclick can be heard quite clearly.

I belive that both Hard/Software Keyclick should be available under the same menu, instead of the current menu layout for it.
A compromise could be an “OF Style” menu layout such as:

Keyclick: Headphone/Line-out only

            Piezo only
            Headphone/Line-out + Piezo
            Off
          

*Personally* I think it would be best if the menu was simply Keyclick: Yes/No and hardware keyclicks were disabled upon headphone/line-out detection.
I see no need to have the piezo enabled while headphone/line-out is present as software keyclick already caters for this and it is likely that one wouldn’t be able to hear the piezo in this state (if you have decent headphones that is…), but this is purely personal opinion.

Example: If keyclick is enabled and headphones/line-out is present then software keyclick should be used, if keyclick is disabled and phones/line-out are *NOT* present then (and only then) should the piezo be activated, thus making keyclick available in any state.

The real “value” I see this patch having (upon further development) is the ability to maintain keyclicks regardless as to whether or not headphones/line-out is present.
I believe that blind/visually impared users would recieve the most benifit form this feature, as it would greatly assist in navigating through the menu structure.

[St.]

I find the software keyclick to be an annoying noise, that breaks up my audio experience (particularly when playing my music through car/stereo speakers), and find the hardware keyclick to be (persaonally) soothing, and nice for detecting feedback in the aforementioned cases. Particularly, when using my ipod on my radio show, it would be a bit distracting to my listeners if they could hear me clicking around to find my next song. My opinion is decidedly still personal like yours, but I do think my example is still valid.

Your example menu is perfect, though. I think that was similarly proposed a while ago, and simply never got worked on. I am very much for integrating the behavior to end up in the same menu as the software click, I just don’t want to automatically lose it when I plug in to my ipod. I’ve been following the patch for quite a while now, and I hope it will eventually get picked up again by a kind soul (preferably that person will look into some of the points I’ve made before regarding possible changes to the “keyclick repeats” option, as well).

Maybe the software keyclick pitch/duration could be fine-tuned to make it less annoying? ;-) It ought to sound as much like the iPod RetailOS as possible, I’d think.

I do think the example of wanting to hear the click locally, but not send it through a PA is useful.

I don’t have an iPod (i.e. no piezo), so can’t comment too much, but is there a reason why someone would prefer the software click over the hardware one? I’m thinking that the option should be simplified down to: Keyclick: Off / Software / Speaker / Headphone (the latter two being piezo).

On targets without hardware clicks, the option would just be On/Off , of course.

An “Auto mode” would probably useful, i.e. piezo when headphones are out, keyclick when they are plugged in.

Soap commented on 2010-01-12 11:13

kugel, saint,
Am I really the one one for which the plug status of my headphones bears no correlation to which type of keyclick I “need”?

First: Hardware keyclicks (at least the pitch/duration Apple used) are non-obtrusive if you are listening to music (low audibility) - I don’t see the reason for any amount of code to do a fancy-smancy “switch keyclick type on headphone event”. Is there much of a case to be made for ever turning off the piezo, much less dynamically?

Second: I’d be willing to wager that more people connect a line-level device to their iPod through the headphone jack than through the dock port. A “piezo when headphones are out, keyclick when they are in” setting does not add value to them, as headphone insertion status does not correlate at all to their ability to hear the piezo.

I meant it as an additional option, don’t worry. I’m not going to do the work anyway (I have no ipod).

And surely deactivating the pieze when it’s not needed makes sense, it at least saves a little execution time (which might positively affect the battery life). The code needed would probably 1 line.

Soap commented on 2010-01-12 11:33

My point (yea, there was an attempt at a point in all that rambling) that I’m not sure how well one can determine (outside of the simple peizo on/off menu item) when the piezo is inaudible/not needed. I was attempting to point out that headphone insertion status is (at least) not as strongly indicative of piezo audibility as the earlier arguments appeared to make it.

Well, it a lot depends on the headphones of course. My samsung yh-j70 has that hw pieze feature too, and I can’t hear it through any of my IEMs.

Resync to r25279 by Hayden Pearce.
Implemented piezo driver for iPod Nano 2G.

This was reported to be broken on iPod Nano 1G but it’s working on my iPod Nano 2G.

It seems that on iPod Video doesn’t work any more.

St. commented on 2010-04-29 12:13

It appears to only work with the iPod Nano2g…

Someone that understands the code, and likes the iPods enough needs to give this patch some _serious_ love.
To finally see this make it into SVN (taking into account the comments re: Menu Structure as above) would be a great addition to the iPod ports IMO, and would add some completeness to the ports.
It is definitely quite obvious that the “click” is missing when first using Rockbox.
It is a real pity that this patch has sat on the shelf for so long now…

[St.]

Probably my fault. I think I scared everyone off this patch with my ramblings. Sorry guys. Just feeling empassioned :\

It’s out of date again. I really like this patch(I am using ipod nano 2g). Can someone update it?

Resync to r27014

I never eared the key click since months on an ipodvideo 5.5g 64 MB.
Is it a hardware problem of my DAP or someone else is experiencing the same?
Thanks for any information.

Apparently it’s broken for all PortalPlayer iPods. I have no idea why/how. Would be nice if some PP expert could figure it out.

Resync to SVN head. Not verified that it works.

I just made some research about my note at http://www.rockbox.org/tracker/task/5111#comment35312 The fw revision r23903 with the patch at http://www.rockbox.org/tracker/task/5111?getfile=21047 is the last that works for the ipodvideo 5.5g.
Starting from r23904 this patch doesn’t work any more.
I hope this can be a useful hint to investigate about the problem, because I would like to see it committed.

Does it not even apply properly to r23904, or does it apply but refuse to work?

I just diffed the patches and the code they’re adding is almost identical.
The most recent one has two unrelated changes (sorry for that), which shouldn’t afffect HW keyclick functionality at all, but I’ll of course clean that up later.
The addition of iPod Nano 2G support also shouldn’t have affected the other targets, so I can’t see an apparent reason why it’s failing on the PP ipods.
r23904 changed some GUI things, which probably desynced the patch, but as it’s working on the Nano 2G, the target-independent part must be working fine.

Can you try the one from http://www.rockbox.org/tracker/task/5111#comment34150 ?
It seems to be missing the piezo.c and piezo.h files, just take those from another version of the patch. The rest of it seems to be ok.
If this one doesn’t work, this would suggest that another change committed to SVN during that time affected the driver in some way.
Or was it actually http://www.rockbox.org/tracker/task/5111#comment35054 that broke it?

In my comments I was a little bit concise, I’ll try to explain.

The patch I mentioned above applies and compiles with no error against r23904, the new menu option appears in the system settings menu, but I can’t hear the piezoelectric click.
I noticed that the patches differs only by some hunks’ context, I thought it was needed to re-sync to svn.

The patches synced to r23894 and to r24194 were my starting points to make a dichotomous search, building fw until I found the first not working revision.
I added the piezo.{h,c} files missing in one patch by hand, otherwise I got an unresolved symbol error.

The patch that introduced the ipodnano 2g is about the same as the other ones, just contexts changes and some new files added.

So I checked the differences between r23903 and r23904, but as I said, I can’t evaluate if something shifty is breaking.
I just noticed some “send_event” removed and reading the phrase “and removes some GUI events which would be better if they wern’t required…” from  FS#10824  description, I asked to myself if some click wheel event is no more sent, so the piezo driver is not executed. But it’s just a weak theory… Why are other ipod models working?

Ahh, got it: The piezo_init() call in main.c was moved to a place where it was #ifdef’ed out and thus never reached.
Fixed that - works on my iPodColor again :-)

Glad to head that :)

What else needs to be fixed before this can be committed?

That it don’t break building simulator :-) (I didn’t try the last patch yet).

Kugel made a comment further above :- “I don’t like the whole apps/ changes. I think it should rather replace (or co-exist since the piezo can’t be heard with headphones) with the existing sw keyclick feature (clicking at the same times etc) instead of implementing a new totally different layer.”

Some of the apps changes implement the ipod style clicking on volume change, and prevent the click at list end - this also affects the software click. Additionally, the change to BASETHREADS is not needed for the ipodnano2g. One question is, should the software click have these behaviours?

Finally works!!! Thanks very much! :-)

Building simulator still gives errors:
/home/asettico-9.10/Sviluppo/rockbox/rockbox/build_sim/apps/action.o: In function `action_do_keyclick’:
/home/asettico-9.10/Sviluppo/rockbox/rockbox/apps/action.c:489: undefined reference to `piezo_button_beep’ collect2: ld returned 1 exit status

IMHO the only way to get this committed is to split it into (at least) two parts:
- one that addresses only the hardware click (aka speaker click) and makes it behave just like the software (headphone) click with all it’s drawbacks (and, rosso, builds for the sim ;-) )
- one that fixes the repeat / list scroll / volume change issues for both clicks

To be able to use both parts together, they may be maintained further in this thread. As soon as the hardware click is in svn, i would move the second part to a new (not iPod specific) thread, where also interested users of other players might stumble upon it.

Is one of the committers out there willing to support us here? The piezo click is the only thing I miss in vanilla Rockbox compared to the RetailOS…

I might have a try at the first part tonight.

Judging from some comments above, i can imagine two ways to implement the settings menu:

Either a single “Keyclick” menu with the following options:
- Off
- Headphones only
- Speaker only
- Both
- Headphones, if plugged, else speaker

Or two menus:
Keyclick:
- Headphones:
— On
— Off
- Speaker:
— On
— Off
— If headphones unplugged

Which style do you prefer?
Feel free to come up with better wording for the options.

In between me, the options “headphones if plugged/unplugged” are quite usefulness, because you can take the headphones off for a while without disconnect them and continue using the wheel, so speaker clicks can be useful.

@scotty: starting from a fresh trunk and applying only this patch, the sim compiles with errors.

I applied the latest patch available in this FS item (the one provided by @scotty) and ran on an iPod Nano 2g.
At first, the fact is that hardware click works very well… but that patch did more than just enabling the HW piezo, it also defined USB_ENABLE_HID for my target (it doesn’t work because the USB is not yet programmable I think), thus showing the USB HID options in the settings and remote_control plugin. But it compiles without errors.
I would live with this (at the end it’s always good to have more options…), but when I try to use the image viewer, the result when opening it is a scrambled time-input screen (like the one you see when setting time, but with the numbers all wrong). Trying to define the settings makes it even worse, and either accepting the scrambled numbers or pressing Menu to cancel ends up with a plugin error.

As I didn’t apply the patch on a clean trunk (I also had other patches applied, the most recent one was FS#10763 and everything worked well until then), I’m not sure if this happens to everyone or if it was just a incompatibility between patches.

(PS: I tried replacing /firmware/export/config/ipodnano2g.h with the original from SVN and building, but USB HID was still activated as well as HW keyclick. Although the patch changes that file, it doesn’t seem like it is defining USB HID though there)

St. commented on 2011-01-08 17:39

firmware/export/config.h slipped in apparently, which defines #define USB_HAS_INTERRUPT, which in turn enabled HID.
remove it, and it will be fine.
Of course, you can always just disable HID on target…but there’s no reason I can see for this patch to suddenly need to define USB_HAS_INTERRUPT, it simply slipped in erroneously a few revisions (of this patch) ago.

[St.]

Thanks for the tip, St. I removed the defines from that file (yes, because apparently the patch has written twice the defines, separated by 6 lines or so) and now keyclick works, but fortunately I don’t get the USB HID options nor errors on the image viewer.

One thing I’d like to do is to change a bit the sound of the speaker to make it less high (in terms of treble), because the current one is a bit intrusive. But everything else is fine :D

The audio-mixer commit (r30097) broke the patch (only the second hunk for apps/action.c) but I can’t fix it as my FW knowledge is very poor.
Can anyone sync it with trunk?
And what about merge it?
Thanks for any help.

Ops, I did a sync, so I post it.
My previous request is referred to this one.
Thanks.

Recently, I reverted my SVN checkout to the default so I had to reapply this patch with the one on the post above. Unfortunately, config.h keeps on slipping in in the patch… I had to remove USB HID manually. Also, a chunk failed when patching, but I fixed it manually by applying the patch… by hand. :)
I took the opportunity to change the beep of the piezo and make it less intrusive, more like the OF keyclick.
Turns out that for 2nd gen. nano iPods, the code for the piezo is in firmware/target/arm/s5l8700/ipodnano2g/piezo-nano2g.c . To change the pitch and duration, play with line 94 of that file.
I set mine to this:

piezo_start(120, 1);

…and it is really close to the OF keyclick. It’s more pleasant to the ears than the values that the patch brings. On that line of code, the first value is the pitch/period (increase the value to have a lower pitch), the second value is the duration (increase to have a longer beep).

I’m now going about adding a setting for changing the pitch and duration of the beep while running Rockbox. If I ever do something useful, I’ll post a diff here.

Recently, I reverted my SVN checkout to the default so I had to reapply this patch with the one on the post above. Unfortunately, config.h keeps on slipping in in the patch… I had to remove USB HID manually. Also, a chunk failed when patching, but I fixed it manually by applying the patch… by hand. :)
I took the opportunity to change the beep of the piezo and make it less intrusive, more like the OF keyclick.
Turns out that for 2nd gen. nano iPods, the code for the piezo is in firmware/target/arm/s5l8700/ipodnano2g/piezo-nano2g.c . To change the pitch and duration, play with line 94 of that file.
I set mine to this:

piezo_start(120, 1);

…and it is really close to the OF keyclick. It’s more pleasant to the ears than the values that the patch brings. On that line of code, the first value is the pitch/period (increase the value to have a lower pitch), the second value is the duration (increase to have a longer beep).

I’m now going about adding a setting for changing the pitch and duration of the beep while running Rockbox. If I ever do something useful, I’ll post a diff here.

MikeS commented on 2011-07-18 22:12

I noticed the patch introduces action_do_keyclick. There’s already code I added in misc.c (system_sound_play and keyclick_click) where system event sounds are centralized, the intent being to play those event sounds in different ways while having a common API.

MikeS commented on 2011-07-18 22:16

Another thing is, why should this need a thread if you have a timer with interrupts?

The intention of having the separate action_do_keyclick seems to have been to change the keyclick behavior, i.e. on which events it will click. This should be separated out into another patch, and it should use the centralized interface for the hardware part.

The thread seems to be needed on older iPod models where the piezo isn’t controlled by a timer, but by <insert weird signal generator thing here>. On the nano2g, I already removed the thread and am handling it using timer IRQs.

Yeah, I noticed that the piezo.c file that contains the code for other iPods uses a thread, but piezo-nano2g.c uses only timers.

Now that there seems to be code for centralized event sounds (great!) perhaps things should be done in a different way. This patch is a bit old IMO, although it still works.

Sync to r30189: men, I did it! :-D A so great step for me! :-P

I also removed the definition of USB_HAS_INTERRUPT in firmware/export/config.h, due to the remark that states “seems to be broken”.

With the recent changes in the event management, the click wheel works better also in WPS.
Thanks!

This is a updated version which only clicks on repeat the first time except wheels. This makes holding actually buttons not be incredibly annoying.

Anyone know why this isnt in svn yet?

edit: reupp to add the missing files

I’m going to commit this pretty soon unless anyone knows why it should be kept out

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing