Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Applications
  • Assigned To No-one
  • Operating System SW-codec
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by smoking_gnu - 2008-03-20
Last edited by fml2 - 2009-09-20

FS#8768 - Instrument tuner plugin

This plugin is an instrument tuner.
Currently, there’s no real GUI, it just prints the note you’re playing again and again until you press power.

Here’s the c file:

   pitch.c (6.3 KiB)
Closed by  fml2
2009-09-20 16:31
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

The work in this task has been committed in r22663, r22685 and r22753.

OK, Now I’ve cleaned up the code a little bit.
Should be more readable now; no functional changes.

   pitch.c (9.2 KiB)
Project Manager

Looks nice and simple. However, it violates quite a few of our coding guidelines, so it needs some more cleaning.
Furthermore, it uses quite a lot of floating point arithmetics, which we aren’t too fond of. But I think we can let that pass, since it’s only a plugin.

Good work!

Which coding guidelines does it violate?

Project Manager

It mainly violates the guidelines regarding identifiers. From docs/CONTRIBUTING:

Identifiers


We do not want mixed case identifiers.
Variables and function names should be all lower case.
Struct and enum names should be all lower case.
Preprocessor symbols and enum constants should be all upper case.

New version that does not violate any coding guidelines:

   pitch.c (9.3 KiB)
nls commented on 2008-03-22 14:25

Really nice, many people will love this :)

Another nitpick from CONTRIBUTING though,

“Keep lines below 80 columns length.”

OK, if this version violates the coding guidelines again, I’ll start crying.

   pitch.c (9.1 KiB)
Project Manager

Better start crying then… :-)

schmittTriggered=0;

:-)

*cry*

Well, I added a little bar that shows how ‘wrong’ your pitch is.
I also corrected that guideline violation, and if I find another one, I’ll simply give up ;)

If the played note is loud enough, the tuner is now quite usable.
I’d be very grateful if someone actually tried my plugin and tell me any bugs/suggestions…

   pitch.c (10.3 KiB)

Desowin tried it for me on his e200, and for some unexplainable reason it didn’t even start recording !?!
Could anyone else with an player with a built-in mic try this for me?

MikeS commented on 2008-03-24 05:14

This really needs to have recording used as the example plugins and core use it so that it operates with a periodic callback (ie. not constantly stopping and restarting recording). There’s not much room to vary from those formulas and expect it to operate properly. No parts are optional.

The sample buffer should be 32-bit aligned BTW.

I tried a periodic callback that provides a new buffer for recording, but even after several days I couldn’t get it to work properly: it always crashed after 0-10 seconds with some buffer overflow error, no matter how much buffer I gave it… It didn’t help that there’s no documentation at all, and the few plugins that use it are so complicated that I don’t understand what I have to do…

32-bit aligned would be a signed int, right?

Also, the reason for the plugin not working on sansas has to do with the fact that they cannot record with a 44,1K sample rate - I’m working on it.

Project Manager

I made an intrument tuner myself using the “proper” method of recording. I’ll see if I can apply my recording code to your pitch detection algorithm.

Surely, why not?

New version, much better gui. Still doesn’t work on Sansas though..

   pitch.c (15.5 KiB)

I uncommented
print_int_xy(0,160,(int) *rb→rec_freq_sampr);

     print_int_xy(0,200,(int) SAMPLE_RATE);

and both are reported as 22050 on the sansa e280 not 88200 as mentioned in the comments?

Really? I could only test this in the simulator since I don’t own a sansa… Does the actual tuning work too? Does the GUI display correctly on the sansa’s screen?

@MikeS: The sample buffer is now 32-bit aligned → int32_t.

The tuning doesnt work on the sansa - but its not down to the samplerate
The GUI does display correctly - and looks good

Strange… Could you put a “rb→splash(0, “callback!”);” into recording_callback() for testing purposes?
If recording works, it should display “callback” every second or so. If not, recording does not work at all.

The callbacks are working- the splash pops up every sec or so

some observations 9on the sansa):
tc is always 0 hence freq is 0

If I exit pitch and go to recording, the mic is not responsive. If I then exit
recording and restart it, the mic responds.

This suggests something in the code is preventing the mc responding to input

My main problem is that there’s no documentation at all, nowhere!
There are some plugins that use recording, but they just use some functions and do not explain what they do exactly… I know for sure that I’m doing several things wrong, but nobody actually helps me fixing those problems.
So until I get some more information, I’m afraid I can’t help you.
It seems to be working on most targets, and I don’t own a sansa to “reverse engineer” the recording api.
Thanks for testing it nevertheless!

Project Manager

I know I promised to help you with the recording code, I just haven’t had time yet. Sorry about that. Regarding the documentation, it’s a sad fact that we lack plenty of documentation on the Rockbox internals. We try to keep up as weel as we can, but it is hard enough to find the time for the coding itself, and the documentation always has lower priority.

I got recording working on the sansa by changing the gain values from 100 to 39 (the range is given in as3514.c as 0 to 39 with 23 default)

rb→sound_set(SOUND_MIC_GAIN, 39);
rb→audio_set_recording_gain(39,39, AUDIO_GAIN_MIC);

Im not convinced that the sound_set()ine is actually doing anything. I think the gain
is being set by the audio_set_recording_gain()

recording now works although the pitch indication jumps around a bit. maybe some averaging would help.

Good to hear! I only added sound_set() because some other plugin used it… I can’t work on the plugin right now because cygwin doesn’t work anymore and I can’t get the toolchain installed on AndLinux.
The ‘jumpiness’ of the tuner is due to the fact that the Schmitt trigger algorithm only works reliably when the tone is quite loud.
So if you play a loud note, it shouldn’t jump so much. I intend to fix that when I can compile again.

Tested on iaudio X5.
Detection doesn’t work too well using built-in microphone though;
[either that or my guitar is way off-tune ;-) ]

here is a version that works with sansa

I have removed rb→sound_set(SOUND_MIC_GAIN, 39);

gain is set at 39 instead of 100 as per as3514.c
rb→audio_set_recording_gain(39,39, AUDIO_GAIN_MIC);

I added some code in display_freq() that prevents the red bar set at -32 cents when freq=0 (no signal)

if ((int)freq == 0) /* prevents red bar at -32 cents when freq= 0*/

{ ldf = 0;
}
   pitch.c (15.8 KiB)

It wouldn’t compile unless I change line 500 to

enum plugin_status plugin_start(const struct plugin_api* api, const void* parameter)

I added and changed a couple things to get this to compile for my iHP-120, it has a grayscale lcd. Hope I didn’t mess anything up.

   pitch.c (16.7 KiB)

Is anyone still working on this? I’d LOVE to have a tuner in Rockbox, but I tried compiling the most recent patch (posted by Jason Lyons) and couldn’t get it to work correctly in the simulator (so I was timid about trying to put it on my h120).

I’m not working on it anymore. AFAIK there were some changes to the plugin API, so you’d have to adapt this plugin. It shouldn’t be too hard, actually.

So the idea of this plugin excited me enough that I actually got off my butt and modified it to work better. Presented here is my version, with a new, much more effective pitch detection algorithm, as well as a few other improvements.

I would be eager to hear if it works for everyone else. I’ve only tested it on my h120 but I imagine it’ll work on other platforms either out of the box or with very little modification.

Some details: I used the Yin pitch detection algorithm, adapted from the implementation in the Aubio sound processing library. This improved accuracy, sensitivity and latency significantly over the Schmitt algorithm. Now I’m getting a little over two seconds of latency when no sound is playing (which could probably be eliminated by implementing a simple magnitude detection routine on the input stream) and between .7 and 1.2 seconds when playing a sound, with higher frequencies being worked out faster. I think speed could be improved by a factor of three to four by converting to fixed-point math. I made an aborted effort at that but I think my 16.16 data type had insufficient precision. I may try again with a different division of bits, or even go up to 32.32 for the real brute force approach.

The Aubio library also includes an FFT version of Yin, which would reduce the complexity from O(n^2) to O(nlogn), significantly reducing latency. I made an initial foray into that code but it’s pretty convoluted, with lots of data structures that are more complex than we need for this application, as well as dependence on an external FFT library. So that’s more than I really want to figure out, but it might be a good project for someone else who wants to see this plugin improve.

I also made the drawing routines a little more general-purpose, making them properly detect whether they were being compiled for a color, greyscale or B&W screen.

   pitch.c (18.1 KiB)

One thing I forgot to mention is that due to latency issues i kept the sample window down to 1024 samples, which means that with a 44.1kHz sample rate the lowest detectable pitch is about 86Hz, or about an octave and a half below middle C. This should really be fixed for the tuner to be as useful as possible. One possibility would be to operate only on every second sample, or even on every fourth sample, reducing the maximum detectable frequency (not really a problem, since it’s currently ~20kHz, way higher than any musical instrument goes)

fml2 commented on 2009-08-20 22:13

Here’s your work as a patch. The patch of the SOURCES file should be probably corrected. I added the new plugin for all platforms.

fml2 commented on 2009-08-20 22:35

Protect the plugin by HAVE_RECORDING; use cpu_boost only when it’s available; this plugin is an app; renamed the plugin to “pitch_detector”.

Thanks for making those changes – I had done the same on my local system to get it to compile with the rest of the build but I didn’t get around to uploading a patch.

I agree that the plugin should have a different name, though I don’t think pitch_detector is really the best option. This is really designed to be an instrument tuner first and foremost, mimicking the functionality of a digital tuner that you’d buy at a music store. I think I’d prefer it to be called “tuner” (which might be ambiguous to someone seeing it in the list of apps) or instrument_tuner (which might be inconveniently long).

Has anyone else had a chance to try it out on another platform? It would be nice to know if it’ll actually compile and work on other devices, and whether my impression that it’s a better algorithm holds out in other people’s experience.

fml2 commented on 2009-08-21 23:52

Hrm… I have a feeling that the plugin shows half the real frequency. Could anybody confirm?

Also, this plugin fares poorly with a large font. I use 19-Nimbus on my e200, and the text was overlaid in multiple places.

Re half-frequency: interesting. I’m not sure why it’s doing that but it should be easy to fix.

Overlaid text: I’ll look into it. That’s mostly stuff that was already there when I came to it, but it doesn’t seem like it should be too hard to fix.

