Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Recording
  • Assigned To No-one
  • Operating System Iriver H100 series
  • 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 IntelliCoder - 2007-05-22
Last edited by jdgordon - 2007-07-10

FS#7201 - Multiple Recording Directory Support on iHP-120

This group of patches enables support of multiple recording directories on iHP-120 (it should work on other players, I believe). Refer to the attached design document for more details.

This patch addresses the feature requests  FS#6064  and  FS#6268 .

Closed by  jdgordon
2007-07-10 07:42
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

to have mutliple directories the easy way just save a few .cfgs in the recording settings folder.

a few things:

please submit as a single .patch file.
some of those diffs have the files backwards
It looks like you are overly complicating the setting. I dont use recording much (at all really), but imo I think it is enough to have a MAX_PATH buffer in the user_settings struct and the int rec_path setting to determine if we are using "current folder", /recordings or the user set folder. I do like the way you are setting the rec path with a context menu though.

something like the attached would be more inline with how we want the setting done (my h300 isnt playing nice, so I cant test, please test and let me know how it goes)

edit: attachment changed so the default path is / instead of /recordings… <linuxstb_> IMO, 1) It's logical; 2) It removes the need to use an English word; 3) It solves the issue of not being able to set the directory to the root

Hello Jonthan,

Thanks for the feedback.

I have purposefully avoided adding a new MAX_PATH char array in the user_settings struct for two reasons.

1. A couple of people commented in  FS#6268  and/or  FS#6064  that they're adverse to adding a MAX_PATH array in the config block (probably because it takes up RAM while the player is running.)

2. One of the key features in this patch is the support of multiple user-set recording directories. This turns a Rockbox-loaded MP3 player into a highly-organizable voice recorder and in some aspects makes it better than most hand-held voice recorders. One user-set directory is often inadequate for people who need to record lots of stuff on multiple subjects (a college student, for example). Adding a MAX_PATH array in the user_settings struct will allow only one user-set recording folder. So, I had to resort to adding a config file that keeps the list of user-set recording folders.

After reading your comment, I did realize that I have declared an unnecessary char array in recording.c. It should've been a char*. If it gets decided to commit this feature, I'm willing to spend extra time to make this improvement and resubmit the patch.

Lastly, here's the single .patch file you've mentioned.

(The above is a reply to your first post. I'll reply to the second soon).

I'm really reluctant to forfeit the multiple user-set directory feature. Why limit the user-set directory to only one? I agree it complicates things, but for me at least it's convenient to have multiple user set rec folders.

As you pointed out, strings should be moved to .lang file.

I'm not sure about changing REC_BASE_DIR to "/". It's possible to use the root directory as rec folder using "Current Directory" option. I thought it'd be nice to leave the original options intact (/recordings & Current).

I just tested JdGordon's version and found out that it doesn't handle the case where the set recording directory could have been deleted (1) or not correctly. I set the dir to <MMC1> on my Ondio, recorded - worked correctly - then pulled out the MMC, recorded again and at first try to write I got "*PANIC* recfile -1".

Additionally I got "Can't create / directory. Error code -3." after plugging the MMC again and going to the recording screen, I could even record again - the recorded file was found in the root (of the Ondio) then. And afterwards I had 2 <MMC1> folders in the filetree - 1 above the ".rockbox" (as normal), 1 below - I could reach and play the files on my MMC through both of them.

OK, got side tracked a bit I guess, because I think the root of the problem lies in (1) - mentioned it in case this could be caused by something else. There was a discussion in IRC, after I had reported the test result there, about how to handle that case (starting at about 17:05).

petur commented on 2007-05-24 17:59

pixelma: your error is an exception because your target supports removable volumes, and the automatic diur creation doesn't handle that. It should revert to default if even the creation of the dir fails.

petur commented on 2007-05-24 18:03

If we *really* need the ability to support multiple directories, I'd like that as an extension: have a normal directory setting and maybe a plugin to manage recording directories? I'd hate all that code sneaking in for the few users that need multiple dirs (which is a fraction of a fraction of the total users)

just my quick thoughts on this (and I am a recording user)

I'm really reluctant to forfeit the multiple user-set directory feature. Why limit the user-set directory to only one? I agree it complicates things, but for me at least it's convenient to have multiple user set rec folders.

As you pointed out, strings should be moved to .lang file.

I'm not sure about changing REC_BASE_DIR to "/". It's possible to use the root directory as rec folder using "Current Directory" option. I thought it'd be nice to leave the original options intact (/recordings & Current).

Sorry for the double post. I shouldn't have refreshed the page.

I agree with Peter's idea. This patch is for supporting multiple recording directories so that it's possible to switch between them with a click of button. Jonathan's approach, however, addresses the feature requests only (not in full extent, I believe). This patch addresses all the requests mentioned in the aforementioned feature requests,

-Display of current recording directory on WRS (while recording screen).
-Static user-set recording directory

On top of that, it also supports

-Change of recording directories with a click of a button in WRS.
-multiple user-set recording directories.

Jonathan's way is streamlined, whereas this patch is more feature rich. Again, I agree with Peter's view. Probably a fraction of users need multiple user rec dir, and it might be good idea to implement as a plugin (I don't know how to approach this way, however). No reason to add so much code for something that won't be used by majority.

Allen, regarding the not wanting to add the char array to the setting struct, that was a valid point up untill around january iirc, the settings are no longer stored in a binary format in the "config block" (except special setings, which this is not one), they are stored as a text file, so adding that array is fine.

As for multiple record directories, even then you are compicating it… all that is needed is create a .cfg with the only setting being the one for the record dir, "run" it before recording and the path wil be changed.

the reason we want small code (especially here) is because even though we have given up on rombox on he recorder, we would like to bring it back one day…

as for Marianne's problems, reading the IRC logs it seems the way to go is try recreating the missing direcotory, if we cant then ask the user if he wants t record to / or abort.

Jonathan, the whole point of this patch is to accomplish one thing: Allow the user to change the recording folder with a single click among a set of user-set alternatives without ever leaving the WRS (while recording screen). No compromise in that so long as this patch is concerned.

In order to allow the user to alternate between the user-set recording directories without having him/her leave the WRS, the list of folders has be somewhere. In addition, there has to be an extra line of option for changing directories in the WRS. If it's possible to keep a list of strings in a .cfg and manage them with existing APIs, that will simplify some things, of course (I'm not familiar with Rockbox's setting mechanism).

I should emphasize that this patch is not a simple and direct answer to the feature requests.

As for keeping the code small, that's understandable whatever the reason is.

Unless this patch can be streamlined to an acceptable Rockbox convention without sacrificing the key purpose, I believe your approach is fitter for the majority of the users. Its lean and it addresses the feature request directly.

I'd like to comment some of Allen's ideas…

-Display of current recording directory on WRS (while recording screen).
Have you tried this on other targets? For example most of the Archos devices (except Player and OndioSP) are capable of recording, I don't know how an extra line will fit on the small display (112x64 pixels) though I remember that since some info moved up to the statusbar there is a bit of space left. Seems like I have to try it out myself… ← I can' tuse your patch - how did you create it?
There has been some talk about reorganising the WRS in general so that you have a fixed part and also a list part if I remember correctly.
-multiple user-set recording directories.
That is a very specific feature and as Peter said, the ones who would use it are a minority (very minor). I like his idea to have this as a plugin somehow, don't know how it could be done.
I thought it'd be nice to leave the original options intact (/recordings & Current).
As soon as I'm able to set a specific directory as recording dir I wouldn't use "current" anymore because it doesn't give as much control. I always have to remember that this option is set if I enter the WRS or else my recordings end up in *some* directory after listening music (follow playlist on). I only use this because at the moment there is no other way to record something to the MMC, which has much more storage than the internal memory.

I think this version is commitable. It shuold only try writing to the mmc if it is actually connected (and it wont try creating a <MMC1> folder if no mmc is connected).

now, I'm not a frequent rec user, but I would have thought that people would have multiple recording configs for the different situations they record in, So it might be a better idea to create a way to automatically create a recording .cfg in the same way we make theme .cfgs, and then add a "Browse configs" option to the recording settings menu which would view them so you could choose easily….
I think this would fulfill your multiple rec-dirs request better than an external plugin would.

As for displayin the recording directory on the WRS, doesnt it already show the filename? so couldnt we just get that line to scroll and show the full path instead of it being static?

petur commented on 2007-05-27 15:36

Jonathan, I think the idea of making a way to create recording .cfg files and a way to browse them from the recording screen (settings) is the way to go. Having that would also make it easier to switch more settings than just the directory.

As for the screen space on archos, I'm pretty sure it's filled up… try a sim build to see what's on there already…

updated patch, if it cant write to the chosen directory it splashes the messages "Can't write tot he recording directory, Off to abort" and sits waiting for user input, then exits the radio screen

I tried Jonathan's latest patch in combination with the fat_test.patch - like I've been told - on my Ondio.

The first thing I tried was setting the <MMC1> folder as recording directory (the root of the card so to speak) and then tried to record.
This time it even crashed with the MMC inserted on first attempt to write the data: '*PANIC* recfile: -116' if that helps you somehow to figure out what is going on. That's why I didn't even try what would happen when I now remove the MMC.

Reply to Marianne's "Saturday, 26 May 2007, 04:44 GMT-5" post


-Display of current recording directory on WRS (while recording screen).
No, as I have not. However, from the display on the iHP-120 remote control (it's got a small screen that can only display 3 lines of options in the WRS), it seems that when the screen height is not big enough, it supports scrolling automatically. I could scroll down and select the last 4th option after adding the directory line on the remote.

I used a tarball to get the source code instead of using svn. As Jonathan pointed out, the first group of patch files I have submitted have wrong orders. Please use mul.dir.patch, which I submitted in a later post. The following commands work for me w/o problem.

tar xjf rockbox-20070513.tar.bz2
> cd rockbox-20070513
> cp ../mul.dir.patch ./
> patch -p2 < ./mul.dir.patch
> mkdir build_sim
> cd build_sim
> ../tools/configure
> make
> make install
> ./rockboxui
-multiple user-set recording directories.
It seems like Jonathan and Peter is starting to favor this idea (unless I've read it wrong). If it's possible to support the changeability of multiple user-set recording directories from the WRS, I'm all for it. That was the whole point of the patch.
> I thought it'd be nice to leave the original options intact (/recordings & Current).
I don't mind if this option goes, but I still think it'd be nice to have it around.

Please let me know if mul.dir.patch works on Archos.

Reply to Jonathan and Peter


I'm really all for being able to change the recording directories (user-set) from the WRS. The convenience has been amazing. As a heavy recording user, leaving WRS to change recording directory is simply too slow and frustrating.

Would it be feasible to add an extra setting line for directory in the WRS and keep the list of all the user-set directories in a .cfg file?

It might be even possible to give ther user an option to display/hide the directory option display in the WRS from the Recording Settings menu.

new patch, seems to be working nicely.

edit: after "discussion" in irc, this one is no good either, it will ause a spin up before rec starts which is apparently bad…

new version

JdGordon: seems like you tricked yourself… the "recfile: -116" was caused by the fat check patch (see Lear's committed fix).

After that, I successfully tested the following 3 cases:
- recording dir on MMC with it still plugged
- then removed the MMC and tried to enter the recording screen again which gave the "Can't write to recording directory" splash (that's ok to me)

no additional folder occured...

- then set a directory on the internal memory as recording dir (just picked a new one without clearing the other one first) worked ok, too.

The patch adds about 160 bytes to binsize here… sounds quite fair… I think.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing