Rockbox

Hi Matthias!
I included the buttonpress patch (the merged one from the usb charging task) and experience the following strange thing: However I set the option, I always have to press the REC button for a connection. AND: USB charging is always done, even if the setting is turned off.

Can you take a look at this and tell me what you think? For my build including the patches I used see http://www.misticriver.net/showthread.php?t=42675

Thanks and all the best

Norbert

Awesome Matthias. I absolutely love this functionality. Working lovely on my ipod video.

Norbert,
you’re currently the only one where the patch doesn’t work.
So I assume it interferes with some of your other patches!

I’m sorry that I don’t have the time to have a deeper look
at your special build.
I suggest that you create a version with only my patch(es)
and tell me if it still does not work.

If it works then, you’ll have to find out which other patch
make it unworkable. Maybe you try another order for
including your patches?

If the clean build still doesn’t work - we’ll find out together why

Ryan, thanks for the positive feedback about another target…

pss: also enjoying it my nano. … I really hope this can be committed someday.

Hi Matthias!
Forget my complaint. It seems that it is now working in one of the latest builds. At least I got it to work.
Thanks for the patch, good work!
Norbert

Got to say thanks alot… i didn’t actually have a clue the iPod could charge itself while in rockbox until now :S :P. If I knew I would’ve stopped relying the Apple OS months ago!

Sync with today’s CVS…

   buttonpress_for_usb_connect_2... (10.4 KiB)

Sync with today’s CVS…

   buttonpress_for_usb_connect_2... (10.4 KiB)

Sync with today’s CVS…

   buttonpress_for_usb_connect_2... (10.4 KiB)

Sync with today’s CVS…

   buttonpress_for_usb_connect_2... (10.4 KiB)

Hmm don’t think I’ve been able to get it to work with 20070101 source… on either setting, when I plug in my 5.5G 30GB iPod Video the USB logo comes up, and when I press menu it reboots and says Do not disconnect and starts charging.

This really should become CVS…

The fact that this functionality is not in the official build is the only reason why I don’t use them. And it’s not like i didn’t try to patch but Cygwin will not compile any build i edit, and in any case, with rockbox’s generally short battery life charging is cruicial.

Resync with today’s SVN

Does anybody else have some problems with that patch?
If yes, with which targets?
(it still does work here for my H300)

   buttonpress_for_usb_connect_2... (10.9 KiB)

Needs to be updated I believe?

apps/settings.c
Hunk #1 FAILED at 331

Might be conflicting with another patch I have, the brightness patch

I think the patch needs updating to work after the introduction of the new way rockbox saves its config to a file
rather than a hidden sector on the HD it used previously.

Yes, settings.c has apparently been completely reworked. It’s not a conflict with another patch, as this is the first one I tried applying.

I tried figuring out what needs to change but got completely lost in the new code. This patch is so essential. Anyone know if there’s any chance of it ending up in the codebase?

I’d like to see this committed, but haven’t had time to resync it recently. If somebody could do so, I’ll see if I can get it into SVN…

Resync :)

   buttonpress_for_usb_connect_0... (9.4 KiB)

Thanks, that was quick. Note that new tags should be at the end of english.lang.

Unfortunately, now I understand how USB connection works (i.e. this just inverts the holding down/not holding down a button test) I’m not sure that this patch is needed; it is considered option bloat (see IRC logs for discussion). Maybe someone can argue better for it’s inclusion here…!

For my two cents in the argument, though I personally rarely used this patch because I never wanted it the other way around, I think that this patch goes along with how I view rockbox in the fact that the option is there if you need it- kind of like how it would be nice to change to start the native OS by default if you wanted. I like the option being there, and I’m not a coder or anything but I don’t see why it’s any kind of big deal to be included.

Yes, what is the concern behind “option bloat”? I couldn’t see any kind of search on the IRC page and I’m not about to wade through all those messages looking for the discussion on this, so I’m afraid I don’t understand the thinking here.

If the options are tucked away in well-organized menus, why is it a problem if there are many of them? I never use crossfade, but it doesn’t bother me seeing it there in the menu. IMO, the more things Rockbox can do, the better.

Since Rockbox seems to be all about making good software for as many different devices as possible, why shouldn’t it be as configurable as possible? Different devices and usage habits require different customization. This patch may be irrelevant to Archos users (I have no idea how they handle USB), or even iPod users who don’t plug in several times a day. But to me as an iPod user who constantly plugs and unplugs the device, this patch is absolutely essential, and I think many other users would agree.

Or is code size the real concern?

Code size is always a concern, and having too much options simply makes it confusing, which is also a point. Btw, searching the logs shouldn’t be a deal as you know which day to look (and modern browsers have this “find-as-you-type” feature, which is quite handy for such lookups).

FWIW I’m personally in favour of adding this - my particular usage involves USB for power every day, and USB for connection (to update music etc.) only occasionally. But I can see the argument that it’s not that hard to hold down a button while plugging in the cable, and code size *is* a concern.

On which day in the IRC logs did this discussion take place? Guess I missed the part where that was said.

Is code size a concern because Rockbox supports targets with very little memory? If so, given the number of #IFDEFs I already see in the code, couldn’t an argument be made for conditional compilation of options like this too? It seems strange to deny useful features to those of us with a 30GB hard drive for fear of adding a few KB to the firmware.

@Mike: I think the point is that more code in the firmware means more code taking up space in RAM that could’ve otherwise been used to buffer music (think, battery life).

I also think this is an important option though.

@senab: thanks for syncing the patch again.

@pondlife: a big thanks for adding my other charging USB patch to SVN

(the original version was written by pyro, but I rewrote it with suggestions from amiconn)

My two cents about the “code size concern” and “option bloat”: I also follow MisterScoundrel’s arguments. I never used crossfade (and other options, too) and I really don’t see any essential need for some of those.
Each developer (including me) thinks that “his” feature and “his” options are the most important ones… So why not have a complete (enduser) discussion about which options are “essential” and which are not?

BTW, for me some of the crossfade options could be removed in favour of these one here ;)

Matthias: please keep in mind that Rockbox is about playing music. While crossfade is about playing music a convenience function like this isn’t. Which makes such a feature much more an option bloat than e.g. crossfade. When you’ll ask end users they will have quite different ideas what features are “essential”, and for most ipod users this will most likely be some fancy eye candy instead of good music playback. So I don’t think asking end users will help, and developer discussion is taking place (mostly) on irc as already got mentioned.

Not to mention that Crossfade is an actual Feature. The primary purpose of this patch (the option to invert the default behaviour) is additional code to save users a single button press. Considering that the device must be handled while plugging it in anyway, I think in overall terms what is added by this is a very small convenience at best.

As for the portion of it changing the default behaviour while in car adapter mode, I think that should be separated into its own topic for discussion. In reality they’re two different things.

-Does it actually take up more RAM when all you’re doing is making it variable which one it points to when it’s plugged in?
-I think that although this doesn’t actually involve playback, to some it might make a great deal of difference in the daily use of their device. Example: An individual who has his player at work and wants to plug it in to his computer to charge and play directly because he can’t be playing music on that computer. I just think the simplicity of the option when compared to the benefit of the option is definitely a plus.
-Also, just for argument’s sake, when asking the end user his opinion- the iPod population that uses Rockbox is quite different from the general iPod population.

For me the purpose of this patch is to prevent disk corruption with my ipod nano by boot into disk mode by accident.

As I have corrupted my nano’s file system on more than one occasion by rebooting into disk mode by accident when I am trying to charge my ipod then ether unplugging it without stopping the removable disk under windows or when my laptop suspends.

So as such I think the default behaviour should ether be to require a key to be held down for USB mode. I know that a need to hold down the menu button when connecting but sme times I get it wrong.

Remember ipods are supplied without any other form of charger and most people plug them in to a USB port on a computer to charge much more often than to transfer music on or off then.

This entry is two tasks:

The first is “Add option to reverse the behaviour of the USB mode, so that holding a button enables it rather than preventing it.”

The second is “Car mode by default prevents USB connection.”

This task needs to be split into two separate tasks, as there are very likely many people who might agree with one and object to another (for example, me). I will close this task in relatively soon, please split into relevant tasks. We do strongly and frequently suggest that you do not make patches that do more than one thing, this patch clearly changes two behaviours.

@bluebrother: your’re right, rockbox is about playing music.
Crossfade was only an example (although I - and it seems others too - personally never used it)
I’m sure there are a lot of other features which some wants and others not.
I also don’t see the point against asking end users.
If they don’t share the opinion of the developers, what’s wrong with that?
It would show that end users have a different view about what features are bloated
and which are missing. Devs still have the choice to follow that views or not… But it’s a chance to see what a majority of end users think.

BTW, it seems that the (unrepresentative) people here have the opinion that the

default for USB connection should be to charge instead of to connect.

So why not ask the “end user” about that?

About IRC: when working on the album art I often went to IRC and asked for
suggestions/opinions. Mostly I spent a lot of time there and don’t get any answers.
It seems that the hours where I was there didn’t fit to times where core developers
had time to talk to me

At the moment I don’t have the time to spent hours at the IRC, so I don’t do it.

@darkkone: I’m not sure if I’ll find time and mood to split the patch

(and maintain two patches - as the past shows sometimes for a long time).

This patch here is alive for half a year now - in that time you’re the first who

asked for a split and tries to force me to do it with the menace to close it.

If you’re in the mood to close this patch here do it;
I can live with that…

@darkkone
As I see if this task has nothing to do with car adapter mode and is very much about audio playback as it allows the user to change the default behaviour of rockbox to continue audio playback when you plug in as USB cable.

