Rockbox

Tasklist

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

Attached to Project: Rockbox
Opened by Allen Young (IntelliCoder) - Tuesday, 22 May 2007, 01:33 GMT
Last edited by Jonathan Gordon (jdgordon) - Tuesday, 10 July 2007, 07:42 GMT
Task Type Patches
Category Recording
Status Closed
Assigned To No-one
Operating System Iriver H100 series
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

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 .
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Tuesday, 10 July 2007, 07:42 GMT
Reason for closing:  Accepted
Additional comments about closing:  to have mutliple directories the easy way just save a few .cfgs in the recording settings folder.
Comment by Jonathan Gordon (jdgordon) - Tuesday, 22 May 2007, 04:40 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 24 May 2007, 13:29 GMT
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
Comment by Allen Young (IntelliCoder) - Thursday, 24 May 2007, 15:39 GMT
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).
Comment by Allen Young (IntelliCoder) - Thursday, 24 May 2007, 16:05 GMT
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).
Comment by Marianne Arnold (pixelma) - Thursday, 24 May 2007, 16:12 GMT
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).
Comment by Peter D'Hoye (petur) - Thursday, 24 May 2007, 17:59 GMT
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.
Comment by Peter D'Hoye (petur) - Thursday, 24 May 2007, 18:03 GMT
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)
Comment by Allen Young (IntelliCoder) - Thursday, 24 May 2007, 19:28 GMT
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).
Comment by Allen Young (IntelliCoder) - Thursday, 24 May 2007, 19:38 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Friday, 25 May 2007, 01:47 GMT
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.
Comment by Allen Young (IntelliCoder) - Saturday, 26 May 2007, 00:17 GMT
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.
Comment by Marianne Arnold (pixelma) - Saturday, 26 May 2007, 09:44 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Sunday, 27 May 2007, 09:03 GMT
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?
Comment by Peter D'Hoye (petur) - Sunday, 27 May 2007, 15:36 GMT
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...
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 May 2007, 10:15 GMT
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
Comment by Marianne Arnold (pixelma) - Wednesday, 30 May 2007, 23:26 GMT
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.
Comment by Allen Young (IntelliCoder) - Monday, 04 June 2007, 17:02 GMT
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.

Comment by Jonathan Gordon (jdgordon) - Monday, 11 June 2007, 13:13 GMT
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...
Comment by Jonathan Gordon (jdgordon) - Tuesday, 03 July 2007, 10:32 GMT
new version
Comment by Marianne Arnold (pixelma) - Tuesday, 03 July 2007, 21:55 GMT
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...