ReviewOS

also looking at this

craft-native/craft

feat(android): the motion pair, and org.json's numbers — ratchet 54 → 52

#187
Merged glennmichael123 wants to merge feat/android-motion into main
7 files +577 -3
9 days ago

Four migrated modules have declined to format a float, and this pair was passed over three times for the same reason. So the reason gets dealt with rather than routed around again.

numberToString is asked, not reimplemented

Android's JSONObject.numberToString is five lines, three of which are decisions a reimplementation would have to rediscover:

if (number.equals(NEGATIVE_ZERO)) return "-0";
long longValue = number.longValue();
if (doubleValue == (double) longValue) return Long.toString(longValue);
return number.toString();

An integral double prints as an integer (9, not 9.0), negative zero prints as -0, and the fallback is Float.toString or Double.toString depending on which box the number arrived in — which are different functions for the same value (Float.toString(0.1f) is "0.1"; Double.toString((double) 0.1f) is "0.10000000149011612").

android_json_number boxes and calls it. Same judgement bridge_android_notifications makes about hashCode and bridge_android_files about Base64.decode: where the platform already has the thing, ask it rather than rebuild it. Here rebuilding means the shortest-round-trip algorithm for two float widths, where being wrong is a silently different number rather than a failure.

Three JNI calls per number. getCurrentPosition will make seven of those once; a motion stream at 50 Hz makes six per event.

That file has no tests, and says why: there is no formatting in it to check, and a host test would stand up a fake numberToString and assert that the fake agrees with itself. What is testable is the shape the numbers go into, and that is tested here with the formatted strings injected.

One event per sample, carrying both sensors

The accelerometer and gyroscope are registered separately and arrive separately, but every arrival sends both — the one that just changed and the last value the other gave. A device with both produces two craftMotionUpdate events per cycle, each a complete reading.

A sensor that has not reported yet reads as zeroes rather than being absent (lastAccel ?: floatArrayOf(0f, 0f, 0f)), so the first event always claims one of the two is perfectly still, and a page cannot tell that from a device lying on a table.

The last-value state is per-registration, as the shim's is — it lives on the listener object that startMotionUpdates creates fresh — so a stop and start forgets it.

Two things with their own tests

The delay boundaries are all <=, so 20ms is SENSOR_DELAY_FASTEST and 21ms is SENSOR_DELAY_GAME. An off-by-one there is a slightly wrong sample rate and nothing else — no error, no warning.

The rotation's keys are alpha, beta, gamma where the acceleration's are x, y, z. Swapping them would be easy and invisible.

Both mutations were run: < 20 instead of <= 20, and renaming alpha to x, each fail named tests.

Testing

zig build test:android passes with the ratchet at 52. Both libcraft.so targets still build with no NDK.

7 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.