As I have already stated all iPods are supplied without chargers and the most common reason for plugging usb into a rockboxed ipod is to charge it and I suspect there are quite a lot of people like me who still use there mp3 players for audio playback when plugged into USB to charge rather than using a computer.

This is the first time there’s been real discussion of inclusion.

Generally speaking, patches that do two things are less likely to be included because if the developers aren’t fond of either thing it’s likely to keep both out. I’m not simply threatening to close it. It’s just that as the patch stands it falls in the ‘rejected’ category, so it’ll be closed for that, but the only thing that’s really come up for discussion is the configurable button.

I don’t intend it as a threat, merely that the discussion has come and gone on one aspect but that’s not all there is to the patch.

@Dwyloc

The patch description specifically includes a change to car adapter mode behaviour. If you’re choosing to ignore that, it’s fine, but as you can clearly see by actually reading the full description, the patch does in fact change car adapter mode.

@darkkone

Sorry I read the “additional” part of the description as a bug fix to existing functionality not an additional feature when I first read the description, why would any one want to reboot when they plug the power in car adapter mode?

But I have to admit that I had complete forgotten about that part of the patch as I have been using it for so long and I have never used car adapter mode. It is also not mentioned in the title “Button press for USB connection or USB power only”.

If this patch is being dropped I guess I will just have to order a plugin USB charger for my iPod now Rockbox supports them.

Even if it’s seen as a ‘bug fix’, it still means that this patch does two separate things, and should be two separate patches, as they will be independently considered for inclusion.

And the ‘bug fix’ one is, I think, more likely to get a yes, while the other one falls under the general category of ‘not terribly useful.’ All it does is change it so that, while you’re already holding the player, you don’t have to press a single button. Why that’s worth adding yet another small amount of code to the pile is a mystery to me. It’s a single button, one that I have no problem pressing while plugging in my players that can USB charge. It doesn’t add a ‘feature’ it just very, very, very, very slightly makes Rockbox more convenient.

And while the amount of code size is trivial as well, it is non-zero. Remember that several people said ‘I don’t use crossfade’. Were this option to go in, people would surely start saying ‘Well, I don’t use the USB charging reverse option’ when their small features are challenged. It adds next to nothing, and it also opens a door for a lot more argument on other issues.

If you can give me a solid reason why holding down a single button is such a huge problem that it needs code to fix, maybe there’s something more here. But outside of ‘in the car when only one hand is available’ (which doesn’t really count since A) You shouldn’t be plugging it in once you’ve started driving, and B) the other patch that should be split from this would cover it), what situations are there where a single button press is such a burden that it should in _any_ amount decrease audio buffer for everyone?

I am strongly against the default behavior being changed. If it was then I would have to have this patch…

@darkkone: I’ll keep this patch here as is (for the moment) I created an additional (small) new patch which just switches
on USB power when in car adapter mode (without choice).
It’s  FS#6654 .

personally…i plug my ipod up to charge it about 100 times more often than i do to sync up music…yes, holding a button isn’t a big deal…however, personally, i think the idea of changing the default behavior should be something that’s up to the user…when i go to charge my ipod..i don’t want to have to remember to push that button..all i want is to charge it.

btw, this is out of sync.

Arg - this is one of the patches I would desperately like to see in my rockbox… It’s not just a little more convenience. It’s not just pressing the button - you need to keep it pressed… I lost the count on how often my rockbox booted into disk mode because I released the button a few seconds to early And of course, because of the battery consumption I really liked the idea of letting the Ipod playing with USB power.
Maybe rockbox should be more modularized than. I could enjoy unloading the crossfade stuff to increase my audio buffer …

Just for the interested: I modified the patch for easier merging. Now it just switches the behaviour (default is USB charging; for USB disk access, press the menu button on connection). I know that this is nothing for final integration, but it helps me a lot.

   usb_charge_by_default.patch (0.5 KiB)

this *used* to be on my list of patches i wanted…and in fact, when it was out of sync i wouldn’t build a new rockbox…however….here’s something to think about.

the ipod video’s lack firewire communication..however, since the firewire power pins have pretty much been designated as the main power input on the dock connector (they’re tied to a handy voltage regulator)…they function just fine as power. now, up till a couple of weeks ago all i had was my USB cable…mostly because it came with the unit…and that was my main method of charging it…now…everyone should know USB is 5v..which sounds fine…but it’s 500ma at most…which isn’t a whole lot of juice for charging the battery….and with the recent purchase of an ipod travel kit…i got a firewire cable (the car adapter actually requires it)…now, since the 5th gen video’s don’t have firewire in it….all rockbox can do is go into charging mode…personally, i plug mine into the daisy-chain port on my external firewire drive….provides a good bit of power to charge the ipod.

now, naturally…much like this patch…it’s a solution not everyone will agree upon..however, i thought i’d just..share in case anyone else wanted to start doing the same thing.

how do u install the patch