Okay, I haven’t addressed the overlapping text and the octave problem yet, but I’ve successfully implemented several efficiency improvements. First, I got fixed-point working (currently it uses a 12.20 format), which greatly improved latency. It’s only implemented in the pitch detection routine at the moment, but I plan to convert all the floating point operations to fixed point eventually. Pitch detection is the important part, and I’ve managed to reduce average latency to below 1/10th of a second on my h120.

Second, I implemented a volume check so that it’ll only do the grunt work of detecting the pitch if the input is louder than a given threshold. This eliminates the longer delay when no pitch is being played, and makes it respond more more quickly to a button press to exit the plugin. Overall It feels WAY more snappy now.

With the fixed point math there are occasional glitches in the pitch detection. I’m not sure what causes them, but I would guess it has to do with the reduced precision. I have some ideas of how to improve that, though – I don’t think the fixed point code is at its optimum setup for the values involved. I’m much more familiar with the algorithm now, so I have a better idea of what range of values its going to be dealing with.

Still left to do:
1. Convert the rest of the floating point to fixed point.
2. Optimize the cpu_boost and backlight so they only come on when it hears a sound above the volume threshold
3. Improve the recording code to work simultaneously with the main loop
4. Add a menu system where you can control mic volume, volume threshold, Yin threshold
5. Fix the text overlapping problem
6. Fix the octave-off problem
7. Improve the display
8. Eliminate glitches in the detection code

Okay, lots of cool changes in this one. I’ll address each issue by number from my last message:

1. Done. There’s not more floating point in the plugin.
2. Done. In combination with the previously-implemented volume threshold it should be much nicer to your battery life now.
3. Not done.
4. Not done.
5. Done. I pretty much fixed it by eliminating the latency readout. That was pretty much just for debugging anyway.
6. Done. The octave problem exists because the recording routines were providing a stereo stream but I was reading it as if it were mono. On a related note, it’ll now detect down a further octave from where it did before.
7. Tweaked just a little, but could still use some changes. In particular I’d love there to be a large readout of what not is being played, rather than the current block-below-letter method. Anyone know anything about using bitmaps and how to include them in the build?
8. Done, I think. It seems quite usable at this point, at least on my h120. Again, I’m eager to hear of other people’s experiences on other devices.

Unfortunately it still won’t detect low enough to tune the lowest string on a guitar, so I’ll probably up the sample size a bit. It may be too much to ask to have it be able to tune an electric bass, though. On the other hand, the sample size could be tweaked through a menu system (up to a maximum based on static buffer allocation) so that lower instruments could be accommodated.

One problem currently is that it isn’t precisely correct when detecting a reference tone. That A440 on Wikipedia shows up as somewhere between 337 and 440, usually 4 to 8 cents flat. This may be a rounding or precision problem, or there may be something else at play that I’m not aware of. One possible solution is just to do some calibration and introduce a fudge factor. Part of the uncertainty on this, of course, is wither the same thing happens on other platforms. If anyone gets a chance, please post your results of detecting an A440 reference tone.

So here’s what still needs to be done:

3. Improve the recording code to work simultaneously with the main loop.
4. Add a menu system where you can control recording volume, recording source, volume threshold, Yin threshold, and sample size (lowest detectable note).
7. Improve the display.
9. Lower the lowest detectable note.
10. Calibrate it to properly detect a reference tone.

On the sansa, the display blanks out as soon as sound no longer detected
I fixed this by commented out the line rb→backlight_off();
Is this a problem on any other platforms?

Oh, I forgot about that – that there are some platforms on which you can’t see anything if the backlight is off. I suppose the solution for that would be to have a backlight timeout. Or maybe that’ll just be taken care of if I don’t repeatedly tell the backlight to turn on? I’ll do some experimentation.

I believe there’s a define on targets that can’t be read without backlight. I don’t remember what it is though. Or maybe it was in a patch that didn’t get added. I can’t find it now…

It turns out that yes, the backlight will turn itself off automatically after the backlight timeout period, unless backlinght_on() is called. Looks like commenting out rb→backlight_off() was exactly the right thing to do.

fml2 commented on 2009-08-30 08:11

There is a symbol called HAVE_TRANSFLECTIVE_LCD. I’m not sure whether it’s what you’re after though since, for example, on H120 you can also read the display without backlight. And there is a function rb→backlight_on() that overrides the backlight setting (for the duration of the plugin) and sets it to ‘always on’.

Actually, I’m already using rb→backlight_on() and it seems to just trigger the backlight, but the timeout still happens at the appropriate time. I don’t think it’ll be necessary to know whether the screen is visible without the backlight since it seems to follorw the default behavior without a problem.

fml2 commented on 2009-08-31 15:06

Ah, I meant the function “backlight_force_on” from the plugin lib.

Okay, more improvements. Regarding the remaining bullet points:

3. Still not done
4. Done mostly. Recording source still isn’t selectable but everything else is.
7. No further progress
9. Done. The default setting will now detect a note as low as 43Hz. It’s also adjustable so you can get lower latency if you’re only tuning higher-pitched instruments.
10. Not done. I’m still hoping to hear if other people see the same frequency discrepancy when detecting reference tones like the one linked above.

I also did some other improvements:

