Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface → Simulator
  • Assigned To No-one
  • Operating System Another
  • 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 mcuelenaere - 2011-01-30
Last edited by mcuelenaere - 2011-05-07

FS#11914 - Android related patches

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.

Closed by  mcuelenaere
2011-05-07 16:03
Reason for closing:  Fixed
Additional comments about closing:  

These patches are either committed to SVN or obsoleted by other commits.

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)

Yes, I've thought about doing this as a separate target but couldn't come up with a name. Something like make launch?

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.

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.

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?

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.

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?

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.

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 ?

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

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.

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.

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 :(

Do you still intent committing these?

Committed 1, 3, 4, 5 and 6.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing