This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#5111 - Ipod piezo driver
Attached to Project:
Rockbox
Opened by Robert Keevil (obo) - Thursday, 13 April 2006, 01:34 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 16 November 2011, 11:26 GMT+2
Opened by Robert Keevil (obo) - Thursday, 13 April 2006, 01:34 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 16 November 2011, 11:26 GMT+2
|
DetailsFirst attempt at a piezo driver for ipods - works, but needs a lot more done to it yet.
piezo.patch
(5.6 KiB)
apps/main.c | 8 ++ firmware/SOURCES | 3 + firmware/drivers/button.c | 9 +++ firmware/drivers/piezo.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++ firmware/export/piezo.h | 7 ++ 5 files changed, 153 insertions(+) |
This task depends upon
Closed by Jonathan Gordon (jdgordon)
Wednesday, 16 November 2011, 11:26 GMT+2
Reason for closing: Accepted
Additional comments about closing: Holy crap! commited in r30995.
thanks everyone!
Wednesday, 16 November 2011, 11:26 GMT+2
Reason for closing: Accepted
Additional comments about closing: Holy crap! commited in r30995.
thanks everyone!
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.
It also beeps during volume change in the WPS - I haven't yet looked at trying to stop this.
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.
Looks like the piezo commands can be decomposed as:
0x80000000 : 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
Button click duration
Button click frequency
Add check on piezo_button_beep to prevent the queue overflowing - any better ways to do this?
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.
2 - changed the options to a bool only. Retailos pitch and duration may need tweaking.
Maybe a menu option or just to say what and where to edit the patch.
Either way, thanks so much for syncing it. Much appreciated!
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
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?
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.
It does sound different than the standard iPod click, but personally I don't mind it. I might play with it later.
And once again, thanks for taking the time to work on this. It's honestly the only thing I miss about the normal firmware.
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?
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.
-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.
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...
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
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.
As for 2) - I quite like the word Keyclick, I don't think Click "looks" right. My personal opinion.
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.
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).
It's really great to see people this interested in getting this up and running so well! I'm simply overjoyed.
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.
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.
Here it is:
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.
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.
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.
Also, a penny for your thoughts regarding the other stuff I blabbered on about?
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.
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.
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.
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.
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?
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 0x80. I changed the waveform from 0x80 to 0xF0 before when you first said the problem, and reduced the freq from 0x32 to 0x5B (lower freq is an increase in the value). I guess its a case of see if the buzzing comes through your headphones.
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...
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.
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 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?
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.
A must-have patch! Thanx!
+ my first patch ever :D But from what I understand the error is in "settings_list" at some point :)
Greetz
Thanks Craig!
It works very fine, the note in last Glenn's comment is still valid :-)
Thank you for the work.
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
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.
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.
Since I have that great power :P
I just fixed a failed hunk in the patch.
FS#10311, because this one prevents the latter to apply.I will try with just
FS#10311, reports will continue there.I did not merge
FS#10311any more, since it seems that it doesn't add any effect (should be investigate).Readded missing piezo.h/piezo.c files.
Doesn't compile for Sim.
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.]
Bear in mind kugel's comments though - this is in no shape to get committed currently.
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."
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.]
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).
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.
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.
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.
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.
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.]
Is it a hardware problem of my DAP or someone else is experiencing the same?
Thanks for any information.
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.
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?
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#10824description, 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?Fixed that - works on my iPodColor again :-)
What else needs to be fixed before this can be committed?
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?
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
- 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...
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.
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)
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.]
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
Can anyone sync it with trunk?
And what about merge it?
Thanks for any help.
My previous request is referred to this one.
Thanks.
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.
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.
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.
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.
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!
Anyone know why this isnt in svn yet?
edit: reupp to add the missing files