- I’ve been trying to get things to be programmed in a more generalized way. I’ve been able to get rid of all the platform-specific #ifdefs without compromizing compatability. Through the use of he appropriate functions and defines I’ve been able to eliminate special cases (apart from color-vs-grey-vs-b&w) and improve the ability of the plugin to play nice with the hardware.
- I adapted a binary log function to replace the one being used before, resulting in better precision of various calculations.
- The plugin saves its settings between sessions.
- The backlight now correctly obeys the backlight timeout period.

Still remaining to be done:

3. Improve the recording code to work simultaneously with the main loop.
4. Add the ability to select a recording source via the menu.
7. Improve the display with a larger note readout.
10. Calibrate it to properly detect a reference tone.
11. Allow adjustment of some settings with arrow buttons (without having to go to the menu).

I used Sine_wave_440.ogg to check the tuning on the sansa e280 for the latest dif (1st Sept)
The indicated tuning was -17cents(872Hz)
so, it indicates flat, but also indicates an octave high

I checked back with the previous diff 26th August
on the sansa that produces -17cents(436Hz) ie correct octave

I think what happened is that #define SAMPLE_RATE HW_SAMPR_DEFAULT is being set as 44100
rather than 22050 as required. If I set explicitly to 22050 then the latest diff gives -17cents(436Hz)

I hardcoded the following in display_freq(fixed freq) to calibrated out the -17 cents error in my sansa:
freq=fp_mul(freq,float2fixed(1.00986796f));

sansa now reads spot on :)

the factor 1.00986796 represents +17 cents as follows:
1 semitone = 2^(1/12)
1 cent = 1semitone^(1/100)
17cents= 1cent^17
A table of appropriate values for +/- x cents could be added to the menu system

Project Manager

I think the most important question is why it is wrong in the first place.

How wrong is it with an 880Hz tone, or let’s say 300Hz?

Here’s a page of test tones that may be helpful. I’ll give them a try this evening and see how the tuner fares with them: http://www.eminent-tech.com/music/multimediatest.html

I’ve made some progress with figuring out the of-by-n error. After taking some measurements and doing some calculations with them, it seems that the error is relative to period, not frequency. It seems to be consistently adding just under half a sample to each detected pitch period (resulting in a lower pitch readout and greater difference in higher pitches). I haven’t figured out the cause yet but I suspect it’s from one of the support functions of the Yin algorithm. I’ll check over them and see if I can find anything.

I think the pitch error is fixed (without resorting to a kludge, no less) and I’ve made the sample rate platform-independent (For real this time. I think). I think it might be in a good state to add to the main build. Anyone have any thoughts? There are still a few improvements that could be made, but I think it actually works quite well at this point, and perhaps those improvements could be left for a subsequent patch?

Also needed would be an update to the manual, of course….

Nice job David
I can confirm that it works well on the sansa e200 with 0 cents offset at 440Hz

fml2 commented on 2009-09-05 07:33

I’ll test the patch on H120 and Sansa e200 and will report any issues. If it will be OK then we just have to provide a chapter for the manual. And then I’ll commit this. I’m not sure though whether we should commit this for all players or only for H120 and Sansa e200. Of course it should compile on all platforms. I think even if it doesn’t actually work on some platforms we should still commit it for them so that it’s easier for people to test it.

Another question: is the #if in the file SOURCES correct? Don’t we have some players that are capable of recording but can’t run this plugin because they do everything in a dedicated chip (i.e. non SW_CODEC targets)?

Nice job David
I can confirm that it works well on the sansa e200 with 0 cents offset at 440Hz

apologies for double post

nls commented on 2009-09-05 09:16

yeah the inclusion condition in SOURCES needs a tweak, this will not work for hwcodec targets and there are recording capable targets that have no line in or mic and i don’t know if this makes any sense for those (the gigabeat s for example).

so maybe something like this would be better?
#if CONFIG_CODEC==SWCODEC && HAVE_RECORDING && (HAVE_LINE_IN || HAVE_MIC_IN)

I’m not so happy with the typedef’ing, it’s against our coding guidelines. Is a struct needed at all (it has only 1 member)?

(I’m fine with typedef’ing simple data-types, but not really with structs)

Otherwise, it looks really nice.

fml2 commented on 2009-09-05 11:38

I tried the plugin. It works well. I have some notes:

1. The default threshold volume is set too high IMO (not a problem at all)
2. On Sansa e200 and with 19-Nimbus, the cent scale is drawn so that it looks like an underscore line for the numbers
3. It would be nice if pressing LEFT while in the plugin’s menu would return to the tuner (same as the first menu entry)

But you’ve done an awesome job!

fml2 commented on 2009-09-05 11:40

I also corrected a typo (AUDIO_SRC_LINE → AUDIO_SRC_LINEIN). It’s in a comment, but still. And I also changed the #if in the SOURCES file. Attached is the patch.

Wow, thanks for all the comments!

Thomas Martitz: I can understand hesitance about the typedef. I did it that way because I wanted to protect myself from accidentally performing an integer multiply or divide on a fixed variable. That would, of course, produce incorrect results, so I wrapped it all in a struct so the compiler would keep track of it for me. I’m sure it prevented quite a few bugs in the development process. I’m a big believer in doing things like this (probably because I’m mainly an object oriented programmer). If there’s another way to accomplish that protection I’d love to know about it and I’d be happy to use it.

Is it really against the coding guidelines? I guess I haven’t read them in a while, but I’m not sure what’s wrong with it. I did it that way to isolate the implementation details of the type – so I didn’t have to say “struct fixed” every time I declared a fixed-point variable – so I could treat it just like a normal data type as much as possible. It caused the annoyance of having to use macros in place of operators (oh, if only I could use operator overloading!), but it also made it much easier to change the internal representation at a later stage if desired (for instance, switching to a 32.32 fixed point representation).

All that said, it would certainly be easy to switch it from a struct to a plain signed integer representation. I would just fear that in doing that we’d be opening the door to future uncaught instances of improper operator usage. Am I being too anal?

Alexander Levin:
Would we only commit for h120 and e200 because those are the only ones that have been tested? Surely there are people out there who could test it on other platforms if asked (say, a simple test routine of starting the plugin, comparing the output when listening to the a440 test tone and testing the menu a bit).

And your subsequent message:
1. I wasn’t sure how to deal with the default volume level and threshold. I didn’t have anything better to choose, so I just set it to maximum volume and an arbitrary number for threshold. I welcome other ideas for default numbers for those.
2. Perhaps the text should be moved up a few pixels. That wouldn’t be problematic at all, and may become irrelevant if my large-letter readout idea ever becomes a reality.
3. Yes, I agree. I guess that should probably be addressed before committing (from an obviousness-of-controls standpoint).

You don’t need to repeat the pros of typedef’ing. I’m just saying it’s against our coding guidelines (see docs/CONTRIBUTING).

fml2 commented on 2009-09-06 09:42

David,

to 1: It’s not a problem at all. I just noted that the threshold was too high for my guitar, but that daoesn’t have to mean anything
to 2: why not compute the Y dynamically based on the font size?

Project Manager

Regarding the typedefs: We in the Rockbox project are firm believers of KISS. We prefer the code to be simple, without unnecessary abstractions. That means that we like to see the data type directly in the declaration, and not having to look up a typedef somewhere else. If it is a struct, we want to know that when we read the code.

fml2 commented on 2009-09-07 13:59

Here is a patch that also returns to the tuner plugin when LEFT is pressed in the menu.

Oops. I’ve been working on some other changes as well. Can you point out where you made changes and I’ll integrate them manually?

fml2 commented on 2009-09-07 20:21

I just added the default clause in the switch statement after the call to “do_menu”. I also moved “case 0” to the bottom of the switch so that “case 0” and “default” do the same.

I’m also working on the plugin – trying to fix the GUI, i.e. make it independent of the font size. We should cooperate (on IRC?) to minimize merge work.

fml2 commented on 2009-09-07 20:37

Here’s a patch that makes drawing independent of the font size. All GUI related constants that were present are now replaced with values computed during the plugin initialisation (based on the font dimension)

I’m working on some fairly major modifications to the GUI, including font-size-sensitivity and resolution-dependent placement of widgets. I would guess that you and I have been doing some of the same things. I’ll be ready to upload within the next day or so.

Oh, I just saw your sentenced that mentioned communication. Yes, that would be good :)

What hours are good for you on IRC? I’m kind of limited as to when I can work on this stuff – mostly the morning and late evening (California time).

What I’m working on right now is to add in a larger readout of the note that’s being played (using bitmaps) and adding in a couple gui-related menu items (whether to display flats or sharps for accidentals and an option to display (or not) the Hz readout (it’ll probably be turned off by default).

Another thing that I just recently thought of that this plugin really should be able to do is to change the pitch calibration. A first step would be to have a menu item to adjust where A is (instead of 440Hz), but it would also be cool to be able to adjust up or down some number of steps (good for tuning non-concert-pitch instruments like clarinets, trumpets, saxophones, etc).

fml2 commented on 2009-09-08 09:20

David, I think before we implement some fancy features in the plugin, we should commit it in its current form. Then we’ll have two advantages: people will be able to test it and it will be much easier to make changes (less conflicts and merging).

That’s a good point. Maybe we should just commit what we’ve got now? Or should I post the changes I’ve made? They do make the UI significantly nicer.

fml2 commented on 2009-09-08 16:47

I committed what we have until now in r22663.

Okay, cool. I’ll start a new patch for the changes I’m working on now.

fml2 commented on 2009-09-08 18:12

You can start a new patch or you can, if you wish, continue to contribute patches in this task. I left it open on purpose ;-) Do it as you feel is more appropriate. And thank you for the great work!

First of all: amazing progress - the plugin seems quite usable now (compared to my original work… ;)
But while I indeed created the plugin and did some basic proof-of-concept stuff, it was actually David Johnston who did most of the hard work: therefore I think his name should be added to theCREDITS too.

fml2 commented on 2009-09-08 21:25

This is not the first contribution by David Johnston, so his name is already in the CREDITS.

Here’s a new patch that greatly improves the UI, making the layout entirely relative to the screen resolution and having a nice big readout for the detected note. I also added a few menu options including an option to turn off the Hz readout (I’ve set it to be off by default) and to use flats instead of sharps for accidentals.

Note that this patch won’t run without the image files I’m going to include in my next comment.

These are the image files for the above patch. These files should go in apps/plugins/bitmaps/native

Oh, and one more thing. I haven’t tested this patch on platforms other than the h120 but I’ve included bitmaps for all the screen sizes I think are possible. Some of them may need some tweaking in terms of size or something like that. Let me know how it works for you.

I also tweaked it so it’ll once again display the UI in the simulator, though the pitch detection still won’t work. It just displays a dummy frequency.

I get the following errors when compiling using pitch_ui.diff on sansa
$ make
CC apps/plugins/pitch_detector.c
/cygdrive/c/sansa/rockbox-21932/apps/plugins/pitch_detector.c:219: warning: excess elements in struc
t initializer
/cygdrive/c/sansa/rockbox-21932/apps/plugins/pitch_detector.c:219: warning: (near initialization for
‘note_bitmaps[0]’)
/cygdrive/c/sansa/rockbox-21932/apps/plugins/pitch_detector.c: In function ‘init_everything’:
/cygdrive/c/sansa/rockbox-21932/apps/plugins/pitch_detector.c:1130: error: ‘struct picture’ has no m
ember named ‘slide_height’ make: *** [/cygdrive/c/sansa/rockbox-21932/build/apps/plugins/pitch_detector.o] Error 1

Any thoughts?

fml2 commented on 2009-09-11 12:05

I like you patch! The error bar seems a bit too large on Sansa e200, but OTOH it’s easy to see :-)

I made small improvements:

1. The lower horizontal line was drawn too low so that it wasn’t visible
2. The last vertical line wasn’t drawn (but the zero line was drawn twice)
3. The zero label was placed a bit to the right. I now differentiate label and bar x-coord.

Attached is the patch that fixes this.

Re compile errors: are you sure you have the most recent revision? slide_height was added very recently, I think.

Steve: are you sure you have the most recent revision? slide_height was added just a week ago, I think.

Alexander, thanks for those changes. It’s definitely better. I hadn’t gotten around to tweaking it so those edge lines would remain visible.

I can see how the bar would be kind of big on any screen that’s taller than it is wide. Perhaps the bar height should be relative to the minimum of the height and width? Here’s a patch for that. I tried it out in the e200 sim and it looks pretty good. What’s missing, though, is transparency on those note letters. Or maybe the whole screen should just be blanked out white instead? Any opinions?

I downloaded pitch_detector.c committed in r22663 then patched using pitch_detector5.diff
Ive attached the resulting file, which gives me the errors I noted before.

fml2 commented on 2009-09-12 13:57

David, I like the change. On Sansa the error bar was indeed a bit too high. I removed some now unused functions and simplified the structure with the pictures (no need to use an array of structs there). See the attached patch.

IMO we don’t necessarily need transparent bitmaps for the notes. On Sansa, a dark letter on white background looks quite OK.

@Steve: the file you attached compiles just fine here.

Steve: the errors you mentioned are from changes external to pitch_detector.c. In my changes to the UI I used external bitmaps, and at some point while I was developing them I had the same issues you mention – the bitmap interface changed so “struct picture” now includes the member ‘slide_height’. In order to compile correctly you need to update the rest of your Rockbox source code to the current version, not just pitch_detector.c.

Okay, I just took out some extraneous calls to lcd_update(), which should help speed things up a bit (depending on how slow lcd_update() is). It’s now updating the display only once per iteration, which eliminates the flashiness in the simulator as well.

fml2 commented on 2009-09-12 21:51

I’ve committed what you’ve done so far in r22685. Keep it going! :-)

Thanks David and Alexander
Im up and running now
I was downloading the code from daily build http://www.rockbox.org/dl.cgi?bin=source Although the changelog showed the pitch detector changes for eg r22685 the 7zip source didnt contain the required files… so I was manually downloading and adding pitch_detector.c and the compile failed no doubt because the bitmap interface
of the toolchain wasnt up to date. Not sure why the daily build is having this problem….

I downloaded this time from svn and all is well

Thanks David and Alexander
Im up and running now
I was downloading the code from daily build http://www.rockbox.org/dl.cgi?bin=source Although the changelog showed the pitch detector changes for eg r22685 the 7zip source didnt contain the required files… so I was manually downloading and adding pitch_detector.c and the compile failed no doubt because the bitmap interface
of the toolchain wasnt up to date. Not sure why the daily build is having this problem….

I downloaded this time from svn and all is well

Here is a diff that adds a new menu option to tune the reference pitch A between 435Hz and 445Hz in 1 Hz steps.
This is the first diff ive submitted , hopefully ive done it correctly!

fml2 commented on 2009-09-15 12:32

Steve, the patch is technically correct and applies without any problems. I have some notes though:

1. Variable names. The names “reference_table”, “ref”, “xx” and the struct member “reference” are not good IMO. They are too general and do not tell at all what they mean. Also, some comments about these tables would do no harm.
2. I’d rename the menu entry “Reference Frequency” to “Frequency of A” (or something like that) so that it’s clear what the setting means.
3. Maybe this is because of the sim (I have not tried it on target), but when I set the reference frequency to 435, the displayed note is pure A with the displayed frequency 445 (it’s fixed in the sim). When I set the ref. freq. to 445 then the displayed note is G# (with the raw frequency still being 445). So I think there might be some problems in the implementation.

Once these issues are resolved I’ll be happy to commit the patch!

Ive updated the patch as follows:

