Rockbox

Tasklist

FS#11914 - Android related patches

Attached to Project: Rockbox
Opened by Maurus Cuelenaere (mcuelenaere) - Sunday, 30 January 2011, 23:32 GMT
Last edited by Maurus Cuelenaere (mcuelenaere) - Saturday, 07 May 2011, 16:03 GMT
Task Type Patches
Category Simulator
Status Closed
Assigned To No-one
Operating System Another
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

These are some Android related cleanups and feature additions. I've considered sending these to the mailing list, but posted them here as devs are more familiar with Flyspray.

Non-cleanup patches are 1, 8, 9 and 10; these are to be reviewed (others are rather minor).

I've labeled the patches that are incomplete as WIP:
0008-Android-sync-audio-volume-with-Android-system-volume-WIP has the problem that it can't figure out what the intention of the volume change is, e.g. fade-out doesn't require the global music stream volume to go to 0
0009-Android-graceful-shutdown-WIP requires some more changes to both apps/ and firmware/ code (e.g. power_off() isn't called IIRC, so the wakeup will never be signaled)
0010-Android-signal-handlers-WIP works, but doesn't provide any useful info atm. I've looked at libunwind and GCC's backtrace support, but those require adding a library and thus bloating Rockbox for a minor (dev) feature. This could help tracking down native code bugs when going public though.
This task depends upon

Closed by  Maurus Cuelenaere (mcuelenaere)
Saturday, 07 May 2011, 16:03 GMT
Reason for closing:  Fixed
Additional comments about closing:  These patches are either committed to SVN or obsoleted by other commits.
Comment by Antoine Cellerier (dionoea) - Monday, 31 January 2011, 09:59 GMT
About patch 0006, I'm not sure that I want the make install target to start the activity. (That's not always needed when testing widgets for example)
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 10:02 GMT
Yes, I've thought about doing this as a separate target but couldn't come up with a name. Something like make launch?
Comment by Antoine Cellerier (dionoea) - Monday, 31 January 2011, 10:11 GMT
Sounds good.

Also, I don't know if other people have noticed, but there seems to be some issue with the java side makefile target dependencies. I've already had it build only some of the classes when running "make apk" right after "make clean". Running "make apk" a second time will build the remaining classes. Looks like something might be messed up somewhere but I haven't been able to find why.
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 10:26 GMT
I haven't noticed that issue, what did occur for me though was that after doing make apk - which ran successfully - I did a make install which resulted in redoing all the Java steps before uploading the apk.
Comment by Thomas Martitz (kugel.) - Monday, 31 January 2011, 19:04 GMT
About 1:
- I like that the default language is changed, but I don't think we should have two language systems. We can pass localized strings from the native code that is translated with the classic system.

2, 3, 4, 5) go for it

6) If we can make a make-target depend on the install target we could do something like "make install run" (or launch, start, ....) which would be kind of nice. IMO the install target shouldn't start the app on its own.

7) Looks good, but I think we should eventually broadcast key events (if possible) instead doing rockbox-local intents, this way the widget could potentially work with other apps as well (Android handles which app gets the multimedia key events first). Do I understand it right that it starts up the native code if rockbox isn't running? Does it then also execute the action?

8) I'm very happy with the current, separated volume handling. Do we need to change it? People have also told me that it's nice that Rockbox offers a more fine-grained volume control that the global one.

9) I'm fairly sure calling wakeup_wait() from OS context (i.e. outside of a rockbox thread) cannot work correctly. We should probably use a OS primitive to wait for the native code here.

10) What is this good for? Debugging purpose?
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 20:10 GMT
1) Sure, np. I'll also be preparing a follow-up patch for this which should add all currently supported Rockbox languages as values-[lang]/strings.xml (only containing <string name="rockbox_language_file">).
2-5) Ack.
6) I'll add a "launch" target.
7a) This patch should possibly be splitted into 2: one "constantizing" the intents and the other refactoring onCreate(). Isn't that a security risk? Or will you only accept such intents from whitelisted apps (e.g. the widget)?
7b) Are you talking about the if(!rbLibLoaded) startService(); hunk? If so, that isn't an addition but a move (look above in the patch).
8) I implemented this because I thought it simplifies user experience, e.g. it could be confusing for new users to have separate volume controls. I can drop this though if wanted.
9) Could be, I'm not familiar with how Rockbox and Java threads react to each other. I used wakeup_* because that was the easiest to implement :)
10) Yes, as the 1st post says: "This could help tracking down native code bugs when going public". A native code backtrace could be useful; but this is more of a PoC right now.
Comment by Thomas Martitz (kugel.) - Monday, 31 January 2011, 20:44 GMT
6) "run" is shorter ;)
7a) We already listen to global multimedia button broadcasts (for wired headset remotes and cyanogenmod lockscreen controls), this is hardly a security risk. The widget currently doesn't send the broadcast but rockbox-local intents.
8) It reduces flexibility and would be inconvenient for me. As for confused users, I think they'll be fine as long as the volume buttons change volume (they don't necessarily have to use rockbox' volume at all)
9) I suspect the UI thread calls onDestroy(), which is not a rockbox thread. So if it calls into the native code it should be considered as interrupt context.
10) Having backtrace() in debug builds would be awesome, but IIRC it needs register to be used frame pointer. We currently build with -fomit-frame-pointer, but we should be able to change it for debug builds. What if throw an exception and print the backtrace from the java side?
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 20:54 GMT
6) I just finished this patch, you can change it to "run" if you want to ;)
10) I haven't tried throwing exceptions, I'm not sure how useful a Java backtrace would be though (as that would stop at the entry point at the Java side).
I can't imagine that Google didn't add backtrace support for native code (probably only enabled on debug builds), so this could all be a duplicate effort though.
Comment by Thomas Martitz (kugel.) - Monday, 31 January 2011, 20:59 GMT
Java backtraces also include native functions. Thinking about it, I'm not sure if it's even possible to print (a useful) backtrace from a signal handler. Perhaps if you make use of the 3rd (ucontext_t) paramter passed to the handler? http://www.linuxjournal.com/files/linuxjournal.com/linuxjournal/articles/063/6391/6391l3.html ?
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 21:02 GMT
IIRC that's just something like:
# 1 native code
# 2 RockboxService::startService()
...

I've looked at the ucontext_t parameter, but I don't think Android provides something useful here (NDK doesn't have ucontext_t, haven't looked at bionic though).

Also, see http://stackoverflow.com/questions/1083154/how-can-i-catch-sigsegv-segmentation-fault-and-get-a-stack-trace-under-jni-on-a/1789879#1789879
Comment by Thomas Martitz (kugel.) - Monday, 31 January 2011, 21:12 GMT
In our case we perhaps don't get a useful backtrace (it perhaps depends on how we compile the binary), but if you look at other exceptions which come from the java library it contains native functions in the backtrace just fine.
Comment by Maurus Cuelenaere (mcuelenaere) - Monday, 31 January 2011, 21:35 GMT
Throwing an exception results without calling env->FatalError() results in the VM getting killed without logging anything.

With calling FatalError() and compiling without -fomit-frame-pointer this results in:

D/dalvikvm( 453): Trying to load lib /data/data/org.rockbox/lib/librockbox.so 0x40512300
D/dalvikvm( 453): Added shared lib /data/data/org.rockbox/lib/librockbox.so 0x40512300
D/dalvikvm( 453): No JNI_OnLoad found in /data/data/org.rockbox/lib/librockbox.so 0x40512300, skipping init
W/dalvikvm( 453): JNI WARNING: JNI method called with exception raised
W/dalvikvm( 453): in Lorg/rockbox/RockboxService;.main ()V (FatalError)
W/dalvikvm( 453): Pending exception is:
I/dalvikvm( 453): Ljava/lang/RuntimeException;: test
I/dalvikvm( 453): at org.rockbox.RockboxService.main(Native Method)
I/dalvikvm( 453): at org.rockbox.RockboxService.access$300(RockboxService.java:55)
I/dalvikvm( 453): at org.rockbox.RockboxService$1.run(RockboxService.java:279)
I/dalvikvm( 453): at java.lang.Thread.run(Thread.java:1019)
I/dalvikvm( 453): "Rockbox thread" prio=5 tid=9 NATIVE
I/dalvikvm( 453): | group="main" sCount=1 dsCount=0 obj=0x4051d670 self=0x1e6b10
I/dalvikvm( 453): | sysTid=461 nice=0 sched=0/0 cgrp=default handle=1991184
I/dalvikvm( 453): | schedstat=( 5180165097 8168308923 633 )
I/dalvikvm( 453): at org.rockbox.RockboxService.main(Native Method)
I/dalvikvm( 453): at org.rockbox.RockboxService.access$300(RockboxService.java:55)
I/dalvikvm( 453): at org.rockbox.RockboxService$1.run(RockboxService.java:279)
I/dalvikvm( 453): at java.lang.Thread.run(Thread.java:1019)
I/dalvikvm( 453):
E/dalvikvm( 453): VM aborting
D/Zygote ( 33): Process 453 terminated by signal (11)

So we probably just need to figure out how to correctly compile our library in order for dalvikvm to print a nice backtrace.
Comment by Thomas Martitz (kugel.) - Monday, 31 January 2011, 21:41 GMT
Is this from the signal handler? If yes then I'm not surprised that backtrace is incomplete, the signal handler is an entirely new context (perhaps one which the VM doesn't see as signals are IIRC dispatched by the kernel?). It's unfortunate of course :(
Comment by Thomas Martitz (kugel.) - Friday, 11 March 2011, 10:03 GMT
Do you still intent committing these?
Comment by Maurus Cuelenaere (mcuelenaere) - Friday, 11 March 2011, 16:09 GMT
Committed 1, 3, 4, 5 and 6.

Loading...