1. Variable names. Ive used more meaningful names, and added comments.
2. Renamed the menu entry “Reference Frequency” to “Frequency of A (Hz)” 3. On the Sansa E200 it appears to work correctly. I tested as follows using the NCH tone generator www.nch.com.au/tonegen

input tone 435 Hz
Frequency of A set to 435 Hz indicated tone A: 1 cents (435.2Hz)
Frequency of A set to 445 Hz indicated tone A:-38 cents(435.17Hz)

input tone 445 Hz
Frequency of A set to 435Hz indicated tone A:41 cents (445.28Hz)
Frequency of A set to 445Hz indicated tone A:2 cents(445.23Hz)

Would be good if this could be confirmed on another target

fml2 commented on 2009-09-16 10:28

Just a few cosmetic changes: removed some TABs, added more comments, inserted a space in to the menu entry (before ‘(Hz)’)

Another question: wouldn’t it be more simple to multiply the given frequency by a factor to normalise it to 440, and then proceed as if the frequency of A were 440? The factor would be (440/freqA). You already have this table, but as (1/x). What do you think?

It may be simpler but possibly slower..

My implementation required 5 fp_add(lfreq[ ],lfreq_A) and 1 fp_mul(freqs[ ],freq_A[ ])

The suggested implementation requires 1 fp_add(lfreq,lfreq_A) and 3 fp_mul(freq,freq_A[ ])

since fp_mul is significantly slower than fp_add , I think this may run slower

anyway, I have attached a patch for this method, which gives similar results as pitch_detector10_diff when tested using NCH tone generator on sansa e200

fml2 commented on 2009-09-16 21:13

Steve, my idea was to recalculate the frequency and its log only once, at the beginning of the function (i.e. one fp_add and one fp_mul) and leave everything else as it was. Only in the end, when the frequency is displayed, the original frequency should be used. Attached is the patch that (hopefully) does this. I took your patch and pulled the fp_mul’s to the top. I also corrected some spaces and TABs. Please set your editor so that no TABs are used but rather spaces (indentation is 4 spaces).

I have not tried this patch as I don’t have the ‘infrastructure’. Could you please test it?

thats certainly neater then my method and I can confirm it works fine on sansa e200

There’s no need to worry about speed here. The pitch detection algorithm has a complexity of n^2, which means that it’ll perform millions of operations on a 1024-sample listening window. A few extra multiplies or adds outside the main algorithm loop will make no perceivable difference.

Of much more concern here is the precision of the calculations. In general divides should be avoided or else performed after any relevant multiplies. Adds and subtractions don’t really affect precision. In using a baked-in multiplier just remember that it’ll reduce precision, especially if the multiplier is between -1 and 1. So we want to come up with a method with the least possible math, especially multiplications by small values.

fml2 commented on 2009-09-16 22:19

Errrmmm… David, you’re an expert here in the things related to the math. My comments were only about the cosmetic things (comments, spaces vs. TABs etc.) and the least intrusive way to make the frequency of A variable. I tought that if we’d scale the frequency in the beginning then the rest of the algorithm would remain the same since everything is logarithmic. But if it negatively affects precision then I sure won’t insist on it. Do you have a better suggestion? Or what we’ve come up with so far also acceptable? I made my proposal not because of the speed but because of the easy understanding.

I haven’t fully examined what you put up but I was more directly responding to Steve’s concerns about speed. My point was mainly that we don’t need to worry about it. I’ll take a look at what you posted tonight, but I would guess it’ll work fine. My biggest concern is that it still be fairly accurate when using a non-standard frequency for A. There’s necessarily a limitation with the fixed-point math, but hopefully it’ll be sufficient to keep the rounding error below one cent. I’m sorry if what I said came off as a criticism at all – that wasn’t my intention.

fml2 commented on 2009-09-16 23:11

David, you don’t have to be sorry. I didn’t feel offended by your comments at all. I just tried to draw the line of my knowledge so that I don’t take responsibility for the math side of the matter :-) But please take a look at the patches 9 and 11a and tell us what do you think about how they affect precision.

My idea (implemented in the patch 11a) is to leave 95% of the algorithm as is and only to scale the frequency in the beginning. So, for example, if the reference frequency is set to be 435 then we’d multiply the input frequency by (400/435). If the input frequency is 435 (easy to understand example) then the result of this (i.e. the scaled input frequency) will be 400. The rest of the algorithm (finding out the note and the error) works as if the input frequency were 400, i.e. your original algorithm is used. Only in the end do we display the original input frequency, i.e. 435.

fml2 commented on 2009-09-17 09:06

Weird: when I play (or whistle) sound of ~440 Hz the displayed frequency on my Sansa e200 is about 1700 Hz. But the note is shown as A. This is with the patch 11a.

Project Manager

440Hz*4 = 1760Hz. That is still an A, only 2 octaves higher. Which leads me to a request: I would like it to display the octave as well.

fml2 commented on 2009-09-17 09:25

You may be right with the assumption about x4. But why? Is it still the problem with a wrongly detected frequency? We had it in the beginning (x2). Was it corrected in the opposite direction (x2 instead of /2)? :-)

And yes, indication of octave would be good. How would you propose to do this? I’d display it as a smaller bitmap (as an index) right to the displayed note. But it would be a separate patch.

I think we should close this task after we’ve committed the change with the variable frequency of A and do other changes (e.g. displaying the octave) in a separate task. This one has become quite long and untrackable.

Project Manager

I guess your whistling generates quite a few overtones.

Regarding the octave display, I would just display the octave right after the note, e.g “A2” or “D#4”. That would require some new bitmaps though…

Project Manager

By the way, I tried to tune my guitar the other day, but failed miserably. The pitch detector couldn’t reliably detect any note, it detected frequencies all over the spectrum. It behaved the same on both iAudio X5 and iriver H140. :-(

/me wonders about the point of a variable tone a.

Isn’t the tone a a defacto standard tone for a reason? I *maybe* see a point if its octave is configurable, but as it seems to always show a (without indication of the octave) it’s pointless as well.

fml: your example of 400Hz is one of the examples which should really be avoided with fixed point math. The precision is really bad with so small (400/435) factors.

From Wikipedia: Many electronic tuners used by musicians emit a tone of 440Hz, corresponding to a pitch of A above Middle C (A4). More sophisticated tuners offer a choice of other frequencies, alternative A tones to account for differences in temperament.

Clearly, we need to be sophisticated!

fml2 commented on 2009-09-17 12:17

Linus, I also tried to tune my guitar. I have problems with the 1st (e) string – the note can’t be identified. But lower strings can be tuned. For example, the 6th and the 5th strings are clearly indicated as E and A, but only in the first second. After that, the display goes mad and shows different notes (I think it’s due to the other strings beginning to sound as well thus producing overtones). But the first second suffices.

As to variable frequency of A: of course I doubt that this feature will be used much. Serious musicians for whom this feature might be of interest have another means of tuning their instruments. But if we can have it – why not?

fml2 commented on 2009-09-17 16:35

Here’s an updated version of the patch 11a. The change is that the frequency is not scaled if the reference A is set to the default, thus avoiding precision loss. I also put the two tables for freq_A into a struct. Ideally, I’d also fill the menu options from this table but I don’t know how to do this since the options are const.

IMO, the frequencies and the respective logs for the scale should be put in a struct as well.

Variable pitch is useful because some orchestras go by a different standard for whatever reason. Also, people playing period instruments often tune their instruments according to how it would have sounded way back when, which generally means A < 440.

I actually can’t get it to register on my guitar at all either. Actually, it’ll occasionally get what seems to be the right note but I don’t know if it’s just a fluke. Mostly it seems to just display randomness. I would guess that the algorithm is getting overwhelmed by overtones. I’m sure a stringed instrument is going to produce more complex waveforms than a wind instrument. As far as other instruments go, though, I’m able to get accurate detection on recorder, flute, bagpipe, whistling, and singing. I’m surprised that the guitar would be so problematic. I’ve even tried varying the algorithm settings but to no avail.

Alexander: I think your method is pretty good for having a different reference frequency. Nicely minimal in terms of number precision-losing operations. For what it’s worth I doubt it would matter if you do the multiplication when the pitch is default, since the multiplier would be exactly 1, which wouldn’t produce any precision loss. The problems come in with values near 1, but not with exactly 1 (which produces no change at all).

Project Manager

Perhaps a low-pass filter would help the guitar tuning?

fml2 commented on 2009-09-18 08:28

OK, then I’ll commit the patch 11b on Monday (I’ll probably remove the if around the default A_freq since David said it doesn’t matter) – I’d like to give people who want to test it more time to do it. Everything else (displaying the octave, low pass filter, …) would go in their own patches. OK?

How about populating the menu items for the frequency of A on the fly? It’s a bit of an academic question, the current code is OK. I just wonder how this could be done.

On the sansa, the tuning is very stable and stops gracefully after betwenn 1-2 secs , although the top E string is slightly too short for comfort.
The stability may be due to the 22KHz recording rate acting as a 10Khz LPF, so the LPF may well be useful on other platforms that record at 44KHz
I would suggest the LPF function be switchable

I’d still like to request a broader range for changing the base pitch, though. I’m afraid I don’t have time to do it at the moment but I think it should go at least down to 415, or preferably to 400.

Re low pass filter: Maybe that indicates that it would generally work better with a lower sample rate? I admit I haven’t experimented with that. Since it’s only likely to be used for sampling things below 2 or 3kHz (I tried it with my garklein recorder, an instrument that’s only ~15cm long, and its A is at 1760 Hz) it could maybe even be knocked down to 11Hz sample rate. A menu option is probably a good idea.

Of course, a halved sample rate will also give a huge boost to the speed of the algorithm and could reduce its memory footprint.

fml2 commented on 2009-09-19 12:05

Here’s another patch that slightly improves selecting of the frequency of A – now it’s done via an int setting. This should make it easier to add more values for freq_a later (in a separate patch).

fml2 commented on 2009-09-19 13:34

He-he, here’s another improvement: the menu item for frequency of A is now _before_ the “Reset settings” item. And the default answer for “Reset” is now always “No”. It was “yes” after “yes” has been selected once.

In a separate patch, I plan to use set_bool for boolean settings (instead set_option). I didn’t want to mix these changes into this patch.

fml2 commented on 2009-09-20 16:30

The last part has been committed in r22753.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing